lib: make tls.checkServerIdentity() more strict

CVE-2016-7099

PR-URL: https://github.com/nodejs/node-private/pull/61
Reviewed-By: Rod Vagg <rod@vagg.org>

lib/tls.js

@@ -78,138 +78,154 @@ exports.convertNPNProtocols = function convertNPNProtocols(NPNProtocols, out) {
   }
 };
 
-exports.checkServerIdentity = function checkServerIdentity(host, cert) {
-  // Create regexp to much hostnames
-  function regexpify(host, wildcards) {
-    // Add trailing dot (make hostnames uniform)
-    if (!/\.$/.test(host)) host += '.';
+function unfqdn(host) {
+  return host.replace(/[.]$/, '');
+}
 
-    // 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;
-        });
+function check(hostParts, pattern, wildcards) {
+  // Empty strings, null, undefined, etc. never match.
+  if (!pattern)
+    return false;
 
-    return new RegExp('^' + re + '$', 'i');
+  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,
-      reason = 'Unknown reason';
+  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) {
-      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);
+  // 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;
+}
+
+exports.checkServerIdentity = 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;
-    });
-    if (!valid) {
-      reason = util.format('IP: %s is not in the cert\'s list: %s',
-                           host,
-                           ips.join(', '));
+    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);
+    }
+
+    function noWildcard(pattern) {
+      return 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) {
+      var 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 {
-    // Transform hostname to canonical form
-    if (!/\.$/.test(host)) 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 (util.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) {
-      return re.test(host);
-    });
-
-    if (!valid) {
-      if (cert.subjectaltname) {
-        reason = util.format('Host: %s is not in the cert\'s altnames: %s',
-                             host,
-                             cert.subjectaltname);
-      } else {
-        reason = util.format('Host: %s is not cert\'s CN: %s',
-                             host,
-                             cert.subject.CN);
-      }
-    }
+    reason = 'Cert is empty';
   }
 
   if (!valid) {
     var err = new Error(
-        util.format('Hostname/IP doesn\'t match certificate\'s altnames: %j',
-                    reason));
+        'Hostname/IP doesn\'t match certificate\'s altnames: "' + reason + '"');
     err.reason = reason;
     err.host = host;
     err.cert = cert;

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

@@ -25,6 +25,23 @@ var util = require('util');
 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' } } },
@@ -34,15 +51,42 @@ 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
+  {
+    host: 'a.com',
+    cert: { },
+    error: 'Cert is empty'
+  },
 
   // Multiple CN fields
   {
@@ -206,6 +250,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) {