From 864b97b24dc07c13fafd2cd717e33035c28e67e2 Mon Sep 17 00:00:00 2001
From: Mateusz Krawczuk <krawczukmat@gmail.com>
Date: Thu, 6 Aug 2020 00:32:07 +0200
Subject: [PATCH] tls: use recently added matching SecureContext in default
 SNICallback

PR-URL: https://github.com/nodejs/node/pull/36072
Fixes: https://github.com/nodejs/node/issues/34110
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
---
 doc/api/tls.md                                |  3 +
 lib/_tls_wrap.js                              |  3 +-
 .../test-tls-secure-context-usage-order.js    | 99 +++++++++++++++++++
 3 files changed, 104 insertions(+), 1 deletion(-)
 create mode 100644 test/parallel/test-tls-secure-context-usage-order.js

diff --git a/doc/api/tls.md b/doc/api/tls.md
index bedf4e28e7bc04..2c8414f2988c9c 100644
--- a/doc/api/tls.md
+++ b/doc/api/tls.md
@@ -627,6 +627,9 @@ added: v0.5.3
 The `server.addContext()` method adds a secure context that will be used if
 the client request's SNI name matches the supplied `hostname` (or wildcard).
 
+When there are multiple matching contexts, the most recently added one is
+used.
+
 ### `server.address()`
 <!-- YAML
 added: v0.6.0
diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index 1447253bf73224..b5fbe4d36b2578 100644
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -1458,7 +1458,8 @@ Server.prototype[EE.captureRejectionSymbol] = function(
 function SNICallback(servername, callback) {
   const contexts = this.server._contexts;
 
-  for (const elem of contexts) {
+  for (let i = contexts.length - 1; i >= 0; --i) {
+    const elem = contexts[i];
     if (RegExpPrototypeTest(elem[0], servername)) {
       callback(null, elem[1]);
       return;
diff --git a/test/parallel/test-tls-secure-context-usage-order.js b/test/parallel/test-tls-secure-context-usage-order.js
new file mode 100644
index 00000000000000..c79a3eac775822
--- /dev/null
+++ b/test/parallel/test-tls-secure-context-usage-order.js
@@ -0,0 +1,99 @@
+'use strict';
+const common = require('../common');
+const fixtures = require('../common/fixtures');
+
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+// This test ensures that when a TLS connection is established, the server
+// selects the most recently added SecureContext that matches the servername.
+
+const assert = require('assert');
+const tls = require('tls');
+
+function loadPEM(n) {
+  return fixtures.readKey(`${n}.pem`);
+}
+
+const serverOptions = {
+  key: loadPEM('agent2-key'),
+  cert: loadPEM('agent2-cert'),
+  requestCert: true,
+  rejectUnauthorized: false,
+};
+
+const badSecureContext = {
+  key: loadPEM('agent1-key'),
+  cert: loadPEM('agent1-cert'),
+  ca: [ loadPEM('ca2-cert') ]
+};
+
+const goodSecureContext = {
+  key: loadPEM('agent1-key'),
+  cert: loadPEM('agent1-cert'),
+  ca: [ loadPEM('ca1-cert') ]
+};
+
+const server = tls.createServer(serverOptions, (c) => {
+  // The 'a' and 'b' subdomains are used to distinguish between client
+  // connections.
+  // Connection to subdomain 'a' is made when the 'bad' secure context is
+  // the only one in use.
+  if ('a.example.com' === c.servername) {
+    assert.strictEqual(c.authorized, false);
+  }
+  // Connection to subdomain 'b' is made after the 'good' context has been
+  // added.
+  if ('b.example.com' === c.servername) {
+    assert.strictEqual(c.authorized, true);
+  }
+});
+
+// 1. Add the 'bad' secure context. A connection using this context will not be
+// authorized.
+server.addContext('*.example.com', badSecureContext);
+
+server.listen(0, () => {
+  const options = {
+    port: server.address().port,
+    key: loadPEM('agent1-key'),
+    cert: loadPEM('agent1-cert'),
+    ca: [loadPEM('ca1-cert')],
+    servername: 'a.example.com',
+    rejectUnauthorized: false,
+  };
+
+  // 2. Make a connection using servername 'a.example.com'. Since a 'bad'
+  // secure context is used, this connection should not be authorized.
+  const client = tls.connect(options, () => {
+    client.end();
+  });
+
+  client.on('close', common.mustCall(() => {
+    // 3. Add a 'good' secure context.
+    server.addContext('*.example.com', goodSecureContext);
+
+    options.servername = 'b.example.com';
+    // 4. Make a connection using servername 'b.example.com'. This connection
+    // should be authorized because the 'good' secure context is the most
+    // recently added matching context.
+
+    const other = tls.connect(options, () => {
+      other.end();
+    });
+
+    other.on('close', common.mustCall(() => {
+      // 5. Make another connection using servername 'b.example.com' to ensure
+      // that the array of secure contexts is not reversed in place with each
+      // SNICallback call, as someone might be tempted to refactor this piece of
+      // code by using Array.prototype.reverse() method.
+      const onemore = tls.connect(options, () => {
+        onemore.end();
+      });
+
+      onemore.on('close', common.mustCall(() => {
+        server.close();
+      }));
+    }));
+  }));
+});