tls: fix handling of x509 subject and issuer

When subject and verifier are represented as strings, escape special characters (such as '+') to guarantee unambiguity. Previously, different distinguished names could result in the same string when encoded. In particular, inserting a '+' in a single-value Relative Distinguished Name (e.g., L or OU) would produce a string that is indistinguishable from a multi-value Relative Distinguished Name. Third-party code that correctly interprets the generated string representation as a multi-value Relative Distinguished Name could then be vulnerable to an injection attack, e.g., when an attacker includes a single-value RDN with type OU and value 'HR + CN=example.com', the string representation produced by unpatched versions of Node.js would be 'OU=HR + CN=example.com', which represents a multi-value RDN. Node.js itself is not vulnerable to this attack because the current implementation that parses such strings into objects does not handle '+' at all. This oversight leads to incorrect results, but at the same time appears to prevent injection attacks (as described above). With this change, the JavaScript objects representing the subject and issuer Relative Distinguished Names are constructed in C++ directly, instead of (incorrectly) encoding them as strings and then (incorrectly) decoding the strings in JavaScript. This addresses CVE-2021-44533. Co-authored-by: Akshay K <[email protected]> CVE-ID: CVE-2021-44533 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]>
nodejs · Jan 8, 2022 · 1f7fdff · 1f7fdff
1 parent b11b4cc
commit 1f7fdff
Show file tree

Hide file tree

Showing 16 changed files with 656 additions and 13 deletions.
diff --git a/lib/_tls_common.js b/lib/_tls_common.js
@@ -126,11 +126,13 @@ function translatePeerCertificate(c) {
   if (!c)
     return null;
 
-  if (c.issuer != null) c.issuer = parseCertString(c.issuer);
+  // TODO(tniessen): can we remove parseCertString without breaking anything?
+  if (typeof c.issuer === 'string') c.issuer = parseCertString(c.issuer);
   if (c.issuerCertificate != null && c.issuerCertificate !== c) {
     c.issuerCertificate = translatePeerCertificate(c.issuerCertificate);
   }
-  if (c.subject != null) c.subject = parseCertString(c.subject);
+  // TODO(tniessen): can we remove parseCertString without breaking anything?
+  if (typeof c.subject === 'string') c.subject = parseCertString(c.subject);
   if (c.infoAccess != null) {
     const info = c.infoAccess;
     c.infoAccess = ObjectCreate(null);

diff --git a/src/crypto/crypto_common.cc b/src/crypto/crypto_common.cc
@@ -42,6 +42,7 @@ using v8::Value;
 
 namespace crypto {
 static constexpr int X509_NAME_FLAGS =
+    ASN1_STRFLGS_ESC_2253 |
     ASN1_STRFLGS_ESC_CTRL |
     ASN1_STRFLGS_UTF8_CONVERT |
     XN_FLAG_SEP_MULTILINE |
@@ -964,6 +965,93 @@ MaybeLocal<Value> GetSubject(
   return ToV8Value(env, bio);
 }
 
+template <X509_NAME* get_name(const X509*)>
+static MaybeLocal<Value> GetX509NameObject(Environment* env, X509* cert) {
+  X509_NAME* name = get_name(cert);
+  CHECK_NOT_NULL(name);
+
+  int cnt = X509_NAME_entry_count(name);
+  CHECK_GE(cnt, 0);
+
+  Local<Object> result =
+      Object::New(env->isolate(), Null(env->isolate()), nullptr, nullptr, 0);
+  if (result.IsEmpty()) {
+    return MaybeLocal<Value>();
+  }
+
+  for (int i = 0; i < cnt; i++) {
+    X509_NAME_ENTRY* entry = X509_NAME_get_entry(name, i);
+    CHECK_NOT_NULL(entry);
+
+    // We intentionally ignore the value of X509_NAME_ENTRY_set because the
+    // representation as an object does not allow grouping entries into sets
+    // anyway, and multi-value RDNs are rare, i.e., the vast majority of
+    // Relative Distinguished Names contains a single type-value pair only.
+    const ASN1_OBJECT* type = X509_NAME_ENTRY_get_object(entry);
+    const ASN1_STRING* value = X509_NAME_ENTRY_get_data(entry);
+
+    // If OpenSSL knows the type, use the short name of the type as the key, and
+    // the numeric representation of the type's OID otherwise.
+    int type_nid = OBJ_obj2nid(type);
+    char type_buf[80];
+    const char* type_str;
+    if (type_nid != NID_undef) {
+      type_str = OBJ_nid2sn(type_nid);
+      CHECK_NOT_NULL(type_str);
+    } else {
+      OBJ_obj2txt(type_buf, sizeof(type_buf), type, true);
+      type_str = type_buf;
+    }
+
+    Local<String> v8_name;
+    if (!String::NewFromUtf8(env->isolate(), type_str).ToLocal(&v8_name)) {
+      return MaybeLocal<Value>();
+    }
+
+    // The previous implementation used X509_NAME_print_ex, which escapes some
+    // characters in the value. The old implementation did not decode/unescape
+    // values correctly though, leading to ambiguous and incorrect
+    // representations. The new implementation only converts to Unicode and does
+    // not escape anything.
+    unsigned char* value_str;
+    int value_str_size = ASN1_STRING_to_UTF8(&value_str, value);
+    if (value_str_size < 0) {
+      return Undefined(env->isolate());
+    }
+
+    Local<String> v8_value;
+    if (!String::NewFromUtf8(env->isolate(),
+                             reinterpret_cast<const char*>(value_str),
+                             NewStringType::kNormal,
+                             value_str_size).ToLocal(&v8_value)) {
+      OPENSSL_free(value_str);
+      return MaybeLocal<Value>();
+    }
+
+    OPENSSL_free(value_str);
+
+    // For backward compatibility, we only create arrays if multiple values
+    // exist for the same key. That is not great but there is not much we can
+    // change here without breaking things. Note that this creates nested data
+    // structures, yet still does not allow representing Distinguished Names
+    // accurately.
+    if (result->HasOwnProperty(env->context(), v8_name).ToChecked()) {
+      Local<Value> accum =
+          result->Get(env->context(), v8_name).ToLocalChecked();
+      if (!accum->IsArray()) {
+        accum = Array::New(env->isolate(), &accum, 1);
+        result->Set(env->context(), v8_name, accum).Check();
+      }
+      Local<Array> array = accum.As<Array>();
+      array->Set(env->context(), array->Length(), v8_value).Check();
+    } else {
+      result->Set(env->context(), v8_name, v8_value).Check();
+    }
+  }
+
+  return result;
+}
+
 MaybeLocal<Value> GetCipherName(Environment* env, const SSLPointer& ssl) {
   return GetCipherName(env, SSL_get_current_cipher(ssl.get()));
 }
@@ -1194,22 +1282,44 @@ MaybeLocal<Value> GetPeerCert(
   return result;
 }
 
-MaybeLocal<Object> X509ToObject(Environment* env, X509* cert) {
+MaybeLocal<Object> X509ToObject(
+    Environment* env,
+    X509* cert,
+    bool names_as_string) {
   EscapableHandleScope scope(env->isolate());
   Local<Context> context = env->context();
   Local<Object> info = Object::New(env->isolate());
 
   BIOPointer bio(BIO_new(BIO_s_mem()));
 
+  if (names_as_string) {
+    // TODO(tniessen): this branch should not have to exist. It is only here
+    // because toLegacyObject() does not actually return a legacy object, and
+    // instead represents subject and issuer as strings.
+    if (!Set<Value>(context,
+                    info,
+                    env->subject_string(),
+                    GetSubject(env, bio, cert)) ||
+        !Set<Value>(context,
+                    info,
+                    env->issuer_string(),
+                    GetIssuerString(env, bio, cert))) {
+      return MaybeLocal<Object>();
+    }
+  } else {
+    if (!Set<Value>(context,
+                    info,
+                    env->subject_string(),
+                    GetX509NameObject<X509_get_subject_name>(env, cert)) ||
+        !Set<Value>(context,
+                    info,
+                    env->issuer_string(),
+                    GetX509NameObject<X509_get_issuer_name>(env, cert))) {
+      return MaybeLocal<Object>();
+    }
+  }
+
   if (!Set<Value>(context,
-                  info,
-                  env->subject_string(),
-                  GetSubject(env, bio, cert)) ||
-      !Set<Value>(context,
-                  info,
-                  env->issuer_string(),
-                  GetIssuerString(env, bio, cert)) ||
-      !Set<Value>(context,
                   info,
                   env->subjectaltname_string(),
                   GetSubjectAltNameString(env, bio, cert)) ||

diff --git a/src/crypto/crypto_common.h b/src/crypto/crypto_common.h
@@ -116,7 +116,8 @@ v8::MaybeLocal<v8::Object> ECPointToBuffer(
 
 v8::MaybeLocal<v8::Object> X509ToObject(
     Environment* env,
-    X509* cert);
+    X509* cert,
+    bool names_as_string = false);
 
 v8::MaybeLocal<v8::Value> GetValidTo(
     Environment* env,

diff --git a/src/crypto/crypto_x509.cc b/src/crypto/crypto_x509.cc
@@ -470,7 +470,7 @@ void X509Certificate::ToLegacy(const FunctionCallbackInfo<Value>& args) {
   X509Certificate* cert;
   ASSIGN_OR_RETURN_UNWRAP(&cert, args.Holder());
   Local<Value> ret;
-  if (X509ToObject(env, cert->get()).ToLocal(&ret))
+  if (X509ToObject(env, cert->get(), true).ToLocal(&ret))
     args.GetReturnValue().Set(ret);
 }
 

diff --git a/test/fixtures/x509-escaping/create-certs.js b/test/fixtures/x509-escaping/create-certs.js
@@ -500,3 +500,144 @@ for (let i = 0; i < infoAccessExtensions.length; i++) {
   });
   writeFileSync(`./info-${i}-cert.pem`, `${pem}\n`);
 }
+
+const subjects = [
+  [
+    [
+      { type: oid.localityName, value: UTF8String.encode('Somewhere') }
+    ],
+    [
+      { type: oid.commonName, value: UTF8String.encode('evil.example.com') }
+    ]
+  ],
+  [
+    [
+      {
+        type: oid.localityName,
+        value: UTF8String.encode('Somewhere\0evil.example.com'),
+      }
+    ]
+  ],
+  [
+    [
+      {
+        type: oid.localityName,
+        value: UTF8String.encode('Somewhere\nCN=evil.example.com')
+      }
+    ]
+  ],
+  [
+    [
+      {
+        type: oid.localityName,
+        value: UTF8String.encode('Somewhere, CN = evil.example.com')
+      }
+    ]
+  ],
+  [
+    [
+      {
+        type: oid.localityName,
+        value: UTF8String.encode('Somewhere/CN=evil.example.com')
+      }
+    ]
+  ],
+  [
+    [
+      {
+        type: oid.localityName,
+        value: UTF8String.encode('M\u00fcnchen\\\nCN=evil.example.com')
+      }
+    ]
+  ],
+  [
+    [
+      { type: oid.localityName, value: UTF8String.encode('Somewhere') },
+      { type: oid.commonName, value: UTF8String.encode('evil.example.com') },
+    ]
+  ],
+  [
+    [
+      {
+        type: oid.localityName,
+        value: UTF8String.encode('Somewhere + CN=evil.example.com'),
+      }
+    ]
+  ],
+  [
+    [
+      { type: oid.localityName, value: UTF8String.encode('L1') },
+      { type: oid.localityName, value: UTF8String.encode('L2') },
+    ],
+    [
+      { type: oid.localityName, value: UTF8String.encode('L3') },
+    ]
+  ],
+  [
+    [
+      { type: oid.localityName, value: UTF8String.encode('L1') },
+    ],
+    [
+      { type: oid.localityName, value: UTF8String.encode('L2') },
+    ],
+    [
+      { type: oid.localityName, value: UTF8String.encode('L3') },
+    ],
+  ],
+];
+
+for (let i = 0; i < subjects.length; i++) {
+  const tbs = {
+    version: 'v3',
+    serialNumber: new BN('01', 16),
+    signature: {
+      algorithm: oid.sha256WithRSAEncryption,
+      parameters: null_
+    },
+    issuer: {
+      type: 'rdnSequence',
+      value: subjects[i]
+    },
+    validity: {
+      notBefore: { type: 'utcTime', value: now },
+      notAfter: { type: 'utcTime', value: now + days * 86400000 }
+    },
+    subject: {
+      type: 'rdnSequence',
+      value: subjects[i]
+    },
+    subjectPublicKeyInfo: {
+      algorithm: {
+        algorithm: oid.rsaEncryption,
+        parameters: null_
+      },
+      subjectPublicKey: {
+        unused: 0,
+        data: publicKey
+      }
+    }
+  };
+
+  // Self-sign the certificate.
+  const tbsDer = rfc5280.TBSCertificate.encode(tbs, 'der');
+  const signature = crypto.createSign(digest).update(tbsDer).sign(privateKey);
+
+  // Construct the signed certificate.
+  const cert = {
+    tbsCertificate: tbs,
+    signatureAlgorithm: {
+      algorithm: oid.sha256WithRSAEncryption,
+      parameters: null_
+    },
+    signature: {
+      unused: 0,
+      data: signature
+    }
+  };
+
+  // Store the signed certificate.
+  const pem = rfc5280.Certificate.encode(cert, 'pem', {
+    label: 'CERTIFICATE'
+  });
+  writeFileSync(`./subj-${i}-cert.pem`, `${pem}\n`);
+}
diff --git a/test/fixtures/x509-escaping/subj-0-cert.pem b/test/fixtures/x509-escaping/subj-0-cert.pem
@@ -0,0 +1,28 @@
+-----BEGIN CERTIFICATE-----
+MIIE1zCCAr+gAwIBAgIBATANBgkqhkiG9w0BAQsFADAvMRIwEAYDVQQHDAlTb21l
+d2hlcmUxGTAXBgNVBAMMEGV2aWwuZXhhbXBsZS5jb20wHhcNMjExMjIwMTQ1NzM1
+WhcNMzExMjE4MTQ1NzM1WjAvMRIwEAYDVQQHDAlTb21ld2hlcmUxGTAXBgNVBAMM
+EGV2aWwuZXhhbXBsZS5jb20wggIiMA0GCSqGSIb3DQEBAQUAA4ICDwAwggIKAoIC
+AQCxEWd00u9E9T/ko6WcCKjhZ7tjnfVylnA7M0EHOwvdivgD46eAb1omsonLagiV
+rZrG7EpYuMhtz+g3Yv1d0nvFvv8ge9UIdnN8EDTDzLpJ3KbNqHURraiXuBDqa3rd
+Y4JBakCcuYHl1bj1OTew7xl1FWc1je04rBTQGTFIRdmJZYyc9bIw9WkY6msod0w1
+PDcLhZS3emh/eYaL4zAQWrVhQfWzf4rZzFaI/a5n0o75aUkTuvxDDQ51V2d6WkSU
+3KbOnf2JW+QJXfzsNOeiYA9AnfY59evr4GEeG8VZdGuUG39uDCIWmAUT8elhXXqV
+q+NdBqc6VUNLDJbqCmMx/Ecp48EHO6X5uXm0xViZIVPNIzqiiRhVt4nFfwPQZrTg
+aq2+tD7/zD1yED4O1FhlDl5twH2N7+oG06HsEluQdLPrj7IedpneGVKMs078Ddov
+7j6icYv/RZHVetDlrzDDHjLJWwxyAWzdGdkhtMGPd6B9i4TtF/PU3J3nbpLn5XfE
+BFu4jJ+w+5Wvk5a60gF1ERy/OLBM/e8sro2sEBIpp1tN1wJVBZOtTIi4VVDhwDRQ
+Uiwb2d1Re7GQ7+mcz5D/01qxW6S+w0IKrpwJUjR3mpa0OU98KfKVJkeyeEBLkEhD
+dnGTDqZ9E/ickGosrW2gAAYKgzXk725dpxTdpLEosfDbpwIDAQABMA0GCSqGSIb3
+DQEBCwUAA4ICAQAnszSuVqfEmpjf2VMvk9TUuiop0tejHP+hB30IURJqA9K51edx
+IRszXXU4Sj8uHT88RpKxgDm/GcfEA0l2rWZ6Mal6pmUyjteJJPMVA6fgeNM8XvtJ
+eoxi2wm8FzxXJrPK7fOMG5/fLb7ENUZYFRHVFJ+Gk290DP7x81Gzb5tcsolrVqW+
+TZdV2aBZya28NjgXncjinIlD61I6LzoQbDInab5nEPKMRuRTXMLfbAypXrPAbsfz
++Z6ZKhfNEo0/5cI4iG8MQXM1HgbFCkWOTPPeR53lo+1f9dN3IZ+1PYUjkOJzuxUZ
+HIA+Dy+S1ocfK582LqohexhjeC5AL74rJJcgns9ORxz2GN1buIRTzi9XL2egp7cd
++XgZ3phpY4mIM0bH+DJ7eIqkM17WkEwJ3vazu7tEmIldc06Pmt2vFEcQB3T0bsw7
+lBZdwSEkqTb+IexaQerSyztuxKc2DhOLTqZfVPCd2LWhasNSHzGmanI3vmBy98MN
+LZzo7+G1BDMyMsl3DwEiwOGYARXJklU1LxCj6nVCTymNToLXtF2xHcZuK94Pqol9
+n8zMCUYNOr7USWA25GwfpN65UHN7YXsOl9XIMWl+iVA5QepAI9sL0n3CyFW0ZXgn
+DsZkfikYa+xhQSUANV4zDx1X8FxZmT0Op/+mhkvwL1+YKUHJy3WdXrIFgw==
+-----END CERTIFICATE-----
diff --git a/test/fixtures/x509-escaping/subj-1-cert.pem b/test/fixtures/x509-escaping/subj-1-cert.pem
@@ -0,0 +1,28 @@
+-----BEGIN CERTIFICATE-----
+MIIEwzCCAqugAwIBAgIBATANBgkqhkiG9w0BAQsFADAlMSMwIQYDVQQHDBpTb21l
+d2hlcmUAZXZpbC5leGFtcGxlLmNvbTAeFw0yMTEyMjAxNDU3MzVaFw0zMTEyMTgx
+NDU3MzVaMCUxIzAhBgNVBAcMGlNvbWV3aGVyZQBldmlsLmV4YW1wbGUuY29tMIIC
+IjANBgkqhkiG9w0BAQEFAAOCAg8AMIICCgKCAgEAsRFndNLvRPU/5KOlnAio4We7
+Y531cpZwOzNBBzsL3Yr4A+OngG9aJrKJy2oIla2axuxKWLjIbc/oN2L9XdJ7xb7/
+IHvVCHZzfBA0w8y6Sdymzah1Ea2ol7gQ6mt63WOCQWpAnLmB5dW49Tk3sO8ZdRVn
+NY3tOKwU0BkxSEXZiWWMnPWyMPVpGOprKHdMNTw3C4WUt3pof3mGi+MwEFq1YUH1
+s3+K2cxWiP2uZ9KO+WlJE7r8Qw0OdVdnelpElNymzp39iVvkCV387DTnomAPQJ32
+OfXr6+BhHhvFWXRrlBt/bgwiFpgFE/HpYV16lavjXQanOlVDSwyW6gpjMfxHKePB
+Bzul+bl5tMVYmSFTzSM6ookYVbeJxX8D0Ga04GqtvrQ+/8w9chA+DtRYZQ5ebcB9
+je/qBtOh7BJbkHSz64+yHnaZ3hlSjLNO/A3aL+4+onGL/0WR1XrQ5a8wwx4yyVsM
+cgFs3RnZIbTBj3egfYuE7Rfz1Nyd526S5+V3xARbuIyfsPuVr5OWutIBdREcvziw
+TP3vLK6NrBASKadbTdcCVQWTrUyIuFVQ4cA0UFIsG9ndUXuxkO/pnM+Q/9NasVuk
+vsNCCq6cCVI0d5qWtDlPfCnylSZHsnhAS5BIQ3Zxkw6mfRP4nJBqLK1toAAGCoM1
+5O9uXacU3aSxKLHw26cCAwEAATANBgkqhkiG9w0BAQsFAAOCAgEAmjKOoKxLwPY4
+e65pYTUSBctPZ2juW5uNs8UvH5O32OC9RhENJBIIKn3B9Z/wkexR2zcvaQmJObLW
+6mkR7O0tNgsXVYJFzLRBfjM/nyP6nafiCUekmoh9Kojq6x5IQQgEsK+Uw123kkoI
+w/h3hBYBq8+CFPnYtBLZBVVFMNGaATXrYJPCcjVrtAHYxIWaDN2R+1DWLRIV72sF
+hu4xGz0kmUbzforl/FA3gdgM7mwfZMF4+EoQZi5mShdWnyfzAHIbtahnA4lPNtx9
+vBqYIZ/a2ITsXmWc2KGs/rRG+SDLzg+H1Xudvu/y2d1ULpZQfT6bg6Ro855FiU9h
+TyHHQGGqlC9/DjHy//wERsFEJZh5/j21LGyalEjgfOYtzPkjZlIweYr8LlHTrauo
+/gWihriaaWAkD+2fwQ09CUHdvOG6yoT+j/E50FsekfqV3tKMwoZoph6dF1TWQg32
+JXV0akpd5ff1cca8sZgJfUksDfSkrwG7fl3tje30vQTlvNrhu2MCKFGQwyXed3qg
+86lx+sTZjxMYvqWWysKTx8aIJ95XAK2jJ2OEVI2X6cdgoAp6aMkycbttik4hDoPJ
+eAWaZo2UFs2MGoUbX9m4RzPqPuBHNFqoV6yRyS1K/3KWyxVVvamZY0Qgzmoi4coB
+hRlTO6GDkF7u1YQ7eZi7pP7U8OcklfE=
+-----END CERTIFICATE-----