Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 757a022

Browse filesBrowse files
davidbenjuanarbol
davidben
authored and
juanarbol
committed
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>
1 parent c6457cb commit 757a022
Copy full SHA for 757a022

File tree

Expand file treeCollapse file tree

7 files changed

+51
-44
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

7 files changed

+51
-44
lines changed
Open diff view settings
Collapse file

‎src/crypto/crypto_tls.cc‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_tls.cc
+9-14Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -728,30 +728,25 @@ void TLSWrap::ClearOut() {
728728
// change OpenSSL's error queue, modify ssl_, or even destroy ssl_
729729
// altogether.
730730
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-
741731
HandleScope handle_scope(env()->isolate());
742732
Local<Value> error;
733+
int err = SSL_get_error(ssl_.get(), read);
743734
switch (err) {
744735
case SSL_ERROR_ZERO_RETURN:
745-
// Ignore ZERO_RETURN after EOF, it is basically not an error.
746-
if (eof_) return;
747-
error = env()->zero_return_string();
748-
break;
736+
if (!eof_) {
737+
eof_ = true;
738+
EmitRead(UV_EOF);
739+
}
740+
return;
749741

750742
case SSL_ERROR_SSL:
751743
case SSL_ERROR_SYSCALL:
752744
{
745+
unsigned long ssl_err = ERR_peek_error(); // NOLINT(runtime/int)
746+
753747
Local<Context> context = env()->isolate()->GetCurrentContext();
754748
if (UNLIKELY(context.IsEmpty())) return;
749+
const std::string error_str = GetBIOError();
755750
Local<String> message = OneByteString(
756751
env()->isolate(), error_str.c_str(), error_str.size());
757752
if (UNLIKELY(message.IsEmpty())) return;
Collapse file

‎src/env_properties.h‎

Copy file name to clipboardExpand all lines: src/env_properties.h
+1-2Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -321,8 +321,7 @@
321321
V(writable_string, "writable") \
322322
V(write_host_object_string, "_writeHostObject") \
323323
V(write_queue_size_string, "writeQueueSize") \
324-
V(x_forwarded_string, "x-forwarded-for") \
325-
V(zero_return_string, "ZERO_RETURN")
324+
V(x_forwarded_string, "x-forwarded-for")
326325

327326
#define PER_ISOLATE_TEMPLATE_PROPERTIES(V) \
328327
V(async_wrap_ctor_template, v8::FunctionTemplate) \
Collapse file

‎test/parallel/test-tls-client-auth.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-tls-client-auth.js
+2-1Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,8 @@ connect({
7979
}, function(err, pair, cleanup) {
8080
assert.strictEqual(pair.server.err.code,
8181
'ERR_SSL_PEER_DID_NOT_RETURN_A_CERTIFICATE');
82-
assert.strictEqual(pair.client.err.code, 'ECONNRESET');
82+
assert.strictEqual(pair.client.err.code,
83+
'ERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILURE');
8384
return cleanup();
8485
});
8586

Collapse file

‎test/parallel/test-tls-empty-sni-context.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-tls-empty-sni-context.js
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,6 @@ const server = tls.createServer(options, (c) => {
2626
}, common.mustNotCall());
2727

2828
c.on('error', common.mustCall((err) => {
29-
assert.match(err.message, /Client network socket disconnected/);
29+
assert.strictEqual(err.code, 'ERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILURE');
3030
}));
3131
}));
Collapse file

‎test/parallel/test-tls-min-max-version.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-tls-min-max-version.js
+28-15Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -120,23 +120,27 @@ test(U, U, 'TLS_method', U, U, 'TLSv1_2_method', 'TLSv1.2');
120120
test(U, U, 'TLS_method', U, U, 'TLSv1_1_method', 'TLSv1.1');
121121
test(U, U, 'TLS_method', U, U, 'TLSv1_method', 'TLSv1');
122122

123+
// OpenSSL 1.1.1 and 3.0 use a different error code and alert (sent to the
124+
// client) when no protocols are enabled on the server.
125+
const NO_PROTOCOLS_AVAILABLE_SERVER = common.hasOpenSSL3 ?
126+
'ERR_SSL_NO_PROTOCOLS_AVAILABLE' : 'ERR_SSL_INTERNAL_ERROR';
127+
const NO_PROTOCOLS_AVAILABLE_SERVER_ALERT = common.hasOpenSSL3 ?
128+
'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION' : 'ERR_SSL_TLSV1_ALERT_INTERNAL_ERROR';
129+
123130
// SSLv23 also means "any supported protocol" greater than the default
124131
// minimum (which is configurable via command line).
125132
if (DEFAULT_MIN_VERSION === 'TLSv1.3') {
126133
test(U, U, 'TLSv1_2_method', U, U, 'SSLv23_method',
127-
U, 'ECONNRESET', common.hasOpenSSL3 ?
128-
'ERR_SSL_NO_PROTOCOLS_AVAILABLE' : 'ERR_SSL_INTERNAL_ERROR');
134+
U, NO_PROTOCOLS_AVAILABLE_SERVER_ALERT, NO_PROTOCOLS_AVAILABLE_SERVER);
129135
} else {
130136
test(U, U, 'TLSv1_2_method', U, U, 'SSLv23_method', 'TLSv1.2');
131137
}
132138

133139
if (DEFAULT_MIN_VERSION === 'TLSv1.3') {
134140
test(U, U, 'TLSv1_1_method', U, U, 'SSLv23_method',
135-
U, 'ECONNRESET', common.hasOpenSSL3 ?
136-
'ERR_SSL_NO_PROTOCOLS_AVAILABLE' : 'ERR_SSL_INTERNAL_ERROR');
141+
U, NO_PROTOCOLS_AVAILABLE_SERVER_ALERT, NO_PROTOCOLS_AVAILABLE_SERVER);
137142
test(U, U, 'TLSv1_method', U, U, 'SSLv23_method',
138-
U, 'ECONNRESET', common.hasOpenSSL3 ?
139-
'ERR_SSL_NO_PROTOCOLS_AVAILABLE' : 'ERR_SSL_INTERNAL_ERROR');
143+
U, NO_PROTOCOLS_AVAILABLE_SERVER_ALERT, NO_PROTOCOLS_AVAILABLE_SERVER);
140144
test(U, U, 'SSLv23_method', U, U, 'TLSv1_1_method',
141145
U, 'ERR_SSL_NO_PROTOCOLS_AVAILABLE', 'ERR_SSL_UNEXPECTED_MESSAGE');
142146
test(U, U, 'SSLv23_method', U, U, 'TLSv1_method',
@@ -145,9 +149,11 @@ if (DEFAULT_MIN_VERSION === 'TLSv1.3') {
145149

146150
if (DEFAULT_MIN_VERSION === 'TLSv1.2') {
147151
test(U, U, 'TLSv1_1_method', U, U, 'SSLv23_method',
148-
U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL');
152+
U, 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION',
153+
'ERR_SSL_UNSUPPORTED_PROTOCOL');
149154
test(U, U, 'TLSv1_method', U, U, 'SSLv23_method',
150-
U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL');
155+
U, 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION',
156+
'ERR_SSL_UNSUPPORTED_PROTOCOL');
151157
test(U, U, 'SSLv23_method', U, U, 'TLSv1_1_method',
152158
U, 'ERR_SSL_UNSUPPORTED_PROTOCOL', 'ERR_SSL_WRONG_VERSION_NUMBER');
153159
test(U, U, 'SSLv23_method', U, U, 'TLSv1_method',
@@ -157,7 +163,8 @@ if (DEFAULT_MIN_VERSION === 'TLSv1.2') {
157163
if (DEFAULT_MIN_VERSION === 'TLSv1.1') {
158164
test(U, U, 'TLSv1_1_method', U, U, 'SSLv23_method', 'TLSv1.1');
159165
test(U, U, 'TLSv1_method', U, U, 'SSLv23_method',
160-
U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL');
166+
U, 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION',
167+
'ERR_SSL_UNSUPPORTED_PROTOCOL');
161168
test(U, U, 'SSLv23_method', U, U, 'TLSv1_1_method', 'TLSv1.1');
162169
test(U, U, 'SSLv23_method', U, U, 'TLSv1_method',
163170
U, 'ERR_SSL_UNSUPPORTED_PROTOCOL', 'ERR_SSL_WRONG_VERSION_NUMBER');
@@ -179,9 +186,11 @@ test(U, U, 'TLSv1_method', U, U, 'TLSv1_method', 'TLSv1');
179186
// The default default.
180187
if (DEFAULT_MIN_VERSION === 'TLSv1.2') {
181188
test(U, U, 'TLSv1_1_method', U, U, U,
182-
U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL');
189+
U, 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION',
190+
'ERR_SSL_UNSUPPORTED_PROTOCOL');
183191
test(U, U, 'TLSv1_method', U, U, U,
184-
U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL');
192+
U, 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION',
193+
'ERR_SSL_UNSUPPORTED_PROTOCOL');
185194

186195
if (DEFAULT_MAX_VERSION === 'TLSv1.2') {
187196
test(U, U, U, U, U, 'TLSv1_1_method',
@@ -191,17 +200,20 @@ if (DEFAULT_MIN_VERSION === 'TLSv1.2') {
191200
} else {
192201
// TLS1.3 client hellos are are not understood by TLS1.1 or below.
193202
test(U, U, U, U, U, 'TLSv1_1_method',
194-
U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL');
203+
U, 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION',
204+
'ERR_SSL_UNSUPPORTED_PROTOCOL');
195205
test(U, U, U, U, U, 'TLSv1_method',
196-
U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL');
206+
U, 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION',
207+
'ERR_SSL_UNSUPPORTED_PROTOCOL');
197208
}
198209
}
199210

200211
// The default with --tls-v1.1.
201212
if (DEFAULT_MIN_VERSION === 'TLSv1.1') {
202213
test(U, U, 'TLSv1_1_method', U, U, U, 'TLSv1.1');
203214
test(U, U, 'TLSv1_method', U, U, U,
204-
U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL');
215+
U, 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION',
216+
'ERR_SSL_UNSUPPORTED_PROTOCOL');
205217
test(U, U, U, U, U, 'TLSv1_1_method', 'TLSv1.1');
206218

207219
if (DEFAULT_MAX_VERSION === 'TLSv1.2') {
@@ -210,7 +222,8 @@ if (DEFAULT_MIN_VERSION === 'TLSv1.1') {
210222
} else {
211223
// TLS1.3 client hellos are are not understood by TLS1.1 or below.
212224
test(U, U, U, U, U, 'TLSv1_method',
213-
U, 'ECONNRESET', 'ERR_SSL_UNSUPPORTED_PROTOCOL');
225+
U, 'ERR_SSL_TLSV1_ALERT_PROTOCOL_VERSION',
226+
'ERR_SSL_UNSUPPORTED_PROTOCOL');
214227
}
215228
}
216229

Collapse file

‎test/parallel/test-tls-psk-circuit.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-tls-psk-circuit.js
+5-7Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -49,24 +49,22 @@ function test(secret, opts, error) {
4949
} else {
5050
const client = tls.connect(options, common.mustNotCall());
5151
client.on('error', common.mustCall((err) => {
52-
assert.strictEqual(err.message, error);
52+
assert.strictEqual(err.code, error);
5353
server.close();
5454
}));
5555
}
5656
}));
5757
}
5858

59-
const DISCONNECT_MESSAGE =
60-
'Client network socket disconnected before ' +
61-
'secure TLS connection was established';
62-
6359
test({ psk: USERS.UserA, identity: 'UserA' });
6460
test({ psk: USERS.UserA, identity: 'UserA' }, { maxVersion: 'TLSv1.2' });
6561
test({ psk: USERS.UserA, identity: 'UserA' }, { minVersion: 'TLSv1.3' });
6662
test({ psk: USERS.UserB, identity: 'UserB' });
6763
test({ psk: USERS.UserB, identity: 'UserB' }, { minVersion: 'TLSv1.3' });
6864
// Unrecognized user should fail handshake
69-
test({ psk: USERS.UserB, identity: 'UserC' }, {}, DISCONNECT_MESSAGE);
65+
test({ psk: USERS.UserB, identity: 'UserC' }, {},
66+
'ERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILURE');
7067
// Recognized user but incorrect secret should fail handshake
71-
test({ psk: USERS.UserA, identity: 'UserB' }, {}, DISCONNECT_MESSAGE);
68+
test({ psk: USERS.UserA, identity: 'UserB' }, {},
69+
'ERR_SSL_SSLV3_ALERT_ILLEGAL_PARAMETER');
7270
test({ psk: USERS.UserB, identity: 'UserB' });
Collapse file

‎test/parallel/test-tls-set-ciphers.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-tls-set-ciphers.js
+5-4Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -88,12 +88,13 @@ test('TLS_AES_256_GCM_SHA384', U, 'TLS_AES_256_GCM_SHA384');
8888

8989
// Do not have shared ciphers.
9090
test('TLS_AES_256_GCM_SHA384', 'TLS_CHACHA20_POLY1305_SHA256',
91-
U, 'ECONNRESET', 'ERR_SSL_NO_SHARED_CIPHER');
91+
U, 'ERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILURE', 'ERR_SSL_NO_SHARED_CIPHER');
9292

93-
test('AES128-SHA', 'AES256-SHA', U, 'ECONNRESET', 'ERR_SSL_NO_SHARED_CIPHER');
93+
test('AES128-SHA', 'AES256-SHA', U, 'ERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILURE',
94+
'ERR_SSL_NO_SHARED_CIPHER');
9495
test('AES128-SHA:TLS_AES_256_GCM_SHA384',
9596
'TLS_CHACHA20_POLY1305_SHA256:AES256-SHA',
96-
U, 'ECONNRESET', 'ERR_SSL_NO_SHARED_CIPHER');
97+
U, 'ERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILURE', 'ERR_SSL_NO_SHARED_CIPHER');
9798

9899
// Cipher order ignored, TLS1.3 chosen before TLS1.2.
99100
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' })
109110
// TLS_AES_128_CCM_8_SHA256 & TLS_AES_128_CCM_SHA256 are not enabled by
110111
// default, but work.
111112
test('TLS_AES_128_CCM_8_SHA256', U,
112-
U, 'ECONNRESET', 'ERR_SSL_NO_SHARED_CIPHER');
113+
U, 'ERR_SSL_SSLV3_ALERT_HANDSHAKE_FAILURE', 'ERR_SSL_NO_SHARED_CIPHER');
113114

114115
test('TLS_AES_128_CCM_8_SHA256', 'TLS_AES_128_CCM_8_SHA256',
115116
'TLS_AES_128_CCM_8_SHA256');

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.