Skip to content

Commit

Permalink
tls: don't treat fatal TLS alerts as EOF
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@davidben @juanarbol
davidben authored and juanarbol committed Jan 22, 2023
1 parent c6457cb commit 757a022
Show file tree
Hide file tree
Showing 7 changed files with 51 additions and 44 deletions.
23 changes: 9 additions & 14 deletions src/crypto/crypto_tls.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 1 addition & 2 deletions src/env_properties.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) \
Expand Down
3 changes: 2 additions & 1 deletion test/parallel/test-tls-client-auth.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});

Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-tls-empty-sni-context.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}));
}));
43 changes: 28 additions & 15 deletions test/parallel/test-tls-min-max-version.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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',
Expand All @@ -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');
Expand All @@ -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',
Expand All @@ -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') {
Expand All @@ -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');
}
}

Expand Down
12 changes: 5 additions & 7 deletions test/parallel/test-tls-psk-circuit.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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' });
9 changes: 5 additions & 4 deletions test/parallel/test-tls-set-ciphers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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');
Expand Down

0 comments on commit 757a022

Please sign in to comment.