tls: destroy sockets after fatal errors

lib/internal/tls/wrap.js

@@ -593,8 +593,10 @@ function onerror(err) {
     // Ignore server's authorization errors
     owner.destroy();
   } else {
-    // Emit error
-    owner._emitTLSError(err);
+    // A failed TLS connection cannot make progress on pending writes.
+    // Preserve connection details until the error has been handled.
+    owner._closeAfterHandlingError = true;
+    owner.destroy(err);
   }
 }
 

lib/net.js

@@ -368,12 +368,13 @@ function closeSocketHandle(self, isException, isCleanupPending = false) {
   if (self._handle) {
     self._handle.close(() => {
       debug('emit close');
-      self.emit('close', isException);
       if (isCleanupPending) {
+        // The handle is closed and must not be used by close listeners.
         self._handle.onread = noop;
         self._handle = null;
         self._sockname = null;
       }
+      self.emit('close', isException);
     });
   }
 }

test/parallel/test-http2-tls-fatal-alert.js

@@ -0,0 +1,63 @@
+'use strict';
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const assert = require('assert');
+const http2 = require('http2');
+const tls = require('tls');
+const fixtures = require('../common/fixtures');
+
+for (const customConnection of [false, true]) {
+  const server = http2.createSecureServer({
+    key: fixtures.readKey('agent1-key.pem'),
+    cert: fixtures.readKey('agent1-cert.pem'),
+    requestCert: true,
+    rejectUnauthorized: true,
+    minVersion: 'TLSv1.3',
+    maxVersion: 'TLSv1.3',
+  });
+  server.on('tlsClientError', common.mustCall((err) => {
+    assert.strictEqual(err.code, 'ERR_SSL_PEER_DID_NOT_RETURN_A_CERTIFICATE');
+  }));
+  server.listen(0, '127.0.0.1', common.mustCall(() => {
+    const options = {
+      host: '127.0.0.1',
+      port: server.address().port,
+      servername: 'localhost',
+      rejectUnauthorized: false,
+      ALPNProtocols: ['h2'],
+      minVersion: 'TLSv1.3',
+      maxVersion: 'TLSv1.3',
+    };
+    const socket = customConnection ? tls.connect(options) : undefined;
+    const session = http2.connect(`https://127.0.0.1:${server.address().port}`, {
+      ...options,
+      ...(customConnection ? { createConnection: () => socket } : {}),
+    });
+    const transport = socket || session.socket;
+    // Destroying the socket must not emit the TLS error a second time.
+    transport.on('error', common.mustCall());
+    let errorEmitted = false;
+    session.on('connect', common.mustCall(() => {
+      assert.strictEqual(transport.getProtocol(), 'TLSv1.3');
+    }));
+    session.on('error', common.mustCall((err) => {
+      // OpenSSL and BoringSSL use different names for the same TLS alert.
+      assert.ok([
+        'ERR_SSL_TLSV13_ALERT_CERTIFICATE_REQUIRED',
+        'ERR_SSL_TLSV1_ALERT_CERTIFICATE_REQUIRED',
+      ].includes(err.code), `Unexpected TLS error code: ${err.code}`);
+      errorEmitted = true;
+    }));
+    transport.on('close', common.mustCall(function() {
+      // The session's socket proxy may already be detached.
+      assert(this.destroyed);
+    }));
+    session.on('close', common.mustCall(() => {
+      assert(errorEmitted);
+      assert(session.destroyed);
+      if (socket) assert(socket.destroyed);
+      server.close(common.mustCall());
+    }));
+  }));
+}

test/parallel/test-tls-client-error-close.js

@@ -0,0 +1,42 @@
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const assert = require('assert');
+const net = require('net');
+const tls = require('tls');
+const fixtures = require('../common/fixtures');
+
+const server = tls.createServer({
+  key: fixtures.readKey('agent1-key.pem'),
+  cert: fixtures.readKey('agent1-cert.pem'),
+}, common.mustNotCall());
+
+server.on('tlsClientError', common.mustCall((err, socket) => {
+  assert.match(err.code, /^ERR_SSL_/);
+  assert(socket.destroyed);
+  // Connection details must remain available while handling the error.
+  assert.strictEqual(socket.remoteAddress, '127.0.0.1');
+  assert.strictEqual(socket.remoteFamily, 'IPv4');
+  assert(socket.remotePort > 0);
+  socket.on('close', common.mustCall(() => {
+    // The SSL state has been freed before close listeners run.
+    assert.strictEqual(socket.getProtocol(), null);
+    server.close(common.mustCall());
+  }));
+}));
+
+server.listen(0, '127.0.0.1', common.mustCall(() => {
+  const client = net.connect(server.address().port, '127.0.0.1');
+  client.on('connect', common.mustCall(() => {
+    // Reject the handshake without a TLS client.
+    client.write(Buffer.alloc(64, 42));
+  }));
+  client.on('error', common.mustCallAtLeast((err) => {
+    assert.strictEqual(err.code, 'ECONNRESET');
+  }, 0));
+  client.on('close', common.mustCall());
+  client.resume();
+}));

test/parallel/test-tls-fatal-error-close.js

@@ -0,0 +1,59 @@
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const assert = require('assert');
+const net = require('net');
+const tls = require('tls');
+const fixtures = require('../common/fixtures');
+
+const server = tls.createServer({
+  key: fixtures.readKey('agent1-key.pem'),
+  cert: fixtures.readKey('agent1-cert.pem'),
+  minVersion: 'TLSv1.3',
+  maxVersion: 'TLSv1.3',
+}, common.mustCall((socket) => {
+  const remotePort = socket.remotePort;
+  let errorEmitted = false;
+  socket.on('error', common.mustCall((err) => {
+    assert.match(err.code, /^ERR_SSL_/);
+    assert(socket.destroyed);
+    assert.strictEqual(socket.errored, err);
+    // Destroy the socket while preserving connection details for this handler.
+    assert.strictEqual(socket.remoteAddress, '127.0.0.1');
+    assert.strictEqual(socket.remoteFamily, 'IPv4');
+    assert.strictEqual(socket.remotePort, remotePort);
+    assert.strictEqual(socket.getProtocol(), 'TLSv1.3');
+    errorEmitted = true;
+  }));
+  socket.on('close', common.mustCall(() => {
+    assert(errorEmitted);
+    assert(socket.destroyed);
+    assert.strictEqual(socket.getProtocol(), null);
+    client.destroy();
+    server.close(common.mustCall());
+  }));
+}));
+server.on('tlsClientError', common.mustNotCall());
+
+let client;
+server.listen(0, '127.0.0.1', common.mustCall(() => {
+  const transport = net.connect(server.address().port, '127.0.0.1');
+  client = tls.connect({
+    socket: transport,
+    rejectUnauthorized: false,
+    minVersion: 'TLSv1.3',
+    maxVersion: 'TLSv1.3',
+  });
+  client.on('error', () => {});
+  client.on('close', common.mustCall());
+  server.once('secureConnection', common.mustCall(() => {
+    // Corrupt an application-data record without closing the TCP connection.
+    // The server must close on the TLS error without application cleanup.
+    const record = Buffer.alloc(37);
+    record.set([23, 3, 3, 0, 32]);
+    transport.write(record);
+  }));
+}));