From 8b89d4d42156046326dffb32c51e256ee7045a53 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 6 Sep 2022 17:41:26 -0400 Subject: [PATCH] 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. PR-URL: https://github.com/nodejs/node/pull/44563 Reviewed-By: Luigi Pinca --- 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 070c904cc8fef7..f14adec767af41 100644 --- a/src/crypto/crypto_tls.cc +++ b/src/crypto/crypto_tls.cc @@ -728,30 +728,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 32f4134ad94327..d621ebbd09425d 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -320,8 +320,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');