Swiftdiff options
| author | Nick Hudson <nick.hudson@isode.com> | 2013-08-12 14:12:41 (GMT) |
|---|---|---|
| committer | Nick Hudson <nick.hudson@isode.com> | 2013-09-02 16:19:08 (GMT) |
| commit | d10646423d70934b598b5d8ff2d8910034941029 (patch) | |
| tree | 982621f5c98459fb477e753f0a1589817df8d258 /src/com/isode | |
| parent | 34e195e8e2db1de0b71460afcce2417d34fc7717 (diff) | |
| download | stroke-d10646423d70934b598b5d8ff2d8910034941029.zip stroke-d10646423d70934b598b5d8ff2d8910034941029.tar.bz2 | |
Changes to handle close notify gracefully
It was noticed that in certain cases, Stroke got stuck when connecting
to a server over TLS, and the server closed down the connection.
Investigation showed that this appears to be caused by the JSSEContext
code not properly coping with a "close notify" SSL message. What
happens in this case is that the SSLEngine generates a response to the
server's "close notify", and expects this to be sent back over the
network.
The original JSSEContext code saw the "CLOSED" status from the
SSLEngine.unwrap(), but assumed that no more data would be generated
by the engine (which was wrong, because the engine wants to send a close
response back), and so got stuck in a loop.
This patch therefore fixes the JSSEContext code to deal properly when
it sees a CLOSED from unwrap.
After the close has been received, an error will be emitted by
JSSEContext so that the application knows that the SSLEngine can no
longer be used (in practice we have always seen the socket closing,
which generates its own error to the application, but it was
recommended that we should have this check in case a server sends a
close notify and does NOT close the socket as well).
It appears that many servers don't actually send the "close notify",
and just drop the connection, which is (presumably) why we'd not seen
this behaviour before.
Test-information:
Tested by connecting to the aforementioned server. This time, when
the connection times out (and the closenotify is sent), we no longer
see a loop, but the application realises what's happens and attempts
to reconnect.
I have been running with this patch in my copy of MLC for two weeks
and have noticed no difference in behaviour - so far as I can tell the
code is not exercised when talking to M-Link but at any rate the patch
isn't causing anything to break.
Change-Id: Id007c923c510ef1b4ce53192105b00296c65c757
Diffstat (limited to 'src/com/isode')
| -rw-r--r-- | src/com/isode/stroke/tls/java/JSSEContext.java | 86 |
1 files changed, 66 insertions, 20 deletions
diff --git a/src/com/isode/stroke/tls/java/JSSEContext.java b/src/com/isode/stroke/tls/java/JSSEContext.java index 8c351bb..77b9447 100644 --- a/src/com/isode/stroke/tls/java/JSSEContext.java +++ b/src/com/isode/stroke/tls/java/JSSEContext.java @@ -90,7 +90,12 @@ public class JSSEContext extends TLSContext { * Whether the handshake has finished */ private boolean handshakeCompleted = false; - + + /** + * Whether the server has sent a close notification + */ + private boolean closeNotifyReceived = false; + /** * Determine whether an error has occurred. If an error has occurred, then * you probably don't want to try doing any more stuff. @@ -280,12 +285,26 @@ public class JSSEContext extends TLSContext { */ unwrapDone = true; break; - case CLOSED: - /* Engine closed - don't expect this here */ - emitError(null, "SSLEngine.unwrap returned " + status); - return bytesConsumed; - - case OK: + case CLOSED: + /* This is taken to mean that the server end has + * sent "SSL close notify alert". Once it sees this, + * ths SSLEngine will respond by generating an + * appropriate handshake response that should be + * sent to the server (as per RFC 2246 7.2.1). + * In this case, the SSLEngine will move into a + * NEED_WRAP state, and we should emit whatever data + * it's generated over the network. + */ + closeNotifyReceived = true; + + /* Tell the SSLEngine that the application won't be + * sending any more data (this probably has no effect + * but it does no harm). + */ + sslEngine.closeOutbound(); + return bytesConsumed; + + case OK: /* Some stuff was unwrapped. */ bytesConsumed += sslEngineResult.bytesConsumed(); bytesProduced = sslEngineResult.bytesProduced(); @@ -381,15 +400,16 @@ public class JSSEContext extends TLSContext { */ synchronized(sendMutex) { plainToSend.flip(); - /* If the SSL handshake hasn't completed, then there may not be - * any data from the client in plainToSend - */ - if (!plainToSend.hasRemaining() && handshakeCompleted) { - /* Nothing more to be encrypted */ - plainToSend.compact(); - return bytesSentToSocket; - } - try { + /* It does no harm to call SSLEngine.wrap if there is nothing in + * "plainToSend" and this will be required in at least two + * cases: + * - during the initial handshake, when the application hasn't + * yet sent anything, but wrap() must be called to generate + * the TLS handshake data + * - during closure, when wrap() will generate the response to + * the server's close notify message + */ + try { boolean wrapDone = false; do { sslEngineResult = sslEngine.wrap(plainToSend, wrappedToSend); @@ -428,9 +448,21 @@ public class JSSEContext extends TLSContext { } switch (status) { + case CLOSED : + /* Engine closed - this is expected if a close notify has + * been sent by the server, and the SSLEngine has finished + * generating the response to that message. + */ + if (!closeNotifyReceived) { + emitError(null, "SSLEngine.wrap returned " + status); + return (bytesSentToSocket); + } + /* CLOSED was expected, so fall through to OK, in order + * to send the close response back to the server + */ case OK: - /* This is the only status we expect here. It means the - * data was successfully wrapped and that there's something + /* This is the status we expect here. It means data + * was successfully wrapped and that there's something * to be sent. */ wrappedToSend.flip(); @@ -444,8 +476,6 @@ public class JSSEContext extends TLSContext { case BUFFER_UNDERFLOW: /* Can't happen for a wrap */ - case CLOSED : - /* Engine closed - don't expect this here */ case BUFFER_OVERFLOW: /* We already dealt with this, so don't expect to come here */ @@ -935,6 +965,15 @@ public class JSSEContext extends TLSContext { /* */ } while (processHandshakeStatus()); + if (closeNotifyReceived) { + /* This is the only way to let the application know that close + * notify was received. This check is done after finishing with + * the handshake checks, so that the SSLEngine's response to + * the close notify has been dealt with. + */ + emitError(null, "SSL Close notify received"); + return; + } /* Loop round so long as there are still bytes from the network * to be processed @@ -949,6 +988,13 @@ public class JSSEContext extends TLSContext { onError.emit(); return; } + if (closeNotifyReceived) { + /* After the server has closed the TLS connection, no more + * application data should be sent. + */ + emitError(null, + "handleDataFromApplication called after SSLEngine closed"); + } byte[] b = data.getData(); /* Need to cope in the case that the application sends a ByteArray |