Skip to content

Commit 743f0c9

Browse files
bnoordhuisevanlucas
bnoordhuis
authored and
evanlucas
committed
lib: make tls.checkServerIdentity() more strict
PR-URL: https://github.com/nodejs/node-private/pull/75 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: James M Snell <[email protected]>
1 parent e5998c4 commit 743f0c9

File tree

2 files changed

+168
-114
lines changed

2 files changed

+168
-114
lines changed

‎lib/tls.js

+117-114
Original file line numberDiff line numberDiff line change
@@ -66,134 +66,137 @@ exports.convertALPNProtocols = function(protocols, out) {
6666
}
6767
};
6868

69-
exports.checkServerIdentity = function checkServerIdentity(host, cert) {
70-
// Create regexp to much hostnames
71-
function regexpify(host, wildcards) {
72-
// Add trailing dot (make hostnames uniform)
73-
if (!host || !host.endsWith('.')) host += '.';
74-
75-
// The same applies to hostname with more than one wildcard,
76-
// if hostname has wildcard when wildcards are not allowed,
77-
// or if there are less than two dots after wildcard (i.e. *.com or *d.com)
78-
//
79-
// also
80-
//
81-
// "The client SHOULD NOT attempt to match a presented identifier in
82-
// which the wildcard character comprises a label other than the
83-
// left-most label (e.g., do not match bar.*.example.net)."
84-
// RFC6125
85-
if (!wildcards && /\*/.test(host) || /[\.\*].*\*/.test(host) ||
86-
/\*/.test(host) && !/\*.*\..+\..+/.test(host)) {
87-
return /$./;
88-
}
69+
function unfqdn(host) {
70+
return host.replace(/[.]$/, '');
71+
}
8972

90-
// Replace wildcard chars with regexp's wildcard and
91-
// escape all characters that have special meaning in regexps
92-
// (i.e. '.', '[', '{', '*', and others)
93-
var re = host.replace(
94-
/\*([a-z0-9\\-_\.])|[\.,\-\\\^\$+?*\[\]\(\):!\|{}]/g,
95-
function(all, sub) {
96-
if (sub) return '[a-z0-9\\-_]*' + (sub === '-' ? '\\-' : sub);
97-
return '\\' + all;
98-
});
99-
100-
return new RegExp('^' + re + '$', 'i');
101-
}
73+
function splitHost(host) {
74+
// String#toLowerCase() is locale-sensitive so we use
75+
// a conservative version that only lowercases A-Z.
76+
const replacer = (c) => String.fromCharCode(32 + c.charCodeAt(0));
77+
return unfqdn(host).replace(/[A-Z]/g, replacer).split('.');
78+
}
79+
80+
function check(hostParts, pattern, wildcards) {
81+
// Empty strings, null, undefined, etc. never match.
82+
if (!pattern)
83+
return false;
84+
85+
const patternParts = splitHost(pattern);
86+
87+
if (hostParts.length !== patternParts.length)
88+
return false;
89+
90+
// Pattern has empty components, e.g. "bad..example.com".
91+
if (patternParts.includes(''))
92+
return false;
93+
94+
// RFC 6125 allows IDNA U-labels (Unicode) in names but we have no
95+
// good way to detect their encoding or normalize them so we simply
96+
// reject them. Control characters and blanks are rejected as well
97+
// because nothing good can come from accepting them.
98+
const isBad = (s) => /[^\u0021-\u007F]/u.test(s);
99+
if (patternParts.some(isBad))
100+
return false;
101+
102+
// Check host parts from right to left first.
103+
for (let i = hostParts.length - 1; i > 0; i -= 1)
104+
if (hostParts[i] !== patternParts[i])
105+
return false;
106+
107+
const hostSubdomain = hostParts[0];
108+
const patternSubdomain = patternParts[0];
109+
const patternSubdomainParts = patternSubdomain.split('*');
110+
111+
// Short-circuit when the subdomain does not contain a wildcard.
112+
// RFC 6125 does not allow wildcard substitution for components
113+
// containing IDNA A-labels (Punycode) so match those verbatim.
114+
if (patternSubdomainParts.length === 1 || patternSubdomain.includes('xn--'))
115+
return hostSubdomain === patternSubdomain;
116+
117+
if (!wildcards)
118+
return false;
119+
120+
// More than one wildcard is always wrong.
121+
if (patternSubdomainParts.length > 2)
122+
return false;
123+
124+
// *.tld wildcards are not allowed.
125+
if (patternParts.length <= 2)
126+
return false;
127+
128+
const [prefix, suffix] = patternSubdomainParts;
129+
130+
if (prefix.length + suffix.length > hostSubdomain.length)
131+
return false;
102132

103-
var dnsNames = [];
104-
var uriNames = [];
133+
if (!hostSubdomain.startsWith(prefix))
134+
return false;
135+
136+
if (!hostSubdomain.endsWith(suffix))
137+
return false;
138+
139+
return true;
140+
}
141+
142+
exports.checkServerIdentity = function checkServerIdentity(host, cert) {
143+
const subject = cert.subject;
144+
const altNames = cert.subjectaltname;
145+
const dnsNames = [];
146+
const uriNames = [];
105147
const ips = [];
106-
var matchCN = true;
107-
var valid = false;
108-
var reason = 'Unknown reason';
109-
110-
// There're several names to perform check against:
111-
// CN and altnames in certificate extension
112-
// (DNS names, IP addresses, and URIs)
113-
//
114-
// Walk through altnames and generate lists of those names
115-
if (cert.subjectaltname) {
116-
cert.subjectaltname.split(/, /g).forEach(function(altname) {
117-
var option = altname.match(/^(DNS|IP Address|URI):(.*)$/);
118-
if (!option)
119-
return;
120-
if (option[1] === 'DNS') {
121-
dnsNames.push(option[2]);
122-
} else if (option[1] === 'IP Address') {
123-
ips.push(option[2]);
124-
} else if (option[1] === 'URI') {
125-
var uri = url.parse(option[2]);
126-
if (uri) uriNames.push(uri.hostname);
127-
}
128-
});
129-
}
130148

131-
// If hostname is an IP address, it should be present in the list of IP
132-
// addresses.
133-
if (net.isIP(host)) {
134-
valid = ips.some(function(ip) {
135-
return ip === host;
136-
});
137-
if (!valid) {
138-
reason = `IP: ${host} is not in the cert's list: ${ips.join(', ')}`;
139-
}
140-
} else if (cert.subject) {
141-
// Transform hostname to canonical form
142-
if (!host || !host.endsWith('.')) host += '.';
143-
144-
// Otherwise check all DNS/URI records from certificate
145-
// (with allowed wildcards)
146-
dnsNames = dnsNames.map(function(name) {
147-
return regexpify(name, true);
148-
});
149-
150-
// Wildcards ain't allowed in URI names
151-
uriNames = uriNames.map(function(name) {
152-
return regexpify(name, false);
153-
});
154-
155-
dnsNames = dnsNames.concat(uriNames);
156-
157-
if (dnsNames.length > 0) matchCN = false;
158-
159-
// Match against Common Name (CN) only if no supported identifiers are
160-
// present.
161-
//
162-
// "As noted, a client MUST NOT seek a match for a reference identifier
163-
// of CN-ID if the presented identifiers include a DNS-ID, SRV-ID,
164-
// URI-ID, or any application-specific identifier types supported by the
165-
// client."
166-
// RFC6125
167-
if (matchCN) {
168-
var commonNames = cert.subject.CN;
169-
if (Array.isArray(commonNames)) {
170-
for (var i = 0, k = commonNames.length; i < k; ++i) {
171-
dnsNames.push(regexpify(commonNames[i], true));
172-
}
173-
} else {
174-
dnsNames.push(regexpify(commonNames, true));
149+
host = '' + host;
150+
151+
if (altNames) {
152+
for (const name of altNames.split(', ')) {
153+
if (name.startsWith('DNS:')) {
154+
dnsNames.push(name.slice(4));
155+
} else if (name.startsWith('URI:')) {
156+
const uri = url.parse(name.slice(4));
157+
uriNames.push(uri.hostname); // TODO(bnoordhuis) Also use scheme.
158+
} else if (name.startsWith('IP Address:')) {
159+
ips.push(name.slice(11));
175160
}
176161
}
162+
}
177163

178-
valid = dnsNames.some(function(re) {
179-
return re.test(host);
180-
});
164+
let valid = false;
165+
let reason = 'Unknown reason';
181166

182-
if (!valid) {
183-
if (cert.subjectaltname) {
184-
reason =
185-
`Host: ${host} is not in the cert's altnames: ` +
186-
`${cert.subjectaltname}`;
187-
} else {
188-
reason = `Host: ${host} is not cert's CN: ${cert.subject.CN}`;
189-
}
167+
if (net.isIP(host)) {
168+
valid = ips.includes(host);
169+
if (!valid)
170+
reason = `IP: ${host} is not in the cert's list: ${ips.join(', ')}`;
171+
// TODO(bnoordhuis) Also check URI SANs that are IP addresses.
172+
} else if (subject) {
173+
host = unfqdn(host); // Remove trailing dot for error messages.
174+
const hostParts = splitHost(host);
175+
const wildcard = (pattern) => check(hostParts, pattern, true);
176+
const noWildcard = (pattern) => check(hostParts, pattern, false);
177+
178+
// Match against Common Name only if no supported identifiers are present.
179+
if (dnsNames.length === 0 && ips.length === 0 && uriNames.length === 0) {
180+
const cn = subject.CN;
181+
182+
if (Array.isArray(cn))
183+
valid = cn.some(wildcard);
184+
else if (cn)
185+
valid = wildcard(cn);
186+
187+
if (!valid)
188+
reason = `Host: ${host}. is not cert's CN: ${cn}`;
189+
} else {
190+
valid = dnsNames.some(wildcard) || uriNames.some(noWildcard);
191+
if (!valid)
192+
reason = `Host: ${host}. is not in the cert's altnames: ${altNames}`;
190193
}
191194
} else {
192195
reason = 'Cert is empty';
193196
}
194197

195198
if (!valid) {
196-
var err = new Error(
199+
const err = new Error(
197200
`Hostname/IP doesn't match certificate's altnames: "${reason}"`);
198201
err.reason = reason;
199202
err.host = host;

‎test/parallel/test-tls-check-server-identity.js

+51
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,23 @@ var tls = require('tls');
1111

1212

1313
var tests = [
14+
// False-y values.
15+
{
16+
host: false,
17+
cert: { subject: { CN: 'a.com' } },
18+
error: 'Host: false. is not cert\'s CN: a.com'
19+
},
20+
{
21+
host: null,
22+
cert: { subject: { CN: 'a.com' } },
23+
error: 'Host: null. is not cert\'s CN: a.com'
24+
},
25+
{
26+
host: undefined,
27+
cert: { subject: { CN: 'a.com' } },
28+
error: 'Host: undefined. is not cert\'s CN: a.com'
29+
},
30+
1431
// Basic CN handling
1532
{ host: 'a.com', cert: { subject: { CN: 'a.com' } } },
1633
{ host: 'a.com', cert: { subject: { CN: 'A.COM' } } },
@@ -20,15 +37,35 @@ var tests = [
2037
error: 'Host: a.com. is not cert\'s CN: b.com'
2138
},
2239
{ host: 'a.com', cert: { subject: { CN: 'a.com.' } } },
40+
{
41+
host: 'a.com',
42+
cert: { subject: { CN: '.a.com' } },
43+
error: 'Host: a.com. is not cert\'s CN: .a.com'
44+
},
2345

2446
// Wildcards in CN
2547
{ host: 'b.a.com', cert: { subject: { CN: '*.a.com' } } },
48+
{
49+
host: 'ba.com',
50+
cert: { subject: { CN: '*.a.com' } },
51+
error: 'Host: ba.com. is not cert\'s CN: *.a.com'
52+
},
53+
{
54+
host: '\n.b.com',
55+
cert: { subject: { CN: '*n.b.com' } },
56+
error: 'Host: \n.b.com. is not cert\'s CN: *n.b.com'
57+
},
2658
{ host: 'b.a.com', cert: {
2759
subjectaltname: 'DNS:omg.com',
2860
subject: { CN: '*.a.com' } },
2961
error: 'Host: b.a.com. is not in the cert\'s altnames: ' +
3062
'DNS:omg.com'
3163
},
64+
{
65+
host: 'b.a.com',
66+
cert: { subject: { CN: 'b*b.a.com' } },
67+
error: 'Host: b.a.com. is not cert\'s CN: b*b.a.com'
68+
},
3269

3370
// Empty Cert
3471
{
@@ -199,6 +236,20 @@ var tests = [
199236
error: 'Host: localhost. is not in the cert\'s altnames: ' +
200237
'DNS:a.com'
201238
},
239+
// IDNA
240+
{
241+
host: 'xn--bcher-kva.example.com',
242+
cert: { subject: { CN: '*.example.com' } },
243+
},
244+
// RFC 6125, section 6.4.3: "[...] the client SHOULD NOT attempt to match
245+
// a presented identifier where the wildcard character is embedded within
246+
// an A-label [...]"
247+
{
248+
host: 'xn--bcher-kva.example.com',
249+
cert: { subject: { CN: 'xn--*.example.com' } },
250+
error: 'Host: xn--bcher-kva.example.com. is not cert\'s CN: ' +
251+
'xn--*.example.com',
252+
},
202253
];
203254

204255
tests.forEach(function(test, i) {

0 commit comments

Comments
 (0)