Am 06.05.2017 um 14:42 schrieb Support: > I have been testing this again and can replicate the issue when the > protocol="org.apache.coyote.http11.Http11AprProtocol" is set. > I don’t know why this worked in my initial testing but it is now always > failing when using APR, either when directly specified in the protocol or > when auto-selecting APR with protocol set to HTTP 1.1. > > I cannot replicate the issue at all when using NIO or BIO.
I was able to reproduce the bug and I think I have fixed the issue in +deb7u13. I would appreciate it if you could try out the latest packages before I upload them just to make sure I have not overlooked something. [1] You can also build tomcat7 from source. The updated CVE-2017-5647.patch is attached to this e-mail. This bug only affects Tomcat 7 in Wheezy by the way, Jessie is not affected. Regards, Markus [1] https://people.debian.org/~apo/tomcat7/
From: Markus Koschany <a...@debian.org> Date: Wed, 12 Apr 2017 18:12:40 +0200 Subject: CVE-2017-5647 Bug-Debian: https://bugs.debian.org/860068 Origin: http://svn.apache.org/r1789008 --- java/org/apache/coyote/AbstractProtocol.java | 6 +- .../apache/coyote/http11/Http11AprProcessor.java | 44 ++++++++----- .../apache/coyote/http11/Http11NioProcessor.java | 35 +++++++++-- java/org/apache/tomcat/util/net/AprEndpoint.java | 47 +++++++++----- java/org/apache/tomcat/util/net/NioEndpoint.java | 73 ++++++++++++---------- .../tomcat/util/net/SendfileKeepAliveState.java | 39 ++++++++++++ java/org/apache/tomcat/util/net/SendfileState.java | 37 +++++++++++ 7 files changed, 205 insertions(+), 76 deletions(-) create mode 100644 java/org/apache/tomcat/util/net/SendfileKeepAliveState.java create mode 100644 java/org/apache/tomcat/util/net/SendfileState.java diff --git a/java/org/apache/coyote/AbstractProtocol.java b/java/org/apache/coyote/AbstractProtocol.java index e86eff8..81aaebd 100644 --- a/java/org/apache/coyote/AbstractProtocol.java +++ b/java/org/apache/coyote/AbstractProtocol.java @@ -606,9 +606,9 @@ public abstract class AbstractProtocol implements ProtocolHandler, release(socket, processor, false, true); } else if (state == SocketState.SENDFILE) { // Sendfile in progress. If it fails, the socket will be - // closed. If it works, the socket will be re-added to the - // poller - release(socket, processor, false, false); + // closed. If it works, the socket either be added to the + // poller (or equivalent) to await more data or processed + // if there are any pipe-lined requests remaining. } else if (state == SocketState.UPGRADED) { // Need to keep the connection associated with the processor longPoll(socket, processor); diff --git a/java/org/apache/coyote/http11/Http11AprProcessor.java b/java/org/apache/coyote/http11/Http11AprProcessor.java index 24f7c5e..18fd30b 100644 --- a/java/org/apache/coyote/http11/Http11AprProcessor.java +++ b/java/org/apache/coyote/http11/Http11AprProcessor.java @@ -35,6 +35,7 @@ import org.apache.tomcat.jni.Socket; import org.apache.tomcat.util.ExceptionUtils; import org.apache.tomcat.util.net.AbstractEndpoint.Handler.SocketState; import org.apache.tomcat.util.net.AprEndpoint; +import org.apache.tomcat.util.net.SendfileKeepAliveState; import org.apache.tomcat.util.net.SSLSupport; import org.apache.tomcat.util.net.SocketStatus; import org.apache.tomcat.util.net.SocketWrapper; @@ -221,24 +222,33 @@ public class Http11AprProcessor extends AbstractHttp11Processor<Long> { // Do sendfile as needed: add socket to sendfile and end if (sendfileData != null && !error) { sendfileData.socket = socketWrapper.getSocket().longValue(); - sendfileData.keepAlive = keepAlive; - if (!((AprEndpoint)endpoint).getSendfile().add(sendfileData)) { - // Didn't send all of the data to sendfile. - if (sendfileData.socket == 0) { - // The socket is no longer set. Something went wrong. - // Close the connection. Too late to set status code. - if (log.isDebugEnabled()) { - log.debug(sm.getString( - "http11processor.sendfile.error")); - } - error = true; - } else { - // The sendfile Poller will add the socket to the main - // Poller once sendfile processing is complete - sendfileInProgress = true; - } - return true; + if (keepAlive) { + if (getInputBuffer().available() == 0) { + sendfileData.keepAliveState = SendfileKeepAliveState.OPEN; + } else { + sendfileData.keepAliveState = SendfileKeepAliveState.PIPELINED; + } + } else { + sendfileData.keepAliveState = SendfileKeepAliveState.NONE; } + switch (((AprEndpoint)endpoint).getSendfile().add(sendfileData)) { + case DONE: + return false; + case PENDING: + // The sendfile Poller will add the socket to the main + // Poller once sendfile processing is complete + sendfileInProgress = true; + return true; + case ERROR: + // Something went wrong. + // Close the connection. Too late to set status code. + if (log.isDebugEnabled()) { + log.debug(sm.getString( + "http11processor.sendfile.error")); + } + error = true; + return true; + } } return false; } diff --git a/java/org/apache/coyote/http11/Http11NioProcessor.java b/java/org/apache/coyote/http11/Http11NioProcessor.java index 32b0bc6..acc1b96 100644 --- a/java/org/apache/coyote/http11/Http11NioProcessor.java +++ b/java/org/apache/coyote/http11/Http11NioProcessor.java @@ -35,6 +35,7 @@ import org.apache.tomcat.util.net.NioEndpoint; import org.apache.tomcat.util.net.NioEndpoint.KeyAttachment; import org.apache.tomcat.util.net.SSLSupport; import org.apache.tomcat.util.net.SecureNioChannel; +import org.apache.tomcat.util.net.SendfileKeepAliveState; import org.apache.tomcat.util.net.SocketStatus; import org.apache.tomcat.util.net.SocketWrapper; @@ -276,19 +277,41 @@ public class Http11NioProcessor extends AbstractHttp11Processor<NioChannel> { @Override protected boolean breakKeepAliveLoop( SocketWrapper<NioChannel> socketWrapper) { + openSocket = keepAlive; // Do sendfile as needed: add socket to sendfile and end if (sendfileData != null && !error) { ((KeyAttachment) socketWrapper).setSendfileData(sendfileData); - sendfileData.keepAlive = keepAlive; + if (keepAlive) { + if (getInputBuffer().available() == 0) { + sendfileData.keepAliveState = SendfileKeepAliveState.OPEN; + } else { + sendfileData.keepAliveState = SendfileKeepAliveState.PIPELINED; + } + } else { + sendfileData.keepAliveState = SendfileKeepAliveState.NONE; + } SelectionKey key = socketWrapper.getSocket().getIOChannel().keyFor( socketWrapper.getSocket().getPoller().getSelector()); //do the first write on this thread, might as well - openSocket = socketWrapper.getSocket().getPoller().processSendfile(key, - (KeyAttachment) socketWrapper, true); - return true; + switch (socketWrapper.getSocket().getPoller().processSendfile( + key, (KeyAttachment) socketWrapper, true)) { + case DONE: + // If sendfile is complete, no need to break keep-alive loop + sendfileData = null; + return false; + case PENDING: + sendfileInProgress = true; + return true; + case ERROR: + // Write failed + if (log.isDebugEnabled()) { + log.debug(sm.getString("http11processor.sendfile.error")); + } + return true; + } + } + return false; } - return false; - } @Override diff --git a/java/org/apache/tomcat/util/net/AprEndpoint.java b/java/org/apache/tomcat/util/net/AprEndpoint.java index 3ac6283..fa6ec69 100644 --- a/java/org/apache/tomcat/util/net/AprEndpoint.java +++ b/java/org/apache/tomcat/util/net/AprEndpoint.java @@ -1353,7 +1353,7 @@ public class AprEndpoint extends AbstractEndpoint { // Position public long pos; // KeepAlive flag - public boolean keepAlive; + public SendfileKeepAliveState keepAliveState = SendfileKeepAliveState.NONE; } @@ -1439,7 +1439,7 @@ public class AprEndpoint extends AbstractEndpoint { * @return true if all the data has been sent right away, and false * otherwise */ - public boolean add(SendfileData data) { + public SendfileState add(SendfileData data) { // Initialize fd from data given try { data.fdpool = Socket.pool(data.socket); @@ -1447,7 +1447,7 @@ public class AprEndpoint extends AbstractEndpoint { // Pool not created so no need to destroy it. log.error(sm.getString("endpoint.sendfile.error"), e); data.socket = 0; - return false; + return SendfileState.ERROR; } try { data.fd = File.open @@ -1466,7 +1466,7 @@ public class AprEndpoint extends AbstractEndpoint { // No need to close socket, this will be done by // calling code since data.socket == 0 data.socket = 0; - return false; + return SendfileState.ERROR; } else { // Break the loop and add the socket to poller. break; @@ -1479,14 +1479,14 @@ public class AprEndpoint extends AbstractEndpoint { Pool.destroy(data.fdpool); // Set back socket to blocking mode Socket.timeoutSet(data.socket, socketProperties.getSoTimeout() * 1000); - return true; + return SendfileState.DONE; } } } catch (Exception e) { log.error(sm.getString("endpoint.sendfile.error"), e); Pool.destroy(data.fdpool); data.socket = 0; - return false; + return SendfileState.ERROR; } // Add socket to the list. Newly added sockets will wait // at most for pollTime before being polled @@ -1495,7 +1495,7 @@ public class AprEndpoint extends AbstractEndpoint { addCount++; this.notify(); } - return false; + return SendfileState.PENDING; } /** @@ -1611,18 +1611,33 @@ public class AprEndpoint extends AbstractEndpoint { state.pos = state.pos + nw; if (state.pos >= state.end) { remove(state); - if (state.keepAlive) { + switch (state.keepAliveState) { + case NONE: { + // Close the socket since this is + // the end of the not keep-alive request. + destroySocket(state.socket); + break; + } + case PIPELINED: { // Destroy file descriptor pool, which should close the file Pool.destroy(state.fdpool); Socket.timeoutSet(state.socket, socketProperties.getSoTimeout() * 1000); - // If all done put the socket back in the poller for - // processing of further requests - getPoller().add(state.socket, - getKeepAliveTimeout()); - } else { - // Close the socket since this is - // the end of not keep-alive request. - destroySocket(state.socket); + // Process the pipelined request data + if (!processSocket(state.socket, SocketStatus.OPEN)) { + destroySocket(state.socket); + } + break; + } + case OPEN: { + // Destroy file descriptor pool, which should close the file + Pool.destroy(state.fdpool); + Socket.timeoutSet(state.socket, socketProperties.getSoTimeout() * 1000); + // Put the socket back in the poller for + // processing of further requests + getPoller().add(state.socket, + getKeepAliveTimeout()); + break; + } } } } diff --git a/java/org/apache/tomcat/util/net/NioEndpoint.java b/java/org/apache/tomcat/util/net/NioEndpoint.java index a2f7a12..9b5f25e 100644 --- a/java/org/apache/tomcat/util/net/NioEndpoint.java +++ b/java/org/apache/tomcat/util/net/NioEndpoint.java @@ -1276,17 +1276,8 @@ public class NioEndpoint extends AbstractEndpoint { return result; } - /** - * @deprecated Replaced by processSendfile(sk, attachment, event) - */ - @Deprecated - public boolean processSendfile(SelectionKey sk, - KeyAttachment attachment, - @SuppressWarnings("unused") boolean reg, boolean event) { - return processSendfile(sk, attachment, event); - } - - public boolean processSendfile(SelectionKey sk, KeyAttachment attachment, boolean event) { + public SendfileState processSendfile(SelectionKey sk, KeyAttachment attachment, + boolean calledByProcessor) { NioChannel sc = null; try { unreg(sk, attachment, sk.readyOps()); @@ -1301,14 +1292,15 @@ public class NioEndpoint extends AbstractEndpoint { File f = new File(sd.fileName); if ( !f.exists() ) { cancelledKey(sk,SocketStatus.ERROR,false); - return false; + return SendfileState.ERROR; } - sd.fchannel = new FileInputStream(f).getChannel(); + @SuppressWarnings("resource") // Closed when channel is closed + FileInputStream fis = new FileInputStream(f); + sd.fchannel = fis.getChannel(); } //configure output channel sc = attachment.getChannel(); - sc.setSendFile(true); //ssl channel is slightly different WritableByteChannel wc = ((sc instanceof SecureNioChannel)?sc:sc.getIOChannel()); @@ -1341,48 +1333,61 @@ public class NioEndpoint extends AbstractEndpoint { sd.fchannel.close(); } catch (Exception ignore) { } - if ( sd.keepAlive ) { + // For calls from outside the Poller, the caller is + // responsible for registering the socket for the + // appropriate event(s) if sendfile completes. + if (!calledByProcessor) { + switch (sd.keepAliveState) { + case NONE: { if (log.isDebugEnabled()) { - log.debug("Connection is keep alive, registering back for OP_READ"); + log.debug("Send file connection is being closed"); } - if (event) { - this.add(attachment.getChannel(),SelectionKey.OP_READ); - } else { - reg(sk,attachment,SelectionKey.OP_READ); + cancelledKey(sk,SocketStatus.STOP,false); + break; + } + case PIPELINED: { + if (log.isDebugEnabled()) { + log.debug("Connection is keep alive, processing pipe-lined data"); } - } else { - if (log.isDebugEnabled()) { - log.debug("Send file connection is being closed"); + if (!processSocket(sc, SocketStatus.OPEN, true)) { + cancelledKey(sk, SocketStatus.DISCONNECT, false); + } + break; + } + case OPEN: { + if (log.isDebugEnabled()) { + log.debug("Connection is keep alive, registering back for OP_READ"); + } + reg(sk, attachment, SelectionKey.OP_READ); + break; + } } - cancelledKey(sk,SocketStatus.STOP,false); - return false; } + return SendfileState.DONE; } else { if (log.isDebugEnabled()) { log.debug("OP_WRITE for sendfile: " + sd.fileName); } - if (event) { + if (calledByProcessor) { add(attachment.getChannel(),SelectionKey.OP_WRITE); } else { reg(sk,attachment,SelectionKey.OP_WRITE); } + return SendfileState.PENDING; } }catch ( IOException x ) { if ( log.isDebugEnabled() ) log.debug("Unable to complete sendfile request:", x); - if (!event) { + if (!calledByProcessor) { cancelledKey(sk,SocketStatus.ERROR,false); } - return false; + return SendfileState.ERROR; }catch ( Throwable t ) { log.error("",t); - if (!event) { + if (!calledByProcessor) { cancelledKey(sk, SocketStatus.ERROR, false); } - return false; - }finally { - if (sc!=null) sc.setSendFile(false); + return SendfileState.ERROR; } - return true; } protected void unreg(SelectionKey sk, KeyAttachment attachment, int readyOps) { @@ -1746,6 +1751,6 @@ public class NioEndpoint extends AbstractEndpoint { public long pos; public long length; // KeepAlive flag - public boolean keepAlive; + public SendfileKeepAliveState keepAliveState = SendfileKeepAliveState.NONE; } } diff --git a/java/org/apache/tomcat/util/net/SendfileKeepAliveState.java b/java/org/apache/tomcat/util/net/SendfileKeepAliveState.java new file mode 100644 index 0000000..b27a9f1 --- /dev/null +++ b/java/org/apache/tomcat/util/net/SendfileKeepAliveState.java @@ -0,0 +1,39 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.util.net; + +public enum SendfileKeepAliveState { + + /** + * Keep-alive is not in use. The socket can be closed when the response has + * been written. + */ + NONE, + + /** + * Keep-alive is in use and there is pipelined data in the input buffer to + * be read as soon as the current response has been written. + */ + PIPELINED, + + /** + * Keep-alive is in use. The socket should be added to the poller (or + * equivalent) to await more data as soon as the current response has been + * written. + */ + OPEN +} diff --git a/java/org/apache/tomcat/util/net/SendfileState.java b/java/org/apache/tomcat/util/net/SendfileState.java new file mode 100644 index 0000000..b354e2f --- /dev/null +++ b/java/org/apache/tomcat/util/net/SendfileState.java @@ -0,0 +1,37 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.tomcat.util.net; + +public enum SendfileState { + + /** + * The sending of the file has started but has not completed. Sendfile is + * still using the socket. + */ + PENDING, + + /** + * The file has been fully sent. Sendfile is no longer using the socket. + */ + DONE, + + /** + * Something went wrong. The file may or may not have been sent. The socket + * is in an unknown state. + */ + ERROR +}
signature.asc
Description: OpenPGP digital signature
