tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ma...@apache.org
Subject svn commit: r1804463 - in /tomcat/trunk: java/org/apache/coyote/ java/org/apache/coyote/http11/ java/org/apache/tomcat/util/net/ webapps/docs/
Date Tue, 08 Aug 2017 19:29:55 GMT
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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message