lib: make tls.checkServerIdentity() more strict

Incorporates changes from commit e345253 ("tls: better error reporting
at cert validation") to test/simple/test-tls-check-server-identity.js
to make back-porting the patch easier.

CVE-2016-7099

PR-URL: nodejs-private/node-private#62
Reviewed-By: Rod Vagg <rod@vagg.org>

Author: bnoordhuis

lib/tls.js

@@ -96,115 +96,163 @@ function convertNPNProtocols(NPNProtocols, out) {
   }
 }
 
+function unfqdn(host) {
+  return host.replace(/[.]$/, '');
+}
 
-function checkServerIdentity(host, cert) {
-  // Create regexp to much hostnames
-  function regexpify(host, wildcards) {
-    // Add trailing dot (make hostnames uniform)
-    if (!/\.$/.test(host)) 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 /$./;
-    }
+function splitHost(host) {
+  // String#toLowerCase() is locale-sensitive so we use
+  // a conservative version that only lowercases A-Z.
+  function replacer(c) {
+    return String.fromCharCode(32 + c.charCodeAt(0));
+  };
+  return unfqdn(host).replace(/[A-Z]/g, replacer).split('.');
+}
 
-    // Replace wildcard chars with regexp's wildcard and
-    // escape all characters that have special meaning in regexps
-    // (i.e. '.', '[', '{', '*', and others)
-    var re = host.replace(
-        /\*([a-z0-9\\-_\.])|[\.,\-\\\^\$+?*\[\]\(\):!\|{}]/g,
-        function(all, sub) {
-          if (sub) return '[a-z0-9\\-_]*' + (sub === '-' ? '\\-' : sub);
-          return '\\' + all;
-        });
-
-    return new RegExp('^' + re + '$', 'i');
+function check(hostParts, pattern, wildcards) {
+  // Empty strings, null, undefined, etc. never match.
+  if (!pattern)
+    return false;
+
+  var patternParts = splitHost(pattern);
+
+  if (hostParts.length !== patternParts.length)
+    return false;
+
+  // Pattern has empty components, e.g. "bad..example.com".
+  if (patternParts.indexOf('') !== -1)
+    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.
+  function isBad(s) {
+    return /[^\u0021-\u007F]/.test(s);
   }
 
-  var dnsNames = [],
-      uriNames = [],
-      ips = [],
-      matchCN = true,
-      valid = false;
+  if (patternParts.some(isBad))
+    return false;
 
-  // 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) {
-      if (/^DNS:/.test(altname)) {
-        dnsNames.push(altname.slice(4));
-      } else if (/^IP Address:/.test(altname)) {
-        ips.push(altname.slice(11));
-      } else if (/^URI:/.test(altname)) {
-        var uri = url.parse(altname.slice(4));
-        if (uri) uriNames.push(uri.hostname);
+  // Check host parts from right to left first.
+  for (var i = hostParts.length - 1; i > 0; i -= 1)
+    if (hostParts[i] !== patternParts[i])
+      return false;
+
+  var hostSubdomain = hostParts[0];
+  var patternSubdomain = patternParts[0];
+  var 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.indexOf('xn--') !== -1) {
+    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;
+
+  var prefix = patternSubdomainParts[0];
+  var suffix = patternSubdomainParts[1];
+
+  if (prefix.length + suffix.length > hostSubdomain.length)
+    return false;
+
+  if (prefix.length > 0 && hostSubdomain.slice(0, prefix.length) !== prefix)
+    return false;
+
+  if (suffix.length > 0 && hostSubdomain.slice(-suffix.length) !== suffix)
+    return false;
+
+  return true;
+}
+
+function _checkServerIdentity(host, cert) {
+  var subject = cert.subject;
+  var altNames = cert.subjectaltname;
+  var dnsNames = [];
+  var uriNames = [];
+  var ips = [];
+
+  host = '' + host;
+
+  if (altNames) {
+    altNames.split(', ').forEach(function(name) {
+      if (/^DNS:/.test(name)) {
+        dnsNames.push(name.slice(4));
+      } else if (/^URI:/.test(name)) {
+        var uri = url.parse(name.slice(4));
+        uriNames.push(uri.hostname);  // TODO(bnoordhuis) Also use scheme.
+      } else if (/^IP Address:/.test(name)) {
+        ips.push(name.slice(11));
       }
     });
   }
 
-  // If hostname is an IP address, it should be present in the list of IP
-  // addresses.
+  var valid = false;
+  var reason = 'Unknown reason';
+
   if (net.isIP(host)) {
-    valid = ips.some(function(ip) {
-      return ip === host;
-    });
-  } else {
-    // Transform hostname to canonical form
-    if (!/\.$/.test(host)) host += '.';
+    valid = ips.indexOf(host) !== -1;
+    if (!valid)
+      reason = 'IP: ' + host + ' is not in the cert\'s list: ' + ips.join(', ');
+    // TODO(bnoordhuis) Also check URI SANs that are IP addresses.
+  } else if (subject) {
+    host = unfqdn(host);  // Remove trailing dot for error messages.
+    var hostParts = splitHost(host);
+
+    function wildcard(pattern) {
+      return check(hostParts, pattern, true);
+    }
 
-    // Otherwise check all DNS/URI records from certificate
-    // (with allowed wildcards)
-    dnsNames = dnsNames.map(function(name) {
-      return regexpify(name, true);
-    });
+    function noWildcard(pattern) {
+      return check(hostParts, pattern, false);
+    }
 
-    // Wildcards ain't allowed in URI names
-    uriNames = uriNames.map(function(name) {
-      return regexpify(name, false);
-    });
+    // Match against Common Name only if no supported identifiers are present.
+    if (dnsNames.length === 0 && ips.length === 0 && uriNames.length === 0) {
+      var cn = subject.CN;
 
-    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));
+      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';
+  }
 
-    valid = dnsNames.some(function(re) {
-      return re.test(host);
-    });
+  if (!valid) {
+    var err = new Error('Hostname/IP doesn\'t match certificate\'s altnames');
+    err.reason = reason;
+    err.host = host;
+    err.cert = cert;
+    return err;
   }
+}
+exports._checkServerIdentity = _checkServerIdentity;
 
-  return valid;
+function checkServerIdentity(host, cert) {
+  return !!_checkServerIdentity(host, cert);
 }
 exports.checkServerIdentity = checkServerIdentity;
 
@@ -1384,12 +1432,8 @@ exports.connect = function(/* [port, host], options, cb */) {
 
     // Verify that server's identity matches it's certificate's names
     if (!verifyError) {
-      var validCert = checkServerIdentity(hostname,
-                                          pair.cleartext.getPeerCertificate());
-      if (!validCert) {
-        verifyError = new Error('Hostname/IP doesn\'t match certificate\'s ' +
-                                'altnames');
-      }
+      verifyError = _checkServerIdentity(hostname,
+                                         pair.cleartext.getPeerCertificate());
     }
 
     if (verifyError) {