lib: make tls.checkServerIdentity() more strict

PR-URL: https://github.com/nodejs/node-private/pull/75
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

lib/tls.js

@@ -66,134 +66,137 @@ exports.convertALPNProtocols = function(protocols, out) {
   }
 };
 
-exports.checkServerIdentity = function checkServerIdentity(host, cert) {
+function unfqdn(host) {
-  // Create regexp to much hostnames
+  return host.replace(/[.]$/, '');
-  function regexpify(host, wildcards) {
+}
-    // Add trailing dot (make hostnames uniform)
-    if (!host || !host.endsWith('.')) host += '.';
-
-    // The same applies to hostname with more than one wildcard,
-    // if hostname has wildcard when wildcards are not allowed,
-    // or if there are less than two dots after wildcard (i.e. *.com or *d.com)
-    //
-    // also
-    //
-    // "The client SHOULD NOT attempt to match a presented identifier in
-    // which the wildcard character comprises a label other than the
-    // left-most label (e.g., do not match bar.*.example.net)."
-    // RFC6125
-    if (!wildcards && /\*/.test(host) || /[\.\*].*\*/.test(host) ||
-        /\*/.test(host) && !/\*.*\..+\..+/.test(host)) {
-      return /$./;
-    }
 
-    // Replace wildcard chars with regexp's wildcard and
+function splitHost(host) {
-    // escape all characters that have special meaning in regexps
+  // String#toLowerCase() is locale-sensitive so we use
-    // (i.e. '.', '[', '{', '*', and others)
+  // a conservative version that only lowercases A-Z.
-    var re = host.replace(
+  const replacer = (c) => String.fromCharCode(32 + c.charCodeAt(0));
-        /\*([a-z0-9\\-_\.])|[\.,\-\\\^\$+?*\[\]\(\):!\|{}]/g,
+  return unfqdn(host).replace(/[A-Z]/g, replacer).split('.');
-        function(all, sub) {
+}
-          if (sub) return '[a-z0-9\\-_]*' + (sub === '-' ? '\\-' : sub);
+
-          return '\\' + all;
+function check(hostParts, pattern, wildcards) {
-        });
+  // Empty strings, null, undefined, etc. never match.
-
+  if (!pattern)
-    return new RegExp('^' + re + '$', 'i');
+    return false;
-  }
+
+  const patternParts = splitHost(pattern);
+
+  if (hostParts.length !== patternParts.length)
+    return false;
+
+  // Pattern has empty components, e.g. "bad..example.com".
+  if (patternParts.includes(''))
+    return false;
+
+  // RFC 6125 allows IDNA U-labels (Unicode) in names but we have no
+  // good way to detect their encoding or normalize them so we simply
+  // reject them.  Control characters and blanks are rejected as well
+  // because nothing good can come from accepting them.
+  const isBad = (s) => /[^\u0021-\u007F]/u.test(s);
+  if (patternParts.some(isBad))
+    return false;
+
+  // Check host parts from right to left first.
+  for (let i = hostParts.length - 1; i > 0; i -= 1)
+    if (hostParts[i] !== patternParts[i])
+      return false;
+
+  const hostSubdomain = hostParts[0];
+  const patternSubdomain = patternParts[0];
+  const patternSubdomainParts = patternSubdomain.split('*');
+
+  // Short-circuit when the subdomain does not contain a wildcard.
+  // RFC 6125 does not allow wildcard substitution for components
+  // containing IDNA A-labels (Punycode) so match those verbatim.
+  if (patternSubdomainParts.length === 1 || patternSubdomain.includes('xn--'))
+    return hostSubdomain === patternSubdomain;
+
+  if (!wildcards)
+    return false;
+
+  // More than one wildcard is always wrong.
+  if (patternSubdomainParts.length > 2)
+    return false;
+
+  // *.tld wildcards are not allowed.
+  if (patternParts.length <= 2)
+    return false;
+
+  const [prefix, suffix] = patternSubdomainParts;
+
+  if (prefix.length + suffix.length > hostSubdomain.length)
+    return false;
 
-  var dnsNames = [];
+  if (!hostSubdomain.startsWith(prefix))
-  var uriNames = [];
+    return false;
+
+  if (!hostSubdomain.endsWith(suffix))
+    return false;
+
+  return true;
+}
+
+exports.checkServerIdentity = function checkServerIdentity(host, cert) {
+  const subject = cert.subject;
+  const altNames = cert.subjectaltname;
+  const dnsNames = [];
+  const uriNames = [];
   const ips = [];
-  var matchCN = true;
-  var valid = false;
-  var reason = 'Unknown reason';
-
-  // There're several names to perform check against:
-  // CN and altnames in certificate extension
-  // (DNS names, IP addresses, and URIs)
-  //
-  // Walk through altnames and generate lists of those names
-  if (cert.subjectaltname) {
-    cert.subjectaltname.split(/, /g).forEach(function(altname) {
-      var option = altname.match(/^(DNS|IP Address|URI):(.*)$/);
-      if (!option)
-        return;
-      if (option[1] === 'DNS') {
-        dnsNames.push(option[2]);
-      } else if (option[1] === 'IP Address') {
-        ips.push(option[2]);
-      } else if (option[1] === 'URI') {
-        var uri = url.parse(option[2]);
-        if (uri) uriNames.push(uri.hostname);
-      }
-    });
-  }
 
-  // If hostname is an IP address, it should be present in the list of IP
+  host = '' + host;
-  // addresses.
+
-  if (net.isIP(host)) {
+  if (altNames) {
-    valid = ips.some(function(ip) {
+    for (const name of altNames.split(', ')) {
-      return ip === host;
+      if (name.startsWith('DNS:')) {
-    });
+        dnsNames.push(name.slice(4));
-    if (!valid) {
+      } else if (name.startsWith('URI:')) {
-      reason = `IP: ${host} is not in the cert's list: ${ips.join(', ')}`;
+        const uri = url.parse(name.slice(4));
-    }
+        uriNames.push(uri.hostname);  // TODO(bnoordhuis) Also use scheme.
-  } else if (cert.subject) {
+      } else if (name.startsWith('IP Address:')) {
-    // Transform hostname to canonical form
+        ips.push(name.slice(11));
-    if (!host || !host.endsWith('.')) host += '.';
-
-    // Otherwise check all DNS/URI records from certificate
-    // (with allowed wildcards)
-    dnsNames = dnsNames.map(function(name) {
-      return regexpify(name, true);
-    });
-
-    // Wildcards ain't allowed in URI names
-    uriNames = uriNames.map(function(name) {
-      return regexpify(name, false);
-    });
-
-    dnsNames = dnsNames.concat(uriNames);
-
-    if (dnsNames.length > 0) matchCN = false;
-
-    // Match against Common Name (CN) only if no supported identifiers are
-    // present.
-    //
-    // "As noted, a client MUST NOT seek a match for a reference identifier
-    //  of CN-ID if the presented identifiers include a DNS-ID, SRV-ID,
-    //  URI-ID, or any application-specific identifier types supported by the
-    //  client."
-    // RFC6125
-    if (matchCN) {
-      var commonNames = cert.subject.CN;
-      if (Array.isArray(commonNames)) {
-        for (var i = 0, k = commonNames.length; i < k; ++i) {
-          dnsNames.push(regexpify(commonNames[i], true));
-        }
-      } else {
-        dnsNames.push(regexpify(commonNames, true));
       }
     }
+  }
 
-    valid = dnsNames.some(function(re) {
+  let valid = false;
-      return re.test(host);
+  let reason = 'Unknown reason';
-    });
 
-    if (!valid) {
+  if (net.isIP(host)) {
-      if (cert.subjectaltname) {
+    valid = ips.includes(host);
-        reason =
+    if (!valid)
-            `Host: ${host} is not in the cert's altnames: ` +
+      reason = `IP: ${host} is not in the cert's list: ${ips.join(', ')}`;
-            `${cert.subjectaltname}`;
+    // TODO(bnoordhuis) Also check URI SANs that are IP addresses.
-      } else {
+  } else if (subject) {
-        reason = `Host: ${host} is not cert's CN: ${cert.subject.CN}`;
+    host = unfqdn(host);  // Remove trailing dot for error messages.
-      }
+    const hostParts = splitHost(host);
+    const wildcard = (pattern) => check(hostParts, pattern, true);
+    const noWildcard = (pattern) => check(hostParts, pattern, false);
+
+    // Match against Common Name only if no supported identifiers are present.
+    if (dnsNames.length === 0 && ips.length === 0 && uriNames.length === 0) {
+      const cn = subject.CN;
+
+      if (Array.isArray(cn))
+        valid = cn.some(wildcard);
+      else if (cn)
+        valid = wildcard(cn);
+
+      if (!valid)
+        reason = `Host: ${host}. is not cert's CN: ${cn}`;
+    } else {
+      valid = dnsNames.some(wildcard) || uriNames.some(noWildcard);
+      if (!valid)
+        reason = `Host: ${host}. is not in the cert's altnames: ${altNames}`;
     }
   } else {
     reason = 'Cert is empty';
   }
 
   if (!valid) {
-    var err = new Error(
+    const err = new Error(
         `Hostname/IP doesn't match certificate's altnames: "${reason}"`);
     err.reason = reason;
     err.host = host;

test/parallel/test-tls-check-server-identity.js

@@ -11,6 +11,23 @@ var tls = require('tls');
 
 
 var tests = [
+  // False-y values.
+  {
+    host: false,
+    cert: { subject: { CN: 'a.com' } },
+    error: 'Host: false. is not cert\'s CN: a.com'
+  },
+  {
+    host: null,
+    cert: { subject: { CN: 'a.com' } },
+    error: 'Host: null. is not cert\'s CN: a.com'
+  },
+  {
+    host: undefined,
+    cert: { subject: { CN: 'a.com' } },
+    error: 'Host: undefined. is not cert\'s CN: a.com'
+  },
+
   // Basic CN handling
   { host: 'a.com', cert: { subject: { CN: 'a.com' } } },
   { host: 'a.com', cert: { subject: { CN: 'A.COM' } } },
@@ -20,15 +37,35 @@ var tests = [
     error: 'Host: a.com. is not cert\'s CN: b.com'
   },
   { host: 'a.com', cert: { subject: { CN: 'a.com.' } } },
+  {
+    host: 'a.com',
+    cert: { subject: { CN: '.a.com' } },
+    error: 'Host: a.com. is not cert\'s CN: .a.com'
+  },
 
   // Wildcards in CN
   { host: 'b.a.com', cert: { subject: { CN: '*.a.com' } } },
+  {
+    host: 'ba.com',
+    cert: { subject: { CN: '*.a.com' } },
+    error: 'Host: ba.com. is not cert\'s CN: *.a.com'
+  },
+  {
+    host: '\n.b.com',
+    cert: { subject: { CN: '*n.b.com' } },
+    error: 'Host: \n.b.com. is not cert\'s CN: *n.b.com'
+  },
   { host: 'b.a.com', cert: {
     subjectaltname: 'DNS:omg.com',
     subject: { CN: '*.a.com' } },
     error: 'Host: b.a.com. is not in the cert\'s altnames: ' +
            'DNS:omg.com'
   },
+  {
+    host: 'b.a.com',
+    cert: { subject: { CN: 'b*b.a.com' } },
+    error: 'Host: b.a.com. is not cert\'s CN: b*b.a.com'
+  },
 
   // Empty Cert
   {
@@ -199,6 +236,20 @@ var tests = [
     error: 'Host: localhost. is not in the cert\'s altnames: ' +
            'DNS:a.com'
   },
+  // IDNA
+  {
+    host: 'xn--bcher-kva.example.com',
+    cert: { subject: { CN: '*.example.com' } },
+  },
+  // RFC 6125, section 6.4.3: "[...] the client SHOULD NOT attempt to match
+  // a presented identifier where the wildcard character is embedded within
+  // an A-label [...]"
+  {
+    host: 'xn--bcher-kva.example.com',
+    cert: { subject: { CN: 'xn--*.example.com' } },
+    error: 'Host: xn--bcher-kva.example.com. is not cert\'s CN: ' +
+            'xn--*.example.com',
+  },
 ];
 
 tests.forEach(function(test, i) {