From 9ee1aaf3d38b15b53975efa9bc3528c7591e461d Mon Sep 17 00:00:00 2001 From: Pete Bentley Date: Fri, 1 May 2020 19:19:08 +0100 Subject: [PATCH] Throw SSLException if SSLEngine inbound is closed before outbound. Fixes #839. This should be _mostly_ uncontroversial as it is already documented to do so[1] but could cause app compat issues. A quick scan of AOSP suggests no major issues however there is a CTS test for the old behaviour[2] which will need changing. The bulk of this change is regression tests for the correct behaviour for the various possible orderings of close calls and TLS close alerts. The behaviour change test is closingInboundBeforeClosingOutboundShouldFail() in place of closingInboundShouldOnlyCloseInbound(). Changes outside ConscryptEngineTest are minimal. Close behaviour before handshaking starts is undefined and we differ from the RI, but I don't think that's problematic. Obviously also needs documenting in Conscrypt and Android release notes. This also means that STATE_CLOSED_INBOUND is never reached, which means it can be eliminated in a future CL allowing some minor simplifications. NB This can be merged independently of #844 and I'll rebase that change on top of it. [1] https://developer.android.com/reference/javax/net/ssl/SSLEngine#closeInbound() [2] https://cs.android.com/android/platform/superproject/+/master:libcore/harmony-tests/src/test/java/org/apache/harmony/tests/javax/net/ssl/SSLEngineTest.java;l=611 --- .../java/org/conscrypt/ConscryptEngine.java | 6 +- .../org/conscrypt/ConscryptEngineSocket.java | 4 +- .../org/conscrypt/ConscryptEngineTest.java | 175 +++++++++++++++++- .../javax/net/ssl/TestSSLEnginePair.java | 2 +- 4 files changed, 178 insertions(+), 9 deletions(-) diff --git a/common/src/main/java/org/conscrypt/ConscryptEngine.java b/common/src/main/java/org/conscrypt/ConscryptEngine.java index 8df11b5b0..d5cbbaf68 100644 --- a/common/src/main/java/org/conscrypt/ConscryptEngine.java +++ b/common/src/main/java/org/conscrypt/ConscryptEngine.java @@ -457,7 +457,7 @@ private void beginHandshakeInternal() throws SSLException { } @Override - public void closeInbound() { + public void closeInbound() throws SSLException { synchronized (ssl) { if (state == STATE_CLOSED || state == STATE_CLOSED_INBOUND) { return; @@ -466,7 +466,7 @@ public void closeInbound() { if (state == STATE_CLOSED_OUTBOUND) { transitionTo(STATE_CLOSED); } else { - transitionTo(STATE_CLOSED_INBOUND); + throw new SSLException("Closed SSLEngine inbound before outbound"); } freeIfDone(); } else { @@ -1337,7 +1337,7 @@ private SSLEngineResult.Status getEngineStatus() { } } - private void closeAll() { + private void closeAll() throws SSLException { closeOutbound(); closeInbound(); } diff --git a/common/src/main/java/org/conscrypt/ConscryptEngineSocket.java b/common/src/main/java/org/conscrypt/ConscryptEngineSocket.java index 8c3cfa861..688df6cfb 100644 --- a/common/src/main/java/org/conscrypt/ConscryptEngineSocket.java +++ b/common/src/main/java/org/conscrypt/ConscryptEngineSocket.java @@ -451,9 +451,9 @@ public final void close() throws IOException { super.close(); } finally { // Close the engine. - engine.closeInbound(); engine.closeOutbound(); - + engine.closeInbound(); + // Release any resources we're holding if (in != null) { in.release(); diff --git a/openjdk/src/test/java/org/conscrypt/ConscryptEngineTest.java b/openjdk/src/test/java/org/conscrypt/ConscryptEngineTest.java index f10c388ec..a58aaf5b8 100644 --- a/openjdk/src/test/java/org/conscrypt/ConscryptEngineTest.java +++ b/openjdk/src/test/java/org/conscrypt/ConscryptEngineTest.java @@ -26,6 +26,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.same; import static org.mockito.Mockito.when; @@ -176,22 +177,166 @@ public void closingOutboundAfterHandshakeShouldOnlyCloseOutbound() throws Except } @Test - public void closingInboundShouldOnlyCloseInbound() throws Exception { + public void closingInboundBeforeHandshakeShouldCloseAll() throws Exception { + setupEngines(TestKeyStore.getClient(), TestKeyStore.getServer()); + assertFalse(clientEngine.isInboundDone()); + assertFalse(clientEngine.isOutboundDone()); + assertFalse(serverEngine.isInboundDone()); + assertFalse(serverEngine.isOutboundDone()); + + clientEngine.closeInbound(); + serverEngine.closeInbound(); + + assertTrue(clientEngine.isInboundDone()); + assertTrue(clientEngine.isOutboundDone()); + assertTrue(serverEngine.isInboundDone()); + assertTrue(serverEngine.isOutboundDone()); + } + + @Test + public void closingInboundBeforeClosingOutboundShouldFail() throws Exception { setupEngines(TestKeyStore.getClient(), TestKeyStore.getServer()); doHandshake(true); + try { + clientEngine.closeInbound(); + fail(); + } catch (SSLException e) { + // Expected + } + + try { + serverEngine.closeInbound(); + fail(); + } catch (SSLException e) { + // Expected + } + assertFalse(clientEngine.isInboundDone()); assertFalse(clientEngine.isOutboundDone()); assertFalse(serverEngine.isInboundDone()); assertFalse(serverEngine.isOutboundDone()); + } + + @Test + public void closingInboundAfterClosingOutboundShouldSucceed_NoAlerts() throws Exception { + // Client and server both close inbound after closing outbound without seeing the other's + // close alert (e.g. due to the transport being closed already). + setupEngines(TestKeyStore.getClient(), TestKeyStore.getServer()); + doHandshake(true); + clientEngine.closeOutbound(); + serverEngine.closeOutbound(); + // After closeOutbound, wrap should produce the TLS close alerts. + assertTrue(wrapClosed(clientEngine).hasRemaining()); + assertTrue(wrapClosed(serverEngine).hasRemaining()); + + assertFalse(clientEngine.isInboundDone()); + assertTrue(clientEngine.isOutboundDone()); + assertFalse(serverEngine.isInboundDone()); + assertTrue(serverEngine.isOutboundDone()); + + // Closing inbound should now succeed and produce no new handshake data. clientEngine.closeInbound(); serverEngine.closeInbound(); + assertFalse(wrapClosed(clientEngine).hasRemaining()); + assertFalse(wrapClosed(serverEngine).hasRemaining()); + // Final state, everything closed. assertTrue(clientEngine.isInboundDone()); - assertFalse(clientEngine.isOutboundDone()); + assertTrue(clientEngine.isOutboundDone()); assertTrue(serverEngine.isInboundDone()); - assertFalse(serverEngine.isOutboundDone()); + assertTrue(serverEngine.isOutboundDone()); + } + + @Test + public void closingInboundAfterClosingOutboundShouldSucceed_AlertsArriveBeforeCloseInbound() + throws Exception { + // Client and server both call closeOutbound simultaneously. The alerts produced + // by this both arrive at the peer before it calls closeInbound, making it a no-op. + setupEngines(TestKeyStore.getClient(), TestKeyStore.getServer()); + doHandshake(true); + + clientEngine.closeOutbound(); + // After closeOutbound, wrap should produce a single output buffer with the TLS close alert + ByteBuffer clientOutput = wrapClosed(clientEngine); + assertTrue(clientOutput.hasRemaining()); + + // Unwrapping that on the server engine should cause it to close and send its own alert + // but produce no plaintext output. + assertFalse(unwrapClosed(serverEngine, clientOutput).hasRemaining()); + ByteBuffer serverOutput = wrapClosed(serverEngine); + assertTrue(serverOutput.hasRemaining()); + + // At his point, server should be fully closed after processing client's alert and sending + // its own. Client inbound is still open. + assertFalse(clientEngine.isInboundDone()); + assertTrue(clientEngine.isOutboundDone()); + assertTrue(serverEngine.isInboundDone()); + assertTrue(serverEngine.isOutboundDone()); + + // Explicitly closing the server outbound should produce no new extra handshake data + serverEngine.closeOutbound(); + assertFalse(wrapClosed(serverEngine).hasRemaining()); + + // Unwrapping the server output on the client should cause it to close as above + // but produce no new handshaking data and no new plaintext. + assertFalse(unwrapClosed(clientEngine, serverOutput).hasRemaining()); + assertFalse(wrapClosed(clientEngine).hasRemaining()); + + // Everything should be fully closed by this point. + assertTrue(clientEngine.isInboundDone()); + assertTrue(clientEngine.isOutboundDone()); + assertTrue(serverEngine.isInboundDone()); + assertTrue(serverEngine.isOutboundDone()); + + // Closing inbounds should now be a no-op and also produce no new handshake data. + clientEngine.closeInbound(); + serverEngine.closeInbound(); + assertFalse(wrapClosed(clientEngine).hasRemaining()); + assertFalse(wrapClosed(serverEngine).hasRemaining()); + } + + @Test + public void closingInboundAfterClosingOutboundShouldSucceed_AlertsArriveAfterCloseInbound() + throws Exception { + // Client and server both call closeOutbound simultaneously. The alerts produced + // by this both arrive at the peer after it calls closeInbound. Verify that + // this produces no plainttext or further handshake data. + setupEngines(TestKeyStore.getClient(), TestKeyStore.getServer()); + doHandshake(true); + + // After closeOutbound, wrap should produce a single output buffer with the TLS close alert + clientEngine.closeOutbound(); + ByteBuffer clientOutput = wrapClosed(clientEngine); + assertTrue(clientOutput.hasRemaining()); + serverEngine.closeOutbound(); + ByteBuffer serverOutput = wrapClosed(serverEngine); + assertTrue(serverOutput.hasRemaining()); + + assertFalse(clientEngine.isInboundDone()); + assertTrue(clientEngine.isOutboundDone()); + assertFalse(serverEngine.isInboundDone()); + assertTrue(serverEngine.isOutboundDone()); + + // Closing inbounds should now succeed and produce no new handshake data. + clientEngine.closeInbound(); + serverEngine.closeInbound(); + assertFalse(wrapClosed(clientEngine).hasRemaining()); + assertFalse(wrapClosed(serverEngine).hasRemaining()); + + // Everything should be fully closed by this point. + assertTrue(clientEngine.isInboundDone()); + assertTrue(clientEngine.isOutboundDone()); + assertTrue(serverEngine.isInboundDone()); + assertTrue(serverEngine.isOutboundDone()); + + // Unwrapping the previous handshake data should also succeed, and produce no + //new plaintext or handshake data + assertFalse(unwrapClosed(serverEngine, clientOutput).hasRemaining()); + assertFalse(wrapClosed(serverEngine).hasRemaining()); + assertFalse(unwrapClosed(clientEngine, serverOutput).hasRemaining()); + assertFalse(wrapClosed(clientEngine).hasRemaining()); } @Test @@ -493,6 +638,30 @@ private List wrap(ByteBuffer input, SSLEngine engine) throws SSLExce return wrapped; } + private ByteBuffer wrapClosed(SSLEngine engine) throws SSLException { + // Call wrap with empty input and return any handshaking data produced, asserting + // the engine is already in a closed state. + ByteBuffer emptyInput = bufferType.newBuffer(0); + ByteBuffer encryptedBuffer = + bufferType.newBuffer(engine.getSession().getPacketBufferSize()); + SSLEngineResult wrapResult = engine.wrap(emptyInput, encryptedBuffer); + assertEquals(Status.CLOSED, wrapResult.getStatus()); + encryptedBuffer.flip(); + return encryptedBuffer; + } + + private ByteBuffer unwrapClosed(SSLEngine engine, ByteBuffer encrypted) throws SSLException { + // Unwrap a single buffer and return the plaintext, asserting the engine is already in a + // closed state. + final ByteBuffer decryptedBuffer + = bufferType.newBuffer(engine.getSession().getPacketBufferSize()); + + SSLEngineResult unwrapResult = engine.unwrap(encrypted, decryptedBuffer); + assertEquals(Status.CLOSED, unwrapResult.getStatus()); + decryptedBuffer.flip(); + return decryptedBuffer; + } + private byte[] unwrap(ByteBuffer[] encryptedBuffers, SSLEngine engine) throws IOException { ByteArrayOutputStream cleartextStream = new ByteArrayOutputStream(); int decryptedBufferSize = 8192; diff --git a/testing/src/main/java/org/conscrypt/javax/net/ssl/TestSSLEnginePair.java b/testing/src/main/java/org/conscrypt/javax/net/ssl/TestSSLEnginePair.java index a693e9d00..3554bb2fa 100644 --- a/testing/src/main/java/org/conscrypt/javax/net/ssl/TestSSLEnginePair.java +++ b/testing/src/main/java/org/conscrypt/javax/net/ssl/TestSSLEnginePair.java @@ -146,8 +146,8 @@ public static void close(SSLEngine[] engines) { try { for (SSLEngine engine : engines) { if (engine != null) { - engine.closeInbound(); engine.closeOutbound(); + engine.closeInbound(); } } } catch (Exception e) {