diff --git a/lib/tls.js b/lib/tls.js index f17a2fd0c5b0aa..2fd7a9e013fefd 100644 --- a/lib/tls.js +++ b/lib/tls.js @@ -60,6 +60,7 @@ const net = require('net'); const { getOptionValue } = require('internal/options'); const { getRootCertificates, getSSLCiphers } = internalBinding('crypto'); const { Buffer } = require('buffer'); +const { URL } = require('internal/url'); // Only used for Security Revert const { canonicalizeIP } = internalBinding('cares_wrap'); const _tls_common = require('_tls_common'); const _tls_wrap = require('_tls_wrap'); @@ -274,6 +275,7 @@ exports.checkServerIdentity = function checkServerIdentity(hostname, cert) { const subject = cert.subject; const altNames = cert.subjectaltname; const dnsNames = []; + const uriNames = []; const ips = []; hostname = '' + hostname; @@ -285,6 +287,12 @@ exports.checkServerIdentity = function checkServerIdentity(hostname, cert) { ArrayPrototypeForEach(splitAltNames, (name) => { if (StringPrototypeStartsWith(name, 'DNS:')) { ArrayPrototypePush(dnsNames, StringPrototypeSlice(name, 4)); + } else if (process.REVERT_CVE_2021_44531 && + 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))); } @@ -294,6 +302,9 @@ 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)) { @@ -301,12 +312,17 @@ exports.checkServerIdentity = function checkServerIdentity(hostname, cert) { if (!valid) reason = `IP: ${hostname} is not in the cert's list: ` + ArrayPrototypeJoin(ips, ', '); - } else if (dnsNames.length > 0 || subject?.CN) { + // TODO(bnoordhuis) Also check URI SANs that are IP addresses. + } else if ((process.REVERT_CVE_2021_44531 && (hasAltNames || subject)) || + (dnsNames.length > 0 || subject?.CN)) { const hostParts = splitHost(hostname); const wildcard = (pattern) => check(hostParts, pattern, true); - if (dnsNames.length > 0) { - valid = ArrayPrototypeSome(dnsNames, wildcard); + if ((process.REVERT_CVE_2021_44531 && hasAltNames) || + (dnsNames.length > 0)) { + const noWildcard = (pattern) => check(hostParts, pattern, false); + valid = ArrayPrototypeSome(dnsNames, wildcard) || + ArrayPrototypeSome(uriNames, noWildcard); if (!valid) reason = `Host: ${hostname}. is not in the cert's altnames: ${altNames}`; diff --git a/src/crypto/crypto_common.cc b/src/crypto/crypto_common.cc index 4be308fb655017..86da106306f76a 100644 --- a/src/crypto/crypto_common.cc +++ b/src/crypto/crypto_common.cc @@ -6,6 +6,7 @@ #include "crypto/crypto_common.h" #include "node.h" #include "node_internals.h" +#include "node_revert.h" #include "node_url.h" #include "string_bytes.h" #include "memory_tracker-inl.h" @@ -620,6 +621,44 @@ MaybeLocal GetValidFrom( return ToV8Value(env, bio); } +// deprecated, only used for security revert +bool SafeX509ExtPrint(const BIOPointer& out, X509_EXTENSION* ext) { + const X509V3_EXT_METHOD* method = X509V3_EXT_get(ext); + + if (method != X509V3_EXT_get_nid(NID_subject_alt_name)) + return false; + + GENERAL_NAMES* names = static_cast(X509V3_EXT_d2i(ext)); + if (names == nullptr) + return false; + + for (int i = 0; i < sk_GENERAL_NAME_num(names); i++) { + GENERAL_NAME* gen = sk_GENERAL_NAME_value(names, i); + + if (i != 0) + BIO_write(out.get(), ", ", 2); + + if (gen->type == GEN_DNS) { + ASN1_IA5STRING* name = gen->d.dNSName; + + BIO_write(out.get(), "DNS:", 4); + BIO_write(out.get(), name->data, name->length); + } else { + STACK_OF(CONF_VALUE)* nval = i2v_GENERAL_NAME( + const_cast(method), gen, nullptr); + if (nval == nullptr) { + sk_GENERAL_NAME_pop_free(names, GENERAL_NAME_free); + return false; + } + X509V3_EXT_val_prn(out.get(), nval, 0, 0); + sk_CONF_VALUE_pop_free(nval, X509V3_conf_free); + } + } + sk_GENERAL_NAME_pop_free(names, GENERAL_NAME_free); + + return true; +} + static inline bool IsSafeAltName(const char* name, size_t length, bool utf8) { for (size_t i = 0; i < length; i++) { char c = name[i]; @@ -844,6 +883,10 @@ bool SafeX509SubjectAltNamePrint(const BIOPointer& out, X509_EXTENSION* ext) { const X509V3_EXT_METHOD* method = X509V3_EXT_get(ext); CHECK(method == X509V3_EXT_get_nid(NID_subject_alt_name)); + if (IsReverted(SECURITY_REVERT_CVE_2021_44532)) { + return SafeX509ExtPrint(out, ext); + } + GENERAL_NAMES* names = static_cast(X509V3_EXT_d2i(ext)); if (names == nullptr) return false; @@ -869,6 +912,10 @@ bool SafeX509InfoAccessPrint(const BIOPointer& out, X509_EXTENSION* ext) { const X509V3_EXT_METHOD* method = X509V3_EXT_get(ext); CHECK(method == X509V3_EXT_get_nid(NID_info_access)); + if (IsReverted(SECURITY_REVERT_CVE_2021_44532)) { + return (X509V3_EXT_print(out.get(), ext, 0, 0) == 1); + } + AUTHORITY_INFO_ACCESS* descs = static_cast(X509V3_EXT_d2i(ext)); if (descs == nullptr) diff --git a/src/node_revert.h b/src/node_revert.h index edf6ad772eecb0..83dcb62c37a342 100644 --- a/src/node_revert.h +++ b/src/node_revert.h @@ -16,6 +16,8 @@ namespace node { #define SECURITY_REVERSIONS(XX) \ + XX(CVE_2021_44531, "CVE-2021-44531", "Cert Verif Bypass via URI SAN") \ + XX(CVE_2021_44532, "CVE-2021-44532", "Cert Verif Bypass via Str Inject") \ // XX(CVE_2016_PEND, "CVE-2016-PEND", "Vulnerability Title") enum reversion { diff --git a/test/parallel/test-revert-CVE-2021-44531.js b/test/parallel/test-revert-CVE-2021-44531.js new file mode 100644 index 00000000000000..fad2c4bfe1ee6d --- /dev/null +++ b/test/parallel/test-revert-CVE-2021-44531.js @@ -0,0 +1,50 @@ +// Flags: --security-revert=CVE-2021-44531 +'use strict'; +const common = require('../common'); + +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const util = require('util'); + +const tls = require('tls'); + +const tests = [ + // Likewise for "URI:" Subject Alternative Names. + // See also https://github.com/nodejs/node/issues/8108. + { + host: '8.8.8.8', + cert: { subject: { CN: '8.8.8.8' }, subjectaltname: 'URI:http://8.8.8.8/' }, + error: 'IP: 8.8.8.8 is not in the cert\'s list: ' + }, + // Empty Subject w/URI name + { + host: 'a.b.a.com', cert: { + subjectaltname: 'URI:http://a.b.a.com/', + } + }, + // URI names + { + host: 'a.b.a.com', cert: { + subjectaltname: 'URI:http://a.b.a.com/', + subject: {} + } + }, + { + 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/' + }, +]; + +tests.forEach(function(test, i) { + const err = tls.checkServerIdentity(test.host, test.cert); + assert.strictEqual(err && err.reason, + test.error, + `Test# ${i} failed: ${util.inspect(test)} \n` + + `${test.error} != ${(err && err.reason)}`); +}); diff --git a/test/parallel/test-tls-0-dns-altname-revert-CVE-2021-44532.js b/test/parallel/test-tls-0-dns-altname-revert-CVE-2021-44532.js new file mode 100644 index 00000000000000..8624c46f8272e4 --- /dev/null +++ b/test/parallel/test-tls-0-dns-altname-revert-CVE-2021-44532.js @@ -0,0 +1,54 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. +// Flags: --security-revert=CVE-2021-44532 +'use strict'; +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +// Check getPeerCertificate can properly handle '\0' for fix CVE-2009-2408. + +const assert = require('assert'); +const tls = require('tls'); +const fixtures = require('../common/fixtures'); + +const server = tls.createServer({ + key: fixtures.readSync(['0-dns', '0-dns-key.pem']), + cert: fixtures.readSync(['0-dns', '0-dns-cert.pem']) +}, common.mustCall((c) => { + c.once('data', common.mustCall(() => { + c.destroy(); + server.close(); + })); +})).listen(0, common.mustCall(() => { + const c = tls.connect(server.address().port, { + rejectUnauthorized: false + }, common.mustCall(() => { + const cert = c.getPeerCertificate(); + assert.strictEqual(cert.subjectaltname, + 'DNS:good.example.org\0.evil.example.com, ' + + 'DNS:just-another.example.com, ' + + 'IP Address:8.8.8.8, ' + + 'IP Address:8.8.4.4, ' + + 'DNS:last.example.com'); + c.write('ok'); + })); +})); diff --git a/test/parallel/test-x509-escaping-revert-CVE-2021-44532.js b/test/parallel/test-x509-escaping-revert-CVE-2021-44532.js new file mode 100644 index 00000000000000..2f31daad10de81 --- /dev/null +++ b/test/parallel/test-x509-escaping-revert-CVE-2021-44532.js @@ -0,0 +1,343 @@ +// Flags: --security-revert=CVE-2021-44532 +'use strict'; + +const common = require('../common'); +if (!common.hasCrypto) + common.skip('missing crypto'); + +const assert = require('assert'); +const { X509Certificate } = require('crypto'); +const tls = require('tls'); +const fixtures = require('../common/fixtures'); + +const { hasOpenSSL3 } = common; + +// Test escaping rules for subject alternative names. +{ + const expectedSANs = [ + 'DNS:good.example.com, DNS:evil.example.com', + // URIs should not require escaping. + 'URI:http://example.com/', + 'URI:http://example.com/?a=b&c=d', + // Unless they contain commas. + 'URI:http://example.com/a,b', + // Percent encoding should not require escaping. + 'URI:http://example.com/a%2Cb', + // Malicious attempts should be escaped. + 'URI:http://example.com/a, DNS:good.example.com', + // Non-ASCII characters in DNS names should be treated as Latin-1. + 'DNS:ex�mple.com', + // It should not be possible to cause unescaping without escaping. + 'DNS:"evil.example.com"', + // IPv4 addresses should be represented as usual. + 'IP Address:8.8.8.8', + 'IP Address:8.8.4.4', + // For backward-compatibility, include invalid IP address lengths. + hasOpenSSL3 ? 'IP Address:' : 'IP Address:', + hasOpenSSL3 ? 'IP Address:' : 'IP Address:', + // IPv6 addresses are represented as OpenSSL does. + 'IP Address:A0B:C0D:E0F:0:0:0:7A7B:7C7D', + // Regular email addresses don't require escaping. + 'email:foo@example.com', + // ... but should be escaped if they contain commas. + 'email:foo@example.com, DNS:good.example.com', + 'DirName:/C=DE/L=Hannover', + // TODO(tniessen): support UTF8 in DirName + 'DirName:/C=DE/L=M\\xC3\\xBCnchen', + 'DirName:/C=DE/L=Berlin, DNS:good.example.com', + 'DirName:/C=DE/L=Berlin, DNS:good.example.com\\x00' + + 'evil.example.com', + 'DirName:/C=DE/L=Berlin, DNS:good.example.com\\\\x00' + + 'evil.example.com', + // These next two tests might be surprising. OpenSSL applies its own rules + // first, which introduce backslashes, which activate node's escaping. + // Unfortunately, there are also differences between OpenSSL 1.1.1 and 3.0. + 'DirName:/C=DE/L=Berlin\\x0D\\x0A', + hasOpenSSL3 ? + 'DirName:/C=DE/L=Berlin\\/CN=good.example.com' : + 'DirName:/C=DE/L=Berlin/CN=good.example.com', + // TODO(tniessen): even OIDs that are well-known (such as the following, + // which is sha256WithRSAEncryption) should be represented numerically only. + 'Registered ID:sha256WithRSAEncryption', + // This is an OID that will likely never be assigned to anything, thus + // OpenSSL hould not know it. + 'Registered ID:1.3.9999.12.34', + hasOpenSSL3 ? + 'othername: XmppAddr::abc123' : + 'othername:', + hasOpenSSL3 ? + 'othername: XmppAddr::abc123, DNS:good.example.com' : + 'othername:', + hasOpenSSL3 ? + null : + 'othername:', + // This is unsupported because the OID is not recognized. + hasOpenSSL3 ? + 'othername: 1.3.9999.12.34::abc123' : + 'othername:', + hasOpenSSL3 ? 'othername: SRVName::abc123' : 'othername:', + // This is unsupported because it is an SRVName with a UTF8String value, + // which is not allowed for SRVName. + hasOpenSSL3 ? + null : 'othername:', + hasOpenSSL3 ? + null : + 'othername:', + ]; + + const serverKey = fixtures.readSync('x509-escaping/server-key.pem', 'utf8'); + + for (let i = 0; i < expectedSANs.length; i++) { + const pem = fixtures.readSync(`x509-escaping/alt-${i}-cert.pem`, 'utf8'); + + // Test the subjectAltName property of the X509Certificate API. + const cert = new X509Certificate(pem); + assert.strictEqual(cert.subjectAltName, expectedSANs[i]); + + // Test that the certificate obtained by checkServerIdentity has the correct + // subjectaltname property. + const server = tls.createServer({ + key: serverKey, + cert: pem, + }, common.mustCall((conn) => { + conn.destroy(); + server.close(); + })).listen(common.mustCall(() => { + const { port } = server.address(); + tls.connect(port, { + ca: pem, + servername: 'example.com', + checkServerIdentity: (hostname, peerCert) => { + assert.strictEqual(hostname, 'example.com'); + assert.strictEqual(peerCert.subjectaltname, expectedSANs[i]); + }, + }, common.mustCall()); + })); + } +} + +// Test escaping rules for authority info access. +{ + const expectedInfoAccess = [ + { + text: 'OCSP - URI:http://good.example.com/\n' + + 'OCSP - URI:http://evil.example.com/', + legacy: { + 'OCSP - URI': [ + 'http://good.example.com/', + 'http://evil.example.com/', + ], + }, + }, + { + text: 'CA Issuers - URI:http://ca.example.com/\n' + + 'OCSP - URI:http://evil.example.com\n' + + 'OCSP - DNS:good.example.com\n' + + 'OCSP - URI:http://ca.nodejs.org/ca.cert', + legacy: { + 'CA Issuers - URI': [ + 'http://ca.example.com/', + ], + 'OCSP - DNS': [ + 'good.example.com', + ], + 'OCSP - URI': [ + 'http://evil.example.com', + 'http://ca.nodejs.org/ca.cert', + ], + }, + }, + { + text: '1.3.9999.12.34 - URI:http://ca.example.com/', + legacy: { + '1.3.9999.12.34 - URI': [ + 'http://ca.example.com/', + ], + }, + }, + hasOpenSSL3 ? { + text: 'OCSP - othername: XmppAddr::good.example.com\n' + + 'OCSP - othername: 1.3.9999.12.34::abc123\n' + + 'OCSP - othername: SRVName::abc123', + legacy: { + 'OCSP - othername': [ + ' XmppAddr::good.example.com', + ' 1.3.9999.12.34::abc123', + ' SRVName::abc123', + ], + }, + } : { + text: 'OCSP - othername:\n' + + 'OCSP - othername:\n' + + 'OCSP - othername:', + legacy: { + 'OCSP - othername': [ + '', + '', + '', + ], + }, + }, + hasOpenSSL3 ? { + text: null, + legacy: null, + } : { + text: 'OCSP - othername:', + legacy: { + 'OCSP - othername': [ + '', + ] + }, + }, + ]; + + const serverKey = fixtures.readSync('x509-escaping/server-key.pem', 'utf8'); + + for (let i = 0; i < expectedInfoAccess.length; i++) { + const pem = fixtures.readSync(`x509-escaping/info-${i}-cert.pem`, 'utf8'); + const expected = expectedInfoAccess[i]; + + // Test the subjectAltName property of the X509Certificate API. + const cert = new X509Certificate(pem); + assert.strictEqual(cert.infoAccess, expected.text ? + `${expected.text}${hasOpenSSL3 ? '' : '\n'}` : + expected.text); + + // Test that the certificate obtained by checkServerIdentity has the correct + // subjectaltname property. + const server = tls.createServer({ + key: serverKey, + cert: pem, + }, common.mustCall((conn) => { + conn.destroy(); + server.close(); + })).listen(common.mustCall(() => { + const { port } = server.address(); + tls.connect(port, { + ca: pem, + servername: 'example.com', + checkServerIdentity: (hostname, peerCert) => { + assert.strictEqual(hostname, 'example.com'); + assert.deepStrictEqual(peerCert.infoAccess, + expected.legacy ? + Object.assign(Object.create(null), + expected.legacy) : + expected.legacy); + }, + }, common.mustCall()); + })); + } +} + +// The internal parsing logic must match the JSON specification exactly. +{ + // This list is partially based on V8's own JSON tests. + const invalidJSON = [ + '"\\a invalid escape"', + '"\\v invalid escape"', + '"\\\' invalid escape"', + '"\\x42 invalid escape"', + '"\\u202 invalid escape"', + '"\\012 invalid escape"', + '"Unterminated string', + '"Unterminated string\\"', + '"Unterminated string\\\\\\"', + '"\u0000 control character"', + '"\u001e control character"', + '"\u001f control character"', + ]; + + for (const invalidStringLiteral of invalidJSON) { + // Usually, checkServerIdentity returns an error upon verification failure. + // In this case, however, it should throw an error since this is not a + // verification error. Node.js itself will never produce invalid JSON string + // literals, so this can only happen when users construct invalid subject + // alternative name strings (that do not follow escaping rules). + assert.throws(() => { + tls.checkServerIdentity('example.com', { + subjectaltname: `DNS:${invalidStringLiteral}`, + }); + }, { + code: 'ERR_TLS_CERT_ALTNAME_FORMAT', + message: 'Invalid subject alternative name string' + }); + } +} + +// While node does not produce commas within SAN entries, it should parse them +// correctly (i.e., not simply split at commas). +{ + // Regardless of the quotes, splitting this SAN string at commas would + // cause checkServerIdentity to see 'DNS:b.example.com' and thus to accept + // the certificate for b.example.com. + const san = 'DNS:"a.example.com, DNS:b.example.com, DNS:c.example.com"'; + + // This is what node used to do, and which is not correct! + const hostname = 'b.example.com'; + assert.strictEqual(san.split(', ')[1], `DNS:${hostname}`); + + // The new implementation should parse the string correctly. + const err = tls.checkServerIdentity(hostname, { subjectaltname: san }); + assert(err); + assert.strictEqual(err.code, 'ERR_TLS_CERT_ALTNAME_INVALID'); + assert.strictEqual(err.message, 'Hostname/IP does not match certificate\'s ' + + 'altnames: Host: b.example.com. is not in ' + + 'the cert\'s altnames: DNS:"a.example.com, ' + + 'DNS:b.example.com, DNS:c.example.com"'); +} + +// The subject MUST be ignored if a dNSName subject alternative name exists. +{ + const key = fixtures.readKey('incorrect_san_correct_subject-key.pem'); + const cert = fixtures.readKey('incorrect_san_correct_subject-cert.pem'); + + // The hostname is the CN, but not a SAN entry. + const servername = 'good.example.com'; + const certX509 = new X509Certificate(cert); + assert.strictEqual(certX509.subject, `CN=${servername}`); + assert.strictEqual(certX509.subjectAltName, 'DNS:evil.example.com'); + + // Try connecting to a server that uses the self-signed certificate. + const server = tls.createServer({ key, cert }, common.mustNotCall()); + server.listen(common.mustCall(() => { + const { port } = server.address(); + const socket = tls.connect(port, { + ca: cert, + servername, + }, common.mustNotCall()); + socket.on('error', common.mustCall((err) => { + assert.strictEqual(err.code, 'ERR_TLS_CERT_ALTNAME_INVALID'); + assert.strictEqual(err.message, 'Hostname/IP does not match ' + + "certificate's altnames: Host: " + + "good.example.com. is not in the cert's" + + ' altnames: DNS:evil.example.com'); + })); + })).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. + })); + })); +}