From 426b4a69936bf3667b3575092fe3b3336217c66a Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 6 Sep 2022 15:29:58 -0400 Subject: [PATCH 1/2] 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. --- src/crypto/crypto_tls.cc | 34 ++++++++++++++++------------------ src/crypto/crypto_tls.h | 2 -- 2 files changed, 16 insertions(+), 20 deletions(-) diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index 1135448b1e2941..fe92327a715cc8 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -677,11 +677,6 @@ void TLSWrap::OnStreamAfterWrite(WriteWrap* req_wrap, int status) { EncOut(); } -int TLSWrap::GetSSLError(int status) const { - // ssl_ might already be destroyed for reading EOF from a close notify alert. - return ssl_ != nullptr ? SSL_get_error(ssl_.get(), status) : 0; -} - void TLSWrap::ClearOut() { Debug(this, "Trying to read cleartext output"); // Ignore cycling data if ClientHello wasn't yet parsed @@ -735,19 +730,25 @@ void TLSWrap::ClearOut() { } } - int flags = SSL_get_shutdown(ssl_.get()); - if (!eof_ && flags & SSL_RECEIVED_SHUTDOWN) { - eof_ = true; - EmitRead(UV_EOF); - } - // We need to check whether an error occurred or the connection was // shutdown cleanly (SSL_ERROR_ZERO_RETURN) even when read == 0. - // See node#1642 and SSL_read(3SSL) for details. + // See node#1642 and SSL_read(3SSL) for details. SSL_get_error must be + // called immediately after SSL_read, without calling into JS, which may + // change OpenSSL's error queue, modify ssl_, or even destroy ssl_ + // altogether. if (read <= 0) { + int err = SSL_get_error(ssl_.get(), read); + unsigned long ssl_err = ERR_peek_error(); // NOLINT(runtime/int) + const std::string error_str = GetBIOError(); + + int flags = SSL_get_shutdown(ssl_.get()); + if (!eof_ && flags & SSL_RECEIVED_SHUTDOWN) { + eof_ = true; + EmitRead(UV_EOF); + } + HandleScope handle_scope(env()->isolate()); Local error; - int err = GetSSLError(read); switch (err) { case SSL_ERROR_ZERO_RETURN: // Ignore ZERO_RETURN after EOF, it is basically not an error. @@ -758,11 +759,8 @@ void TLSWrap::ClearOut() { case SSL_ERROR_SSL: case SSL_ERROR_SYSCALL: { - unsigned long ssl_err = ERR_peek_error(); // NOLINT(runtime/int) - Local context = env()->isolate()->GetCurrentContext(); if (UNLIKELY(context.IsEmpty())) return; - const std::string error_str = GetBIOError(); Local message = OneByteString( env()->isolate(), error_str.c_str(), error_str.size()); if (UNLIKELY(message.IsEmpty())) return; @@ -838,7 +836,7 @@ void TLSWrap::ClearIn() { } // Error or partial write - int err = GetSSLError(written); + int err = SSL_get_error(ssl_.get(), written); if (err == SSL_ERROR_SSL || err == SSL_ERROR_SYSCALL) { Debug(this, "Got SSL error (%d)", err); write_callback_scheduled_ = true; @@ -1014,7 +1012,7 @@ int TLSWrap::DoWrite(WriteWrap* w, if (written == -1) { // If we stopped writing because of an error, it's fatal, discard the data. - int err = GetSSLError(written); + int err = SSL_get_error(ssl_.get(), written); if (err == SSL_ERROR_SSL || err == SSL_ERROR_SYSCALL) { // TODO(@jasnell): What are we doing with the error? Debug(this, "Got SSL error (%d), returning UV_EPROTO", err); diff --git a/src/crypto/crypto_tls.h b/src/crypto/crypto_tls.h index 765c6c476b12e7..fea1cf8c2a4653 100644 --- a/src/crypto/crypto_tls.h +++ b/src/crypto/crypto_tls.h @@ -166,8 +166,6 @@ class TLSWrap : public AsyncWrap, int SetCACerts(SecureContext* sc); - int GetSSLError(int status) const; - static int SelectSNIContextCallback(SSL* s, int* ad, void* arg); static void CertCbDone(const v8::FunctionCallbackInfo& args); From 030b8c23a0670f2caee51eca2704e40dd5ab87b9 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 6 Sep 2022 17:41:26 -0400 Subject: [PATCH 2/2] tls: don't treat fatal TLS alerts as EOF SSL_RECEIVED_SHUTDOWN means not just close_notify or fatal alert. From what I can tell, this was just a mistake? OnStreamRead's comment suggests eof_ was intended to be for close_notify. This fixes a bug in TLSSocket error reporting that seems to have made it into existing tests. If we receive a fatal alert, EmitRead(UV_EOF) would, via onConnectEnd in _tls_wrap.js, synthesize an ECONNRESET before the alert itself is surfaced. As a result, TLS alerts received during the handshake are misreported by Node. See the tests that had to be updated as part of this. --- src/crypto/crypto_tls.cc | 23 +++++------ src/env_properties.h | 3 +- test/parallel/test-tls-client-auth.js | 3 +- test/parallel/test-tls-empty-sni-context.js | 2 +- test/parallel/test-tls-min-max-version.js | 43 ++++++++++++++------- test/parallel/test-tls-psk-circuit.js | 12 +++--- test/parallel/test-tls-set-ciphers.js | 9 +++-- 7 files changed, 51 insertions(+), 44 deletions(-) diff --git a/src/crypto/crypto_tls.cc b/src/crypto/crypto_tls.cc index fe92327a715cc8..fd1e817498bf0e 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -737,30 +737,25 @@ void TLSWrap::ClearOut() { // change OpenSSL's error queue, modify ssl_, or even destroy ssl_ // altogether. if (read <= 0) { - int err = SSL_get_error(ssl_.get(), read); - unsigned long ssl_err = ERR_peek_error(); // NOLINT(runtime/int) - const std::string error_str = GetBIOError(); - - int flags = SSL_get_shutdown(ssl_.get()); - if (!eof_ && flags & SSL_RECEIVED_SHUTDOWN) { - eof_ = true; - EmitRead(UV_EOF); - } - HandleScope handle_scope(env()->isolate()); Local error; + int err = SSL_get_error(ssl_.get(), read); switch (err) { case SSL_ERROR_ZERO_RETURN: - // Ignore ZERO_RETURN after EOF, it is basically not an error. - if (eof_) return; - error = env()->zero_return_string(); - break; + if (!eof_) { + eof_ = true; + EmitRead(UV_EOF); + } + return; case SSL_ERROR_SSL: case SSL_ERROR_SYSCALL: { + unsigned long ssl_err = ERR_peek_error(); // NOLINT(runtime/int) + Local context = env()->isolate()->GetCurrentContext(); if (UNLIKELY(context.IsEmpty())) return; + const std::string error_str = GetBIOError(); Local message = OneByteString( env()->isolate(), error_str.c_str(), error_str.size()); if (UNLIKELY(message.IsEmpty())) return; diff --git a/src/env_properties.h b/src/env_properties.h index e650e4926fb3df..27461518a81aa8 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -321,8 +321,7 @@ V(writable_string, "writable") \ V(write_host_object_string, "_writeHostObject") \ V(write_queue_size_string, "writeQueueSize") \ - V(x_forwarded_string, "x-forwarded-for") \ - V(zero_return_string, "ZERO_RETURN") + V(x_forwarded_string, "x-forwarded-for") #define PER_ISOLATE_TEMPLATE_PROPERTIES(V) \ V(async_wrap_ctor_template, v8::FunctionTemplate) \ diff --git a/test/parallel/test-tls-client-auth.js b/test/parallel/test-tls-client-auth.js index 476238961986cd..04756924e5e0e6 100644 --- a/test/parallel/test-tls-client-auth.js +++ b/test/parallel/test-tls-client-auth.js @@ -79,7 +79,8 @@ connect({ }, function(err, pair, cleanup) { assert.strictEqual(pair.server.err.code, 'ERR_SSL_PEER_DID_NOT_RETURN_A_CERTIFICATE'); - assert.strictEqual(pair.client.err.code, 'ECONNRESET'); + assert.strictEqual(pair.client.err.code, + 'ERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILURE'); return cleanup(); }); diff --git a/test/parallel/test-tls-empty-sni-context.js b/test/parallel/test-tls-empty-sni-context.js index 0c999b2c448f47..cb76430f65330f 100644 --- a/test/parallel/test-tls-empty-sni-context.js +++ b/test/parallel/test-tls-empty-sni-context.js @@ -26,6 +26,6 @@ const server = tls.createServer(options, (c) => { }, common.mustNotCall()); c.on('error', common.mustCall((err) => { - assert.match(err.message, /Client network socket disconnected/); + assert.strictEqual(err.code, 'ERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILURE'); })); })); diff --git a/test/parallel/test-tls-min-max-version.js b/test/parallel/test-tls-min-max-version.js index ff337961f9a426..5cea41ca7e0bd6 100644 --- a/test/parallel/test-tls-min-max-version.js +++ b/test/parallel/test-tls-min-max-version.js @@ -120,23 +120,27 @@ test(U, U, 'TLS_method', U, U, 'TLSv1_2_method', 'TLSv1.2'); test(U, U, 'TLS_method', U, U, 'TLSv1_1_method', 'TLSv1.1'); test(U, U, 'TLS_method', U, U, 'TLSv1_method', 'TLSv1'); +// OpenSSL 1.1.1 and 3.0 use a different error code and alert (sent to the +// client) when no protocols are enabled on the server. +const NO_PROTOCOLS_AVAILABLE_SERVER = common.hasOpenSSL3 ? + 'ERR_SSL_NO_PROTOCOLS_AVAILABLE' : 'ERR_SSL_INTERNAL_ERROR'; +const NO_PROTOCOLS_AVAILABLE_SERVER_ALERT = common.hasOpenSSL3 ? + 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION' : 'ERR_SSL_TLSV1_ALERT_INTERNAL_ERROR'; + // SSLv23 also means "any supported protocol" greater than the default // minimum (which is configurable via command line). if (DEFAULT_MIN_VERSION === 'TLSv1.3') { test(U, U, 'TLSv1_2_method', U, U, 'SSLv23_method', - U, 'ECONNRESET', common.hasOpenSSL3 ? - 'ERR_SSL_NO_PROTOCOLS_AVAILABLE' : 'ERR_SSL_INTERNAL_ERROR'); + U, NO_PROTOCOLS_AVAILABLE_SERVER_ALERT, NO_PROTOCOLS_AVAILABLE_SERVER); } else { test(U, U, 'TLSv1_2_method', U, U, 'SSLv23_method', 'TLSv1.2'); } if (DEFAULT_MIN_VERSION === 'TLSv1.3') { test(U, U, 'TLSv1_1_method', U, U, 'SSLv23_method', - U, 'ECONNRESET', common.hasOpenSSL3 ? - 'ERR_SSL_NO_PROTOCOLS_AVAILABLE' : 'ERR_SSL_INTERNAL_ERROR'); + U, NO_PROTOCOLS_AVAILABLE_SERVER_ALERT, NO_PROTOCOLS_AVAILABLE_SERVER); test(U, U, 'TLSv1_method', U, U, 'SSLv23_method', - U, 'ECONNRESET', common.hasOpenSSL3 ? - 'ERR_SSL_NO_PROTOCOLS_AVAILABLE' : 'ERR_SSL_INTERNAL_ERROR'); + U, NO_PROTOCOLS_AVAILABLE_SERVER_ALERT, NO_PROTOCOLS_AVAILABLE_SERVER); test(U, U, 'SSLv23_method', U, U, 'TLSv1_1_method', U, 'ERR_SSL_NO_PROTOCOLS_AVAILABLE', 'ERR_SSL_UNEXPECTED_MESSAGE'); test(U, U, 'SSLv23_method', U, U, 'TLSv1_method', @@ -145,9 +149,11 @@ if (DEFAULT_MIN_VERSION === 'TLSv1.3') { if (DEFAULT_MIN_VERSION === 'TLSv1.2') { test(U, U, 'TLSv1_1_method', U, U, 'SSLv23_method', - U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL'); + U, 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION', + 'ERR_SSL_UNSUPPORTED_PROTOCOL'); test(U, U, 'TLSv1_method', U, U, 'SSLv23_method', - U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL'); + U, 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION', + 'ERR_SSL_UNSUPPORTED_PROTOCOL'); test(U, U, 'SSLv23_method', U, U, 'TLSv1_1_method', U, 'ERR_SSL_UNSUPPORTED_PROTOCOL', 'ERR_SSL_WRONG_VERSION_NUMBER'); test(U, U, 'SSLv23_method', U, U, 'TLSv1_method', @@ -157,7 +163,8 @@ if (DEFAULT_MIN_VERSION === 'TLSv1.2') { if (DEFAULT_MIN_VERSION === 'TLSv1.1') { test(U, U, 'TLSv1_1_method', U, U, 'SSLv23_method', 'TLSv1.1'); test(U, U, 'TLSv1_method', U, U, 'SSLv23_method', - U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL'); + U, 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION', + 'ERR_SSL_UNSUPPORTED_PROTOCOL'); test(U, U, 'SSLv23_method', U, U, 'TLSv1_1_method', 'TLSv1.1'); test(U, U, 'SSLv23_method', U, U, 'TLSv1_method', U, 'ERR_SSL_UNSUPPORTED_PROTOCOL', 'ERR_SSL_WRONG_VERSION_NUMBER'); @@ -179,9 +186,11 @@ test(U, U, 'TLSv1_method', U, U, 'TLSv1_method', 'TLSv1'); // The default default. if (DEFAULT_MIN_VERSION === 'TLSv1.2') { test(U, U, 'TLSv1_1_method', U, U, U, - U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL'); + U, 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION', + 'ERR_SSL_UNSUPPORTED_PROTOCOL'); test(U, U, 'TLSv1_method', U, U, U, - U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL'); + U, 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION', + 'ERR_SSL_UNSUPPORTED_PROTOCOL'); if (DEFAULT_MAX_VERSION === 'TLSv1.2') { test(U, U, U, U, U, 'TLSv1_1_method', @@ -191,9 +200,11 @@ if (DEFAULT_MIN_VERSION === 'TLSv1.2') { } else { // TLS1.3 client hellos are are not understood by TLS1.1 or below. test(U, U, U, U, U, 'TLSv1_1_method', - U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL'); + U, 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION', + 'ERR_SSL_UNSUPPORTED_PROTOCOL'); test(U, U, U, U, U, 'TLSv1_method', - U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL'); + U, 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION', + 'ERR_SSL_UNSUPPORTED_PROTOCOL'); } } @@ -201,7 +212,8 @@ if (DEFAULT_MIN_VERSION === 'TLSv1.2') { if (DEFAULT_MIN_VERSION === 'TLSv1.1') { test(U, U, 'TLSv1_1_method', U, U, U, 'TLSv1.1'); test(U, U, 'TLSv1_method', U, U, U, - U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL'); + U, 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION', + 'ERR_SSL_UNSUPPORTED_PROTOCOL'); test(U, U, U, U, U, 'TLSv1_1_method', 'TLSv1.1'); if (DEFAULT_MAX_VERSION === 'TLSv1.2') { @@ -210,7 +222,8 @@ if (DEFAULT_MIN_VERSION === 'TLSv1.1') { } else { // TLS1.3 client hellos are are not understood by TLS1.1 or below. test(U, U, U, U, U, 'TLSv1_method', - U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL'); + U, 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION', + 'ERR_SSL_UNSUPPORTED_PROTOCOL'); } } diff --git a/test/parallel/test-tls-psk-circuit.js b/test/parallel/test-tls-psk-circuit.js index 4bcdf3686060c2..cef6735032ea6e 100644 --- a/test/parallel/test-tls-psk-circuit.js +++ b/test/parallel/test-tls-psk-circuit.js @@ -49,24 +49,22 @@ function test(secret, opts, error) { } else { const client = tls.connect(options, common.mustNotCall()); client.on('error', common.mustCall((err) => { - assert.strictEqual(err.message, error); + assert.strictEqual(err.code, error); server.close(); })); } })); } -const DISCONNECT_MESSAGE = - 'Client network socket disconnected before ' + - 'secure TLS connection was established'; - test({ psk: USERS.UserA, identity: 'UserA' }); test({ psk: USERS.UserA, identity: 'UserA' }, { maxVersion: 'TLSv1.2' }); test({ psk: USERS.UserA, identity: 'UserA' }, { minVersion: 'TLSv1.3' }); test({ psk: USERS.UserB, identity: 'UserB' }); test({ psk: USERS.UserB, identity: 'UserB' }, { minVersion: 'TLSv1.3' }); // Unrecognized user should fail handshake -test({ psk: USERS.UserB, identity: 'UserC' }, {}, DISCONNECT_MESSAGE); +test({ psk: USERS.UserB, identity: 'UserC' }, {}, + 'ERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILURE'); // Recognized user but incorrect secret should fail handshake -test({ psk: USERS.UserA, identity: 'UserB' }, {}, DISCONNECT_MESSAGE); +test({ psk: USERS.UserA, identity: 'UserB' }, {}, + 'ERR_SSL_SSLV3_ALERT_ILLEGAL_PARAMETER'); test({ psk: USERS.UserB, identity: 'UserB' }); diff --git a/test/parallel/test-tls-set-ciphers.js b/test/parallel/test-tls-set-ciphers.js index c2d9740201d74a..b66c419cf5f4d1 100644 --- a/test/parallel/test-tls-set-ciphers.js +++ b/test/parallel/test-tls-set-ciphers.js @@ -88,12 +88,13 @@ test('TLS_AES_256_GCM_SHA384', U, 'TLS_AES_256_GCM_SHA384'); // Do not have shared ciphers. test('TLS_AES_256_GCM_SHA384', 'TLS_CHACHA20_POLY1305_SHA256', - U, 'ECONNRESET', 'ERR_SSL_NO_SHARED_CIPHER'); + U, 'ERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILURE', 'ERR_SSL_NO_SHARED_CIPHER'); -test('AES128-SHA', 'AES256-SHA', U, 'ECONNRESET', 'ERR_SSL_NO_SHARED_CIPHER'); +test('AES128-SHA', 'AES256-SHA', U, 'ERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILURE', + 'ERR_SSL_NO_SHARED_CIPHER'); test('AES128-SHA:TLS_AES_256_GCM_SHA384', 'TLS_CHACHA20_POLY1305_SHA256:AES256-SHA', - U, 'ECONNRESET', 'ERR_SSL_NO_SHARED_CIPHER'); + U, 'ERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILURE', 'ERR_SSL_NO_SHARED_CIPHER'); // Cipher order ignored, TLS1.3 chosen before TLS1.2. test('AES256-SHA:TLS_AES_256_GCM_SHA384', U, 'TLS_AES_256_GCM_SHA384'); @@ -109,7 +110,7 @@ test(U, 'AES256-SHA', 'TLS_AES_256_GCM_SHA384', U, U, { maxVersion: 'TLSv1.3' }) // TLS_AES_128_CCM_8_SHA256 & TLS_AES_128_CCM_SHA256 are not enabled by // default, but work. test('TLS_AES_128_CCM_8_SHA256', U, - U, 'ECONNRESET', 'ERR_SSL_NO_SHARED_CIPHER'); + U, 'ERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILURE', 'ERR_SSL_NO_SHARED_CIPHER'); test('TLS_AES_128_CCM_8_SHA256', 'TLS_AES_128_CCM_8_SHA256', 'TLS_AES_128_CCM_8_SHA256');