tls: fix bugs of double TLS

lib/_tls_wrap.js

@@ -545,25 +545,36 @@ function TLSSocket(socket, opts) {
   this[kPendingSession] = null;
 
   let wrap;
-  if ((socket instanceof net.Socket && socket._handle) || !socket) {
-    // 1. connected socket
-    // 2. no socket, one will be created with net.Socket().connect
-    wrap = socket;
+  let handle;
+  let wrapHasActiveWriteFromPrevOwner;
+
+  if (socket) {
+    if (socket instanceof net.Socket && socket._handle) {
+      // 1. connected socket
+      wrap = socket;
+    } else {
+      // 2. socket has no handle so it is js not c++
+      // 3. unconnected sockets are wrapped
+      // TLS expects to interact from C++ with a net.Socket that has a C++ stream
+      // handle, but a JS stream doesn't have one. Wrap it up to make it look like
+      // a socket.
+      wrap = new JSStreamSocket(socket);
+    }
+
+    handle = wrap._handle;
+    wrapHasActiveWriteFromPrevOwner = wrap.writableLength > 0;
   } else {
-    // 3. socket has no handle so it is js not c++
-    // 4. unconnected sockets are wrapped
-    // TLS expects to interact from C++ with a net.Socket that has a C++ stream
-    // handle, but a JS stream doesn't have one. Wrap it up to make it look like
-    // a socket.
-    wrap = new JSStreamSocket(socket);
+    // 4. no socket, one will be created with net.Socket().connect
+    wrap = null;
+    wrapHasActiveWriteFromPrevOwner = false;
   }
 
   // Just a documented property to make secure sockets
   // distinguishable from regular ones.
   this.encrypted = true;
 
   ReflectApply(net.Socket, this, [{
-    handle: this._wrapHandle(wrap),
+    handle: this._wrapHandle(wrap, handle, wrapHasActiveWriteFromPrevOwner),
     allowHalfOpen: socket ? socket.allowHalfOpen : tlsOptions.allowHalfOpen,
     pauseOnCreate: tlsOptions.pauseOnConnect,
     manualStart: true,
@@ -582,6 +593,21 @@ function TLSSocket(socket, opts) {
   if (enableTrace && this._handle)
     this._handle.enableTrace();
 
+  if (wrapHasActiveWriteFromPrevOwner) {
+    // `wrap` is a streams.Writable in JS. This empty write will be queued
+    // and hence finish after all existing writes, which is the timing
+    // we want to start to send any tls data to `wrap`.
+    wrap.write('', (err) => {
+      if (err) {
+        debug('error got before writing any tls data to the underlying stream');
+        this.destroy(err);
+        return;
+      }
+
+      this._handle.writesIssuedByPrevListenerDone();
+    });
+  }
+
   // Read on next tick so the caller has a chance to setup listeners
   process.nextTick(initRead, this, socket);
 }
@@ -642,11 +668,14 @@ TLSSocket.prototype.disableRenegotiation = function disableRenegotiation() {
   this[kDisableRenegotiation] = true;
 };
 
-TLSSocket.prototype._wrapHandle = function(wrap, handle) {
-  if (!handle && wrap) {
-    handle = wrap._handle;
-  }
-
+/**
+ *
+ * @param {null|net.Socket} wrap
+ * @param {null|object} handle
+ * @param {boolean} wrapHasActiveWriteFromPrevOwner
+ * @returns {object}
+ */
+TLSSocket.prototype._wrapHandle = function(wrap, handle, wrapHasActiveWriteFromPrevOwner) {
   const options = this._tlsOptions;
   if (!handle) {
     handle = options.pipe ?
@@ -663,7 +692,10 @@ TLSSocket.prototype._wrapHandle = function(wrap, handle) {
   if (!(context.context instanceof NativeSecureContext)) {
     throw new ERR_TLS_INVALID_CONTEXT('context');
   }
-  const res = tls_wrap.wrap(handle, context.context, !!options.isServer);
+
+  const res = tls_wrap.wrap(handle, context.context,
+                            !!options.isServer,
+                            wrapHasActiveWriteFromPrevOwner);
   res._parent = handle;  // C++ "wrap" object: TCPWrap, JSStream, ...
   res._parentWrap = wrap;  // JS object: net.Socket, JSStreamSocket, ...
   res._secureContext = context;
@@ -680,7 +712,7 @@ TLSSocket.prototype[kReinitializeHandle] = function reinitializeHandle(handle) {
   const originalServername = this.ssl ? this._handle.getServername() : null;
   const originalSession = this.ssl ? this._handle.getSession() : null;
 
-  this.handle = this._wrapHandle(null, handle);
+  this.handle = this._wrapHandle(null, handle, false);
   this.ssl = this._handle;
 
   net.Socket.prototype[kReinitializeHandle].call(this, this.handle);

src/crypto/crypto_tls.cc

@@ -357,12 +357,15 @@ TLSWrap::TLSWrap(Environment* env,
                  Local<Object> obj,
                  Kind kind,
                  StreamBase* stream,
-                 SecureContext* sc)
+                 SecureContext* sc,
+                 UnderlyingStreamWriteStatus under_stream_ws)
     : AsyncWrap(env, obj, AsyncWrap::PROVIDER_TLSWRAP),
       StreamBase(env),
       env_(env),
       kind_(kind),
-      sc_(sc) {
+      sc_(sc),
+      has_active_write_issued_by_prev_listener_(
+          under_stream_ws == UnderlyingStreamWriteStatus::kHasActive) {
   MakeWeak();
   CHECK(sc_);
   ssl_ = sc_->CreateSSL();
@@ -472,14 +475,19 @@ void TLSWrap::InitSSL() {
 void TLSWrap::Wrap(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
 
-  CHECK_EQ(args.Length(), 3);
+  CHECK_EQ(args.Length(), 4);
   CHECK(args[0]->IsObject());
   CHECK(args[1]->IsObject());
   CHECK(args[2]->IsBoolean());
+  CHECK(args[3]->IsBoolean());
 
   Local<Object> sc = args[1].As<Object>();
   Kind kind = args[2]->IsTrue() ? Kind::kServer : Kind::kClient;
 
+  UnderlyingStreamWriteStatus under_stream_ws =
+      args[3]->IsTrue() ? UnderlyingStreamWriteStatus::kHasActive
+                        : UnderlyingStreamWriteStatus::kVacancy;
+
   StreamBase* stream = StreamBase::FromObject(args[0].As<Object>());
   CHECK_NOT_NULL(stream);
 
@@ -490,7 +498,8 @@ void TLSWrap::Wrap(const FunctionCallbackInfo<Value>& args) {
     return;
   }
 
-  TLSWrap* res = new TLSWrap(env, obj, kind, stream, Unwrap<SecureContext>(sc));
+  TLSWrap* res = new TLSWrap(
+      env, obj, kind, stream, Unwrap<SecureContext>(sc), under_stream_ws);
 
   args.GetReturnValue().Set(res->object());
 }
@@ -596,6 +605,13 @@ void TLSWrap::EncOut() {
     return;
   }
 
+  if (UNLIKELY(has_active_write_issued_by_prev_listener_)) {
+    Debug(this,
+          "Returning from EncOut(), "
+          "has_active_write_issued_by_prev_listener_ is true");
+    return;
+  }
+
   // Split-off queue
   if (established_ && current_write_) {
     Debug(this, "EncOut() write is scheduled");
@@ -666,6 +682,15 @@ void TLSWrap::EncOut() {
 
 void TLSWrap::OnStreamAfterWrite(WriteWrap* req_wrap, int status) {
   Debug(this, "OnStreamAfterWrite(status = %d)", status);
+
+  if (UNLIKELY(has_active_write_issued_by_prev_listener_)) {
+    Debug(this, "Notify write finish to the previous_listener_");
+    CHECK_EQ(write_size_, 0);  // we must have restrained writes
+
+    previous_listener_->OnStreamAfterWrite(req_wrap, status);
+    return;
+  }
+
   if (current_empty_write_) {
     Debug(this, "Had empty write");
     BaseObjectPtr<AsyncWrap> current_empty_write =
@@ -2021,6 +2046,16 @@ void TLSWrap::GetALPNNegotiatedProto(const FunctionCallbackInfo<Value>& args) {
   args.GetReturnValue().Set(result);
 }
 
+void TLSWrap::WritesIssuedByPrevListenerDone(
+    const FunctionCallbackInfo<Value>& args) {
+  TLSWrap* w;
+  ASSIGN_OR_RETURN_UNWRAP(&w, args.Holder());
+
+  Debug(w, "WritesIssuedByPrevListenerDone is called");
+  w->has_active_write_issued_by_prev_listener_ = false;
+  w->EncOut();  // resume all of our restrained writes
+}
+
 void TLSWrap::Cycle() {
   // Prevent recursion
   if (++cycle_depth_ > 1)
@@ -2098,6 +2133,10 @@ void TLSWrap::Initialize(
   SetProtoMethod(isolate, t, "setSession", SetSession);
   SetProtoMethod(isolate, t, "setVerifyMode", SetVerifyMode);
   SetProtoMethod(isolate, t, "start", Start);
+  SetProtoMethod(isolate,
+                 t,
+                 "writesIssuedByPrevListenerDone",
+                 WritesIssuedByPrevListenerDone);
 
   SetProtoMethodNoSideEffect(
       isolate, t, "exportKeyingMaterial", ExportKeyingMaterial);
@@ -2180,6 +2219,7 @@ void TLSWrap::RegisterExternalReferences(ExternalReferenceRegistry* registry) {
   registry->Register(GetSharedSigalgs);
   registry->Register(GetTLSTicket);
   registry->Register(VerifyError);
+  registry->Register(WritesIssuedByPrevListenerDone);
 
 #ifdef SSL_set_max_send_fragment
   registry->Register(SetMaxSendFragment);

src/crypto/crypto_tls.h

@@ -48,6 +48,8 @@ class TLSWrap : public AsyncWrap,
     kServer
   };
 
+  enum class UnderlyingStreamWriteStatus { kHasActive, kVacancy };
+
   static void Initialize(v8::Local<v8::Object> target,
                          v8::Local<v8::Value> unused,
                          v8::Local<v8::Context> context,
@@ -136,7 +138,8 @@ class TLSWrap : public AsyncWrap,
           v8::Local<v8::Object> obj,
           Kind kind,
           StreamBase* stream,
-          SecureContext* sc);
+          SecureContext* sc,
+          UnderlyingStreamWriteStatus under_stream_ws);
 
   static void SSLInfoCallback(const SSL* ssl_, int where, int ret);
   void InitSSL();
@@ -217,6 +220,8 @@ class TLSWrap : public AsyncWrap,
   static void Start(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void VerifyError(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void Wrap(const v8::FunctionCallbackInfo<v8::Value>& args);
+  static void WritesIssuedByPrevListenerDone(
+      const v8::FunctionCallbackInfo<v8::Value>& args);
 
 #ifdef SSL_set_max_send_fragment
   static void SetMaxSendFragment(
@@ -284,6 +289,8 @@ class TLSWrap : public AsyncWrap,
 
   BIOPointer bio_trace_;
 
+  bool has_active_write_issued_by_prev_listener_ = false;
+
  public:
   std::vector<unsigned char> alpn_protos_;  // Accessed by SelectALPNCallback.
   bool alpn_callback_enabled_ = false;      // Accessed by SelectALPNCallback.

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

@@ -0,0 +1,58 @@
+'use strict';
+const common = require('../common');
+const assert = require('assert');
+if (!common.hasCrypto) common.skip('missing crypto');
+const fixtures = require('../common/fixtures');
+const tls = require('tls');
+
+// In reality, this can be a HTTP CONNECT message, signaling the incoming
+// data is TLS encrypted
+const HEAD = 'XXXX';
+
+const subserver = tls.createServer({
+  key: fixtures.readKey('agent1-key.pem'),
+  cert: fixtures.readKey('agent1-cert.pem'),
+})
+  .on('secureConnection', common.mustCall(() => {
+    process.exit(0);
+  }));
+
+const server = tls.createServer({
+  key: fixtures.readKey('agent1-key.pem'),
+  cert: fixtures.readKey('agent1-cert.pem'),
+})
+  .listen(client)
+  .on('secureConnection', (serverTlsSock) => {
+    serverTlsSock.on('data', (chunk) => {
+      assert.strictEqual(chunk.toString(), HEAD);
+      subserver.emit('connection', serverTlsSock);
+    });
+  });
+
+function client() {
+  const down = tls.connect({
+    host: '127.0.0.1',
+    port: server.address().port,
+    rejectUnauthorized: false
+  }).on('secureConnect', () => {
+    down.write(HEAD, common.mustSucceed());
+
+    // Sending tls data on a client TLSSocket with an active write led to a crash:
+    //
+    //  node[16862]: ../src/crypto/crypto_tls.cc:963:virtual int node::crypto::TLSWrap::DoWrite(node::WriteWrap*,
+    //    uv_buf_t*, size_t, uv_stream_t*): Assertion `!current_write_' failed.
+    //  1: 0xb090e0 node::Abort() [node]
+    //  2: 0xb0915e  [node]
+    //  3: 0xca8413 node::crypto::TLSWrap::DoWrite(node::WriteWrap*, uv_buf_t*, unsigned long, uv_stream_s*) [node]
+    //  4: 0xcaa549 node::StreamBase::Write(uv_buf_t*, unsigned long, uv_stream_s*, v8::Local<v8::Object>) [node]
+    //  5: 0xca88d7 node::crypto::TLSWrap::EncOut() [node]
+    //  6: 0xd3df3e  [node]
+    //  7: 0xd3f35f v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [node]
+    //  8: 0x15d9ef9  [node]
+    // Aborted
+    tls.connect({
+      socket: down,
+      rejectUnauthorized: false
+    });
+  });
+}