Skip to content

Commit

Permalink
src: add cve reverts and associated tests
Browse files Browse the repository at this point in the history
Signed-off-by: Michael Dawson <[email protected]>
Co-authored-by: Akshay K <[email protected]>
Backport-PR-URL: nodejs-private/node-private#303
PR-URL: nodejs-private/node-private#300
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
  • Loading branch information
2 people authored and BethGriggs committed Jan 8, 2022
1 parent 1f7fdff commit df3141f
Show file tree
Hide file tree
Showing 6 changed files with 515 additions and 3 deletions.
22 changes: 19 additions & 3 deletions lib/tls.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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;
Expand All @@ -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)));
}
Expand All @@ -294,19 +302,27 @@ 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, ', ');
} 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}`;
Expand Down
47 changes: 47 additions & 0 deletions src/crypto/crypto_common.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -620,6 +621,44 @@ MaybeLocal<Value> 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<GENERAL_NAMES*>(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<X509V3_EXT_METHOD*>(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];
Expand Down Expand Up @@ -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<GENERAL_NAMES*>(X509V3_EXT_d2i(ext));
if (names == nullptr)
return false;
Expand All @@ -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<AUTHORITY_INFO_ACCESS*>(X509V3_EXT_d2i(ext));
if (descs == nullptr)
Expand Down
2 changes: 2 additions & 0 deletions src/node_revert.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
50 changes: 50 additions & 0 deletions test/parallel/test-revert-CVE-2021-44531.js
Original file line number Diff line number Diff line change
@@ -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)}`);
});
54 changes: 54 additions & 0 deletions test/parallel/test-tls-0-dns-altname-revert-CVE-2021-44532.js
Original file line number Diff line number Diff line change
@@ -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');
}));
}));
Loading

0 comments on commit df3141f

Please sign in to comment.