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) {