crypto,tls: implement safe x509 GeneralName format

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#304 PR-URL: nodejs-private/node-private#300 Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs · Jan 10, 2022 · e52882d · e52882d
1 parent 4a262d4
commit e52882d
Show file tree

Hide file tree

Showing 61 changed files with 2,455 additions and 42 deletions.
diff --git a/doc/api/crypto.md b/doc/api/crypto.md
@@ -2565,11 +2565,27 @@ The SHA-256 fingerprint of this certificate.
 
 <!-- YAML
 added: v15.6.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs-private/node-private/pull/300
+    description: Parts of this string may be encoded as JSON string literals
+                 in response to CVE-2021-44532.
 -->
 
 * Type: {string}
 
-The information access content of this certificate.
+A textual representation of the certificate's authority information access
+extension.
+
+This is a line feed separated list of access descriptions. Each line begins with
+the access method and the kind of the access location, followed by a colon and
+the value associated with the access location.
+
+After the prefix denoting the access method and the kind of the access location,
+the remainder of each line might be enclosed in quotes to indicate that the
+value is a JSON string literal. For backward compatibility, Node.js only uses
+JSON string literals within this property when necessary to avoid ambiguity.
+Third-party code should be prepared to handle both possible entry formats.
 
 ### `x509.issuer`
 
@@ -2646,12 +2662,32 @@ The complete subject of this certificate.
 
 <!-- YAML
 added: v15.6.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs-private/node-private/pull/300
+    description: Parts of this string may be encoded as JSON string literals
+                 in response to CVE-2021-44532.
 -->
 
 * Type: {string}
 
 The subject alternative name specified for this certificate.
 
+This is a comma-separated list of subject alternative names. Each entry begins
+with a string identifying the kind of the subject alternative name followed by
+a colon and the value associated with the entry.
+
+Earlier versions of Node.js incorrectly assumed that it is safe to split this
+property at the two-character sequence `', '` (see [CVE-2021-44532][]). However,
+both malicious and legitimate certificates can contain subject alternative names
+that include this sequence when represented as a string.
+
+After the prefix denoting the type of the entry, the remainder of each entry
+might be enclosed in quotes to indicate that the value is a JSON string literal.
+For backward compatibility, Node.js only uses JSON string literals within this
+property when necessary to avoid ambiguity. Third-party code should be prepared
+to handle both possible entry formats.
+
 ### `x509.toJSON()`
 
 <!-- YAML
@@ -5838,6 +5874,7 @@ See the [list of SSL OP Flags][] for details.
 
 [AEAD algorithms]: https://en.wikipedia.org/wiki/Authenticated_encryption
 [CCM mode]: #ccm-mode
+[CVE-2021-44532]: https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-44532
 [Caveats]: #support-for-weak-or-compromised-algorithms
 [Crypto constants]: #crypto-constants
 [HTML 5.2]: https://www.w3.org/TR/html52/changes.html#features-removed

diff --git a/doc/api/errors.md b/doc/api/errors.md
@@ -2505,6 +2505,15 @@ 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`

diff --git a/lib/_tls_common.js b/lib/_tls_common.js
@@ -25,6 +25,7 @@ const tls = require('tls');
 
 const {
   ArrayPrototypePush,
+  JSONParse,
   ObjectCreate,
   StringPrototypeReplace,
 } = primordials;
@@ -137,6 +138,14 @@ function translatePeerCertificate(c) {
     // XXX: More key validation?
     StringPrototypeReplace(info, /([^\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)
                                ArrayPrototypePush(c.infoAccess[key], val);
                              else

diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -1516,6 +1516,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;

diff --git a/lib/tls.js b/lib/tls.js
@@ -30,20 +30,25 @@ const {
   ArrayPrototypePush,
   ArrayPrototypeReduce,
   ArrayPrototypeSome,
+  JSONParse,
   ObjectDefineProperty,
   ObjectFreeze,
+  RegExpPrototypeExec,
   RegExpPrototypeTest,
   StringFromCharCode,
   StringPrototypeCharCodeAt,
   StringPrototypeEndsWith,
   StringPrototypeIncludes,
+  StringPrototypeIndexOf,
   StringPrototypeReplace,
   StringPrototypeSlice,
   StringPrototypeSplit,
   StringPrototypeStartsWith,
+  StringPrototypeSubstring,
 } = primordials;
 
 const {
+  ERR_TLS_CERT_ALTNAME_FORMAT,
   ERR_TLS_CERT_ALTNAME_INVALID,
   ERR_OUT_OF_RANGE
 } = require('internal/errors').codes;
@@ -227,6 +232,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;
+}
+
 exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
   const subject = cert.subject;
   const altNames = cert.subjectaltname;
@@ -237,7 +281,9 @@ exports.checkServerIdentity = function checkServerIdentity(hostname, cert) {
   hostname = '' + hostname;
 
   if (altNames) {
-    const splitAltNames = StringPrototypeSplit(altNames, ', ');
+    const splitAltNames = StringPrototypeIncludes(altNames, '"') ?
+      splitEscapedAltNames(altNames) :
+      StringPrototypeSplit(altNames, ', ');
     ArrayPrototypeForEach(splitAltNames, (name) => {
       if (StringPrototypeStartsWith(name, 'DNS:')) {
         ArrayPrototypePush(dnsNames, StringPrototypeSlice(name, 4));