Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throw SSLException if SSLEngine inbound is closed before outbound. #845

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Throw SSLException if SSLEngine inbound is closed before outbound. #845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4a57a16
Throw SSLException if SSLEngine inbound is closed before outbound.
prbprbprb May 1, 2020
bf98bf9
Merge branch 'master' into engine_close_inbound
prbprbprb Jun 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions common/src/main/java/org/conscrypt/ConscryptEngine.java
View file
Open in desktop
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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the documentation indicates at all that this will throw SSLException if you don't call closeOutbound first. This will probably cause you a lot of app compat headaches; it's already difficult enough for app developers to use SSLEngine correctly and this seems to add to the gotchas.

Also concerning is that the stated reason for throwing does not seem to be covered:

@throws SSLException if this engine has not received the proper SSL/TLS close notification message from the peer.

Is this handled implicitly by the state transitions in reaction to BoringSSL's signal that it received end-of-stream from the other side?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, my wording is a poor there, I'll fix it... I carried it over from the bug report.

Both peers are required to send close alerts at the end of the application data stream and not to close their inbound connection until they have received one from the peer. RFC 5246 also states that on receipt of a close alert, the connection transitions to fully closed and sends its own alert and "If the application protocol will not transfer any additional data, but will only close the underlying transport connection, then the implementation MAY choose to close the transport without waiting for the responding close_notify", which is what ConscryptEngine does.

So, if an app has called closeOutbound or a close alert has been received from the peer then it's OK to call closeInbound. The bulk of the tests codify that, but need better wording/docs I think.

However I didn't realise the spec changed for TLS 1.3 and now says "Each party MUST send a "close_notify" alert before closing its write side of the connection, unless it has already sent some error alert. This does not have any effect on its read side of the connection. Note that this is a change from versions of TLS prior to TLS 1.3 in which implementations were required to react to a "close_notify" by discarding pending writes and sending an immediate "close_notify" alert of their own" - that doesn't change the requirement to send a close_notify before closing the inbound connection, but it might mean we need to revisit the way we respond to incoming close alerts in a future change.

And of course this is related to PR #844 where ConscryptEngineSocket prevents the close alerts from flowing anyway by closing the underlying socket too soon.

Agree about app compat issues though, and so this change should be gated in some way. I'm designing something so we can gate on targetSdkLevel in the Android platform and some other criterion (using the same Annotations) on other platforms. Meanwhile I'll move most of the tests to a separate PR and clarify the wording.

}
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
Open in desktop
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
178 changes: 175 additions & 3 deletions openjdk/src/test/java/org/conscrypt/ConscryptEngineTest.java
View file
Open in desktop
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,9 +177,8 @@ public void closingOutboundAfterHandshakeShouldOnlyCloseOutbound() throws Except
}

@Test
public void closingInboundShouldOnlyCloseInbound() throws Exception {
public void closingInboundBeforeHandshakeShouldCloseAll() throws Exception {
setupEngines(TestKeyStore.getClient(), TestKeyStore.getServer());
doHandshake(true);

assertFalse(clientEngine.isInboundDone());
assertFalse(clientEngine.isOutboundDone());
Expand All @@ -189,11 +189,159 @@ public void closingInboundShouldOnlyCloseInbound() throws Exception {
serverEngine.closeInbound();

assertTrue(clientEngine.isInboundDone());
assertFalse(clientEngine.isOutboundDone());
assertTrue(clientEngine.isOutboundDone());
assertTrue(serverEngine.isInboundDone());
assertTrue(serverEngine.isOutboundDone());
}

@Test
public void closingInboundBeforeOutboundIsClosedShouldFail() 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());
assertTrue(clientEngine.isOutboundDone());
assertTrue(serverEngine.isInboundDone());
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, causing the
// connection to be closed immediately (RFC 2246, §7.2.1) and so the
// closeInbound call should be a no-op which generates no data.
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
// unwrapping the alerts produces no further 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
public void mutualAuthWithSameCertsShouldSucceed() throws Exception {
doMutualAuthHandshake(TestKeyStore.getServer(), TestKeyStore.getServer(), ClientAuth.NONE);
Expand Down Expand Up @@ -493,6 +641,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
Open in desktop
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