From d4371b0dd31eda214432d974587af118770de8a2 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Wed, 4 Jan 2023 16:39:43 +0100 Subject: [PATCH 1/9] Return correct value from SSLSession.getPacketSize() when using native SSL implementation 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 https://github.com/netty/netty/issues/13073 --- bom/pom.xml | 2 +- .../ssl/ReferenceCountedOpenSslEngine.java | 2 +- .../io/netty/handler/ssl/SSLEngineTest.java | 54 +++++++++++++++++++ pom.xml | 2 +- 4 files changed, 57 insertions(+), 3 deletions(-) diff --git a/bom/pom.xml b/bom/pom.xml index ac353101cc1..ee59a02391c 100644 --- a/bom/pom.xml +++ b/bom/pom.xml @@ -65,7 +65,7 @@ - 2.0.54.Final + 2.0.55.Final diff --git a/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java b/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java index 8347c8ba0dc..d737fd8f351 100644 --- 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 index 6a187fded36..7220eca2837 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java @@ -1677,6 +1677,9 @@ protected void handshake(BufferType type, boolean delegate, SSLEngine clientEngi if (isHandshakeFinished(clientResult)) { clientHandshakeFinished = true; + } else { + clientAppReadBuffer = increaseAppReadBufferIfNeeded( + clientEngine, clientResult, type, clientAppReadBuffer); } } else { assertEquals(0, sTOc.remaining()); @@ -1691,6 +1694,9 @@ protected void handshake(BufferType type, boolean delegate, SSLEngine clientEngi if (isHandshakeFinished(serverResult)) { serverHandshakeFinished = true; + } else { + serverAppReadBuffer = increaseAppReadBufferIfNeeded( + serverEngine, serverResult, type, serverAppReadBuffer); } } else { assertFalse(cTOs.hasRemaining()); @@ -1708,6 +1714,20 @@ protected void handshake(BufferType type, boolean delegate, SSLEngine clientEngi cTOsHasRemaining || sTOcHasRemaining); } + private ByteBuffer increaseAppReadBufferIfNeeded(SSLEngine engine, SSLEngineResult result, + BufferType type, ByteBuffer dstBuffer) { + if (result.getStatus() == Status.BUFFER_OVERFLOW) { + // We need to increase the destination buffer + int appSize = engine.getSession().getApplicationBufferSize(); + assertNotEquals(appSize, dstBuffer.capacity()); + dstBuffer.flip(); + ByteBuffer tmpBuffer = allocateBuffer(type, appSize + dstBuffer.remaining()); + tmpBuffer.put(dstBuffer); + return tmpBuffer; + } + return dstBuffer; + } + private static boolean isHandshakeFinished(SSLEngineResult result) { return result.getHandshakeStatus() == SSLEngineResult.HandshakeStatus.FINISHED; } @@ -4243,6 +4263,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 index d8f433032cc..e89676d571a 100644 --- a/pom.xml +++ b/pom.xml @@ -598,7 +598,7 @@ fedora,suse,arch netty-tcnative - 2.0.54.Final + 2.0.55.Final ${os.detected.classifier} org.conscrypt conscrypt-openjdk-uber From 7b17cf39d87c9cd1b8a8774ce4559ec9b4f767f1 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Thu, 5 Jan 2023 08:50:33 +0100 Subject: [PATCH 2/9] Also handle overflow for wrap --- .../io/netty/handler/ssl/SSLEngineTest.java | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java index 7220eca2837..9338a4a4a6a 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java @@ -1644,6 +1644,9 @@ protected void handshake(BufferType type, boolean delegate, SSLEngine clientEngi if (isHandshakeFinished(clientResult)) { clientHandshakeFinished = true; + } else { + cTOs = increaseDstBufferIfNeeded(clientResult, + clientEngine.getSession().getPacketBufferSize(), type, cTOs); } } @@ -1655,6 +1658,9 @@ protected void handshake(BufferType type, boolean delegate, SSLEngine clientEngi if (isHandshakeFinished(serverResult)) { serverHandshakeFinished = true; + } else { + sTOc = increaseDstBufferIfNeeded(serverResult, + serverEngine.getSession().getPacketBufferSize(), type, sTOc); } } @@ -1678,8 +1684,8 @@ protected void handshake(BufferType type, boolean delegate, SSLEngine clientEngi if (isHandshakeFinished(clientResult)) { clientHandshakeFinished = true; } else { - clientAppReadBuffer = increaseAppReadBufferIfNeeded( - clientEngine, clientResult, type, clientAppReadBuffer); + clientAppReadBuffer = increaseDstBufferIfNeeded(clientResult, + clientEngine.getSession().getApplicationBufferSize(), type, clientAppReadBuffer); } } else { assertEquals(0, sTOc.remaining()); @@ -1695,8 +1701,8 @@ protected void handshake(BufferType type, boolean delegate, SSLEngine clientEngi if (isHandshakeFinished(serverResult)) { serverHandshakeFinished = true; } else { - serverAppReadBuffer = increaseAppReadBufferIfNeeded( - serverEngine, serverResult, type, serverAppReadBuffer); + serverAppReadBuffer = increaseDstBufferIfNeeded(serverResult, + serverEngine.getSession().getApplicationBufferSize(), type, serverAppReadBuffer); } } else { assertFalse(cTOs.hasRemaining()); @@ -1714,14 +1720,13 @@ protected void handshake(BufferType type, boolean delegate, SSLEngine clientEngi cTOsHasRemaining || sTOcHasRemaining); } - private ByteBuffer increaseAppReadBufferIfNeeded(SSLEngine engine, SSLEngineResult result, + private ByteBuffer increaseDstBufferIfNeeded(SSLEngineResult result, int maxBufferSize, BufferType type, ByteBuffer dstBuffer) { if (result.getStatus() == Status.BUFFER_OVERFLOW) { // We need to increase the destination buffer - int appSize = engine.getSession().getApplicationBufferSize(); - assertNotEquals(appSize, dstBuffer.capacity()); + assertNotEquals(maxBufferSize, dstBuffer.remaining()); dstBuffer.flip(); - ByteBuffer tmpBuffer = allocateBuffer(type, appSize + dstBuffer.remaining()); + ByteBuffer tmpBuffer = allocateBuffer(type, maxBufferSize + dstBuffer.remaining()); tmpBuffer.put(dstBuffer); return tmpBuffer; } From 35e6d9e11320f895ffb05bb7a7582c0f4ef03756 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Thu, 5 Jan 2023 10:07:41 +0100 Subject: [PATCH 3/9] Remove assert --- handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java index 9338a4a4a6a..0930419421c 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java @@ -1724,7 +1724,6 @@ private ByteBuffer increaseDstBufferIfNeeded(SSLEngineResult result, int maxBuff BufferType type, ByteBuffer dstBuffer) { if (result.getStatus() == Status.BUFFER_OVERFLOW) { // We need to increase the destination buffer - assertNotEquals(maxBufferSize, dstBuffer.remaining()); dstBuffer.flip(); ByteBuffer tmpBuffer = allocateBuffer(type, maxBufferSize + dstBuffer.remaining()); tmpBuffer.put(dstBuffer); From 533e7127a215f249c3c3caac912bb29d892b7bf2 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Fri, 6 Jan 2023 12:14:39 +0100 Subject: [PATCH 4/9] compact or clear --- .../java/io/netty/handler/ssl/SSLEngineTest.java | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java index 0930419421c..4c9a31563d7 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java @@ -1708,11 +1708,9 @@ protected void handshake(BufferType type, boolean delegate, SSLEngine clientEngi assertFalse(cTOs.hasRemaining()); } - cTOsHasRemaining = cTOs.hasRemaining(); - sTOcHasRemaining = sTOc.hasRemaining(); + cTOsHasRemaining = compactOrClear(cTOs); + sTOcHasRemaining = compactOrClear(sTOc); - sTOc.compact(); - cTOs.compact(); } 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 @@ -1720,6 +1718,16 @@ protected void handshake(BufferType type, boolean delegate, SSLEngine clientEngi cTOsHasRemaining || sTOcHasRemaining); } + + private static boolean compactOrClear(ByteBuffer buffer) { + if (buffer.hasRemaining()) { + buffer.compact(); + return true; + } + buffer.clear(); + return false; + } + private ByteBuffer increaseDstBufferIfNeeded(SSLEngineResult result, int maxBufferSize, BufferType type, ByteBuffer dstBuffer) { if (result.getStatus() == Status.BUFFER_OVERFLOW) { From 0014b898e9e5b57413d969a6ba59f6ac19c460f7 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Fri, 6 Jan 2023 12:18:57 +0100 Subject: [PATCH 5/9] checkstyle --- handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java index 4c9a31563d7..15846bd236c 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java @@ -1718,7 +1718,6 @@ protected void handshake(BufferType type, boolean delegate, SSLEngine clientEngi cTOsHasRemaining || sTOcHasRemaining); } - private static boolean compactOrClear(ByteBuffer buffer) { if (buffer.hasRemaining()) { buffer.compact(); From 323861bd78376955971d84d140c7f2cb3ed78f7c Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Fri, 6 Jan 2023 16:00:27 +0100 Subject: [PATCH 6/9] add assert back --- handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java index 15846bd236c..84db3f46d54 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java @@ -1730,6 +1730,7 @@ private static boolean compactOrClear(ByteBuffer buffer) { private ByteBuffer increaseDstBufferIfNeeded(SSLEngineResult result, int maxBufferSize, BufferType type, ByteBuffer dstBuffer) { if (result.getStatus() == Status.BUFFER_OVERFLOW) { + assertNotEquals(maxBufferSize, dstBuffer.remaining()); // We need to increase the destination buffer dstBuffer.flip(); ByteBuffer tmpBuffer = allocateBuffer(type, maxBufferSize + dstBuffer.remaining()); From 9f617a42e73d62c3227710aa99814704f9c90f70 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Fri, 6 Jan 2023 16:40:09 +0100 Subject: [PATCH 7/9] cleanup --- .../io/netty/handler/ssl/SSLEngineTest.java | 56 ++++++++++++------- 1 file changed, 35 insertions(+), 21 deletions(-) diff --git a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java index 84db3f46d54..aff0c79e795 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java @@ -1644,9 +1644,10 @@ protected void handshake(BufferType type, boolean delegate, SSLEngine clientEngi if (isHandshakeFinished(clientResult)) { clientHandshakeFinished = true; - } else { - cTOs = increaseDstBufferIfNeeded(clientResult, - clientEngine.getSession().getPacketBufferSize(), type, cTOs); + } + + if (clientResult.getStatus() == Status.BUFFER_OVERFLOW) { + cTOs = increaseDstBuffer(clientEngine.getSession().getPacketBufferSize(), type, cTOs); } } @@ -1658,9 +1659,10 @@ protected void handshake(BufferType type, boolean delegate, SSLEngine clientEngi if (isHandshakeFinished(serverResult)) { serverHandshakeFinished = true; - } else { - sTOc = increaseDstBufferIfNeeded(serverResult, - serverEngine.getSession().getPacketBufferSize(), type, sTOc); + } + + if (serverResult.getStatus() == Status.BUFFER_OVERFLOW) { + sTOc = increaseDstBuffer(serverEngine.getSession().getPacketBufferSize(), type, sTOc); } } @@ -1680,11 +1682,14 @@ 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; - } else { - clientAppReadBuffer = increaseDstBufferIfNeeded(clientResult, + } + + if (clientResult.getStatus() == Status.BUFFER_OVERFLOW) { + clientAppReadBuffer = increaseDstBuffer( clientEngine.getSession().getApplicationBufferSize(), type, clientAppReadBuffer); } } else { @@ -1697,12 +1702,14 @@ 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; - } else { - serverAppReadBuffer = increaseDstBufferIfNeeded(serverResult, - serverEngine.getSession().getApplicationBufferSize(), type, serverAppReadBuffer); + } + + if (serverResult.getStatus() == Status.BUFFER_OVERFLOW) { + serverAppReadBuffer = increaseDstBuffer(serverEngine.getSession().getApplicationBufferSize(), type, serverAppReadBuffer); } } else { assertFalse(cTOs.hasRemaining()); @@ -1711,6 +1718,16 @@ protected void handshake(BufferType type, boolean delegate, SSLEngine clientEngi cTOsHasRemaining = compactOrClear(cTOs); sTOcHasRemaining = compactOrClear(sTOc); + serverAppReadBuffer.clear(); + clientAppReadBuffer.clear(); + + 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 @@ -1727,17 +1744,14 @@ private static boolean compactOrClear(ByteBuffer buffer) { return false; } - private ByteBuffer increaseDstBufferIfNeeded(SSLEngineResult result, int maxBufferSize, + private ByteBuffer increaseDstBuffer(int maxBufferSize, BufferType type, ByteBuffer dstBuffer) { - if (result.getStatus() == Status.BUFFER_OVERFLOW) { - assertNotEquals(maxBufferSize, dstBuffer.remaining()); - // We need to increase the destination buffer - dstBuffer.flip(); - ByteBuffer tmpBuffer = allocateBuffer(type, maxBufferSize + dstBuffer.remaining()); - tmpBuffer.put(dstBuffer); - return tmpBuffer; - } - return dstBuffer; + assertNotEquals(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) { From d53f87a220878d7593f365ea1619771f15929f49 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Fri, 6 Jan 2023 16:44:47 +0100 Subject: [PATCH 8/9] checkstyle --- handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java index aff0c79e795..c87d74ceb67 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java @@ -1709,7 +1709,8 @@ protected void handshake(BufferType type, boolean delegate, SSLEngine clientEngi } if (serverResult.getStatus() == Status.BUFFER_OVERFLOW) { - serverAppReadBuffer = increaseDstBuffer(serverEngine.getSession().getApplicationBufferSize(), type, serverAppReadBuffer); + serverAppReadBuffer = increaseDstBuffer( + serverEngine.getSession().getApplicationBufferSize(), type, serverAppReadBuffer); } } else { assertFalse(cTOs.hasRemaining()); From a4cb7c030b418233ff2590dd55394c1fbf4bae18 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Fri, 6 Jan 2023 17:27:44 +0100 Subject: [PATCH 9/9] Use assume --- handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java index c87d74ceb67..b6bdd5c26a2 100644 --- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java +++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java @@ -1747,7 +1747,7 @@ private static boolean compactOrClear(ByteBuffer buffer) { private ByteBuffer increaseDstBuffer(int maxBufferSize, BufferType type, ByteBuffer dstBuffer) { - assertNotEquals(maxBufferSize, dstBuffer.remaining()); + assumeFalse(maxBufferSize == dstBuffer.remaining()); // We need to increase the destination buffer dstBuffer.flip(); ByteBuffer tmpBuffer = allocateBuffer(type, maxBufferSize + dstBuffer.remaining());