tls: don't trigger SNICallback or OCSPRequest from the TLS lib stack

Both events (backed by oncertcb) could potentially write to the socket
synchronously, re-entering SSL mid-handshake and breaking the
connection, so we defer them just like the new 'resumeSession'
behaviour.

Also fixes a small bug in the error path of EmitClientHello, which now
bails out more aggressively instead of resuming handshakes in a V8
teardown scenario.

Co-authored-by: Filip Skokan <panva.ip@gmail.com>
Signed-off-by: Tim Perry <pimterry@gmail.com>

Author: pimterry

src/crypto/crypto_tls.cc

@@ -218,32 +218,18 @@ int SSLCertCallback(SSL* s, void* arg) {
     // handshake will continue after certcb is done.
     return -1;
 
-  Environment* env = w->env();
-  HandleScope handle_scope(env->isolate());
-  Context::Scope context_scope(env->context());
   w->set_cert_cb_running();
 
-  Local<Object> info = Object::New(env->isolate());
+  // The view points into SSL-owned memory, so copy it before deferring.
+  std::string servername;
+  if (auto name = SSLPointer::GetServerName(s)) servername = *name;
 
-  auto servername = SSLPointer::GetServerName(s);
-  Local<String> servername_str =
-      !servername.has_value()
-          ? String::Empty(env->isolate())
-          : OneByteString(env->isolate(), servername.value());
-
-  Local<Value> ocsp = Boolean::New(
-      env->isolate(), SSL_get_tlsext_status_type(s) == TLSEXT_STATUSTYPE_ocsp);
+  w->ScheduleCertCb(std::move(servername),
+                    SSL_get_tlsext_status_type(s) == TLSEXT_STATUSTYPE_ocsp);
 
-  if (info->Set(env->context(), env->servername_string(), servername_str)
-          .IsNothing() ||
-      info->Set(env->context(), env->ocsp_request_string(), ocsp).IsNothing()) {
-    return 1;
-  }
-
-  Local<Value> argv[] = { info };
-  w->MakeCallback(env->oncertcb_string(), arraysize(argv), argv);
-
-  return w->is_cert_cb_running() ? -1 : 1;
+  // Suspend handshake with SSL_ERROR_WANT_X509_LOOKUP, and handshake will
+  // continue after certcb is done.
+  return -1;
 }
 
 int SelectALPNCallback(
@@ -519,16 +505,48 @@ void TLSWrap::EmitClientHello(const std::vector<unsigned char>& session_id,
                 env->tls_ticket_string(),
                 Boolean::New(env->isolate(), has_ticket))
           .IsNothing()) {
-    // Continue the handshake unresumed rather than leaving it suspended.
-    hello_answered_ = true;
-    Cycle();
+    // An exception is pending, so don't re-enter SSL or JS to resume.
     return;
   }
 
   Local<Value> argv[] = {hello_obj};
   MakeCallback(env->onclienthello_string(), arraysize(argv), argv);
 }
 
+// As with the ClientHello, JS must not run on the library's stack: 'oncertcb'
+// handlers synchronously call back into the handle to resume the handshake.
+void TLSWrap::ScheduleCertCb(std::string servername, bool ocsp) {
+  Debug(this, "Scheduling oncertcb");
+  BaseObjectPtr<TLSWrap> strong_ref{this};
+  env()->SetImmediate(
+      [this, strong_ref, servername = std::move(servername), ocsp](
+          Environment* env) {
+        if (ssl_) EmitCertCb(servername, ocsp);
+      });
+}
+
+void TLSWrap::EmitCertCb(const std::string& servername, bool ocsp) {
+  Debug(this, "Emitting oncertcb");
+  Environment* env = this->env();
+  HandleScope handle_scope(env->isolate());
+  Context::Scope context_scope(env->context());
+
+  Local<Object> info = Object::New(env->isolate());
+  if (info->Set(env->context(),
+                env->servername_string(),
+                OneByteString(env->isolate(), servername))
+          .IsNothing() ||
+      info->Set(env->context(),
+                env->ocsp_request_string(),
+                Boolean::New(env->isolate(), ocsp))
+          .IsNothing()) {
+    return;
+  }
+
+  Local<Value> argv[] = {info};
+  MakeCallback(env->oncertcb_string(), arraysize(argv), argv);
+}
+
 void TLSWrap::InitSSL() {
   // Initialize SSL – OpenSSL takes ownership of these.
   enc_in_ = NodeBIO::New(env()).release();

src/crypto/crypto_tls.h

@@ -115,6 +115,9 @@ class TLSWrap : public AsyncWrap,
                           size_t session_id_len,
                           bool has_ticket);
 
+  // Schedules 'oncertcb'. The handshake stays suspended until certCbDone().
+  void ScheduleCertCb(std::string servername, bool ocsp);
+
   // Implement MemoryRetainer:
   void MemoryInfo(MemoryTracker* tracker) const override;
   SET_MEMORY_INFO_NAME(TLSWrap)
@@ -149,6 +152,7 @@ class TLSWrap : public AsyncWrap,
   void WaitForCertCb(CertCb cb, void* arg);
   void EmitClientHello(const std::vector<unsigned char>& session_id,
                        bool has_ticket);
+  void EmitCertCb(const std::string& servername, bool ocsp);
 
   TLSWrap(Environment* env,
           v8::Local<v8::Object> obj,

test/parallel/test-tls-certcb-sync-write.js

@@ -0,0 +1,49 @@
+'use strict';
+
+// Writing to a server TLSSocket synchronously from inside an SNICallback,
+// while the handshake is still waiting on the certificate callback, must not
+// break the connection; the data must be delivered once the handshake ends.
+
+const common = require('../common');
+
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const assert = require('assert');
+const fixtures = require('../common/fixtures');
+const net = require('net');
+const tls = require('tls');
+
+const secureContext = tls.createSecureContext({
+  key: fixtures.readKey('rsa_private.pem'),
+  cert: fixtures.readKey('rsa_cert.crt'),
+});
+
+let serverSocket;
+const server = net.createServer(common.mustCall((raw) => {
+  serverSocket = new tls.TLSSocket(raw, {
+    isServer: true,
+    secureContext,
+    SNICallback: common.mustCall((servername, callback) => {
+      assert.strictEqual(servername, 'localhost');
+      serverSocket.write('from-mid-handshake');
+      callback(null, null);
+    }),
+  });
+  serverSocket.on('error', common.mustNotCall());
+}));
+
+server.listen(0, common.mustCall(() => {
+  const client = tls.connect({
+    port: server.address().port,
+    servername: 'localhost',
+    rejectUnauthorized: false,
+  }, common.mustCall(() => {
+    client.on('data', common.mustCall((data) => {
+      assert.strictEqual(data.toString(), 'from-mid-handshake');
+      client.end();
+      server.close();
+    }));
+  }));
+  client.on('error', common.mustNotCall());
+}));