Skip to content

Commit

Permalink
Throw SSLException if SSLEngine inbound is closed before outbound.
Browse files Browse the repository at this point in the history 
Fixes google#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 google#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
  • Loading branch information
@prbprbprb
prbprbprb committed May 1, 2020
1 parent e26f433 commit 9ee1aaf
Show file tree
Hide file tree
Showing 4 changed files with 178 additions and 9 deletions.
6 changes: 3 additions & 3 deletions common/src/main/java/org/conscrypt/ConscryptEngine.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand Down Expand Up @@ -1337,7 +1337,7 @@ private SSLEngineResult.Status getEngineStatus() {
}
}

private void closeAll() {
private void closeAll() throws SSLException {
closeOutbound();
closeInbound();
}
Expand Down
4 changes: 2 additions & 2 deletions common/src/main/java/org/conscrypt/ConscryptEngineSocket.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
175 changes: 172 additions & 3 deletions openjdk/src/test/java/org/conscrypt/ConscryptEngineTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -493,6 +638,30 @@ private List<ByteBuffer> 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;
Expand Down
2 changes: 1 addition & 1 deletion testing/src/main/java/org/conscrypt/javax/net/ssl/TestSSLEnginePair.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down

0 comments on commit 9ee1aaf

Please sign in to comment.