Fix to handle tls error properly #1661
Closed
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
In ssl.onerror event, `this` refers `ssl` so that `this._secureEstablished` is always undefined. Fix it to refer TLSSocket.
| if (read == -1) { | ||
| // We need to check whether an error occurred or the connection was | ||
| // shutdown cleanly (SSL_ERROR_ZERO_RETURN) even when read == 0 | ||
| // see iojs#1642 and SSL_read(3SSL) for details. |
There was a problem hiding this comment.
Femto-style nit: can you capitalize 'see' and insert a link to the issue?
|
LGTM with suggestions. A regression test would be really nice though. |
|
Oh, right. How did I forget about it in a first place. @shigeki please add a test indeed. |
|
I've just update the comments. I tried to implement a test but it is very hard to simulate Firefox for sending encrypted TLS alert just after handshake completed. Could you give me any idea? |
|
I guess you could use the raw socket to send the TLS alert frame (in a Buffer). And for ciphers you could try using 'NULL-SHA256' to make sure that you can send the non-encrypted alert. The frame could be generated either by hand or with https://github.com/indutny/tls.js . Please let me know if you need any help with any of these. |
|
Using NULL-SHA256! I never think of that. Okay I try it. |
SSL_read() returns 0 when fatal TLS Alert is received. Fix to invoke ssl error callback in this case.
|
@indutny Using |
|
LGTM, landing. |
shigeki
pushed a commit
that referenced
this pull request
May 16, 2015
In ssl.onerror event, `this` refers `ssl` so that `this._secureEstablished` is always undefined. Fix it to refer TLSSocket. PR-URL: #1661 Reviewed-By: Fedor Indutny <fedor@indutny.com>
shigeki
pushed a commit
that referenced
this pull request
May 16, 2015
SSL_read() returns 0 when fatal TLS Alert is received. Fix to invoke ssl error callback in this case. PR-URL: #1661 Reviewed-By: Fedor Indutny <fedor@indutny.com>
|
Thank you, @shigeki |
Fishrock123
pushed a commit
to Fishrock123/node
that referenced
this pull request
May 19, 2015
In ssl.onerror event, `this` refers `ssl` so that `this._secureEstablished` is always undefined. Fix it to refer TLSSocket. PR-URL: nodejs#1661 Reviewed-By: Fedor Indutny <fedor@indutny.com>
Fishrock123
pushed a commit
to Fishrock123/node
that referenced
this pull request
May 19, 2015
SSL_read() returns 0 when fatal TLS Alert is received. Fix to invoke ssl error callback in this case. PR-URL: nodejs#1661 Reviewed-By: Fedor Indutny <fedor@indutny.com>
andrewdeandrade
pushed a commit
to andrewdeandrade/node
that referenced
this pull request
Jun 3, 2015
In ssl.onerror event, `this` refers `ssl` so that `this._secureEstablished` is always undefined. Fix it to refer TLSSocket. PR-URL: nodejs/node#1661 Reviewed-By: Fedor Indutny <fedor@indutny.com>
andrewdeandrade
pushed a commit
to andrewdeandrade/node
that referenced
this pull request
Jun 3, 2015
SSL_read() returns 0 when fatal TLS Alert is received. Fix to invoke ssl error callback in this case. PR-URL: nodejs/node#1661 Reviewed-By: Fedor Indutny <fedor@indutny.com>
davidben
added a commit
to davidben/node
that referenced
this pull request
Sep 7, 2022
OpenSSL's API requires SSL_get_error be called immediately after the failing operation, otherwise the SSL object may have changed state and no longer report information about the failing error. TLSWrap almost heeds this rule, except in TLSWrap::ClearOut. If SSL_read picks up a close_notify (detected by checking SSL_get_shutdown), Node calls out to JS with EmitRead(UV_EOF) and only afterwards proceeds to dispatch on the error. But, by this point, Node has already re-entered JS and indeed JS seems to sometimes call back into TLSWrap::DoShutdown, calling SSL_shutdown. (I think this comes from onStreamRead in stream_base_commons.js?) Instead, SSL_get_error and the error queue should be sampled earlier. This avoids the issue worked around by nodejs#1661, where GetSSLError needed to check if ssl_ was destroyed before calling SSL_get_error. We can now remove that wrapper and just call SSL_get_error directly. (Any case where ssl_ may be destroyed first is a case where ssl_ may change state, so it's a bug either way.) This is the first of two fixes in error-handling here. The EmitRead(UV_EOF) seems to additionally swallow fatal alerts from the peer. Some of the ECONNRESET expectations in the tests aren't actually correct. The next commit will fix this as well.
davidben
added a commit
to davidben/node
that referenced
this pull request
Sep 7, 2022
Like errno, OpenSSL's API requires SSL_get_error and error queue be checked immediately after the failing operation, otherwise the error queue or SSL object may have changed state and no longer report information about the operation the caller wanted. TLSWrap almost heeds this rule, except in TLSWrap::ClearOut. If SSL_read picks up a closing alert (detected by checking SSL_get_shutdown), Node calls out to JS with EmitRead(UV_EOF) and only afterwards proceeds to dispatch on the error. But, by this point, Node has already re-entered JS, which may change the error. In particular, I've observed that, on close_notify, JS seems to sometimes call back into TLSWrap::DoShutdown, calling SSL_shutdown. (I think this comes from onStreamRead in stream_base_commons.js?) Instead, SSL_get_error and the error queue should be sampled earlier. Back in nodejs#1661, Node needed to account for GetSSLError being called after ssl_ was destroyed. This was the real cause. With this fixed, there's no need to account for this. (Any case where ssl_ may be destroyed before SSL_get_error is a case where ssl_ or the error queue could change state, so it's a bug either way.) This is the first of two fixes in error-handling here. The EmitRead(UV_EOF) seems to additionally swallow fatal alerts from the peer. Some of the ECONNRESET expectations in the tests aren't actually correct. The next commit will fix this as well.
nodejs-github-bot
pushed a commit
that referenced
this pull request
Dec 12, 2022
Like errno, OpenSSL's API requires SSL_get_error and error queue be checked immediately after the failing operation, otherwise the error queue or SSL object may have changed state and no longer report information about the operation the caller wanted. TLSWrap almost heeds this rule, except in TLSWrap::ClearOut. If SSL_read picks up a closing alert (detected by checking SSL_get_shutdown), Node calls out to JS with EmitRead(UV_EOF) and only afterwards proceeds to dispatch on the error. But, by this point, Node has already re-entered JS, which may change the error. In particular, I've observed that, on close_notify, JS seems to sometimes call back into TLSWrap::DoShutdown, calling SSL_shutdown. (I think this comes from onStreamRead in stream_base_commons.js?) Instead, SSL_get_error and the error queue should be sampled earlier. Back in #1661, Node needed to account for GetSSLError being called after ssl_ was destroyed. This was the real cause. With this fixed, there's no need to account for this. (Any case where ssl_ may be destroyed before SSL_get_error is a case where ssl_ or the error queue could change state, so it's a bug either way.) This is the first of two fixes in error-handling here. The EmitRead(UV_EOF) seems to additionally swallow fatal alerts from the peer. Some of the ECONNRESET expectations in the tests aren't actually correct. The next commit will fix this as well. PR-URL: #44563 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos
pushed a commit
that referenced
this pull request
Jan 1, 2023
Like errno, OpenSSL's API requires SSL_get_error and error queue be checked immediately after the failing operation, otherwise the error queue or SSL object may have changed state and no longer report information about the operation the caller wanted. TLSWrap almost heeds this rule, except in TLSWrap::ClearOut. If SSL_read picks up a closing alert (detected by checking SSL_get_shutdown), Node calls out to JS with EmitRead(UV_EOF) and only afterwards proceeds to dispatch on the error. But, by this point, Node has already re-entered JS, which may change the error. In particular, I've observed that, on close_notify, JS seems to sometimes call back into TLSWrap::DoShutdown, calling SSL_shutdown. (I think this comes from onStreamRead in stream_base_commons.js?) Instead, SSL_get_error and the error queue should be sampled earlier. Back in #1661, Node needed to account for GetSSLError being called after ssl_ was destroyed. This was the real cause. With this fixed, there's no need to account for this. (Any case where ssl_ may be destroyed before SSL_get_error is a case where ssl_ or the error queue could change state, so it's a bug either way.) This is the first of two fixes in error-handling here. The EmitRead(UV_EOF) seems to additionally swallow fatal alerts from the peer. Some of the ECONNRESET expectations in the tests aren't actually correct. The next commit will fix this as well. PR-URL: #44563 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
juanarbol
pushed a commit
that referenced
this pull request
Jan 26, 2023
Like errno, OpenSSL's API requires SSL_get_error and error queue be checked immediately after the failing operation, otherwise the error queue or SSL object may have changed state and no longer report information about the operation the caller wanted. TLSWrap almost heeds this rule, except in TLSWrap::ClearOut. If SSL_read picks up a closing alert (detected by checking SSL_get_shutdown), Node calls out to JS with EmitRead(UV_EOF) and only afterwards proceeds to dispatch on the error. But, by this point, Node has already re-entered JS, which may change the error. In particular, I've observed that, on close_notify, JS seems to sometimes call back into TLSWrap::DoShutdown, calling SSL_shutdown. (I think this comes from onStreamRead in stream_base_commons.js?) Instead, SSL_get_error and the error queue should be sampled earlier. Back in #1661, Node needed to account for GetSSLError being called after ssl_ was destroyed. This was the real cause. With this fixed, there's no need to account for this. (Any case where ssl_ may be destroyed before SSL_get_error is a case where ssl_ or the error queue could change state, so it's a bug either way.) This is the first of two fixes in error-handling here. The EmitRead(UV_EOF) seems to additionally swallow fatal alerts from the peer. Some of the ECONNRESET expectations in the tests aren't actually correct. The next commit will fix this as well. PR-URL: #44563 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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.
This is a fix of the error handling of tls and tls_wrap. Especially it fixes the error handling when the server receiving a fatal TLS alert just after TLS handshake completed. Testing is very hard to implement within node and
openssl s_clientso that we take this fix to resolve the issue of #1642 from the comments from reporters.CI results are https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/655/ where the test failures on Windows are related to child process not related this.
Fix: #1642
R= @indutny