Skip to content

Commit 9f2c526

Browse files
mhdawsonkumarak
authored andcommittedJan 7, 2022
src: add cve reverts and associated tests
Co-authored-by: Akshay K <iit.akshay@gmail.com> Backport-PR-URL: nodejs-private/node-private#305 PR-URL: nodejs-private/node-private#300 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
1 parent 83d8f88 commit 9f2c526

6 files changed

+556
-3
lines changed
 

‎lib/tls.js

+29-3
Original file line numberDiff line numberDiff line change
@@ -50,9 +50,11 @@ const { isArrayBufferView } = require('internal/util/types');
5050

5151
const net = require('net');
5252
const { getOptionValue } = require('internal/options');
53+
const url = require('url');
5354
const { getRootCertificates, getSSLCiphers } = internalBinding('crypto');
5455
const { Buffer } = require('buffer');
5556
const EventEmitter = require('events');
57+
const { URL } = require('internal/url');
5658
const DuplexPair = require('internal/streams/duplexpair');
5759
const { canonicalizeIP } = internalBinding('cares_wrap');
5860
const _tls_common = require('_tls_common');
@@ -261,10 +263,12 @@ function splitEscapedAltNames(altNames) {
261263
return result;
262264
}
263265

266+
let urlWarningEmitted = false;
264267
exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
265268
const subject = cert.subject;
266269
const altNames = cert.subjectaltname;
267270
const dnsNames = [];
271+
const uriNames = [];
268272
const ips = [];
269273

270274
hostname = '' + hostname;
@@ -276,6 +280,22 @@ exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
276280
for (const name of splitAltNames) {
277281
if (name.startsWith('DNS:')) {
278282
dnsNames.push(name.slice(4));
283+
} else if (process.REVERT_CVE_2021_44531 && name.startsWith('URI:')) {
284+
let uri;
285+
try {
286+
uri = new URL(name.slice(4));
287+
} catch {
288+
uri = url.parse(name.slice(4));
289+
if (!urlWarningEmitted && !process.noDeprecation) {
290+
urlWarningEmitted = true;
291+
process.emitWarning(
292+
`The URI ${name.slice(4)} found in cert.subjectaltname ` +
293+
'is not a valid URI, and is supported in the tls module ' +
294+
'solely for compatibility.',
295+
'DeprecationWarning', 'DEP0109');
296+
}
297+
}
298+
uriNames.push(uri.hostname); // TODO(bnoordhuis) Also use scheme.
279299
} else if (name.startsWith('IP Address:')) {
280300
ips.push(canonicalizeIP(name.slice(11)));
281301
}
@@ -285,19 +305,25 @@ exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
285305
let valid = false;
286306
let reason = 'Unknown reason';
287307

308+
const hasAltNames =
309+
dnsNames.length > 0 || ips.length > 0 || uriNames.length > 0;
310+
288311
hostname = unfqdn(hostname); // Remove trailing dot for error messages.
289312

290313
if (net.isIP(hostname)) {
291314
valid = ips.includes(canonicalizeIP(hostname));
292315
if (!valid)
293316
reason = `IP: ${hostname} is not in the cert's list: ${ips.join(', ')}`;
294317
// TODO(bnoordhuis) Also check URI SANs that are IP addresses.
295-
} else if (dnsNames.length > 0 || (subject && subject.CN)) {
318+
} else if ((process.REVERT_CVE_2021_44531 && (hasAltNames || subject)) ||
319+
(dnsNames.length > 0 || (subject && subject.CN))) {
296320
const hostParts = splitHost(hostname);
297321
const wildcard = (pattern) => check(hostParts, pattern, true);
298322

299-
if (dnsNames.length > 0) {
300-
valid = dnsNames.some(wildcard);
323+
if ((process.REVERT_CVE_2021_44531 && hasAltNames) ||
324+
(dnsNames.length > 0)) {
325+
const noWildcard = (pattern) => check(hostParts, pattern, false);
326+
valid = dnsNames.some(wildcard) || uriNames.some(noWildcard);
301327
if (!valid)
302328
reason =
303329
`Host: ${hostname}. is not in the cert's altnames: ${altNames}`;

‎src/node_crypto_common.cc

+47
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#include "node_crypto_common.h"
77
#include "node.h"
88
#include "node_internals.h"
9+
#include "node_revert.h"
910
#include "node_url.h"
1011
#include "string_bytes.h"
1112
#include "v8.h"
@@ -482,6 +483,44 @@ void AddFingerprintDigest(
482483
}
483484
}
484485

486+
// deprecated, only used for security revert
487+
bool SafeX509ExtPrint(const BIOPointer& out, X509_EXTENSION* ext) {
488+
const X509V3_EXT_METHOD* method = X509V3_EXT_get(ext);
489+
490+
if (method != X509V3_EXT_get_nid(NID_subject_alt_name))
491+
return false;
492+
493+
GENERAL_NAMES* names = static_cast<GENERAL_NAMES*>(X509V3_EXT_d2i(ext));
494+
if (names == nullptr)
495+
return false;
496+
497+
for (int i = 0; i < sk_GENERAL_NAME_num(names); i++) {
498+
GENERAL_NAME* gen = sk_GENERAL_NAME_value(names, i);
499+
500+
if (i != 0)
501+
BIO_write(out.get(), ", ", 2);
502+
503+
if (gen->type == GEN_DNS) {
504+
ASN1_IA5STRING* name = gen->d.dNSName;
505+
506+
BIO_write(out.get(), "DNS:", 4);
507+
BIO_write(out.get(), name->data, name->length);
508+
} else {
509+
STACK_OF(CONF_VALUE)* nval = i2v_GENERAL_NAME(
510+
const_cast<X509V3_EXT_METHOD*>(method), gen, nullptr);
511+
if (nval == nullptr) {
512+
sk_GENERAL_NAME_pop_free(names, GENERAL_NAME_free);
513+
return false;
514+
}
515+
X509V3_EXT_val_prn(out.get(), nval, 0, 0);
516+
sk_CONF_VALUE_pop_free(nval, X509V3_conf_free);
517+
}
518+
}
519+
sk_GENERAL_NAME_pop_free(names, GENERAL_NAME_free);
520+
521+
return true;
522+
}
523+
485524
static inline bool IsSafeAltName(const char* name, size_t length, bool utf8) {
486525
for (size_t i = 0; i < length; i++) {
487526
char c = name[i];
@@ -706,6 +745,10 @@ bool SafeX509SubjectAltNamePrint(const BIOPointer& out, X509_EXTENSION* ext) {
706745
const X509V3_EXT_METHOD* method = X509V3_EXT_get(ext);
707746
CHECK(method == X509V3_EXT_get_nid(NID_subject_alt_name));
708747

748+
if (IsReverted(SECURITY_REVERT_CVE_2021_44532)) {
749+
return SafeX509ExtPrint(out, ext);
750+
}
751+
709752
GENERAL_NAMES* names = static_cast<GENERAL_NAMES*>(X509V3_EXT_d2i(ext));
710753
if (names == nullptr)
711754
return false;
@@ -731,6 +774,10 @@ bool SafeX509InfoAccessPrint(const BIOPointer& out, X509_EXTENSION* ext) {
731774
const X509V3_EXT_METHOD* method = X509V3_EXT_get(ext);
732775
CHECK(method == X509V3_EXT_get_nid(NID_info_access));
733776

777+
if (IsReverted(SECURITY_REVERT_CVE_2021_44532)) {
778+
return (X509V3_EXT_print(out.get(), ext, 0, 0) == 1);
779+
}
780+
734781
AUTHORITY_INFO_ACCESS* descs =
735782
static_cast<AUTHORITY_INFO_ACCESS*>(X509V3_EXT_d2i(ext));
736783
if (descs == nullptr)

‎src/node_revert.h

+2
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
namespace node {
1717

1818
#define SECURITY_REVERSIONS(XX) \
19+
XX(CVE_2021_44531, "CVE-2021-44531", "Cert Verif Bypass via URI SAN") \
20+
XX(CVE_2021_44532, "CVE-2021-44532", "Cert Verif Bypass via Str Inject") \
1921
// XX(CVE_2016_PEND, "CVE-2016-PEND", "Vulnerability Title")
2022

2123
enum reversion {
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
// Flags: --security-revert=CVE-2021-44531
2+
'use strict';
3+
const common = require('../common');
4+
5+
if (!common.hasCrypto)
6+
common.skip('missing crypto');
7+
8+
const assert = require('assert');
9+
const util = require('util');
10+
11+
const tls = require('tls');
12+
13+
common.expectWarning('DeprecationWarning', [
14+
['The URI http://[a.b.a.com]/ found in cert.subjectaltname ' +
15+
'is not a valid URI, and is supported in the tls module ' +
16+
'solely for compatibility.',
17+
'DEP0109'],
18+
]);
19+
20+
const tests = [
21+
// Likewise for "URI:" Subject Alternative Names.
22+
// See also https://github.com/nodejs/node/issues/8108.
23+
{
24+
host: '8.8.8.8',
25+
cert: { subject: { CN: '8.8.8.8' }, subjectaltname: 'URI:http://8.8.8.8/' },
26+
error: 'IP: 8.8.8.8 is not in the cert\'s list: '
27+
},
28+
// Empty Subject w/URI name
29+
{
30+
host: 'a.b.a.com', cert: {
31+
subjectaltname: 'URI:http://a.b.a.com/',
32+
}
33+
},
34+
// URI names
35+
{
36+
host: 'a.b.a.com', cert: {
37+
subjectaltname: 'URI:http://a.b.a.com/',
38+
subject: {}
39+
}
40+
},
41+
{
42+
host: 'a.b.a.com', cert: {
43+
subjectaltname: 'URI:http://*.b.a.com/',
44+
subject: {}
45+
},
46+
error: 'Host: a.b.a.com. is not in the cert\'s altnames: ' +
47+
'URI:http://*.b.a.com/'
48+
},
49+
// Invalid URI
50+
{
51+
host: 'a.b.a.com', cert: {
52+
subjectaltname: 'URI:http://[a.b.a.com]/',
53+
subject: {}
54+
}
55+
},
56+
];
57+
58+
tests.forEach(function(test, i) {
59+
const err = tls.checkServerIdentity(test.host, test.cert);
60+
assert.strictEqual(err && err.reason,
61+
test.error,
62+
`Test# ${i} failed: ${util.inspect(test)} \n` +
63+
`${test.error} != ${(err && err.reason)}`);
64+
});
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
// Copyright Joyent, Inc. and other Node contributors.
2+
//
3+
// Permission is hereby granted, free of charge, to any person obtaining a
4+
// copy of this software and associated documentation files (the
5+
// "Software"), to deal in the Software without restriction, including
6+
// without limitation the rights to use, copy, modify, merge, publish,
7+
// distribute, sublicense, and/or sell copies of the Software, and to permit
8+
// persons to whom the Software is furnished to do so, subject to the
9+
// following conditions:
10+
//
11+
// The above copyright notice and this permission notice shall be included
12+
// in all copies or substantial portions of the Software.
13+
//
14+
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
15+
// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
16+
// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN
17+
// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
18+
// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
19+
// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
20+
// USE OR OTHER DEALINGS IN THE SOFTWARE.
21+
// Flags: --security-revert=CVE-2021-44532
22+
'use strict';
23+
const common = require('../common');
24+
if (!common.hasCrypto)
25+
common.skip('missing crypto');
26+
27+
// Check getPeerCertificate can properly handle '\0' for fix CVE-2009-2408.
28+
29+
const assert = require('assert');
30+
const tls = require('tls');
31+
const fixtures = require('../common/fixtures');
32+
33+
const server = tls.createServer({
34+
key: fixtures.readSync(['0-dns', '0-dns-key.pem']),
35+
cert: fixtures.readSync(['0-dns', '0-dns-cert.pem'])
36+
}, common.mustCall((c) => {
37+
c.once('data', common.mustCall(() => {
38+
c.destroy();
39+
server.close();
40+
}));
41+
})).listen(0, common.mustCall(() => {
42+
const c = tls.connect(server.address().port, {
43+
rejectUnauthorized: false
44+
}, common.mustCall(() => {
45+
const cert = c.getPeerCertificate();
46+
assert.strictEqual(cert.subjectaltname,
47+
'DNS:good.example.org\0.evil.example.com, ' +
48+
'DNS:just-another.example.com, ' +
49+
'IP Address:8.8.8.8, ' +
50+
'IP Address:8.8.4.4, ' +
51+
'DNS:last.example.com');
52+
c.write('ok');
53+
}));
54+
}));
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,360 @@
1+
// Flags: --security-revert=CVE-2021-44532
2+
'use strict';
3+
4+
const common = require('../common');
5+
if (!common.hasCrypto)
6+
common.skip('missing crypto');
7+
8+
const assert = require('assert');
9+
const tls = require('tls');
10+
const fixtures = require('../common/fixtures');
11+
12+
const { hasOpenSSL3 } = common;
13+
14+
// Test escaping rules for subject alternative names.
15+
{
16+
const expectedSANs = [
17+
'DNS:good.example.com, DNS:evil.example.com',
18+
// URIs should not require escaping.
19+
'URI:http://example.com/',
20+
'URI:http://example.com/?a=b&c=d',
21+
// Unless they contain commas.
22+
'URI:http://example.com/a,b',
23+
// Percent encoding should not require escaping.
24+
'URI:http://example.com/a%2Cb',
25+
// Malicious attempts should be escaped.
26+
'URI:http://example.com/a, DNS:good.example.com',
27+
// Non-ASCII characters in DNS names should be treated as Latin-1.
28+
'DNS:ex�mple.com',
29+
// It should not be possible to cause unescaping without escaping.
30+
'DNS:"evil.example.com"',
31+
// IPv4 addresses should be represented as usual.
32+
'IP Address:8.8.8.8',
33+
'IP Address:8.8.4.4',
34+
// For backward-compatibility, include invalid IP address lengths.
35+
hasOpenSSL3 ? 'IP Address:<invalid length=5>' : 'IP Address:<invalid>',
36+
hasOpenSSL3 ? 'IP Address:<invalid length=6>' : 'IP Address:<invalid>',
37+
// IPv6 addresses are represented as OpenSSL does.
38+
'IP Address:A0B:C0D:E0F:0:0:0:7A7B:7C7D',
39+
// Regular email addresses don't require escaping.
40+
'email:foo@example.com',
41+
// ... but should be escaped if they contain commas.
42+
'email:foo@example.com, DNS:good.example.com',
43+
'DirName:/C=DE/L=Hannover',
44+
// TODO(tniessen): support UTF8 in DirName
45+
'DirName:/C=DE/L=M\\xC3\\xBCnchen',
46+
'DirName:/C=DE/L=Berlin, DNS:good.example.com',
47+
'DirName:/C=DE/L=Berlin, DNS:good.example.com\\x00' +
48+
'evil.example.com',
49+
'DirName:/C=DE/L=Berlin, DNS:good.example.com\\\\x00' +
50+
'evil.example.com',
51+
// These next two tests might be surprising. OpenSSL applies its own rules
52+
// first, which introduce backslashes, which activate node's escaping.
53+
// Unfortunately, there are also differences between OpenSSL 1.1.1 and 3.0.
54+
'DirName:/C=DE/L=Berlin\\x0D\\x0A',
55+
hasOpenSSL3 ?
56+
'DirName:/C=DE/L=Berlin\\/CN=good.example.com' :
57+
'DirName:/C=DE/L=Berlin/CN=good.example.com',
58+
// TODO(tniessen): even OIDs that are well-known (such as the following,
59+
// which is sha256WithRSAEncryption) should be represented numerically only.
60+
'Registered ID:sha256WithRSAEncryption',
61+
// This is an OID that will likely never be assigned to anything, thus
62+
// OpenSSL hould not know it.
63+
'Registered ID:1.3.9999.12.34',
64+
hasOpenSSL3 ?
65+
'othername: XmppAddr::abc123' :
66+
'othername:<unsupported>',
67+
hasOpenSSL3 ?
68+
'othername: XmppAddr::abc123, DNS:good.example.com' :
69+
'othername:<unsupported>',
70+
hasOpenSSL3 ?
71+
null :
72+
'othername:<unsupported>',
73+
// This is unsupported because the OID is not recognized.
74+
hasOpenSSL3 ?
75+
'othername: 1.3.9999.12.34::abc123' :
76+
'othername:<unsupported>',
77+
hasOpenSSL3 ? 'othername: SRVName::abc123' : 'othername:<unsupported>',
78+
// This is unsupported because it is an SRVName with a UTF8String value,
79+
// which is not allowed for SRVName.
80+
hasOpenSSL3 ?
81+
null : 'othername:<unsupported>',
82+
hasOpenSSL3 ?
83+
null :
84+
'othername:<unsupported>',
85+
];
86+
87+
const serverKey = fixtures.readSync('x509-escaping/server-key.pem', 'utf8');
88+
89+
for (let i = 0; i < expectedSANs.length; i++) {
90+
const pem = fixtures.readSync(`x509-escaping/alt-${i}-cert.pem`, 'utf8');
91+
92+
// X509Certificate interface is not supported in v12.x & v14.x. Disable
93+
// checks for subjectAltName with expectedSANs. The testcase is ported
94+
// from v17.x
95+
//
96+
// Test the subjectAltName property of the X509Certificate API.
97+
// const cert = new X509Certificate(pem);
98+
// assert.strictEqual(cert.subjectAltName, expectedSANs[i]);
99+
100+
// Test that the certificate obtained by checkServerIdentity has the correct
101+
// subjectaltname property.
102+
const server = tls.createServer({
103+
key: serverKey,
104+
cert: pem,
105+
}, common.mustCall((conn) => {
106+
conn.destroy();
107+
server.close();
108+
})).listen(common.mustCall(() => {
109+
const { port } = server.address();
110+
tls.connect(port, {
111+
ca: pem,
112+
servername: 'example.com',
113+
checkServerIdentity: (hostname, peerCert) => {
114+
assert.strictEqual(hostname, 'example.com');
115+
assert.strictEqual(peerCert.subjectaltname, expectedSANs[i]);
116+
},
117+
}, common.mustCall());
118+
}));
119+
}
120+
}
121+
122+
// Test escaping rules for authority info access.
123+
{
124+
const expectedInfoAccess = [
125+
{
126+
text: 'OCSP - URI:http://good.example.com/\n' +
127+
'OCSP - URI:http://evil.example.com/',
128+
legacy: {
129+
'OCSP - URI': [
130+
'http://good.example.com/',
131+
'http://evil.example.com/',
132+
],
133+
},
134+
},
135+
{
136+
text: 'CA Issuers - URI:http://ca.example.com/\n' +
137+
'OCSP - URI:http://evil.example.com\n' +
138+
'OCSP - DNS:good.example.com\n' +
139+
'OCSP - URI:http://ca.nodejs.org/ca.cert',
140+
legacy: {
141+
'CA Issuers - URI': [
142+
'http://ca.example.com/',
143+
],
144+
'OCSP - DNS': [
145+
'good.example.com',
146+
],
147+
'OCSP - URI': [
148+
'http://evil.example.com',
149+
'http://ca.nodejs.org/ca.cert',
150+
],
151+
},
152+
},
153+
{
154+
text: '1.3.9999.12.34 - URI:http://ca.example.com/',
155+
legacy: {
156+
'1.3.9999.12.34 - URI': [
157+
'http://ca.example.com/',
158+
],
159+
},
160+
},
161+
hasOpenSSL3 ? {
162+
text: 'OCSP - othername: XmppAddr::good.example.com\n' +
163+
'OCSP - othername: 1.3.9999.12.34::abc123\n' +
164+
'OCSP - othername: SRVName::abc123',
165+
legacy: {
166+
'OCSP - othername': [
167+
' XmppAddr::good.example.com',
168+
' 1.3.9999.12.34::abc123',
169+
' SRVName::abc123',
170+
],
171+
},
172+
} : {
173+
text: 'OCSP - othername:<unsupported>\n' +
174+
'OCSP - othername:<unsupported>\n' +
175+
'OCSP - othername:<unsupported>',
176+
legacy: {
177+
'OCSP - othername': [
178+
'<unsupported>',
179+
'<unsupported>',
180+
'<unsupported>',
181+
],
182+
},
183+
},
184+
hasOpenSSL3 ? {
185+
text: null,
186+
legacy: null,
187+
} : {
188+
text: 'OCSP - othername:<unsupported>',
189+
legacy: {
190+
'OCSP - othername': [
191+
'<unsupported>',
192+
]
193+
},
194+
},
195+
];
196+
197+
const serverKey = fixtures.readSync('x509-escaping/server-key.pem', 'utf8');
198+
199+
for (let i = 0; i < expectedInfoAccess.length; i++) {
200+
const pem = fixtures.readSync(`x509-escaping/info-${i}-cert.pem`, 'utf8');
201+
const expected = expectedInfoAccess[i];
202+
203+
// X509Certificate interface is not supported in v12.x & v14.x. Disable
204+
// checks for cert.infoAccess with expected text. The testcase is ported
205+
// from v17.x
206+
//
207+
// Test the subjectAltName property of the X509Certificate API.
208+
// const cert = new X509Certificate(pem);
209+
// assert.strictEqual(cert.infoAccess, expected.text ?
210+
// `${expected.text}${hasOpenSSL3 ? '' : '\n'}` :
211+
// expected.text);
212+
213+
// Test that the certificate obtained by checkServerIdentity has the correct
214+
// subjectaltname property.
215+
const server = tls.createServer({
216+
key: serverKey,
217+
cert: pem,
218+
}, common.mustCall((conn) => {
219+
conn.destroy();
220+
server.close();
221+
})).listen(common.mustCall(() => {
222+
const { port } = server.address();
223+
tls.connect(port, {
224+
ca: pem,
225+
servername: 'example.com',
226+
checkServerIdentity: (hostname, peerCert) => {
227+
assert.strictEqual(hostname, 'example.com');
228+
assert.deepStrictEqual(peerCert.infoAccess,
229+
expected.legacy ?
230+
Object.assign(Object.create(null),
231+
expected.legacy) :
232+
expected.legacy);
233+
},
234+
}, common.mustCall());
235+
}));
236+
}
237+
}
238+
239+
// The internal parsing logic must match the JSON specification exactly.
240+
{
241+
// This list is partially based on V8's own JSON tests.
242+
const invalidJSON = [
243+
'"\\a invalid escape"',
244+
'"\\v invalid escape"',
245+
'"\\\' invalid escape"',
246+
'"\\x42 invalid escape"',
247+
'"\\u202 invalid escape"',
248+
'"\\012 invalid escape"',
249+
'"Unterminated string',
250+
'"Unterminated string\\"',
251+
'"Unterminated string\\\\\\"',
252+
'"\u0000 control character"',
253+
'"\u001e control character"',
254+
'"\u001f control character"',
255+
];
256+
257+
for (const invalidStringLiteral of invalidJSON) {
258+
// Usually, checkServerIdentity returns an error upon verification failure.
259+
// In this case, however, it should throw an error since this is not a
260+
// verification error. Node.js itself will never produce invalid JSON string
261+
// literals, so this can only happen when users construct invalid subject
262+
// alternative name strings (that do not follow escaping rules).
263+
assert.throws(() => {
264+
tls.checkServerIdentity('example.com', {
265+
subjectaltname: `DNS:${invalidStringLiteral}`,
266+
});
267+
}, {
268+
code: 'ERR_TLS_CERT_ALTNAME_FORMAT',
269+
message: 'Invalid subject alternative name string'
270+
});
271+
}
272+
}
273+
274+
// While node does not produce commas within SAN entries, it should parse them
275+
// correctly (i.e., not simply split at commas).
276+
{
277+
// Regardless of the quotes, splitting this SAN string at commas would
278+
// cause checkServerIdentity to see 'DNS:b.example.com' and thus to accept
279+
// the certificate for b.example.com.
280+
const san = 'DNS:"a.example.com, DNS:b.example.com, DNS:c.example.com"';
281+
282+
// This is what node used to do, and which is not correct!
283+
const hostname = 'b.example.com';
284+
assert.strictEqual(san.split(', ')[1], `DNS:${hostname}`);
285+
286+
// The new implementation should parse the string correctly.
287+
const err = tls.checkServerIdentity(hostname, { subjectaltname: san });
288+
assert(err);
289+
assert.strictEqual(err.code, 'ERR_TLS_CERT_ALTNAME_INVALID');
290+
assert.strictEqual(err.message, 'Hostname/IP does not match certificate\'s ' +
291+
'altnames: Host: b.example.com. is not in ' +
292+
'the cert\'s altnames: DNS:"a.example.com, ' +
293+
'DNS:b.example.com, DNS:c.example.com"');
294+
}
295+
296+
// The subject MUST be ignored if a dNSName subject alternative name exists.
297+
{
298+
const key = fixtures.readKey('incorrect_san_correct_subject-key.pem');
299+
const cert = fixtures.readKey('incorrect_san_correct_subject-cert.pem');
300+
301+
// The hostname is the CN, but not a SAN entry.
302+
const servername = 'good.example.com';
303+
304+
// X509Certificate interface is not supported in v12.x & v14.x. Disable
305+
// checks for certX509.subject and certX509.subjectAltName with expected
306+
// value. The testcase is ported from v17.x
307+
//
308+
// const certX509 = new X509Certificate(cert);
309+
// assert.strictEqual(certX509.subject, `CN=${servername}`);
310+
// assert.strictEqual(certX509.subjectAltName, 'DNS:evil.example.com');
311+
312+
// Try connecting to a server that uses the self-signed certificate.
313+
const server = tls.createServer({ key, cert }, common.mustNotCall());
314+
server.listen(common.mustCall(() => {
315+
const { port } = server.address();
316+
const socket = tls.connect(port, {
317+
ca: cert,
318+
servername,
319+
}, common.mustNotCall());
320+
socket.on('error', common.mustCall((err) => {
321+
assert.strictEqual(err.code, 'ERR_TLS_CERT_ALTNAME_INVALID');
322+
assert.strictEqual(err.message, 'Hostname/IP does not match ' +
323+
"certificate's altnames: Host: " +
324+
"good.example.com. is not in the cert's" +
325+
' altnames: DNS:evil.example.com');
326+
}));
327+
})).unref();
328+
}
329+
330+
// The subject MUST NOT be ignored if no dNSName subject alternative name
331+
// exists, even if other subject alternative names exist.
332+
{
333+
const key = fixtures.readKey('irrelevant_san_correct_subject-key.pem');
334+
const cert = fixtures.readKey('irrelevant_san_correct_subject-cert.pem');
335+
336+
// The hostname is the CN, but there is no dNSName SAN entry.
337+
const servername = 'good.example.com';
338+
339+
// X509Certificate interface is not supported in v12.x & v14.x. Disable
340+
// checks for certX509.subject and certX509.subjectAltName with expected
341+
// value. The testcase is ported from v17.x
342+
//
343+
// const certX509 = new X509Certificate(cert);
344+
// assert.strictEqual(certX509.subject, `CN=${servername}`);
345+
// assert.strictEqual(certX509.subjectAltName, 'IP Address:1.2.3.4');
346+
347+
// Connect to a server that uses the self-signed certificate.
348+
const server = tls.createServer({ key, cert }, common.mustCall((socket) => {
349+
socket.destroy();
350+
server.close();
351+
})).listen(common.mustCall(() => {
352+
const { port } = server.address();
353+
tls.connect(port, {
354+
ca: cert,
355+
servername,
356+
}, common.mustCall(() => {
357+
// Do nothing, the server will close the connection.
358+
}));
359+
}));
360+
}

0 commit comments

Comments
 (0)
Please sign in to comment.