Fix #21760 - Close Tls connection if Inbound closed before receiving peer's close_notify (v.2) #21786
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ing peer's close_notify When application closes inboud using engine.closeInbound the engine can generate an alert message and put it into writer.outboundList. As a result the engine can have a new data packet in outbound and its isOutboundDone will be false. We have to ensure that it will be flushed to the network, that's why we have to update lastHandshakeStatus via the actual status of the engine. Otherwise the actor will transition to the flushingOutbound state but will never flush outbound, since engineNeedsWrap precondition will be false. Generally speaking whenever we signal something to the SSLEngine, weshould also read getHandshakeStatus afterwards to understand what we need to do next. This was done everywhere but for this engine.closeInbound.
|
Can one of the repo owners verify this patch? |
|
OK TO TEST |
|
Thanks for the excellent research + writeup and fix. |
akka-ci
added
needs-attention
|
Test FAILed. |
|
Does not look like the failed test is related to this commit. |
|
PLS BUILD |
akka-ci
added
validating
|
Test PASSed. |
johanandren
approved these changes
Nov 3, 2016
drewhk
approved these changes
Nov 4, 2016
| }) | ||
| .runWith(Sink.last) | ||
|
|
||
| Await.result(f, 8.second).utf8String should be(scenario.output.utf8String) |
| @@ -292,6 +292,7 @@ class TLSActor( | |||
| if (tracing) log.debug("closing inbound") | |||
| try engine.closeInbound() | |||
| catch { case ex: SSLException ⇒ outputBunch.enqueue(UserOut, SessionTruncated) } | |||
| lastHandshakeStatus = engine.getHandshakeStatus | |||
There was a problem hiding this comment.
ah, so this needs to be updated here. Thanks!
|
Enough LGTMs I believe :) |
412b
pushed a commit
to 412b/akka
that referenced
this pull request
Nov 17, 2016
…ing peer's close_notify (akka#21786) When application closes inboud using engine.closeInbound the engine can generate an alert message and put it into writer.outboundList. As a result the engine can have a new data packet in outbound and its isOutboundDone will be false. We have to ensure that it will be flushed to the network, that's why we have to update lastHandshakeStatus via the actual status of the engine. Otherwise the actor will transition to the flushingOutbound state but will never flush outbound, since engineNeedsWrap precondition will be false. Generally speaking whenever we signal something to the SSLEngine, weshould also read getHandshakeStatus afterwards to understand what we need to do next. This was done everywhere but for this engine.closeInbound.
14 tasks
jrudolph
added a commit
to jrudolph/akka-http
that referenced
this pull request
Sep 11, 2017
…cenario That is we are simulating a TLS truncation attack here. Before akka/akka#21786 the TLS stage just got stuck (I tested again an old Akka version). Now, all streams are properly closed and also the response entity stream is completed. A stricter version would actually fail the response entity stream when TLS truncation is detected. If that is worthwhile is actually questionable. See akka#235 for more information.
jrudolph
added a commit
to jrudolph/akka-http
that referenced
this pull request
Sep 11, 2017
…cenario That is we are simulating a TLS truncation attack here. Before akka/akka#21786 the TLS stage just got stuck (I tested again an old Akka version). Now, all streams are properly closed and also the response entity stream is completed. A stricter version would actually fail the response entity stream when TLS truncation is detected. If that is worthwhile is actually questionable. See akka#235 for more information.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #21760
Also fixes akka/akka-http#380 and probably akka/akka-http#87
This is a second variant of testing the fix (an alternative is #21785)
Initial discussion is in #21761
When application closes inboud using engine.closeInbound the engine can generate an alert message and put it into writer.outboundList. As a result the engine can have a new data packet in outbound and its isOutboundDone will be false. We have to ensure that it will be flushed to the network, that's why we have to update lastHandshakeStatus via the actual status of the engine. Otherwise the actor will transition to the flushingOutbound state but will never flush outbound, since engineNeedsWrap precondition will be false.
Generally speaking whenever we signal something to the SSLEngine, weshould also read getHandshakeStatus afterwards to understand what we need to do next. This was done everywhere but for this engine.closeInbound.