Hi,
I don't think the link you gave on commit [fe932dd39d] is the reason for FTBFS. I tried building on a VM that matches the certificate date and it was successful. I also tried disabling all ssl related tests and was fine. While doing these all I found TestSendFile test is the culprit. In CVE-2017-5647 security patch a good amount of changes is applied for SendFile*.java and *Nio2*.java. These are mostly about conditions on how long the socket of sendfile keep active and to take away from it. But I couldn't see any those change in its test file. Please take a look on the attached patch. :) --abhijith
From: Markus Koschany <a...@debian.org> Date: Sat, 15 Apr 2017 17:25:03 +0200 Subject: CVE-2017-5647 Bug-Debian: https://bugs.debian.org/860068 Origin: http://svn.apache.org/r1788999 --- java/org/apache/coyote/AbstractProtocol.java | 7 +- .../apache/coyote/http11/Http11AprProcessor.java | 38 +++++++---- .../apache/coyote/http11/Http11Nio2Processor.java | 11 +++- .../apache/coyote/http11/Http11NioProcessor.java | 26 ++++++-- java/org/apache/tomcat/util/net/AprEndpoint.java | 49 +++++++++----- java/org/apache/tomcat/util/net/Nio2Endpoint.java | 25 ++++--- java/org/apache/tomcat/util/net/NioEndpoint.java | 76 ++++++++++++---------- .../tomcat/util/net/SendfileKeepAliveState.java | 39 +++++++++++ java/org/apache/tomcat/util/net/SendfileState.java | 37 +++++++++++ 9 files changed, 222 insertions(+), 86 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 9886cef..cabfbf6 100644 --- a/java/org/apache/coyote/AbstractProtocol.java +++ b/java/org/apache/coyote/AbstractProtocol.java @@ -710,10 +710,9 @@ public abstract class AbstractProtocol<S> implements ProtocolHandler, release(wrapper, 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 - connections.remove(socket); - release(wrapper, 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) { // Don't add sockets back to the poller if this was a // non-blocking write otherwise the poller may trigger diff --git a/java/org/apache/coyote/http11/Http11AprProcessor.java b/java/org/apache/coyote/http11/Http11AprProcessor.java index e4ecd1a..a08da6f 100644 --- a/java/org/apache/coyote/http11/Http11AprProcessor.java +++ b/java/org/apache/coyote/http11/Http11AprProcessor.java @@ -37,6 +37,7 @@ 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.SSLSupport; +import org.apache.tomcat.util.net.SendfileKeepAliveState; import org.apache.tomcat.util.net.SocketStatus; import org.apache.tomcat.util.net.SocketWrapper; @@ -197,22 +198,31 @@ public class Http11AprProcessor extends AbstractHttp11Processor<Long> { // Do sendfile as needed: add socket to sendfile and end if (sendfileData != null && !getErrorState().isError()) { 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")); - } - setErrorState(ErrorState.CLOSE_NOW, null); + if (keepAlive) { + if (getInputBuffer().available() == 0) { + sendfileData.keepAliveState = SendfileKeepAliveState.OPEN; } else { - // The sendfile Poller will add the socket to the main - // Poller once sendfile processing is complete - sendfileInProgress = true; + 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")); } + setErrorState(ErrorState.CLOSE_NOW, null); return true; } } diff --git a/java/org/apache/coyote/http11/Http11Nio2Processor.java b/java/org/apache/coyote/http11/Http11Nio2Processor.java index 6abd200..d13931a 100644 --- a/java/org/apache/coyote/http11/Http11Nio2Processor.java +++ b/java/org/apache/coyote/http11/Http11Nio2Processor.java @@ -35,6 +35,7 @@ import org.apache.tomcat.util.net.Nio2Channel; import org.apache.tomcat.util.net.Nio2Endpoint; import org.apache.tomcat.util.net.SSLSupport; import org.apache.tomcat.util.net.SecureNio2Channel; +import org.apache.tomcat.util.net.SendfileKeepAliveState; import org.apache.tomcat.util.net.SocketStatus; import org.apache.tomcat.util.net.SocketWrapper; @@ -279,7 +280,15 @@ public class Http11Nio2Processor extends AbstractHttp11Processor<Nio2Channel> { // Do sendfile as needed: add socket to sendfile and end if (sendfileData != null && !getErrorState().isError()) { ((Nio2Endpoint.Nio2SocketWrapper) 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; + } switch (((Nio2Endpoint) endpoint) .processSendfile((Nio2Endpoint.Nio2SocketWrapper) socketWrapper)) { case DONE: diff --git a/java/org/apache/coyote/http11/Http11NioProcessor.java b/java/org/apache/coyote/http11/Http11NioProcessor.java index 30aa9e9..983bd06 100644 --- a/java/org/apache/coyote/http11/Http11NioProcessor.java +++ b/java/org/apache/coyote/http11/Http11NioProcessor.java @@ -36,6 +36,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; @@ -270,28 +271,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 && !getErrorState().isError()) { ((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 - if (socketWrapper.getSocket().getPoller().processSendfile(key, - (KeyAttachment) socketWrapper, 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; - } else { + return true; + case ERROR: // Write failed if (log.isDebugEnabled()) { log.debug(sm.getString("http11processor.sendfile.error")); } setErrorState(ErrorState.CLOSE_NOW, null); + return true; } - return true; } return false; } diff --git a/java/org/apache/tomcat/util/net/AprEndpoint.java b/java/org/apache/tomcat/util/net/AprEndpoint.java index 7db7214..66c876c 100644 --- a/java/org/apache/tomcat/util/net/AprEndpoint.java +++ b/java/org/apache/tomcat/util/net/AprEndpoint.java @@ -1973,7 +1973,7 @@ public class AprEndpoint extends AbstractEndpoint<Long> { // Position public long pos; // KeepAlive flag - public boolean keepAlive; + public SendfileKeepAliveState keepAliveState = SendfileKeepAliveState.NONE; } @@ -2061,7 +2061,7 @@ public class AprEndpoint extends AbstractEndpoint<Long> { * @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); @@ -2079,7 +2079,7 @@ public class AprEndpoint extends AbstractEndpoint<Long> { if (!(-nw == Status.EAGAIN)) { Pool.destroy(data.fdpool); data.socket = 0; - return false; + return SendfileState.ERROR; } else { // Break the loop and add the socket to poller. break; @@ -2092,13 +2092,13 @@ public class AprEndpoint extends AbstractEndpoint<Long> { // Set back socket to blocking mode Socket.timeoutSet( data.socket, getSoTimeout() * 1000); - return true; + return SendfileState.DONE; } } } } catch (Exception e) { log.warn(sm.getString("endpoint.sendfile.error"), e); - return false; + return SendfileState.ERROR; } // Add socket to the list. Newly added sockets will wait // at most for pollTime before being polled @@ -2106,7 +2106,7 @@ public class AprEndpoint extends AbstractEndpoint<Long> { addS.add(data); this.notify(); } - return false; + return SendfileState.PENDING; } /** @@ -2216,20 +2216,33 @@ public class AprEndpoint extends AbstractEndpoint<Long> { state.pos = state.pos + nw; if (state.pos >= state.end) { remove(state); - if (state.keepAlive) { - // Destroy file descriptor pool, which should close the file - Pool.destroy(state.fdpool); - Socket.timeoutSet(state.socket, - getSoTimeout() * 1000); - // If all done put the socket back in the - // poller for processing of further requests - getPoller().add( - state.socket, getKeepAliveTimeout(), - true, false); - } else { + switch (state.keepAliveState) { + case NONE: { // Close the socket since this is - // the end of not keep-alive request. + // the end of the not keep-alive request. closeSocket(state.socket); + break; + } + case PIPELINED: { + // Destroy file descriptor pool, which should close the file + Pool.destroy(state.fdpool); + Socket.timeoutSet(state.socket, getSoTimeout() * 1000); + // Process the pipelined request data + if (!processSocket(state.socket, SocketStatus.OPEN_READ)) { + closeSocket(state.socket); + } + break; + } + case OPEN: { + // Destroy file descriptor pool, which should close the file + Pool.destroy(state.fdpool); + Socket.timeoutSet(state.socket, getSoTimeout() * 1000); + // Put the socket back in the poller for + // processing of further requests + getPoller().add(state.socket, + getKeepAliveTimeout(),true,false); + break; + } } } } diff --git a/java/org/apache/tomcat/util/net/Nio2Endpoint.java b/java/org/apache/tomcat/util/net/Nio2Endpoint.java index 0f9a023..389c615 100644 --- a/java/org/apache/tomcat/util/net/Nio2Endpoint.java +++ b/java/org/apache/tomcat/util/net/Nio2Endpoint.java @@ -909,17 +909,22 @@ public class Nio2Endpoint extends AbstractEndpoint<Nio2Channel> { } catch (IOException e) { // Ignore } - if (attachment.keepAlive) { - if (!isInline()) { - awaitBytes(attachment.socket); - } else { - attachment.doneInline = true; - } + if (isInline()) { + attachment.doneInline = true; } else { - if (!isInline()) { + switch (attachment.keepAliveState) { + case NONE: { processSocket(attachment.socket, SocketStatus.DISCONNECT, false); - } else { - attachment.doneInline = true; + break; + } + case PIPELINED: { + processSocket(attachment.socket, SocketStatus.OPEN_READ, true); + break; + } + case OPEN: { + awaitBytes(attachment.socket); + break; + } } } return; @@ -1159,7 +1164,7 @@ public class Nio2Endpoint extends AbstractEndpoint<Nio2Channel> { public long pos; public long length; // KeepAlive flag - public boolean keepAlive; + public SendfileKeepAliveState keepAliveState = SendfileKeepAliveState.NONE; // Internal use only private Nio2SocketWrapper socket; private ByteBuffer buffer; diff --git a/java/org/apache/tomcat/util/net/NioEndpoint.java b/java/org/apache/tomcat/util/net/NioEndpoint.java index 36a1c53..96386a4 100644 --- a/java/org/apache/tomcat/util/net/NioEndpoint.java +++ b/java/org/apache/tomcat/util/net/NioEndpoint.java @@ -1166,7 +1166,9 @@ public class NioEndpoint extends AbstractEndpoint<NioChannel> { return result; } - 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()); @@ -1176,32 +1178,27 @@ public class NioEndpoint extends AbstractEndpoint<NioChannel> { log.trace("Processing send file for: " + sd.fileName); } - //setup the file channel - if ( sd.fchannel == null ) { + if (sd.fchannel == null) { + // Setup the file channel File f = new File(sd.fileName); - if ( !f.exists() ) { - cancelledKey(sk,SocketStatus.ERROR); - return false; - } @SuppressWarnings("resource") // Closed when channel is closed FileInputStream fis = new FileInputStream(f); sd.fchannel = fis.getChannel(); } - //configure output channel + // Configure output channel sc = attachment.getSocket(); - sc.setSendFile(true); - //ssl channel is slightly different + // TLS/SSL channel is slightly different WritableByteChannel wc = ((sc instanceof SecureNioChannel)?sc:sc.getIOChannel()); - //we still have data in the buffer + // We still have data in the buffer if (sc.getOutboundRemaining()>0) { if (sc.flushOutbound()) { attachment.access(); } } else { long written = sd.fchannel.transferTo(sd.pos,sd.length,wc); - if ( written > 0 ) { + if (written > 0) { sd.pos += written; sd.length -= written; attachment.access(); @@ -1214,7 +1211,7 @@ public class NioEndpoint extends AbstractEndpoint<NioChannel> { } } } - if ( sd.length <= 0 && sc.getOutboundRemaining()<=0) { + if (sd.length <= 0 && sc.getOutboundRemaining()<=0) { if (log.isDebugEnabled()) { log.debug("Send file complete for: "+sd.fileName); } @@ -1223,48 +1220,61 @@ public class NioEndpoint extends AbstractEndpoint<NioChannel> { 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.getSocket(),SelectionKey.OP_READ); - } else { - reg(sk,attachment,SelectionKey.OP_READ); + cancelledKey(sk,SocketStatus.STOP); + 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(attachment, SocketStatus.OPEN_READ, true)) { + cancelledKey(sk, SocketStatus.DISCONNECT); + } + 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); - return false; } + return SendfileState.DONE; } else { if (log.isDebugEnabled()) { log.debug("OP_WRITE for sendfile: " + sd.fileName); } - if (event) { + if (calledByProcessor) { add(attachment.getSocket(),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); } - return false; + return SendfileState.ERROR; }catch ( Throwable t ) { log.error("",t); - if (!event) { + if (!calledByProcessor) { cancelledKey(sk, SocketStatus.ERROR); } - return false; - }finally { - if (sc!=null) sc.setSendFile(false); + return SendfileState.ERROR; } - return true; } protected void unreg(SelectionKey sk, KeyAttachment attachment, int readyOps) { @@ -1653,6 +1663,6 @@ public class NioEndpoint extends AbstractEndpoint<NioChannel> { 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 +}
