Skip to content

Commit

Permalink
crypto,tls: implement safe x509 GeneralName format
Browse files Browse the repository at this point in the history
This change introduces JSON-compatible escaping rules for strings that
include X.509 GeneralName components (see RFC 5280). This non-standard
format avoids ambiguities and prevents injection attacks that could
previously lead to X.509 certificates being accepted even though they
were not valid for the target hostname.

These changes affect the format of subject alternative names and the
format of authority information access. The checkServerIdentity function
has been modified to safely handle the new format, eliminating the
possibility of injecting subject alternative names into the verification
logic.

Because each subject alternative name is only encoded as a JSON string
literal if necessary for security purposes, this change will only be
visible in rare cases.

This addresses CVE-2021-44532.

Co-authored-by: Akshay K <iit.akshay@gmail.com>
CVE-ID: CVE-2021-44532
Backport-PR-URL: nodejs-private/node-private#306
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 d4e5d1b commit 19873ab
Show file tree
Hide file tree
Showing 59 changed files with 2,429 additions and 42 deletions.
8 changes: 8 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -1869,6 +1869,14 @@ An unspecified or non-specific system error has occurred within the Node.js
process. The error object will have an `err.info` object property with
additional details.

<a id="ERR_TLS_CERT_ALTNAME_FORMAT"></a>
### `ERR_TLS_CERT_ALTNAME_FORMAT`

This error is thrown by `checkServerIdentity` if a user-supplied
`subjectaltname` property violates encoding rules. Certificate objects produced
by Node.js itself always comply with encoding rules and will never cause
this error.

<a id="ERR_TLS_CERT_ALTNAME_INVALID"></a>
### `ERR_TLS_CERT_ALTNAME_INVALID`

Expand Down
9 changes: 9 additions & 0 deletions lib/_tls_common.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@

const {
ArrayIsArray,
JSONParse,
ObjectCreate,
} = primordials;

Expand Down Expand Up @@ -323,6 +324,14 @@ exports.translatePeerCertificate = function translatePeerCertificate(c) {

// XXX: More key validation?
info.replace(/([^\n:]*):([^\n]*)(?:\n|$)/g, (all, key, val) => {
if (val.charCodeAt(0) === 0x22) {
// The translatePeerCertificate function is only
// used on internally created legacy certificate
// objects, and any value that contains a quote
// will always be a valid JSON string literal,
// so this should never throw.
val = JSONParse(val);
}
if (key in c.infoAccess)
c.infoAccess[key].push(val);
else
Expand Down
2 changes: 2 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -1345,6 +1345,8 @@ E('ERR_STREAM_WRAP', 'Stream has StringDecoder set or is in objectMode', Error);
E('ERR_STREAM_WRITE_AFTER_END', 'write after end', Error);
E('ERR_SYNTHETIC', 'JavaScript Callstack', Error);
E('ERR_SYSTEM_ERROR', 'A system error occurred', SystemError);
E('ERR_TLS_CERT_ALTNAME_FORMAT', 'Invalid subject alternative name string',
SyntaxError);
E('ERR_TLS_CERT_ALTNAME_INVALID', function(reason, host, cert) {
this.reason = reason;
this.host = host;
Expand Down
52 changes: 51 additions & 1 deletion lib/tls.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,19 @@
const {
Array,
ArrayIsArray,
ArrayPrototypePush,
JSONParse,
ObjectDefineProperty,
ObjectFreeze,
RegExpPrototypeExec,
StringPrototypeIncludes,
StringPrototypeIndexOf,
StringPrototypeSplit,
StringPrototypeSubstring,
} = primordials;

const {
ERR_TLS_CERT_ALTNAME_FORMAT,
ERR_TLS_CERT_ALTNAME_INVALID,
ERR_OUT_OF_RANGE
} = require('internal/errors').codes;
Expand Down Expand Up @@ -207,6 +215,45 @@ function check(hostParts, pattern, wildcards) {
return true;
}

// This pattern is used to determine the length of escaped sequences within
// the subject alt names string. It allows any valid JSON string literal.
// This MUST match the JSON specification (ECMA-404 / RFC8259) exactly.
const jsonStringPattern =
// eslint-disable-next-line no-control-regex
/^"(?:[^"\\\u0000-\u001f]|\\(?:["\\/bfnrt]|u[0-9a-fA-F]{4}))*"/;

function splitEscapedAltNames(altNames) {
const result = [];
let currentToken = '';
let offset = 0;
while (offset !== altNames.length) {
const nextSep = StringPrototypeIndexOf(altNames, ', ', offset);
const nextQuote = StringPrototypeIndexOf(altNames, '"', offset);
if (nextQuote !== -1 && (nextSep === -1 || nextQuote < nextSep)) {
// There is a quote character and there is no separator before the quote.
currentToken += StringPrototypeSubstring(altNames, offset, nextQuote);
const match = RegExpPrototypeExec(
jsonStringPattern, StringPrototypeSubstring(altNames, nextQuote));
if (!match) {
throw new ERR_TLS_CERT_ALTNAME_FORMAT();
}
currentToken += JSONParse(match[0]);
offset = nextQuote + match[0].length;
} else if (nextSep !== -1) {
// There is a separator and no quote before it.
currentToken += StringPrototypeSubstring(altNames, offset, nextSep);
ArrayPrototypePush(result, currentToken);
currentToken = '';
offset = nextSep + 2;
} else {
currentToken += StringPrototypeSubstring(altNames, offset);
offset = altNames.length;
}
}
ArrayPrototypePush(result, currentToken);
return result;
}

let urlWarningEmitted = false;
exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
const subject = cert.subject;
Expand All @@ -218,7 +265,10 @@ exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
hostname = '' + hostname;

if (altNames) {
for (const name of altNames.split(', ')) {
const splitAltNames = StringPrototypeIncludes(altNames, '"') ?
splitEscapedAltNames(altNames) :
StringPrototypeSplit(altNames, ', ');
for (const name of splitAltNames) {
if (name.startsWith('DNS:')) {
dnsNames.push(name.slice(4));
} else if (name.startsWith('URI:')) {
Expand Down
Loading

0 comments on commit 19873ab

Please sign in to comment.