Skip to content

Commit

Permalink
src: add cve reverts and associated tests
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
2 people authored and richardlau committed Jan 7, 2022
1 parent 83d8f88 commit 9f2c526
Show file tree
Hide file tree
Showing 6 changed files with 556 additions and 3 deletions.
32 changes: 29 additions & 3 deletions lib/tls.js
Expand Up @@ -50,9 +50,11 @@ const { isArrayBufferView } = require('internal/util/types');

const net = require('net');
const { getOptionValue } = require('internal/options');
const url = require('url');
const { getRootCertificates, getSSLCiphers } = internalBinding('crypto');
const { Buffer } = require('buffer');
const EventEmitter = require('events');
const { URL } = require('internal/url');
const DuplexPair = require('internal/streams/duplexpair');
const { canonicalizeIP } = internalBinding('cares_wrap');
const _tls_common = require('_tls_common');
Expand Down Expand Up @@ -261,10 +263,12 @@ function splitEscapedAltNames(altNames) {
return result;
}

let urlWarningEmitted = false;
exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
const subject = cert.subject;
const altNames = cert.subjectaltname;
const dnsNames = [];
const uriNames = [];
const ips = [];

hostname = '' + hostname;
Expand All @@ -276,6 +280,22 @@ exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
for (const name of splitAltNames) {
if (name.startsWith('DNS:')) {
dnsNames.push(name.slice(4));
} else if (process.REVERT_CVE_2021_44531 && name.startsWith('URI:')) {
let uri;
try {
uri = new URL(name.slice(4));
} catch {
uri = url.parse(name.slice(4));
if (!urlWarningEmitted && !process.noDeprecation) {
urlWarningEmitted = true;
process.emitWarning(
`The URI ${name.slice(4)} found in cert.subjectaltname ` +
'is not a valid URI, and is supported in the tls module ' +
'solely for compatibility.',
'DeprecationWarning', 'DEP0109');
}
}
uriNames.push(uri.hostname); // TODO(bnoordhuis) Also use scheme.
} else if (name.startsWith('IP Address:')) {
ips.push(canonicalizeIP(name.slice(11)));
}
Expand All @@ -285,19 +305,25 @@ 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 = ips.includes(canonicalizeIP(hostname));
if (!valid)
reason = `IP: ${hostname} is not in the cert's list: ${ips.join(', ')}`;
// TODO(bnoordhuis) Also check URI SANs that are IP addresses.
} else if (dnsNames.length > 0 || (subject && subject.CN)) {
} else if ((process.REVERT_CVE_2021_44531 && (hasAltNames || subject)) ||
(dnsNames.length > 0 || (subject && subject.CN))) {
const hostParts = splitHost(hostname);
const wildcard = (pattern) => check(hostParts, pattern, true);

if (dnsNames.length > 0) {
valid = dnsNames.some(wildcard);
if ((process.REVERT_CVE_2021_44531 && hasAltNames) ||
(dnsNames.length > 0)) {
const noWildcard = (pattern) => check(hostParts, pattern, false);
valid = dnsNames.some(wildcard) || uriNames.some(noWildcard);
if (!valid)
reason =
`Host: ${hostname}. is not in the cert's altnames: ${altNames}`;
Expand Down
47 changes: 47 additions & 0 deletions src/node_crypto_common.cc
Expand Up @@ -6,6 +6,7 @@
#include "node_crypto_common.h"
#include "node.h"
#include "node_internals.h"
#include "node_revert.h"
#include "node_url.h"
#include "string_bytes.h"
#include "v8.h"
Expand Down Expand Up @@ -482,6 +483,44 @@ void AddFingerprintDigest(
}
}

// 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 @@ -706,6 +745,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 @@ -731,6 +774,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
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
64 changes: 64 additions & 0 deletions test/parallel/test-revert-CVE-2021-44531.js
@@ -0,0 +1,64 @@
// 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');

common.expectWarning('DeprecationWarning', [
['The URI http://[a.b.a.com]/ found in cert.subjectaltname ' +
'is not a valid URI, and is supported in the tls module ' +
'solely for compatibility.',
'DEP0109'],
]);

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/'
},
// Invalid URI
{
host: 'a.b.a.com', cert: {
subjectaltname: 'URI:http://[a.b.a.com]/',
subject: {}
}
},
];

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
@@ -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');
}));
}));

0 comments on commit 9f2c526

Please sign in to comment.