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: #44563 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
nodejs · RafaelGSS · Jan 6, 2023 · Dec 19, 2022 · Dec 8, 2022 · Dec 10, 2022
commit ca9b9b9ce697ab3277495df557b1e6350fa9779b
diff --git 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<Value> 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> context = env()->isolate()->GetCurrentContext();
           if (UNLIKELY(context.IsEmpty())) return;
+          const std::string error_str = GetBIOError();
           Local<String> 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
@@ -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
@@ -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
@@ -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
@@ -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,17 +200,20 @@ 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');
   }
 }
 
 // The default with --tls-v1.1.
 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
@@ -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
@@ -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');