Return correct value from SSLSession.getPacketSize() when using native SSL implementation (#13095)

normanmaurer · web-flow · commit e530cd610d17 · 2023-01-09T09:48:06.000+01:00
Motivation: We didnt return the maximum size of SSL packet and tried to calculate it. This didnt work as SSL_max_seal_overhead(...) can only be used to calculate the maximum overhead for when encrypting ourself (and not the remote peer). Because of this we sometimes returned a smaller number then what is possible. This had the affect that when users did use getPacketSize() to size the ByteBuffer we could end up in a situation that would never produce a bug enough ByteBuffer and so never finish the handshake. This issue only accoured when users use the SSLEngine directly. When using our SslHandler we were not affected by this as we use a different approach there. Modifications: - Upgrade netty-tcnative to be able to reuse the the defined constant - Add unit test that did loop forever before this change Result: Fixes #13073
diff --git a/bom/pom.xml b/bom/pom.xml
@@ -65,7 +65,7 @@
 
   <properties>
     <!-- Keep in sync with ../pom.xml -->
-    <tcnative.version>2.0.54.Final</tcnative.version>
+    <tcnative.version>2.0.55.Final</tcnative.version>
   </properties>
 
   <build>
diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java
@@ -2627,7 +2627,7 @@ public int getPeerPort() {
 
         @Override
         public int getPacketBufferSize() {
-            return maxEncryptedPacketLength();
+            return SSL.SSL_MAX_ENCRYPTED_LENGTH;
         }
 
         @Override
diff --git a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java
@@ -1645,6 +1645,10 @@ protected void handshake(BufferType type, boolean delegate, SSLEngine clientEngi
                 if (isHandshakeFinished(clientResult)) {
                     clientHandshakeFinished = true;
                 }
+
+                if (clientResult.getStatus() == Status.BUFFER_OVERFLOW) {
+                    cTOs = increaseDstBuffer(clientEngine.getSession().getPacketBufferSize(), type, cTOs);
+                }
             }
 
             if (!serverHandshakeFinished) {
@@ -1656,6 +1660,10 @@ protected void handshake(BufferType type, boolean delegate, SSLEngine clientEngi
                 if (isHandshakeFinished(serverResult)) {
                     serverHandshakeFinished = true;
                 }
+
+                if (serverResult.getStatus() == Status.BUFFER_OVERFLOW) {
+                    sTOc = increaseDstBuffer(serverEngine.getSession().getPacketBufferSize(), type, sTOc);
+                }
             }
 
             cTOs.flip();
@@ -1674,10 +1682,16 @@ protected void handshake(BufferType type, boolean delegate, SSLEngine clientEngi
                 runDelegatedTasks(delegate, clientResult, clientEngine);
                 assertEquals(sTOc.position() - sTOcPos, clientResult.bytesConsumed());
                 assertEquals(clientAppReadBuffer.position() - clientAppReadBufferPos, clientResult.bytesProduced());
+                assertEquals(0, clientAppReadBuffer.position());
 
                 if (isHandshakeFinished(clientResult)) {
                     clientHandshakeFinished = true;
                 }
+
+                if (clientResult.getStatus() == Status.BUFFER_OVERFLOW) {
+                    clientAppReadBuffer = increaseDstBuffer(
+                            clientEngine.getSession().getApplicationBufferSize(), type, clientAppReadBuffer);
+                }
             } else {
                 assertEquals(0, sTOc.remaining());
             }
@@ -1688,26 +1702,59 @@ protected void handshake(BufferType type, boolean delegate, SSLEngine clientEngi
                 runDelegatedTasks(delegate, serverResult, serverEngine);
                 assertEquals(cTOs.position() - cTOsPos, serverResult.bytesConsumed());
                 assertEquals(serverAppReadBuffer.position() - serverAppReadBufferPos, serverResult.bytesProduced());
+                assertEquals(0, serverAppReadBuffer.position());
 
                 if (isHandshakeFinished(serverResult)) {
                     serverHandshakeFinished = true;
                 }
+
+                if (serverResult.getStatus() == Status.BUFFER_OVERFLOW) {
+                    serverAppReadBuffer = increaseDstBuffer(
+                            serverEngine.getSession().getApplicationBufferSize(), type, serverAppReadBuffer);
+                }
             } else {
                 assertFalse(cTOs.hasRemaining());
             }
 
-            cTOsHasRemaining = cTOs.hasRemaining();
-            sTOcHasRemaining = sTOc.hasRemaining();
+            cTOsHasRemaining = compactOrClear(cTOs);
+            sTOcHasRemaining = compactOrClear(sTOc);
+
+            serverAppReadBuffer.clear();
+            clientAppReadBuffer.clear();
 
-            sTOc.compact();
-            cTOs.compact();
+            if (clientEngine.getHandshakeStatus() == SSLEngineResult.HandshakeStatus.NOT_HANDSHAKING) {
+                clientHandshakeFinished = true;
+            }
+
+            if (serverEngine.getHandshakeStatus() == SSLEngineResult.HandshakeStatus.NOT_HANDSHAKING) {
+                serverHandshakeFinished = true;
+            }
         } while (!clientHandshakeFinished || !serverHandshakeFinished ||
                 // We need to ensure we feed all the data to the engine to not end up with a corrupted state.
                 // This is especially important with TLS1.3 which may produce sessions after the "main handshake" is
                 // done
                 cTOsHasRemaining || sTOcHasRemaining);
     }
 
+    private static boolean compactOrClear(ByteBuffer buffer) {
+        if (buffer.hasRemaining()) {
+            buffer.compact();
+            return true;
+        }
+        buffer.clear();
+        return false;
+    }
+
+    private ByteBuffer increaseDstBuffer(int maxBufferSize,
+                                                     BufferType type, ByteBuffer dstBuffer) {
+        assumeFalse(maxBufferSize == dstBuffer.remaining());
+        // We need to increase the destination buffer
+        dstBuffer.flip();
+        ByteBuffer tmpBuffer = allocateBuffer(type, maxBufferSize + dstBuffer.remaining());
+        tmpBuffer.put(dstBuffer);
+        return tmpBuffer;
+    }
+
     private static boolean isHandshakeFinished(SSLEngineResult result) {
         return result.getHandshakeStatus() == SSLEngineResult.HandshakeStatus.FINISHED;
     }
@@ -4243,6 +4290,40 @@ public void testInvalidSNIIsIgnoredAndNotThrow(SSLEngineTestParam param) throws
         }
     }
 
+    @MethodSource("newTestParams")
+    @ParameterizedTest
+    public void testBufferUnderflowPacketSizeDependency(SSLEngineTestParam param) throws Exception {
+        SelfSignedCertificate ssc = new SelfSignedCertificate();
+        clientSslCtx = wrapContext(param, SslContextBuilder.forClient()
+                .keyManager(ssc.certificate(), ssc.privateKey())
+                .trustManager((TrustManagerFactory) null)
+                .sslProvider(sslClientProvider())
+                .sslContextProvider(clientSslContextProvider())
+                .protocols(param.protocols())
+                .ciphers(param.ciphers())
+                .build());
+        serverSslCtx = wrapContext(param, SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey())
+                .sslProvider(sslServerProvider())
+                .sslContextProvider(serverSslContextProvider())
+                .protocols(param.protocols())
+                .ciphers(param.ciphers())
+                .clientAuth(ClientAuth.REQUIRE)
+                .build());
+        SSLEngine clientEngine = null;
+        SSLEngine serverEngine = null;
+        try {
+            clientEngine = wrapEngine(clientSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT));
+            serverEngine = wrapEngine(serverSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT));
+
+            handshake(param.type(), param.delegate(), clientEngine, serverEngine);
+        } catch (SSLHandshakeException expected) {
+            // Expected
+        } finally {
+            cleanupClientSslEngine(clientEngine);
+            cleanupServerSslEngine(serverEngine);
+        }
+    }
+
     protected SSLEngine wrapEngine(SSLEngine engine) {
         return engine;
     }
diff --git a/pom.xml b/pom.xml
@@ -598,7 +598,7 @@
     <os.detection.classifierWithLikes>fedora,suse,arch</os.detection.classifierWithLikes>
     <tcnative.artifactId>netty-tcnative</tcnative.artifactId>
     <!-- Keep in sync with bom/pom.xml -->
-    <tcnative.version>2.0.54.Final</tcnative.version>
+    <tcnative.version>2.0.55.Final</tcnative.version>
     <tcnative.classifier>${os.detected.classifier}</tcnative.classifier>
     <conscrypt.groupId>org.conscrypt</conscrypt.groupId>
     <conscrypt.artifactId>conscrypt-openjdk-uber</conscrypt.artifactId>

Original file line number	Diff line number	Diff line change
`@@ -2627,7 +2627,7 @@ public int getPeerPort() {`
`2627`	`2627`
`2628`	`2628`	`@Override`
`2629`	`2629`	`public int getPacketBufferSize() {`
`2630`		`- return maxEncryptedPacketLength();`
	`2630`	`+ return SSL.SSL_MAX_ENCRYPTED_LENGTH;`
`2631`	`2631`	`}`
`2632`	`2632`
`2633`	`2633`	`@Override`