From 0d7e21ee7bcc79046f898f8c202d2ec87d23d711 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 15 Aug 2016 18:46:27 +0200 Subject: [PATCH] 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: https://github.com/nodejs/node-private/pull/62 Reviewed-By: Rod Vagg --- lib/tls.js | 238 +++++++++++------- test/simple/test-tls-check-server-identity.js | 148 ++++++++--- 2 files changed, 248 insertions(+), 138 deletions(-) diff --git a/lib/tls.js b/lib/tls.js index e3b90832236f8e..588dce3139e378 100644 --- a/lib/tls.js +++ b/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) { diff --git a/test/simple/test-tls-check-server-identity.js b/test/simple/test-tls-check-server-identity.js index 487f16292a6f97..6c115533306877 100644 --- a/test/simple/test-tls-check-server-identity.js +++ b/test/simple/test-tls-check-server-identity.js @@ -25,26 +25,74 @@ 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' } }, result: true }, - { host: 'a.com', cert: { subject: { CN: 'A.COM' } }, result: true }, - { host: 'a.com', cert: { subject: { CN: 'b.com' } }, result: false }, - { host: 'a.com', cert: { subject: { CN: 'a.com.' } }, result: true }, + { host: 'a.com', cert: { subject: { CN: 'a.com' } } }, + { host: 'a.com', cert: { subject: { CN: 'A.COM' } } }, + { + host: 'a.com', + cert: { subject: { CN: 'b.com' } }, + 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' } }, result: true }, + { 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' } }, - result: false + 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 { host: 'foo.com', cert: { subject: { CN: ['foo.com', 'bar.com'] } // CN=foo.com; CN=bar.com; - }, - result: true + } }, // DNS names and CN @@ -53,49 +101,50 @@ var tests = [ subjectaltname: 'DNS:*', subject: { CN: 'b.com' } }, - result: false + error: 'Host: a.com. is not in the cert\'s altnames: ' + + 'DNS:*' }, { host: 'a.com', cert: { subjectaltname: 'DNS:*.com', subject: { CN: 'b.com' } }, - result: false + error: 'Host: a.com. is not in the cert\'s altnames: ' + + 'DNS:*.com' }, { host: 'a.co.uk', cert: { subjectaltname: 'DNS:*.co.uk', subject: { CN: 'b.com' } - }, - result: true + } }, { host: 'a.com', cert: { subjectaltname: 'DNS:*.a.com', subject: { CN: 'a.com' } }, - result: false + error: 'Host: a.com. is not in the cert\'s altnames: ' + + 'DNS:*.a.com' }, { host: 'a.com', cert: { subjectaltname: 'DNS:*.a.com', subject: { CN: 'b.com' } }, - result: false + error: 'Host: a.com. is not in the cert\'s altnames: ' + + 'DNS:*.a.com' }, { host: 'a.com', cert: { subjectaltname: 'DNS:a.com', subject: { CN: 'b.com' } - }, - result: true + } }, { host: 'a.com', cert: { subjectaltname: 'DNS:A.COM', subject: { CN: 'b.com' } - }, - result: true + } }, // DNS names @@ -104,65 +153,64 @@ var tests = [ subjectaltname: 'DNS:*.a.com', subject: {} }, - result: false + error: 'Host: a.com. is not in the cert\'s altnames: ' + + 'DNS:*.a.com' }, { host: 'b.a.com', cert: { subjectaltname: 'DNS:*.a.com', subject: {} - }, - result: true + } }, { host: 'c.b.a.com', cert: { subjectaltname: 'DNS:*.a.com', subject: {} }, - result: false + error: 'Host: c.b.a.com. is not in the cert\'s altnames: ' + + 'DNS:*.a.com' }, { host: 'b.a.com', cert: { subjectaltname: 'DNS:*b.a.com', subject: {} - }, - result: true + } }, { host: 'a-cb.a.com', cert: { subjectaltname: 'DNS:*b.a.com', subject: {} - }, - result: true + } }, { host: 'a.b.a.com', cert: { subjectaltname: 'DNS:*b.a.com', subject: {} }, - result: false + error: 'Host: a.b.a.com. is not in the cert\'s altnames: ' + + 'DNS:*b.a.com' }, // Mutliple DNS names { host: 'a.b.a.com', cert: { subjectaltname: 'DNS:*b.a.com, DNS:a.b.a.com', subject: {} - }, - result: true + } }, // URI names { host: 'a.b.a.com', cert: { subjectaltname: 'URI:http://a.b.a.com/', subject: {} - }, - result: true + } }, { host: 'a.b.a.com', cert: { subjectaltname: 'URI:http://*.b.a.com/', subject: {} }, - result: false + error: 'Host: a.b.a.com. is not in the cert\'s altnames: ' + + 'URI:http://*.b.a.com/' }, // IP addresses { @@ -170,40 +218,58 @@ var tests = [ subjectaltname: 'IP Address:127.0.0.1', subject: {} }, - result: false + error: 'Host: a.b.a.com. is not in the cert\'s altnames: ' + + 'IP Address:127.0.0.1' }, { host: '127.0.0.1', cert: { subjectaltname: 'IP Address:127.0.0.1', subject: {} - }, - result: true + } }, { host: '127.0.0.2', cert: { subjectaltname: 'IP Address:127.0.0.1', subject: {} }, - result: false + error: 'IP: 127.0.0.2 is not in the cert\'s list: ' + + '127.0.0.1' }, { host: '127.0.0.1', cert: { subjectaltname: 'DNS:a.com', subject: {} }, - result: false + error: 'IP: 127.0.0.1 is not in the cert\'s list: ' }, { host: 'localhost', cert: { subjectaltname: 'DNS:a.com', subject: { CN: 'localhost' } }, - result: false + 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) { - assert.equal(tls.checkServerIdentity(test.host, test.cert), - test.result, - 'Test#' + i + ' failed: ' + util.inspect(test)); + var err = tls._checkServerIdentity(test.host, test.cert); + assert.equal(err && err.reason, + test.error, + 'Test#' + i + ' failed: ' + util.inspect(test) + '\n' + + test.error + ' != ' + (err && err.reason)); });