From e530cd610d17cdaaca7f3adf5e7c8d224571e89c Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Mon, 9 Jan 2023 09:48:06 +0100 Subject: [PATCH] Return correct value from SSLSession.getPacketSize() when using native SSL implementation (#13095) 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 | 89 ++++++++++++++++++- pom.xml | 2 +- 4 files changed, 88 insertions(+), 7 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..b6bdd5c26a2 100644 --- 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,19 +1702,33 @@ 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 @@ -1708,6 +1736,25 @@ 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 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 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