FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tls: fix bugs of double TLS · nodejs/node@821b11a · GitHub

/ node Public

Commit 821b11a

Browse files
ywave620
authored and
RafaelGSS
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 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>
1 parent b3a7427 commit 821b11a

7 files changed

Lines changed: 304 additions & 24 deletions

File tree

‎lib/_tls_wrap.js‎

Lines changed: 50 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -545,25 +545,36 @@ function TLSSocket(socket, opts) {
545545
this[kPendingSession] = null;
546546

547547
let wrap;
548-
if ((socket instanceof net.Socket && socket._handle) || !socket) {
549-
// 1. connected socket
550-
// 2. no socket, one will be created with net.Socket().connect
551-
wrap = socket;
548+
let handle;
549+
let wrapHasActiveWriteFromPrevOwner;
550+
551+
if (socket) {
552+
if (socket instanceof net.Socket && socket._handle) {
553+
// 1. connected socket
554+
wrap = socket;
555+
} else {
556+
// 2. socket has no handle so it is js not c++
557+
// 3. unconnected sockets are wrapped
558+
// TLS expects to interact from C++ with a net.Socket that has a C++ stream
559+
// handle, but a JS stream doesn't have one. Wrap it up to make it look like
560+
// a socket.
561+
wrap = new JSStreamSocket(socket);
562+
}
563+
564+
handle = wrap._handle;
565+
wrapHasActiveWriteFromPrevOwner = wrap.writableLength > 0;
552566
} else {
553-
// 3. socket has no handle so it is js not c++
554-
// 4. unconnected sockets are wrapped
555-
// TLS expects to interact from C++ with a net.Socket that has a C++ stream
556-
// handle, but a JS stream doesn't have one. Wrap it up to make it look like
557-
// a socket.
558-
wrap = new JSStreamSocket(socket);
567+
// 4. no socket, one will be created with net.Socket().connect
568+
wrap = null;
569+
wrapHasActiveWriteFromPrevOwner = false;
559570
}
560571

561572
// Just a documented property to make secure sockets
562573
// distinguishable from regular ones.
563574
this.encrypted = true;
564575

565576
ReflectApply(net.Socket, this, [{
566-
handle: this._wrapHandle(wrap),
577+
handle: this._wrapHandle(wrap, handle, wrapHasActiveWriteFromPrevOwner),
567578
allowHalfOpen: socket ? socket.allowHalfOpen : tlsOptions.allowHalfOpen,
568579
pauseOnCreate: tlsOptions.pauseOnConnect,
569580
manualStart: true,
@@ -582,6 +593,21 @@ function TLSSocket(socket, opts) {
582593
if (enableTrace && this._handle)
583594
this._handle.enableTrace();
584595

596+
if (wrapHasActiveWriteFromPrevOwner) {
597+
// `wrap` is a streams.Writable in JS. This empty write will be queued
598+
// and hence finish after all existing writes, which is the timing
599+
// we want to start to send any tls data to `wrap`.
600+
wrap.write('', (err) => {
601+
if (err) {
602+
debug('error got before writing any tls data to the underlying stream');
603+
this.destroy(err);
604+
return;
605+
}
606+
607+
this._handle.writesIssuedByPrevListenerDone();
608+
});
609+
}
610+
585611
// Read on next tick so the caller has a chance to setup listeners
586612
process.nextTick(initRead, this, socket);
587613
}
@@ -642,11 +668,14 @@ TLSSocket.prototype.disableRenegotiation = function disableRenegotiation() {
642668
this[kDisableRenegotiation] = true;
643669
};
644670

645-
TLSSocket.prototype._wrapHandle = function(wrap, handle) {
646-
if (!handle && wrap) {
647-
handle = wrap._handle;
648-
}
649-
671+
/**
672+
*
673+
* @param {null|net.Socket} wrap
674+
* @param {null|object} handle
675+
* @param {boolean} wrapHasActiveWriteFromPrevOwner
676+
* @returns {object}
677+
*/
678+
TLSSocket.prototype._wrapHandle = function(wrap, handle, wrapHasActiveWriteFromPrevOwner) {
650679
const options = this._tlsOptions;
651680
if (!handle) {
652681
handle = options.pipe ?
@@ -663,7 +692,10 @@ TLSSocket.prototype._wrapHandle = function(wrap, handle) {
663692
if (!(context.context instanceof NativeSecureContext)) {
664693
throw new ERR_TLS_INVALID_CONTEXT('context');
665694
}
666-
const res = tls_wrap.wrap(handle, context.context, !!options.isServer);
695+
696+
const res = tls_wrap.wrap(handle, context.context,
697+
!!options.isServer,
698+
wrapHasActiveWriteFromPrevOwner);
667699
res._parent = handle; // C++ "wrap" object: TCPWrap, JSStream, ...
668700
res._parentWrap = wrap; // JS object: net.Socket, JSStreamSocket, ...
669701
res._secureContext = context;
@@ -680,7 +712,7 @@ TLSSocket.prototype[kReinitializeHandle] = function reinitializeHandle(handle) {
680712
const originalServername = this.ssl ? this._handle.getServername() : null;
681713
const originalSession = this.ssl ? this._handle.getSession() : null;
682714

683-
this.handle = this._wrapHandle(null, handle);
715+
this.handle = this._wrapHandle(null, handle, false);
684716
this.ssl = this._handle;
685717

686718
net.Socket.prototype[kReinitializeHandle].call(this, this.handle);

‎src/crypto/crypto_tls.cc‎

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -357,12 +357,15 @@ TLSWrap::TLSWrap(Environment* env,
357357
Local<Object> obj,
358358
Kind kind,
359359
StreamBase* stream,
360-
SecureContext* sc)
360+
SecureContext* sc,
361+
UnderlyingStreamWriteStatus under_stream_ws)
361362
: AsyncWrap(env, obj, AsyncWrap::PROVIDER_TLSWRAP),
362363
StreamBase(env),
363364
env_(env),
364365
kind_(kind),
365-
sc_(sc) {
366+
sc_(sc),
367+
has_active_write_issued_by_prev_listener_(
368+
under_stream_ws == UnderlyingStreamWriteStatus::kHasActive) {
366369
MakeWeak();
367370
CHECK(sc_);
368371
ssl_ = sc_->CreateSSL();
@@ -472,14 +475,19 @@ void TLSWrap::InitSSL() {
472475
void TLSWrap::Wrap(const FunctionCallbackInfo<Value>& args) {
473476
Environment* env = Environment::GetCurrent(args);
474477

475-
CHECK_EQ(args.Length(), 3);
478+
CHECK_EQ(args.Length(), 4);
476479
CHECK(args[0]->IsObject());
477480
CHECK(args[1]->IsObject());
478481
CHECK(args[2]->IsBoolean());
482+
CHECK(args[3]->IsBoolean());
479483

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

487+
UnderlyingStreamWriteStatus under_stream_ws =
488+
args[3]->IsTrue() ? UnderlyingStreamWriteStatus::kHasActive
489+
: UnderlyingStreamWriteStatus::kVacancy;
490+
483491
StreamBase* stream = StreamBase::FromObject(args[0].As<Object>());
484492
CHECK_NOT_NULL(stream);
485493

@@ -490,7 +498,8 @@ void TLSWrap::Wrap(const FunctionCallbackInfo<Value>& args) {
490498
return;
491499
}
492500

493-
TLSWrap* res = new TLSWrap(env, obj, kind, stream, Unwrap<SecureContext>(sc));
501+
TLSWrap* res = new TLSWrap(
502+
env, obj, kind, stream, Unwrap<SecureContext>(sc), under_stream_ws);
494503

495504
args.GetReturnValue().Set(res->object());
496505
}
@@ -596,6 +605,13 @@ void TLSWrap::EncOut() {
596605
return;
597606
}
598607

608+
if (UNLIKELY(has_active_write_issued_by_prev_listener_)) {
609+
Debug(this,
610+
"Returning from EncOut(), "
611+
"has_active_write_issued_by_prev_listener_ is true");
612+
return;
613+
}
614+
599615
// Split-off queue
600616
if (established_ && current_write_) {
601617
Debug(this, "EncOut() write is scheduled");
@@ -666,6 +682,15 @@ void TLSWrap::EncOut() {
666682

667683
void TLSWrap::OnStreamAfterWrite(WriteWrap* req_wrap, int status) {
668684
Debug(this, "OnStreamAfterWrite(status = %d)", status);
685+
686+
if (UNLIKELY(has_active_write_issued_by_prev_listener_)) {
687+
Debug(this, "Notify write finish to the previous_listener_");
688+
CHECK_EQ(write_size_, 0); // we must have restrained writes
689+
690+
previous_listener_->OnStreamAfterWrite(req_wrap, status);
691+
return;
692+
}
693+
669694
if (current_empty_write_) {
670695
Debug(this, "Had empty write");
671696
BaseObjectPtr<AsyncWrap> current_empty_write =
@@ -2021,6 +2046,16 @@ void TLSWrap::GetALPNNegotiatedProto(const FunctionCallbackInfo<Value>& args) {
20212046
args.GetReturnValue().Set(result);
20222047
}
20232048

2049+
void TLSWrap::WritesIssuedByPrevListenerDone(
2050+
const FunctionCallbackInfo<Value>& args) {
2051+
TLSWrap* w;
2052+
ASSIGN_OR_RETURN_UNWRAP(&w, args.Holder());
2053+
2054+
Debug(w, "WritesIssuedByPrevListenerDone is called");
2055+
w->has_active_write_issued_by_prev_listener_ = false;
2056+
w->EncOut(); // resume all of our restrained writes
2057+
}
2058+
20242059
void TLSWrap::Cycle() {
20252060
// Prevent recursion
20262061
if (++cycle_depth_ > 1)
@@ -2098,6 +2133,10 @@ void TLSWrap::Initialize(
20982133
SetProtoMethod(isolate, t, "setSession", SetSession);
20992134
SetProtoMethod(isolate, t, "setVerifyMode", SetVerifyMode);
21002135
SetProtoMethod(isolate, t, "start", Start);
2136+
SetProtoMethod(isolate,
2137+
t,
2138+
"writesIssuedByPrevListenerDone",
2139+
WritesIssuedByPrevListenerDone);
21012140

21022141
SetProtoMethodNoSideEffect(
21032142
isolate, t, "exportKeyingMaterial", ExportKeyingMaterial);
@@ -2180,6 +2219,7 @@ void TLSWrap::RegisterExternalReferences(ExternalReferenceRegistry* registry) {
21802219
registry->Register(GetSharedSigalgs);
21812220
registry->Register(GetTLSTicket);
21822221
registry->Register(VerifyError);
2222+
registry->Register(WritesIssuedByPrevListenerDone);
21832223

21842224
#ifdef SSL_set_max_send_fragment
21852225
registry->Register(SetMaxSendFragment);

‎src/crypto/crypto_tls.h‎

Lines 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();
@@ -217,6 +220,8 @@ class TLSWrap : public AsyncWrap,
217220
static void Start(const v8::FunctionCallbackInfo<v8::Value>& args);
218221
static void VerifyError(const v8::FunctionCallbackInfo<v8::Value>& args);
219222
static void Wrap(const v8::FunctionCallbackInfo<v8::Value>& args);
223+
static void WritesIssuedByPrevListenerDone(
224+
const v8::FunctionCallbackInfo<v8::Value>& args);
220225

221226
#ifdef SSL_set_max_send_fragment
222227
static void SetMaxSendFragment(
@@ -284,6 +289,8 @@ class TLSWrap : public AsyncWrap,
284289

285290
BIOPointer bio_trace_;
286291

292+
bool has_active_write_issued_by_prev_listener_ = false;
293+
287294
public:
288295
std::vector<unsigned char> alpn_protos_; // Accessed by SelectALPNCallback.
289296
bool alpn_callback_enabled_ = false; // Accessed by SelectALPNCallback.

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

Lines 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)
Back | FazBrowse Home | New Git URL