Author: markt Date: Tue Aug 8 19:29:55 2017 New Revision: 1804463 URL: http://svn.apache.org/viewvc?rev=1804463&view=rev Log: Improve the handling of client disconnections during the TLS renegotiation handshake.
Modified: tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java tomcat/trunk/java/org/apache/tomcat/util/net/LocalStrings.properties tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java tomcat/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java tomcat/trunk/java/org/apache/tomcat/util/net/SecureNio2Channel.java tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java tomcat/trunk/java/org/apache/tomcat/util/net/SocketWrapperBase.java tomcat/trunk/webapps/docs/changelog.xml Modified: tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java?rev=1804463&r1=1804462&r2=1804463&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java (original) +++ tomcat/trunk/java/org/apache/coyote/AbstractProcessor.java Tue Aug 8 19:29:55 2017 @@ -371,7 +371,11 @@ public abstract class AbstractProcessor break; } case REQ_SSL_CERTIFICATE: { - sslReHandShake(); + try { + sslReHandShake(); + } catch (IOException ioe) { + setErrorState(ErrorState.CLOSE_CONNECTION_NOW, ioe); + } break; } @@ -645,8 +649,12 @@ public abstract class AbstractProcessor /** * Processors that can perform a TLS re-handshake (e.g. HTTP/1.1) should * override this method and implement the re-handshake. + * + * @throws IOException If authentication is required then there will be I/O + * with the client and this exception will be thrown if + * that goes wrong */ - protected void sslReHandShake() { + protected void sslReHandShake() throws IOException { // NO-OP } Modified: tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java?rev=1804463&r1=1804462&r2=1804463&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java (original) +++ tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java Tue Aug 8 19:29:55 2017 @@ -1242,7 +1242,7 @@ public class Http11Processor extends Abs @Override - protected final void sslReHandShake() { + protected final void sslReHandShake() throws IOException { if (sslSupport != null) { // Consume and buffer the request body, so that it does not // interfere with the client's handshake messages @@ -1251,8 +1251,17 @@ public class Http11Processor extends Abs protocol.getMaxSavePostSize()); inputBuffer.addActiveFilter(inputFilters[Constants.BUFFERED_FILTER]); + /* + * Outside the try/catch because we want I/O errors during + * renegotiation to be thrown for the caller to handle since they + * will be fatal to the connection. + */ + socketWrapper.doClientAuth(sslSupport); try { - socketWrapper.doClientAuth(sslSupport); + /* + * Errors processing the cert chain do not affect the client + * connection so they can be logged and swallowed here. + */ Object sslO = sslSupport.getPeerCertificateChain(); if (sslO != null) { request.setAttribute(SSLSupport.CERTIFICATE_KEY, sslO); Modified: tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java?rev=1804463&r1=1804462&r2=1804463&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java (original) +++ tomcat/trunk/java/org/apache/tomcat/util/net/AprEndpoint.java Tue Aug 8 19:29:55 2017 @@ -2749,11 +2749,16 @@ public class AprEndpoint extends Abstrac @Override - public void doClientAuth(SSLSupport sslSupport) { + public void doClientAuth(SSLSupport sslSupport) throws IOException { long socket = getSocket().longValue(); // Configure connection to require a certificate - SSLSocket.setVerify(socket, SSL.SSL_CVERIFY_REQUIRE, -1); - SSLSocket.renegotiate(socket); + try { + SSLSocket.setVerify(socket, SSL.SSL_CVERIFY_REQUIRE, -1); + SSLSocket.renegotiate(socket); + } catch (Throwable t) { + ExceptionUtils.handleThrowable(t); + throw new IOException(sm.getString("socket.sslreneg"), t); + } } Modified: tomcat/trunk/java/org/apache/tomcat/util/net/LocalStrings.properties URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/LocalStrings.properties?rev=1804463&r1=1804462&r2=1804463&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/tomcat/util/net/LocalStrings.properties (original) +++ tomcat/trunk/java/org/apache/tomcat/util/net/LocalStrings.properties Tue Aug 8 19:29:55 2017 @@ -83,6 +83,7 @@ endpoint.nio.timeoutCme=Exception during endpoint.nio2.exclusiveExecutor=The NIO2 connector requires an exclusive executor to operate properly on shutdown channel.nio.interrupted=The current thread was interrupted +channel.nio.ssl.closeSilentError=As expected, there was an exception trying to close the connection cleanly. channel.nio.ssl.notHandshaking=NOT_HANDSHAKING during handshake channel.nio.ssl.handshakeError=Handshake error channel.nio.ssl.unexpectedStatusDuringWrap=Unexpected status [{0}] during handshake WRAP. Modified: tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java?rev=1804463&r1=1804462&r2=1804463&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java (original) +++ tomcat/trunk/java/org/apache/tomcat/util/net/Nio2Endpoint.java Tue Aug 8 19:29:55 2017 @@ -1589,18 +1589,14 @@ public class Nio2Endpoint extends Abstra @Override - public void doClientAuth(SSLSupport sslSupport) { + public void doClientAuth(SSLSupport sslSupport) throws IOException { SecureNio2Channel sslChannel = (SecureNio2Channel) getSocket(); SSLEngine engine = sslChannel.getSslEngine(); if (!engine.getNeedClientAuth()) { // Need to re-negotiate SSL connection engine.setNeedClientAuth(true); - try { - sslChannel.rehandshake(); - ((JSSESupport) sslSupport).setSession(engine.getSession()); - } catch (IOException ioe) { - log.warn(sm.getString("socket.sslreneg"), ioe); - } + sslChannel.rehandshake(); + ((JSSESupport) sslSupport).setSession(engine.getSession()); } } Modified: tomcat/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java?rev=1804463&r1=1804462&r2=1804463&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java (original) +++ tomcat/trunk/java/org/apache/tomcat/util/net/NioEndpoint.java Tue Aug 8 19:29:55 2017 @@ -1293,18 +1293,14 @@ public class NioEndpoint extends Abstrac @Override - public void doClientAuth(SSLSupport sslSupport) { + public void doClientAuth(SSLSupport sslSupport) throws IOException { SecureNioChannel sslChannel = (SecureNioChannel) getSocket(); SSLEngine engine = sslChannel.getSslEngine(); if (!engine.getNeedClientAuth()) { // Need to re-negotiate SSL connection engine.setNeedClientAuth(true); - try { - sslChannel.rehandshake(getEndpoint().getConnectionTimeout()); - ((JSSESupport) sslSupport).setSession(engine.getSession()); - } catch (IOException ioe) { - log.warn(sm.getString("socket.sslreneg",ioe)); - } + sslChannel.rehandshake(getEndpoint().getConnectionTimeout()); + ((JSSESupport) sslSupport).setSession(engine.getSession()); } } Modified: tomcat/trunk/java/org/apache/tomcat/util/net/SecureNio2Channel.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/SecureNio2Channel.java?rev=1804463&r1=1804462&r2=1804463&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/tomcat/util/net/SecureNio2Channel.java (original) +++ tomcat/trunk/java/org/apache/tomcat/util/net/SecureNio2Channel.java Tue Aug 8 19:29:55 2017 @@ -305,8 +305,11 @@ public class SecureNio2Channel extends N sc.read(netInBuffer, socket, handshakeReadCompletionHandler); } else { try { - sc.read(netInBuffer).get(endpoint.getConnectionTimeout(), - TimeUnit.MILLISECONDS); + int read = sc.read(netInBuffer).get(endpoint.getConnectionTimeout(), + TimeUnit.MILLISECONDS).intValue(); + if (read == -1) { + throw new EOFException(); + } } catch (InterruptedException | ExecutionException | TimeoutException e) { throw new IOException(sm.getString("channel.nio.ssl.handshakeError")); } @@ -449,8 +452,10 @@ public class SecureNio2Channel extends N } } } catch (IOException x) { + closeSilently(); throw x; } catch (Exception cx) { + closeSilently(); IOException x = new IOException(cx); throw x; } @@ -576,18 +581,31 @@ public class SecureNio2Channel extends N closed = (!netOutBuffer.hasRemaining() && (handshake.getHandshakeStatus() != HandshakeStatus.NEED_WRAP)); } + @Override public void close(boolean force) throws IOException { try { close(); } finally { - if ( force || closed ) { + if (force || closed) { closed = true; sc.close(); } } } + + private void closeSilently() { + try { + close(true); + } catch (IOException ioe) { + // This is expected - swallowing the exception is the reason this + // method exists. Log at debug in case someone is interested. + log.debug(sm.getString("channel.nio.ssl.closeSilentError"), ioe); + } + } + + private class FutureRead implements Future<Integer> { private ByteBuffer dst; private Future<Integer> integer; Modified: tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java?rev=1804463&r1=1804462&r2=1804463&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java (original) +++ tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java Tue Aug 8 19:29:55 2017 @@ -395,8 +395,10 @@ public class SecureNioChannel extends Ni } } } catch (IOException x) { + closeSilently(); throw x; } catch (Exception cx) { + closeSilently(); IOException x = new IOException(cx); throw x; } finally { @@ -526,8 +528,8 @@ public class SecureNioChannel extends Ni public void close(boolean force) throws IOException { try { close(); - }finally { - if ( force || closed ) { + } finally { + if (force || closed) { closed = true; sc.socket().close(); sc.close(); @@ -535,6 +537,18 @@ public class SecureNioChannel extends Ni } } + + private void closeSilently() { + try { + close(true); + } catch (IOException ioe) { + // This is expected - swallowing the exception is the reason this + // method exists. Log at debug in case someone is interested. + log.debug(sm.getString("channel.nio.ssl.closeSilentError"), ioe); + } + } + + /** * Reads a sequence of bytes from this channel into the given buffer. * Modified: tomcat/trunk/java/org/apache/tomcat/util/net/SocketWrapperBase.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/SocketWrapperBase.java?rev=1804463&r1=1804462&r2=1804463&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/tomcat/util/net/SocketWrapperBase.java (original) +++ tomcat/trunk/java/org/apache/tomcat/util/net/SocketWrapperBase.java Tue Aug 8 19:29:55 2017 @@ -759,8 +759,12 @@ public abstract class SocketWrapperBase< * @param sslSupport The SSL/TLS support instance currently being used by * the connection that may need updating after the client * authentication + * + * @throws IOException If authentication is required then there will be I/O + * with the client and this exception will be thrown if + * that goes wrong */ - public abstract void doClientAuth(SSLSupport sslSupport); + public abstract void doClientAuth(SSLSupport sslSupport) throws IOException; public abstract SSLSupport getSslSupport(String clientCertProvider); Modified: tomcat/trunk/webapps/docs/changelog.xml URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1804463&r1=1804462&r2=1804463&view=diff ============================================================================== --- tomcat/trunk/webapps/docs/changelog.xml (original) +++ tomcat/trunk/webapps/docs/changelog.xml Tue Aug 8 19:29:55 2017 @@ -64,6 +64,10 @@ which is the most secure / restrictive option in addition to reporting the configuration error. (markt) </fix> + <fix> + Improve the handling of client disconnections during the TLS + renegotiation handshake. (markt) + </fix> </changelog> </subsection> </section> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org