Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
tls: drop support for URI alternative names
Previously, Node.js incorrectly accepted uniformResourceIdentifier (URI)
subject alternative names in checkServerIdentity regardless of the
application protocol. This was incorrect even in the most common cases.
For example, RFC 2818 specifies (and RFC 6125 confirms) that HTTP over
TLS only uses dNSName and iPAddress subject alternative names, but not
uniformResourceIdentifier subject alternative names.

Additionally, name constrained certificate authorities might not be
constrained to specific URIs, allowing them to issue certificates for
URIs that specify hosts that they would not be allowed to issue dNSName
certificates for.

Even for application protocols that make use of URI subject alternative
names (such as SIP, see RFC 5922), Node.js did not implement the
required checks correctly, for example, because checkServerIdentity
ignores the URI scheme.

As a side effect, this also fixes an edge case. When a hostname is not
an IP address and no dNSName subject alternative name exists, the
subject's Common Name should be considered even when an iPAddress
subject alternative name exists.

It remains possible for users to pass a custom checkServerIdentity
function to the TLS implementation in order to implement custom identity
verification logic.

This addresses CVE-2021-44531.

Co-authored-by: Akshay K <iit.akshay@gmail.com>
CVE-ID: CVE-2021-44531
Backport-PR-URL: nodejs-private/node-private#303
PR-URL: nodejs-private/node-private#300
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
2 people authored and BethGriggs committed Jan 8, 2022
1 parent 2e2c455 commit b11b4cc
Show file tree
Hide file tree
Showing 7 changed files with 80 additions and 28 deletions.
12 changes: 12 additions & 0 deletions doc/api/tls.md
Expand Up @@ -1460,6 +1460,11 @@ decrease overall server throughput.

<!-- YAML
added: v0.8.4
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs-private/node-private/pull/300
description: Support for `uniformResourceIdentifier` subject alternative
names has been disabled in response to CVE-2021-44531.
-->

* `hostname` {string} The host name or IP address to verify the certificate
Expand All @@ -1480,6 +1485,12 @@ the checks done with additional verification.
This function is only called if the certificate passed all other checks, such as
being issued by trusted CA (`options.ca`).

Earlier versions of Node.js incorrectly accepted certificates for a given
`hostname` if a matching `uniformResourceIdentifier` subject alternative name
was present (see [CVE-2021-44531][]). Applications that wish to accept
`uniformResourceIdentifier` subject alternative names can use a custom
`options.checkServerIdentity` function that implements the desired behavior.

## `tls.connect(options[, callback])`

<!-- YAML
Expand Down Expand Up @@ -2143,6 +2154,7 @@ added: v11.4.0
`'TLSv1.3'`. If multiple of the options are provided, the lowest minimum is
used.

[CVE-2021-44531]: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-44531
[Chrome's 'modern cryptography' setting]: https://www.chromium.org/Home/chromium-security/education/tls#TOC-Cipher-Suites
[DHE]: https://en.wikipedia.org/wiki/Diffie%E2%80%93Hellman_key_exchange
[ECDHE]: https://en.wikipedia.org/wiki/Elliptic_curve_Diffie%E2%80%93Hellman
Expand Down
21 changes: 4 additions & 17 deletions lib/tls.js
Expand Up @@ -60,7 +60,6 @@ const net = require('net');
const { getOptionValue } = require('internal/options');
const { getRootCertificates, getSSLCiphers } = internalBinding('crypto');
const { Buffer } = require('buffer');
const { URL } = require('internal/url');
const { canonicalizeIP } = internalBinding('cares_wrap');
const _tls_common = require('_tls_common');
const _tls_wrap = require('_tls_wrap');
Expand Down Expand Up @@ -275,7 +274,6 @@ exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
const subject = cert.subject;
const altNames = cert.subjectaltname;
const dnsNames = [];
const uriNames = [];
const ips = [];

hostname = '' + hostname;
Expand All @@ -287,11 +285,6 @@ exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
ArrayPrototypeForEach(splitAltNames, (name) => {
if (StringPrototypeStartsWith(name, 'DNS:')) {
ArrayPrototypePush(dnsNames, StringPrototypeSlice(name, 4));
} else if (StringPrototypeStartsWith(name, 'URI:')) {
const uri = new URL(StringPrototypeSlice(name, 4));

// TODO(bnoordhuis) Also use scheme.
ArrayPrototypePush(uriNames, uri.hostname);
} else if (StringPrototypeStartsWith(name, 'IP Address:')) {
ArrayPrototypePush(ips, canonicalizeIP(StringPrototypeSlice(name, 11)));
}
Expand All @@ -301,25 +294,19 @@ exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
let valid = false;
let reason = 'Unknown reason';

const hasAltNames =
dnsNames.length > 0 || ips.length > 0 || uriNames.length > 0;

hostname = unfqdn(hostname); // Remove trailing dot for error messages.

if (net.isIP(hostname)) {
valid = ArrayPrototypeIncludes(ips, canonicalizeIP(hostname));
if (!valid)
reason = `IP: ${hostname} is not in the cert's list: ` +
ArrayPrototypeJoin(ips, ', ');
// TODO(bnoordhuis) Also check URI SANs that are IP addresses.
} else if (hasAltNames || subject) {
} else if (dnsNames.length > 0 || subject?.CN) {
const hostParts = splitHost(hostname);
const wildcard = (pattern) => check(hostParts, pattern, true);

if (hasAltNames) {
const noWildcard = (pattern) => check(hostParts, pattern, false);
valid = ArrayPrototypeSome(dnsNames, wildcard) ||
ArrayPrototypeSome(uriNames, noWildcard);
if (dnsNames.length > 0) {
valid = ArrayPrototypeSome(dnsNames, wildcard);
if (!valid)
reason =
`Host: ${hostname}. is not in the cert's altnames: ${altNames}`;
Expand All @@ -336,7 +323,7 @@ exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
reason = `Host: ${hostname}. is not cert's CN: ${cn}`;
}
} else {
reason = 'Cert is empty';
reason = 'Cert does not contain a DNS name';
}

if (!valid) {
Expand Down
14 changes: 14 additions & 0 deletions test/fixtures/keys/Makefile
Expand Up @@ -87,6 +87,8 @@ all: \
ec_secp256k1_public.pem \
incorrect_san_correct_subject-cert.pem \
incorrect_san_correct_subject-key.pem \
irrelevant_san_correct_subject-cert.pem \
irrelevant_san_correct_subject-key.pem \

#
# Create Certificate Authority: ca1
Expand Down Expand Up @@ -795,6 +797,18 @@ incorrect_san_correct_subject-cert.pem: incorrect_san_correct_subject-key.pem
incorrect_san_correct_subject-key.pem:
openssl ecparam -name prime256v1 -genkey -noout -out incorrect_san_correct_subject-key.pem

irrelevant_san_correct_subject-cert.pem: irrelevant_san_correct_subject-key.pem
openssl req -x509 \
-key irrelevant_san_correct_subject-key.pem \
-out irrelevant_san_correct_subject-cert.pem \
-sha256 \
-days 3650 \
-subj "/CN=good.example.com" \
-addext "subjectAltName = IP:1.2.3.4"

irrelevant_san_correct_subject-key.pem:
openssl ecparam -name prime256v1 -genkey -noout -out irrelevant_san_correct_subject-key.pem

clean:
rm -f *.pfx *.pem *.srl ca2-database.txt ca2-serial fake-startcom-root-serial *.print *.old fake-startcom-root-issued-certs/*.pem
@> fake-startcom-root-database.txt
Expand Down
11 changes: 11 additions & 0 deletions test/fixtures/keys/irrelevant_san_correct_subject-cert.pem
@@ -0,0 +1,11 @@
-----BEGIN CERTIFICATE-----
MIIBnTCCAUKgAwIBAgIUa28EJmmQ7yZOq3WWNP3SLiJnzcAwCgYIKoZIzj0EAwIw
GzEZMBcGA1UEAwwQZ29vZC5leGFtcGxlLmNvbTAeFw0yMTEyMTExNzE0NDVaFw0z
MTEyMDkxNzE0NDVaMBsxGTAXBgNVBAMMEGdvb2QuZXhhbXBsZS5jb20wWTATBgcq
hkjOPQIBBggqhkjOPQMBBwNCAATEKoJfDvKQ6dD+yvc4DaeH0ZlG8VuGJUVi6iIb
ugY3dKHdmXUIuwwUScgztLc6W8FfvbTxfTF2q90ZBJlr/Klvo2QwYjAdBgNVHQ4E
FgQUu55oRZI5tdQDmViwAvPEbzZuY2owHwYDVR0jBBgwFoAUu55oRZI5tdQDmViw
AvPEbzZuY2owDwYDVR0TAQH/BAUwAwEB/zAPBgNVHREECDAGhwQBAgMEMAoGCCqG
SM49BAMCA0kAMEYCIQDw8z8d7ToB14yxMJxEDF1dhUqMReJFFwPVnvzkr174igIh
AKJ9XL+02sGOE7xZd5C0KqUXeHoIE9shnejnhm3WBrB/
-----END CERTIFICATE-----
5 changes: 5 additions & 0 deletions test/fixtures/keys/irrelevant_san_correct_subject-key.pem
@@ -0,0 +1,5 @@
-----BEGIN EC PRIVATE KEY-----
MHcCAQEEIDsijdVlHMNTvJ4eqeUbpjMMnl72+HLtEIEcbauckCP6oAoGCCqGSM49
AwEHoUQDQgAExCqCXw7ykOnQ/sr3OA2nh9GZRvFbhiVFYuoiG7oGN3Sh3Zl1CLsM
FEnIM7S3OlvBX7208X0xdqvdGQSZa/ypbw==
-----END EC PRIVATE KEY-----
14 changes: 7 additions & 7 deletions test/parallel/test-tls-check-server-identity.js
Expand Up @@ -134,7 +134,7 @@ const tests = [
{
host: 'a.com',
cert: { },
error: 'Cert is empty'
error: 'Cert does not contain a DNS name'
},

// Empty Subject w/DNS name
Expand All @@ -148,7 +148,8 @@ const tests = [
{
host: 'a.b.a.com', cert: {
subjectaltname: 'URI:http://a.b.a.com/',
}
},
error: 'Cert does not contain a DNS name'
},

// Multiple CN fields
Expand Down Expand Up @@ -265,24 +266,23 @@ const tests = [
host: 'a.b.a.com', cert: {
subjectaltname: 'URI:http://a.b.a.com/',
subject: {}
}
},
error: 'Cert does not contain a DNS name'
},
{
host: 'a.b.a.com', cert: {
subjectaltname: 'URI:http://*.b.a.com/',
subject: {}
},
error: 'Host: a.b.a.com. is not in the cert\'s altnames: ' +
'URI:http://*.b.a.com/'
error: 'Cert does not contain a DNS name'
},
// IP addresses
{
host: 'a.b.a.com', cert: {
subjectaltname: 'IP Address:127.0.0.1',
subject: {}
},
error: 'Host: a.b.a.com. is not in the cert\'s altnames: ' +
'IP Address:127.0.0.1'
error: 'Cert does not contain a DNS name'
},
{
host: '127.0.0.1', cert: {
Expand Down
31 changes: 27 additions & 4 deletions test/parallel/test-x509-escaping.js
Expand Up @@ -20,10 +20,6 @@ const { hasOpenSSL3 } = common;
const numLeaves = 5;

for (let i = 0; i < numLeaves; i++) {
// TODO(tniessen): this test case requires proper handling of URI SANs,
// which node currently does not implement.
if (i === 3) continue;

const name = `x509-escaping/google/leaf${i}.pem`;
const leafPEM = fixtures.readSync(name, 'utf8');

Expand Down Expand Up @@ -336,3 +332,30 @@ const { hasOpenSSL3 } = common;
}));
})).unref();
}

// The subject MUST NOT be ignored if no dNSName subject alternative name
// exists, even if other subject alternative names exist.
{
const key = fixtures.readKey('irrelevant_san_correct_subject-key.pem');
const cert = fixtures.readKey('irrelevant_san_correct_subject-cert.pem');

// The hostname is the CN, but there is no dNSName SAN entry.
const servername = 'good.example.com';
const certX509 = new X509Certificate(cert);
assert.strictEqual(certX509.subject, `CN=${servername}`);
assert.strictEqual(certX509.subjectAltName, 'IP Address:1.2.3.4');

// Connect to a server that uses the self-signed certificate.
const server = tls.createServer({ key, cert }, common.mustCall((socket) => {
socket.destroy();
server.close();
})).listen(common.mustCall(() => {
const { port } = server.address();
tls.connect(port, {
ca: cert,
servername,
}, common.mustCall(() => {
// Do nothing, the server will close the connection.
}));
}));
}

0 comments on commit b11b4cc

Please sign in to comment.