src: add cve reverts and associated tests

Co-authored-by: Akshay K <[email protected]>
Backport-PR-URL: nodejs-private/node-private#306
PR-URL: nodejs-private/node-private#300
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>

Author: 2 people

lib/tls.js

@@ -47,9 +47,11 @@ 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');
@@ -252,10 +254,12 @@ 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;
@@ -267,6 +271,22 @@ exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
     for (const name of splitAltNames) {
       if (name.startsWith('DNS:')) {
         dnsNames.push(name.slice(4));
+      } else if (process.REVERT_CVE_2021_44531 && 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)));
       }
@@ -276,19 +296,25 @@ 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)) {
     valid = ips.includes(canonicalizeIP(hostname));
     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 (dnsNames.length > 0 || (subject && subject.CN)) {
+  } else if ((process.REVERT_CVE_2021_44531 && (hasAltNames || subject)) ||
+         (dnsNames.length > 0 || (subject && subject.CN))) {
     const hostParts = splitHost(hostname);
     const wildcard = (pattern) => check(hostParts, pattern, true);
 
-    if (dnsNames.length > 0) {
-      valid = dnsNames.some(wildcard);
+    if ((process.REVERT_CVE_2021_44531 && hasAltNames) ||
+           (dnsNames.length > 0)) {
+      const noWildcard = (pattern) => check(hostParts, pattern, false);
+      valid = dnsNames.some(wildcard) || uriNames.some(noWildcard);
       if (!valid)
         reason =
           `Host: ${hostname}. is not in the cert's altnames: ${altNames}`;

src/node_crypto_common.cc

@@ -5,6 +5,7 @@
 #include "node_crypto_common.h"
 #include "node.h"
 #include "node_internals.h"
+#include "node_revert.h"
 #include "node_url.h"
 #include "string_bytes.h"
 #include "v8.h"
@@ -481,6 +482,44 @@ void AddFingerprintDigest(
   }
 }
 
+// deprecated, only used for security revert
+bool SafeX509ExtPrint(const BIOPointer& out, X509_EXTENSION* ext) {
+  const X509V3_EXT_METHOD* method = X509V3_EXT_get(ext);
+
+  if (method != X509V3_EXT_get_nid(NID_subject_alt_name))
+    return false;
+
+  GENERAL_NAMES* names = static_cast<GENERAL_NAMES*>(X509V3_EXT_d2i(ext));
+  if (names == nullptr)
+    return false;
+
+  for (int i = 0; i < sk_GENERAL_NAME_num(names); i++) {
+    GENERAL_NAME* gen = sk_GENERAL_NAME_value(names, i);
+
+    if (i != 0)
+      BIO_write(out.get(), ", ", 2);
+
+    if (gen->type == GEN_DNS) {
+      ASN1_IA5STRING* name = gen->d.dNSName;
+
+      BIO_write(out.get(), "DNS:", 4);
+      BIO_write(out.get(), name->data, name->length);
+    } else {
+      STACK_OF(CONF_VALUE)* nval = i2v_GENERAL_NAME(
+          const_cast<X509V3_EXT_METHOD*>(method), gen, nullptr);
+      if (nval == nullptr) {
+        sk_GENERAL_NAME_pop_free(names, GENERAL_NAME_free);
+        return false;
+      }
+      X509V3_EXT_val_prn(out.get(), nval, 0, 0);
+      sk_CONF_VALUE_pop_free(nval, X509V3_conf_free);
+    }
+  }
+  sk_GENERAL_NAME_pop_free(names, GENERAL_NAME_free);
+
+  return true;
+}
+
 static inline bool IsSafeAltName(const char* name, size_t length, bool utf8) {
   for (size_t i = 0; i < length; i++) {
     char c = name[i];
@@ -705,6 +744,10 @@ bool SafeX509SubjectAltNamePrint(const BIOPointer& out, X509_EXTENSION* ext) {
   const X509V3_EXT_METHOD* method = X509V3_EXT_get(ext);
   CHECK(method == X509V3_EXT_get_nid(NID_subject_alt_name));
 
+  if (IsReverted(SECURITY_REVERT_CVE_2021_44532)) {
+    return  SafeX509ExtPrint(out, ext);
+  }
+
   GENERAL_NAMES* names = static_cast<GENERAL_NAMES*>(X509V3_EXT_d2i(ext));
   if (names == nullptr)
     return false;
@@ -730,6 +773,10 @@ bool SafeX509InfoAccessPrint(const BIOPointer& out, X509_EXTENSION* ext) {
   const X509V3_EXT_METHOD* method = X509V3_EXT_get(ext);
   CHECK(method == X509V3_EXT_get_nid(NID_info_access));
 
+  if (IsReverted(SECURITY_REVERT_CVE_2021_44532)) {
+    return (X509V3_EXT_print(out.get(), ext, 0, 0) == 1);
+  }
+
   AUTHORITY_INFO_ACCESS* descs =
       static_cast<AUTHORITY_INFO_ACCESS*>(X509V3_EXT_d2i(ext));
   if (descs == nullptr)

src/node_revert.h

@@ -20,6 +20,8 @@ namespace node {
   XX(CVE_2019_9514, "CVE-2019-9514", "HTTP/2 Reset Flood")                 \
   XX(CVE_2019_9516, "CVE-2019-9516", "HTTP/2 0-Length Headers Leak")       \
   XX(CVE_2019_9518, "CVE-2019-9518", "HTTP/2 Empty DATA Frame Flooding")   \
+  XX(CVE_2021_44531, "CVE-2021-44531", "Cert Verif Bypass via URI SAN")    \
+  XX(CVE_2021_44532, "CVE-2021-44532", "Cert Verif Bypass via Str Inject") \
 //  XX(CVE_2016_PEND, "CVE-2016-PEND", "Vulnerability Title")
   // TODO(addaleax): Remove all of the above before Node.js 13 as the comment
   // at the start of the file indicates.

test/parallel/test-revert-CVE-2021-44531.js

@@ -0,0 +1,64 @@
+// Flags: --security-revert=CVE-2021-44531
+'use strict';
+const common = require('../common');
+
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const assert = require('assert');
+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 = [
+  // Likewise for "URI:" Subject Alternative Names.
+  // See also https://github.com/nodejs/node/issues/8108.
+  {
+    host: '8.8.8.8',
+    cert: { subject: { CN: '8.8.8.8' }, subjectaltname: 'URI:http://8.8.8.8/' },
+    error: 'IP: 8.8.8.8 is not in the cert\'s list: '
+  },
+  // Empty Subject w/URI name
+  {
+    host: 'a.b.a.com', cert: {
+      subjectaltname: 'URI:http://a.b.a.com/',
+    }
+  },
+  // URI names
+  {
+    host: 'a.b.a.com', cert: {
+      subjectaltname: 'URI:http://a.b.a.com/',
+      subject: {}
+    }
+  },
+  {
+    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: {}
+    }
+  },
+];
+
+tests.forEach(function(test, i) {
+  const err = tls.checkServerIdentity(test.host, test.cert);
+  assert.strictEqual(err && err.reason,
+                     test.error,
+                     `Test# ${i} failed: ${util.inspect(test)} \n` +
+                     `${test.error} != ${(err && err.reason)}`);
+});

test/parallel/test-tls-0-dns-altname-revert-CVE-2021-44532.js

@@ -0,0 +1,54 @@
+// Copyright Joyent, Inc. and other Node contributors.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and associated documentation files (the
+// "Software"), to deal in the Software without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Software, and to permit
+// persons to whom the Software is furnished to do so, subject to the
+// following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
+// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
+// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
+// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
+// USE OR OTHER DEALINGS IN THE SOFTWARE.
+// Flags: --security-revert=CVE-2021-44532
+'use strict';
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+// Check getPeerCertificate can properly handle '\0' for fix CVE-2009-2408.
+
+const assert = require('assert');
+const tls = require('tls');
+const fixtures = require('../common/fixtures');
+
+const server = tls.createServer({
+  key: fixtures.readSync(['0-dns', '0-dns-key.pem']),
+  cert: fixtures.readSync(['0-dns', '0-dns-cert.pem'])
+}, common.mustCall((c) => {
+  c.once('data', common.mustCall(() => {
+    c.destroy();
+    server.close();
+  }));
+})).listen(0, common.mustCall(() => {
+  const c = tls.connect(server.address().port, {
+    rejectUnauthorized: false
+  }, common.mustCall(() => {
+    const cert = c.getPeerCertificate();
+    assert.strictEqual(cert.subjectaltname,
+                       'DNS:good.example.org\0.evil.example.com, ' +
+                           'DNS:just-another.example.com, ' +
+                           'IP Address:8.8.8.8, ' +
+                           'IP Address:8.8.4.4, ' +
+                           'DNS:last.example.com');
+    c.write('ok');
+  }));
+}));