From e0fe6a635e5929a364986a6c39dc3585b9ddcd85 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= <tniessen@tnie.de>
Date: Wed, 29 Dec 2021 20:23:11 -0500
Subject: [PATCH] tls: drop support for URI alternative names

Previously, Node.js incorrectly accepted uniformResourceIdentifier (URI)
subject alternative names in checkServerIdentity regardless of the
application protocol. This was incorrect even in the most common cases.
For example, RFC 2818 specifies (and RFC 6125 confirms) that HTTP over
TLS only uses dNSName and iPAddress subject alternative names, but not
uniformResourceIdentifier subject alternative names.

Additionally, name constrained certificate authorities might not be
constrained to specific URIs, allowing them to issue certificates for
URIs that specify hosts that they would not be allowed to issue dNSName
certificates for.

Even for application protocols that make use of URI subject alternative
names (such as SIP, see RFC 5922), Node.js did not implement the
required checks correctly, for example, because checkServerIdentity
ignores the URI scheme.

As a side effect, this also fixes an edge case. When a hostname is not
an IP address and no dNSName subject alternative name exists, the
subject's Common Name should be considered even when an iPAddress
subject alternative name exists.

It remains possible for users to pass a custom checkServerIdentity
function to the TLS implementation in order to implement custom identity
verification logic.

This addresses CVE-2021-44531.

Co-authored-by: Akshay K <iit.akshay@gmail.com>
CVE-ID: CVE-2021-44531
Backport-PR-URL: https://github.com/nodejs-private/node-private/pull/306
PR-URL: https://github.com/nodejs-private/node-private/pull/300
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
---
 doc/api/tls.md                                | 12 +++++++
 lib/tls.js                                    | 33 +++--------------
 test/fixtures/keys/Makefile                   | 14 ++++++++
 .../irrelevant_san_correct_subject-cert.pem   | 11 ++++++
 .../irrelevant_san_correct_subject-key.pem    |  5 +++
 .../test-tls-check-server-identity.js         | 28 ++++-----------
 test/parallel/test-x509-escaping.js           | 36 ++++++++++++++++---
 7 files changed, 85 insertions(+), 54 deletions(-)
 create mode 100644 test/fixtures/keys/irrelevant_san_correct_subject-cert.pem
 create mode 100644 test/fixtures/keys/irrelevant_san_correct_subject-key.pem

diff --git a/doc/api/tls.md b/doc/api/tls.md
index c9fabbc5b59cf9..e2c4af9df29c2d 100644
--- a/doc/api/tls.md
+++ b/doc/api/tls.md
@@ -1323,6 +1323,11 @@ decrease overall server throughput.
 ## `tls.checkServerIdentity(hostname, cert)`
 <!-- YAML
 added: v0.8.4
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs-private/node-private/pull/300
+    description: Support for `uniformResourceIdentifier` subject alternative
+                 names has been disabled in response to CVE-2021-44531.
 -->
 
 * `hostname` {string} The host name or IP address to verify the certificate
@@ -1343,6 +1348,12 @@ the checks done with additional verification.
 This function is only called if the certificate passed all other checks, such as
 being issued by trusted CA (`options.ca`).
 
+Earlier versions of Node.js incorrectly accepted certificates for a given
+`hostname` if a matching `uniformResourceIdentifier` subject alternative name
+was present (see [CVE-2021-44531][]). Applications that wish to accept
+`uniformResourceIdentifier` subject alternative names can use a custom
+`options.checkServerIdentity` function that implements the desired behavior.
+
 ## `tls.connect(options[, callback])`
 <!-- YAML
 added: v0.11.3
@@ -2003,6 +2014,7 @@ added: v11.4.0
 [`tls.createServer()`]: #tls_tls_createserver_options_secureconnectionlistener
 [`tls.getCiphers()`]: #tls_tls_getciphers
 [`tls.rootCertificates`]: #tls_tls_rootcertificates
+[CVE-2021-44531]: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-44531
 [Chrome's 'modern cryptography' setting]: https://www.chromium.org/Home/chromium-security/education/tls#TOC-Cipher-Suites
 [DHE]: https://en.wikipedia.org/wiki/Diffie%E2%80%93Hellman_key_exchange
 [ECDHE]: https://en.wikipedia.org/wiki/Elliptic_curve_Diffie%E2%80%93Hellman
diff --git a/lib/tls.js b/lib/tls.js
index cefb47d10f329f..f909b0f3fabf20 100644
--- a/lib/tls.js
+++ b/lib/tls.js
@@ -47,11 +47,9 @@ const { isArrayBufferView } = require('internal/util/types');
 
 const net = require('net');
 const { getOptionValue } = require('internal/options');
-const url = require('url');
 const { getRootCertificates, getSSLCiphers } = internalBinding('crypto');
 const { Buffer } = require('buffer');
 const EventEmitter = require('events');
-const { URL } = require('internal/url');
 const DuplexPair = require('internal/streams/duplexpair');
 const { canonicalizeIP } = internalBinding('cares_wrap');
 const _tls_common = require('_tls_common');
@@ -254,12 +252,10 @@ function splitEscapedAltNames(altNames) {
   return result;
 }
 
-let urlWarningEmitted = false;
 exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
   const subject = cert.subject;
   const altNames = cert.subjectaltname;
   const dnsNames = [];
-  const uriNames = [];
   const ips = [];
 
   hostname = '' + hostname;
@@ -271,23 +267,6 @@ exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
     for (const name of splitAltNames) {
       if (name.startsWith('DNS:')) {
         dnsNames.push(name.slice(4));
-      } else if (name.startsWith('URI:')) {
-        let uri;
-        try {
-          uri = new URL(name.slice(4));
-        } catch {
-          uri = url.parse(name.slice(4));
-          if (!urlWarningEmitted && !process.noDeprecation) {
-            urlWarningEmitted = true;
-            process.emitWarning(
-              `The URI ${name.slice(4)} found in cert.subjectaltname ` +
-              'is not a valid URI, and is supported in the tls module ' +
-              'solely for compatibility.',
-              'DeprecationWarning', 'DEP0109');
-          }
-        }
-
-        uriNames.push(uri.hostname);  // TODO(bnoordhuis) Also use scheme.
       } else if (name.startsWith('IP Address:')) {
         ips.push(canonicalizeIP(name.slice(11)));
       }
@@ -297,9 +276,6 @@ exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
   let valid = false;
   let reason = 'Unknown reason';
 
-  const hasAltNames =
-    dnsNames.length > 0 || ips.length > 0 || uriNames.length > 0;
-
   hostname = unfqdn(hostname);  // Remove trailing dot for error messages.
 
   if (net.isIP(hostname)) {
@@ -307,13 +283,12 @@ exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
     if (!valid)
       reason = `IP: ${hostname} is not in the cert's list: ${ips.join(', ')}`;
     // TODO(bnoordhuis) Also check URI SANs that are IP addresses.
-  } else if (hasAltNames || subject) {
+  } else if (dnsNames.length > 0 || (subject && subject.CN)) {
     const hostParts = splitHost(hostname);
     const wildcard = (pattern) => check(hostParts, pattern, true);
 
-    if (hasAltNames) {
-      const noWildcard = (pattern) => check(hostParts, pattern, false);
-      valid = dnsNames.some(wildcard) || uriNames.some(noWildcard);
+    if (dnsNames.length > 0) {
+      valid = dnsNames.some(wildcard);
       if (!valid)
         reason =
           `Host: ${hostname}. is not in the cert's altnames: ${altNames}`;
@@ -330,7 +305,7 @@ exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
         reason = `Host: ${hostname}. is not cert's CN: ${cn}`;
     }
   } else {
-    reason = 'Cert is empty';
+    reason = 'Cert does not contain a DNS name';
   }
 
   if (!valid) {
diff --git a/test/fixtures/keys/Makefile b/test/fixtures/keys/Makefile
index 49cc29ad1c4b43..95f4edbafffde1 100644
--- a/test/fixtures/keys/Makefile
+++ b/test/fixtures/keys/Makefile
@@ -77,6 +77,8 @@ all: \
   x448_public.pem \
   incorrect_san_correct_subject-cert.pem \
   incorrect_san_correct_subject-key.pem \
+  irrelevant_san_correct_subject-cert.pem \
+  irrelevant_san_correct_subject-key.pem \
 
 #
 # Create Certificate Authority: ca1
@@ -747,6 +749,18 @@ incorrect_san_correct_subject-cert.pem: incorrect_san_correct_subject-key.pem
 incorrect_san_correct_subject-key.pem:
 	openssl ecparam -name prime256v1 -genkey -noout -out incorrect_san_correct_subject-key.pem
 
+irrelevant_san_correct_subject-cert.pem: irrelevant_san_correct_subject-key.pem
+	openssl req -x509 \
+	            -key irrelevant_san_correct_subject-key.pem \
+	            -out irrelevant_san_correct_subject-cert.pem \
+	            -sha256 \
+	            -days 3650 \
+	            -subj "/CN=good.example.com" \
+	            -addext "subjectAltName = IP:1.2.3.4"
+
+irrelevant_san_correct_subject-key.pem:
+	openssl ecparam -name prime256v1 -genkey -noout -out irrelevant_san_correct_subject-key.pem
+
 clean:
 	rm -f *.pfx *.pem *.srl ca2-database.txt ca2-serial fake-startcom-root-serial *.print *.old fake-startcom-root-issued-certs/*.pem
 	@> fake-startcom-root-database.txt
diff --git a/test/fixtures/keys/irrelevant_san_correct_subject-cert.pem b/test/fixtures/keys/irrelevant_san_correct_subject-cert.pem
new file mode 100644
index 00000000000000..cdb74b7de3919a
--- /dev/null
+++ b/test/fixtures/keys/irrelevant_san_correct_subject-cert.pem
@@ -0,0 +1,11 @@
+-----BEGIN CERTIFICATE-----
+MIIBnTCCAUKgAwIBAgIUa28EJmmQ7yZOq3WWNP3SLiJnzcAwCgYIKoZIzj0EAwIw
+GzEZMBcGA1UEAwwQZ29vZC5leGFtcGxlLmNvbTAeFw0yMTEyMTExNzE0NDVaFw0z
+MTEyMDkxNzE0NDVaMBsxGTAXBgNVBAMMEGdvb2QuZXhhbXBsZS5jb20wWTATBgcq
+hkjOPQIBBggqhkjOPQMBBwNCAATEKoJfDvKQ6dD+yvc4DaeH0ZlG8VuGJUVi6iIb
+ugY3dKHdmXUIuwwUScgztLc6W8FfvbTxfTF2q90ZBJlr/Klvo2QwYjAdBgNVHQ4E
+FgQUu55oRZI5tdQDmViwAvPEbzZuY2owHwYDVR0jBBgwFoAUu55oRZI5tdQDmViw
+AvPEbzZuY2owDwYDVR0TAQH/BAUwAwEB/zAPBgNVHREECDAGhwQBAgMEMAoGCCqG
+SM49BAMCA0kAMEYCIQDw8z8d7ToB14yxMJxEDF1dhUqMReJFFwPVnvzkr174igIh
+AKJ9XL+02sGOE7xZd5C0KqUXeHoIE9shnejnhm3WBrB/
+-----END CERTIFICATE-----
diff --git a/test/fixtures/keys/irrelevant_san_correct_subject-key.pem b/test/fixtures/keys/irrelevant_san_correct_subject-key.pem
new file mode 100644
index 00000000000000..b0a96659c63f98
--- /dev/null
+++ b/test/fixtures/keys/irrelevant_san_correct_subject-key.pem
@@ -0,0 +1,5 @@
+-----BEGIN EC PRIVATE KEY-----
+MHcCAQEEIDsijdVlHMNTvJ4eqeUbpjMMnl72+HLtEIEcbauckCP6oAoGCCqGSM49
+AwEHoUQDQgAExCqCXw7ykOnQ/sr3OA2nh9GZRvFbhiVFYuoiG7oGN3Sh3Zl1CLsM
+FEnIM7S3OlvBX7208X0xdqvdGQSZa/ypbw==
+-----END EC PRIVATE KEY-----
diff --git a/test/parallel/test-tls-check-server-identity.js b/test/parallel/test-tls-check-server-identity.js
index 04c77305eb3b89..b7c7d02fcadf57 100644
--- a/test/parallel/test-tls-check-server-identity.js
+++ b/test/parallel/test-tls-check-server-identity.js
@@ -30,13 +30,6 @@ const util = require('util');
 
 const tls = require('tls');
 
-common.expectWarning('DeprecationWarning', [
-  ['The URI http://[a.b.a.com]/ found in cert.subjectaltname ' +
-  'is not a valid URI, and is supported in the tls module ' +
-  'solely for compatibility.',
-   'DEP0109'],
-]);
-
 const tests = [
   // False-y values.
   {
@@ -140,7 +133,7 @@ const tests = [
   {
     host: 'a.com',
     cert: { },
-    error: 'Cert is empty'
+    error: 'Cert does not contain a DNS name'
   },
 
   // Empty Subject w/DNS name
@@ -154,7 +147,8 @@ const tests = [
   {
     host: 'a.b.a.com', cert: {
       subjectaltname: 'URI:http://a.b.a.com/',
-    }
+    },
+    error: 'Cert does not contain a DNS name'
   },
 
   // Multiple CN fields
@@ -271,22 +265,15 @@ const tests = [
     host: 'a.b.a.com', cert: {
       subjectaltname: 'URI:http://a.b.a.com/',
       subject: {}
-    }
+    },
+    error: 'Cert does not contain a DNS name'
   },
   {
     host: 'a.b.a.com', cert: {
       subjectaltname: 'URI:http://*.b.a.com/',
       subject: {}
     },
-    error: 'Host: a.b.a.com. is not in the cert\'s altnames: ' +
-           'URI:http://*.b.a.com/'
-  },
-  // Invalid URI
-  {
-    host: 'a.b.a.com', cert: {
-      subjectaltname: 'URI:http://[a.b.a.com]/',
-      subject: {}
-    }
+    error: 'Cert does not contain a DNS name'
   },
   // IP addresses
   {
@@ -294,8 +281,7 @@ const tests = [
       subjectaltname: 'IP Address:127.0.0.1',
       subject: {}
     },
-    error: 'Host: a.b.a.com. is not in the cert\'s altnames: ' +
-           'IP Address:127.0.0.1'
+    error: 'Cert does not contain a DNS name'
   },
   {
     host: '127.0.0.1', cert: {
diff --git a/test/parallel/test-x509-escaping.js b/test/parallel/test-x509-escaping.js
index 4e0f82767d022e..47015d416f59e0 100644
--- a/test/parallel/test-x509-escaping.js
+++ b/test/parallel/test-x509-escaping.js
@@ -19,10 +19,6 @@ const { hasOpenSSL3 } = common;
   const numLeaves = 5;
 
   for (let i = 0; i < numLeaves; i++) {
-    // TODO(tniessen): this test case requires proper handling of URI SANs,
-    // which node currently does not implement.
-    if (i === 3) continue;
-
     const name = `x509-escaping/google/leaf${i}.pem`;
     const leafPEM = fixtures.readSync(name, 'utf8');
 
@@ -347,3 +343,35 @@ const { hasOpenSSL3 } = common;
     }));
   })).unref();
 }
+
+// The subject MUST NOT be ignored if no dNSName subject alternative name
+// exists, even if other subject alternative names exist.
+{
+  const key = fixtures.readKey('irrelevant_san_correct_subject-key.pem');
+  const cert = fixtures.readKey('irrelevant_san_correct_subject-cert.pem');
+
+  // The hostname is the CN, but there is no dNSName SAN entry.
+  const servername = 'good.example.com';
+
+  // X509Certificate interface is not supported in v12.x & v14.x. Disable
+  // checks for certX509.subject and certX509.subjectAltName with expected
+  // value. The testcase is ported from v17.x
+  //
+  // const certX509 = new X509Certificate(cert);
+  // assert.strictEqual(certX509.subject, `CN=${servername}`);
+  // assert.strictEqual(certX509.subjectAltName, 'IP Address:1.2.3.4');
+
+  // Connect to a server that uses the self-signed certificate.
+  const server = tls.createServer({ key, cert }, common.mustCall((socket) => {
+    socket.destroy();
+    server.close();
+  })).listen(common.mustCall(() => {
+    const { port } = server.address();
+    tls.connect(port, {
+      ca: cert,
+      servername,
+    }, common.mustCall(() => {
+      // Do nothing, the server will close the connection.
+    }));
+  }));
+}