Skip to content

Commit

Permalink
lib: make tls.checkServerIdentity() more strict
Browse files Browse the repository at this point in the history 
PR-URL: nodejs-private/node-private#63
Reviewed-By: Rod Vagg <[email protected]>
  • Loading branch information
@bnoordhuis @rvagg
bnoordhuis authored and rvagg committed Sep 27, 2016
1 parent b5c57ff commit 3ff82de
Show file tree
Hide file tree
Showing 2 changed files with 172 additions and 114 deletions.
235 changes: 121 additions & 114 deletions lib/tls.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,134 +56,141 @@ 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 (!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
// 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 unfqdn(host) {
return host.replace(/[.]$/, '');
}

function splitHost(host) {
// String#toLowerCase() is locale-sensitive so we use
// a conservative version that only lowercases A-Z.
const replacer = (c) => 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;

const 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.
const isBad = (s) => /[^\u0021-\u007F]/.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.indexOf('xn--') !== -1) {
return hostSubdomain === patternSubdomain;
}

var dnsNames = [];
var uriNames = [];

if (!wildcards)

This comment has been minimized.

Sign in to view

Copy link
@michaelkruglos

michaelkruglos Sep 28, 2016

Missing curly braces
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 = patternSubdomainParts[0];
const suffix = patternSubdomainParts[1];

if (prefix.length + suffix.length > hostSubdomain.length)
return false;

if (!hostSubdomain.startsWith(prefix))
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
// addresses.
if (net.isIP(host)) {
valid = ips.some(function(ip) {
return ip === host;
});
if (!valid) {
reason = `IP: ${host} is not in the cert's list: ${ips.join(', ')}`;
}
} else if (cert.subject) {
// Transform hostname to canonical form
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));
host = '' + host;

if (altNames) {
for (const name of altNames.split(', ')) {
if (name.startsWith('DNS:')) {
dnsNames.push(name.slice(4));
} else if (name.startsWith('URI:')) {
const uri = url.parse(name.slice(4));
uriNames.push(uri.hostname); // TODO(bnoordhuis) Also use scheme.
} else if (name.startsWith('IP Address:')) {
ips.push(name.slice(11));
}
}
}

valid = dnsNames.some(function(re) {
return re.test(host);
});
let valid = false;
let reason = 'Unknown reason';

if (!valid) {
if (cert.subjectaltname) {
reason =
`Host: ${host} is not in the cert's altnames: ` +
`${cert.subjectaltname}`;
} else {
reason = `Host: ${host} is not cert's CN: ${cert.subject.CN}`;
}
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.
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;
Expand Down
51 changes: 51 additions & 0 deletions test/parallel/test-tls-check-server-identity.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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' } } },
Expand All @@ -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
{
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit 3ff82de

Please sign in to comment.