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 3eeca52

Browse filesBrowse files
ywave620ruyadorno
ywave620
authored and
ruyadorno
committed
tls: fix bugs of double TLS
Fixs two issues in `TLSWrap`, one of them is reported in #30896. 1. `TLSWrap` has exactly one `StreamListener`, however, that `StreamListener` can be replaced. We have not been rigorous enough here: if an active write has not been finished before the transition, the finish callback of it will be wrongly fired the successor `StreamListener`. 2. A `TLSWrap` does not allow more than one active write, as checked in the assertion about current_write in `TLSWrap::DoWrite()`. However, when users make use of an existing `tls.TLSSocket` to establish double TLS, by either tls.connect({socket: tlssock}) or tlsServer.emit('connection', tlssock) we have both of the user provided `tls.TLSSocket`, tlssock and a brand new created `TLSWrap` writing to the `TLSWrap` bound to tlssock, which easily violates the constranint because two writers have no idea of each other. The design of the fix is: when a `TLSWrap` is created on top of a user provided socket, do not send any data to the socket until all existing writes of the socket are done and ensure registered callbacks of those writes can be fired. PR-URL: #48969 Backport-PR-URL: #49183 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>
1 parent 6de7aa1 commit 3eeca52
Copy full SHA for 3eeca52

File tree

Expand file treeCollapse file tree

7 files changed

+297
-24
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

7 files changed

+297
-24
lines changed
Open diff view settings
Collapse file

‎lib/_tls_wrap.js‎

Copy file name to clipboardExpand all lines: lib/_tls_wrap.js
+50-18Lines changed: 50 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -502,25 +502,36 @@ function TLSSocket(socket, opts) {
502502
this[kPendingSession] = null;
503503

504504
let wrap;
505-
if ((socket instanceof net.Socket && socket._handle) || !socket) {
506-
// 1. connected socket
507-
// 2. no socket, one will be created with net.Socket().connect
508-
wrap = socket;
505+
let handle;
506+
let wrapHasActiveWriteFromPrevOwner;
507+
508+
if (socket) {
509+
if (socket instanceof net.Socket && socket._handle) {
510+
// 1. connected socket
511+
wrap = socket;
512+
} else {
513+
// 2. socket has no handle so it is js not c++
514+
// 3. unconnected sockets are wrapped
515+
// TLS expects to interact from C++ with a net.Socket that has a C++ stream
516+
// handle, but a JS stream doesn't have one. Wrap it up to make it look like
517+
// a socket.
518+
wrap = new JSStreamSocket(socket);
519+
}
520+
521+
handle = wrap._handle;
522+
wrapHasActiveWriteFromPrevOwner = wrap.writableLength > 0;
509523
} else {
510-
// 3. socket has no handle so it is js not c++
511-
// 4. unconnected sockets are wrapped
512-
// TLS expects to interact from C++ with a net.Socket that has a C++ stream
513-
// handle, but a JS stream doesn't have one. Wrap it up to make it look like
514-
// a socket.
515-
wrap = new JSStreamSocket(socket);
524+
// 4. no socket, one will be created with net.Socket().connect
525+
wrap = null;
526+
wrapHasActiveWriteFromPrevOwner = false;
516527
}
517528

518529
// Just a documented property to make secure sockets
519530
// distinguishable from regular ones.
520531
this.encrypted = true;
521532

522533
ReflectApply(net.Socket, this, [{
523-
handle: this._wrapHandle(wrap),
534+
handle: this._wrapHandle(wrap, handle, wrapHasActiveWriteFromPrevOwner),
524535
allowHalfOpen: socket ? socket.allowHalfOpen : tlsOptions.allowHalfOpen,
525536
pauseOnCreate: tlsOptions.pauseOnConnect,
526537
manualStart: true,
@@ -539,6 +550,21 @@ function TLSSocket(socket, opts) {
539550
if (enableTrace && this._handle)
540551
this._handle.enableTrace();
541552

553+
if (wrapHasActiveWriteFromPrevOwner) {
554+
// `wrap` is a streams.Writable in JS. This empty write will be queued
555+
// and hence finish after all existing writes, which is the timing
556+
// we want to start to send any tls data to `wrap`.
557+
wrap.write('', (err) => {
558+
if (err) {
559+
debug('error got before writing any tls data to the underlying stream');
560+
this.destroy(err);
561+
return;
562+
}
563+
564+
this._handle.writesIssuedByPrevListenerDone();
565+
});
566+
}
567+
542568
// Read on next tick so the caller has a chance to setup listeners
543569
process.nextTick(initRead, this, socket);
544570
}
@@ -599,11 +625,14 @@ TLSSocket.prototype.disableRenegotiation = function disableRenegotiation() {
599625
this[kDisableRenegotiation] = true;
600626
};
601627

602-
TLSSocket.prototype._wrapHandle = function(wrap, handle) {
603-
if (!handle && wrap) {
604-
handle = wrap._handle;
605-
}
606-
628+
/**
629+
*
630+
* @param {null|net.Socket} wrap
631+
* @param {null|object} handle
632+
* @param {boolean} wrapHasActiveWriteFromPrevOwner
633+
* @returns {object}
634+
*/
635+
TLSSocket.prototype._wrapHandle = function(wrap, handle, wrapHasActiveWriteFromPrevOwner) {
607636
const options = this._tlsOptions;
608637
if (!handle) {
609638
handle = options.pipe ?
@@ -620,7 +649,10 @@ TLSSocket.prototype._wrapHandle = function(wrap, handle) {
620649
if (!(context.context instanceof NativeSecureContext)) {
621650
throw new ERR_TLS_INVALID_CONTEXT('context');
622651
}
623-
const res = tls_wrap.wrap(handle, context.context, !!options.isServer);
652+
653+
const res = tls_wrap.wrap(handle, context.context,
654+
!!options.isServer,
655+
wrapHasActiveWriteFromPrevOwner);
624656
res._parent = handle; // C++ "wrap" object: TCPWrap, JSStream, ...
625657
res._parentWrap = wrap; // JS object: net.Socket, JSStreamSocket, ...
626658
res._secureContext = context;
@@ -637,7 +669,7 @@ TLSSocket.prototype[kReinitializeHandle] = function reinitializeHandle(handle) {
637669
const originalServername = this.ssl ? this._handle.getServername() : null;
638670
const originalSession = this.ssl ? this._handle.getSession() : null;
639671

640-
this.handle = this._wrapHandle(null, handle);
672+
this.handle = this._wrapHandle(null, handle, false);
641673
this.ssl = this._handle;
642674

643675
net.Socket.prototype[kReinitializeHandle].call(this, this.handle);
Collapse file

‎src/crypto/crypto_tls.cc‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_tls.cc
+44-4Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -319,12 +319,15 @@ TLSWrap::TLSWrap(Environment* env,
319319
Local<Object> obj,
320320
Kind kind,
321321
StreamBase* stream,
322-
SecureContext* sc)
322+
SecureContext* sc,
323+
UnderlyingStreamWriteStatus under_stream_ws)
323324
: AsyncWrap(env, obj, AsyncWrap::PROVIDER_TLSWRAP),
324325
StreamBase(env),
325326
env_(env),
326327
kind_(kind),
327-
sc_(sc) {
328+
sc_(sc),
329+
has_active_write_issued_by_prev_listener_(
330+
under_stream_ws == UnderlyingStreamWriteStatus::kHasActive) {
328331
MakeWeak();
329332
CHECK(sc_);
330333
ssl_ = sc_->CreateSSL();
@@ -434,14 +437,19 @@ void TLSWrap::InitSSL() {
434437
void TLSWrap::Wrap(const FunctionCallbackInfo<Value>& args) {
435438
Environment* env = Environment::GetCurrent(args);
436439

437-
CHECK_EQ(args.Length(), 3);
440+
CHECK_EQ(args.Length(), 4);
438441
CHECK(args[0]->IsObject());
439442
CHECK(args[1]->IsObject());
440443
CHECK(args[2]->IsBoolean());
444+
CHECK(args[3]->IsBoolean());
441445

442446
Local<Object> sc = args[1].As<Object>();
443447
Kind kind = args[2]->IsTrue() ? Kind::kServer : Kind::kClient;
444448

449+
UnderlyingStreamWriteStatus under_stream_ws =
450+
args[3]->IsTrue() ? UnderlyingStreamWriteStatus::kHasActive
451+
: UnderlyingStreamWriteStatus::kVacancy;
452+
445453
StreamBase* stream = StreamBase::FromObject(args[0].As<Object>());
446454
CHECK_NOT_NULL(stream);
447455

@@ -452,7 +460,8 @@ void TLSWrap::Wrap(const FunctionCallbackInfo<Value>& args) {
452460
return;
453461
}
454462

455-
TLSWrap* res = new TLSWrap(env, obj, kind, stream, Unwrap<SecureContext>(sc));
463+
TLSWrap* res = new TLSWrap(
464+
env, obj, kind, stream, Unwrap<SecureContext>(sc), under_stream_ws);
456465

457466
args.GetReturnValue().Set(res->object());
458467
}
@@ -558,6 +567,13 @@ void TLSWrap::EncOut() {
558567
return;
559568
}
560569

570+
if (UNLIKELY(has_active_write_issued_by_prev_listener_)) {
571+
Debug(this,
572+
"Returning from EncOut(), "
573+
"has_active_write_issued_by_prev_listener_ is true");
574+
return;
575+
}
576+
561577
// Split-off queue
562578
if (established_ && current_write_) {
563579
Debug(this, "EncOut() write is scheduled");
@@ -628,6 +644,15 @@ void TLSWrap::EncOut() {
628644

629645
void TLSWrap::OnStreamAfterWrite(WriteWrap* req_wrap, int status) {
630646
Debug(this, "OnStreamAfterWrite(status = %d)", status);
647+
648+
if (UNLIKELY(has_active_write_issued_by_prev_listener_)) {
649+
Debug(this, "Notify write finish to the previous_listener_");
650+
CHECK_EQ(write_size_, 0); // we must have restrained writes
651+
652+
previous_listener_->OnStreamAfterWrite(req_wrap, status);
653+
return;
654+
}
655+
631656
if (current_empty_write_) {
632657
Debug(this, "Had empty write");
633658
BaseObjectPtr<AsyncWrap> current_empty_write =
@@ -1974,6 +1999,16 @@ void TLSWrap::GetALPNNegotiatedProto(const FunctionCallbackInfo<Value>& args) {
19741999
args.GetReturnValue().Set(result);
19752000
}
19762001

2002+
void TLSWrap::WritesIssuedByPrevListenerDone(
2003+
const FunctionCallbackInfo<Value>& args) {
2004+
TLSWrap* w;
2005+
ASSIGN_OR_RETURN_UNWRAP(&w, args.Holder());
2006+
2007+
Debug(w, "WritesIssuedByPrevListenerDone is called");
2008+
w->has_active_write_issued_by_prev_listener_ = false;
2009+
w->EncOut(); // resume all of our restrained writes
2010+
}
2011+
19772012
void TLSWrap::Cycle() {
19782013
// Prevent recursion
19792014
if (++cycle_depth_ > 1)
@@ -2050,6 +2085,10 @@ void TLSWrap::Initialize(
20502085
SetProtoMethod(isolate, t, "setSession", SetSession);
20512086
SetProtoMethod(isolate, t, "setVerifyMode", SetVerifyMode);
20522087
SetProtoMethod(isolate, t, "start", Start);
2088+
SetProtoMethod(isolate,
2089+
t,
2090+
"writesIssuedByPrevListenerDone",
2091+
WritesIssuedByPrevListenerDone);
20532092

20542093
SetProtoMethodNoSideEffect(
20552094
isolate, t, "exportKeyingMaterial", ExportKeyingMaterial);
@@ -2131,6 +2170,7 @@ void TLSWrap::RegisterExternalReferences(ExternalReferenceRegistry* registry) {
21312170
registry->Register(GetSharedSigalgs);
21322171
registry->Register(GetTLSTicket);
21332172
registry->Register(VerifyError);
2173+
registry->Register(WritesIssuedByPrevListenerDone);
21342174

21352175
#ifdef SSL_set_max_send_fragment
21362176
registry->Register(SetMaxSendFragment);
Collapse file

‎src/crypto/crypto_tls.h‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_tls.h
+8-1Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,8 @@ class TLSWrap : public AsyncWrap,
4848
kServer
4949
};
5050

51+
enum class UnderlyingStreamWriteStatus { kHasActive, kVacancy };
52+
5153
static void Initialize(v8::Local<v8::Object> target,
5254
v8::Local<v8::Value> unused,
5355
v8::Local<v8::Context> context,
@@ -136,7 +138,8 @@ class TLSWrap : public AsyncWrap,
136138
v8::Local<v8::Object> obj,
137139
Kind kind,
138140
StreamBase* stream,
139-
SecureContext* sc);
141+
SecureContext* sc,
142+
UnderlyingStreamWriteStatus under_stream_ws);
140143

141144
static void SSLInfoCallback(const SSL* ssl_, int where, int ret);
142145
void InitSSL();
@@ -216,6 +219,8 @@ class TLSWrap : public AsyncWrap,
216219
static void Start(const v8::FunctionCallbackInfo<v8::Value>& args);
217220
static void VerifyError(const v8::FunctionCallbackInfo<v8::Value>& args);
218221
static void Wrap(const v8::FunctionCallbackInfo<v8::Value>& args);
222+
static void WritesIssuedByPrevListenerDone(
223+
const v8::FunctionCallbackInfo<v8::Value>& args);
219224

220225
#ifdef SSL_set_max_send_fragment
221226
static void SetMaxSendFragment(
@@ -283,6 +288,8 @@ class TLSWrap : public AsyncWrap,
283288

284289
BIOPointer bio_trace_;
285290

291+
bool has_active_write_issued_by_prev_listener_ = false;
292+
286293
public:
287294
std::vector<unsigned char> alpn_protos_; // Accessed by SelectALPNCallback.
288295
};
Collapse file

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

Copy file name to clipboard
+58Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
'use strict';
2+
const common = require('../common');
3+
const assert = require('assert');
4+
if (!common.hasCrypto) common.skip('missing crypto');
5+
const fixtures = require('../common/fixtures');
6+
const tls = require('tls');
7+
8+
// In reality, this can be a HTTP CONNECT message, signaling the incoming
9+
// data is TLS encrypted
10+
const HEAD = 'XXXX';
11+
12+
const subserver = tls.createServer({
13+
key: fixtures.readKey('agent1-key.pem'),
14+
cert: fixtures.readKey('agent1-cert.pem'),
15+
})
16+
.on('secureConnection', common.mustCall(() => {
17+
process.exit(0);
18+
}));
19+
20+
const server = tls.createServer({
21+
key: fixtures.readKey('agent1-key.pem'),
22+
cert: fixtures.readKey('agent1-cert.pem'),
23+
})
24+
.listen(client)
25+
.on('secureConnection', (serverTlsSock) => {
26+
serverTlsSock.on('data', (chunk) => {
27+
assert.strictEqual(chunk.toString(), HEAD);
28+
subserver.emit('connection', serverTlsSock);
29+
});
30+
});
31+
32+
function client() {
33+
const down = tls.connect({
34+
host: '127.0.0.1',
35+
port: server.address().port,
36+
rejectUnauthorized: false
37+
}).on('secureConnect', () => {
38+
down.write(HEAD, common.mustSucceed());
39+
40+
// Sending tls data on a client TLSSocket with an active write led to a crash:
41+
//
42+
// node[16862]: ../src/crypto/crypto_tls.cc:963:virtual int node::crypto::TLSWrap::DoWrite(node::WriteWrap*,
43+
// uv_buf_t*, size_t, uv_stream_t*): Assertion `!current_write_' failed.
44+
// 1: 0xb090e0 node::Abort() [node]
45+
// 2: 0xb0915e [node]
46+
// 3: 0xca8413 node::crypto::TLSWrap::DoWrite(node::WriteWrap*, uv_buf_t*, unsigned long, uv_stream_s*) [node]
47+
// 4: 0xcaa549 node::StreamBase::Write(uv_buf_t*, unsigned long, uv_stream_s*, v8::Local<v8::Object>) [node]
48+
// 5: 0xca88d7 node::crypto::TLSWrap::EncOut() [node]
49+
// 6: 0xd3df3e [node]
50+
// 7: 0xd3f35f v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [node]
51+
// 8: 0x15d9ef9 [node]
52+
// Aborted
53+
tls.connect({
54+
socket: down,
55+
rejectUnauthorized: false
56+
});
57+
});
58+
}

0 commit comments

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