Skip to content

Commit

Permalink
lib: make tls.checkServerIdentity() more strict
Browse files Browse the repository at this point in the history 
CVE-2016-7099

PR-URL: nodejs-private/node-private#61
Reviewed-By: Rod Vagg <rod@vagg.org>
  • Loading branch information
@bnoordhuis @rvagg
bnoordhuis authored and rvagg committed Sep 27, 2016
1 parent 6d97790 commit 9dbde2f
Show file tree
Hide file tree
Showing 2 changed files with 187 additions and 113 deletions.
242 changes: 129 additions & 113 deletions lib/tls.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 += '.';

// 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 unfqdn(host) {
return host.replace(/[.]$/, '');
}

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('.');
}

function check(hostParts, pattern, wildcards) {
// Empty strings, null, undefined, etc. never match.
if (!pattern)
return false;

var patternParts = splitHost(pattern);

// 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');
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';

// 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 (patternParts.some(isBad))
return false;

// 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.
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(', '));
}
} else {
// Transform hostname to canonical form
if (!/\.$/.test(host)) host += '.';
var valid = false;
var reason = 'Unknown reason';

// Otherwise check all DNS/URI records from certificate
// (with allowed wildcards)
dnsNames = dnsNames.map(function(name) {
return regexpify(name, true);
});
if (net.isIP(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);

// Wildcards ain't allowed in URI names
uriNames = uriNames.map(function(name) {
return regexpify(name, false);
});
function wildcard(pattern) {
return check(hostParts, pattern, true);
}

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));
}
function noWildcard(pattern) {
return check(hostParts, pattern, false);
}

valid = dnsNames.some(function(re) {
return re.test(host);
});
// 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 (!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);
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(
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;
Expand Down
58 changes: 58 additions & 0 deletions test/simple/test-tls-check-server-identity.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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' } } },
Expand All @@ -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
{
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit 9dbde2f

Please sign in to comment.