Commit c6457cb
tls: fix re-entrancy issue with TLS close_notify
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>1 parent 67efe2a commit c6457cb
2 files changed
Lines changed: 16 additions & 20 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -668,11 +668,6 @@ void TLSWrap::OnStreamAfterWrite(WriteWrap* req_wrap, int status) { | |||
| 668 | 668 | EncOut(); | |
| 669 | 669 | } | |
| 670 | 670 | ||
| 671 | - int TLSWrap::GetSSLError(int status) const { | ||
| 672 | - // ssl_ might already be destroyed for reading EOF from a close notify alert. | ||
| 673 | - return ssl_ != nullptr ? SSL_get_error(ssl_.get(), status) : 0; | ||
| 674 | - } | ||
| 675 | - | ||
| 676 | 671 | void TLSWrap::ClearOut() { | |
| 677 | 672 | Debug(this, "Trying to read cleartext output"); | |
| 678 | 673 | // Ignore cycling data if ClientHello wasn't yet parsed | |
@@ -726,19 +721,25 @@ void TLSWrap::ClearOut() { | |||
| 726 | 721 | } | |
| 727 | 722 | } | |
| 728 | 723 | ||
| 729 | - int flags = SSL_get_shutdown(ssl_.get()); | ||
| 730 | - if (!eof_ && flags & SSL_RECEIVED_SHUTDOWN) { | ||
| 731 | - eof_ = true; | ||
| 732 | - EmitRead(UV_EOF); | ||
| 733 | - } | ||
| 734 | - | ||
| 735 | 724 | // We need to check whether an error occurred or the connection was | |
| 736 | 725 | // shutdown cleanly (SSL_ERROR_ZERO_RETURN) even when read == 0. | |
| 737 | - // See node#1642 and SSL_read(3SSL) for details. | ||
| 726 | + // See node#1642 and SSL_read(3SSL) for details. SSL_get_error must be | ||
| 727 | + // called immediately after SSL_read, without calling into JS, which may | ||
| 728 | + // change OpenSSL's error queue, modify ssl_, or even destroy ssl_ | ||
| 729 | + // altogether. | ||
| 738 | 730 | if (read <= 0) { | |
| 731 | + int err = SSL_get_error(ssl_.get(), read); | ||
| 732 | + unsigned long ssl_err = ERR_peek_error(); // NOLINT(runtime/int) | ||
| 733 | + const std::string error_str = GetBIOError(); | ||
| 734 | + | ||
| 735 | + int flags = SSL_get_shutdown(ssl_.get()); | ||
| 736 | + if (!eof_ && flags & SSL_RECEIVED_SHUTDOWN) { | ||
| 737 | + eof_ = true; | ||
| 738 | + EmitRead(UV_EOF); | ||
| 739 | + } | ||
| 740 | + | ||
| 739 | 741 | HandleScope handle_scope(env()->isolate()); | |
| 740 | 742 | Local<Value> error; | |
| 741 | - int err = GetSSLError(read); | ||
| 742 | 743 | switch (err) { | |
| 743 | 744 | case SSL_ERROR_ZERO_RETURN: | |
| 744 | 745 | // Ignore ZERO_RETURN after EOF, it is basically not an error. | |
@@ -749,11 +750,8 @@ void TLSWrap::ClearOut() { | |||
| 749 | 750 | case SSL_ERROR_SSL: | |
| 750 | 751 | case SSL_ERROR_SYSCALL: | |
| 751 | 752 | { | |
| 752 | - unsigned long ssl_err = ERR_peek_error(); // NOLINT(runtime/int) | ||
| 753 | - | ||
| 754 | 753 | Local<Context> context = env()->isolate()->GetCurrentContext(); | |
| 755 | 754 | if (UNLIKELY(context.IsEmpty())) return; | |
| 756 | - const std::string error_str = GetBIOError(); | ||
| 757 | 755 | Local<String> message = OneByteString( | |
| 758 | 756 | env()->isolate(), error_str.c_str(), error_str.size()); | |
| 759 | 757 | if (UNLIKELY(message.IsEmpty())) return; | |
@@ -829,7 +827,7 @@ void TLSWrap::ClearIn() { | |||
| 829 | 827 | } | |
| 830 | 828 | ||
| 831 | 829 | // Error or partial write | |
| 832 | - int err = GetSSLError(written); | ||
| 830 | + int err = SSL_get_error(ssl_.get(), written); | ||
| 833 | 831 | if (err == SSL_ERROR_SSL || err == SSL_ERROR_SYSCALL) { | |
| 834 | 832 | Debug(this, "Got SSL error (%d)", err); | |
| 835 | 833 | write_callback_scheduled_ = true; | |
@@ -1005,7 +1003,7 @@ int TLSWrap::DoWrite(WriteWrap* w, | |||
| 1005 | 1003 | ||
| 1006 | 1004 | if (written == -1) { | |
| 1007 | 1005 | // If we stopped writing because of an error, it's fatal, discard the data. | |
| 1008 | - int err = GetSSLError(written); | ||
| 1006 | + int err = SSL_get_error(ssl_.get(), written); | ||
| 1009 | 1007 | if (err == SSL_ERROR_SSL || err == SSL_ERROR_SYSCALL) { | |
| 1010 | 1008 | // TODO(@jasnell): What are we doing with the error? | |
| 1011 | 1009 | Debug(this, "Got SSL error (%d), returning UV_EPROTO", err); | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -167,8 +167,6 @@ class TLSWrap : public AsyncWrap, | |||
| 167 | 167 | ||
| 168 | 168 | int SetCACerts(SecureContext* sc); | |
| 169 | 169 | ||
| 170 | - int GetSSLError(int status) const; | ||
| 171 | - | ||
| 172 | 170 | static int SelectSNIContextCallback(SSL* s, int* ad, void* arg); | |
| 173 | 171 | ||
| 174 | 172 | static void CertCbDone(const v8::FunctionCallbackInfo<v8::Value>& args); | |
0 commit comments