Skip to content

Commit 8c2db2c

Browse files
tniessenkumarak
authored and
richardlau
committed
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 <iit.akshay@gmail.com> CVE-ID: CVE-2021-44533 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>
1 parent e0fe6a6 commit 8c2db2c

15 files changed

+660
-12
lines changed

‎lib/_tls_common.js

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -313,11 +313,13 @@ exports.translatePeerCertificate = function translatePeerCertificate(c) {
313313
if (!c)
314314
return null;
315315

316-
if (c.issuer != null) c.issuer = parseCertString(c.issuer);
316+
// TODO(tniessen): can we remove parseCertString without breaking anything?
317+
if (typeof c.issuer === 'string') c.issuer = parseCertString(c.issuer);
317318
if (c.issuerCertificate != null && c.issuerCertificate !== c) {
318319
c.issuerCertificate = translatePeerCertificate(c.issuerCertificate);
319320
}
320-
if (c.subject != null) c.subject = parseCertString(c.subject);
321+
// TODO(tniessen): can we remove parseCertString without breaking anything?
322+
if (typeof c.subject === 'string') c.subject = parseCertString(c.subject);
321323
if (c.infoAccess != null) {
322324
const info = c.infoAccess;
323325
c.infoAccess = ObjectCreate(null);

‎src/node_crypto_common.cc

Lines changed: 120 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ using v8::Value;
4040
namespace crypto {
4141

4242
static constexpr int X509_NAME_FLAGS =
43+
ASN1_STRFLGS_ESC_2253 |
4344
ASN1_STRFLGS_ESC_CTRL |
4445
ASN1_STRFLGS_UTF8_CONVERT |
4546
XN_FLAG_SEP_MULTILINE |
@@ -796,6 +797,94 @@ v8::MaybeLocal<v8::Value> GetInfoAccessString(
796797
return ToV8Value(env, bio);
797798
}
798799

800+
template <X509_NAME* get_name(const X509*)>
801+
static MaybeLocal<Value> GetX509NameObject(Environment* env, X509* cert) {
802+
X509_NAME* name = get_name(cert);
803+
CHECK_NOT_NULL(name);
804+
805+
int cnt = X509_NAME_entry_count(name);
806+
CHECK_GE(cnt, 0);
807+
808+
Local<Object> result =
809+
Object::New(env->isolate(), Null(env->isolate()), nullptr, nullptr, 0);
810+
if (result.IsEmpty()) {
811+
return MaybeLocal<Value>();
812+
}
813+
814+
for (int i = 0; i < cnt; i++) {
815+
X509_NAME_ENTRY* entry = X509_NAME_get_entry(name, i);
816+
CHECK_NOT_NULL(entry);
817+
818+
// We intentionally ignore the value of X509_NAME_ENTRY_set because the
819+
// representation as an object does not allow grouping entries into sets
820+
// anyway, and multi-value RDNs are rare, i.e., the vast majority of
821+
// Relative Distinguished Names contains a single type-value pair only.
822+
const ASN1_OBJECT* type = X509_NAME_ENTRY_get_object(entry);
823+
const ASN1_STRING* value = X509_NAME_ENTRY_get_data(entry);
824+
825+
// If OpenSSL knows the type, use the short name of the type as the key, and
826+
// the numeric representation of the type's OID otherwise.
827+
int type_nid = OBJ_obj2nid(type);
828+
char type_buf[80];
829+
const char* type_str;
830+
if (type_nid != NID_undef) {
831+
type_str = OBJ_nid2sn(type_nid);
832+
CHECK_NOT_NULL(type_str);
833+
} else {
834+
OBJ_obj2txt(type_buf, sizeof(type_buf), type, true);
835+
type_str = type_buf;
836+
}
837+
838+
Local<String> v8_name;
839+
if (!String::NewFromUtf8(env->isolate(), type_str,
840+
NewStringType::kNormal).ToLocal(&v8_name)) {
841+
return MaybeLocal<Value>();
842+
}
843+
844+
// The previous implementation used X509_NAME_print_ex, which escapes some
845+
// characters in the value. The old implementation did not decode/unescape
846+
// values correctly though, leading to ambiguous and incorrect
847+
// representations. The new implementation only converts to Unicode and does
848+
// not escape anything.
849+
unsigned char* value_str;
850+
int value_str_size = ASN1_STRING_to_UTF8(&value_str, value);
851+
if (value_str_size < 0) {
852+
return Undefined(env->isolate());
853+
}
854+
855+
Local<String> v8_value;
856+
if (!String::NewFromUtf8(env->isolate(),
857+
reinterpret_cast<const char*>(value_str),
858+
NewStringType::kNormal,
859+
value_str_size).ToLocal(&v8_value)) {
860+
OPENSSL_free(value_str);
861+
return MaybeLocal<Value>();
862+
}
863+
864+
OPENSSL_free(value_str);
865+
866+
// For backward compatibility, we only create arrays if multiple values
867+
// exist for the same key. That is not great but there is not much we can
868+
// change here without breaking things. Note that this creates nested data
869+
// structures, yet still does not allow representing Distinguished Names
870+
// accurately.
871+
if (result->HasOwnProperty(env->context(), v8_name).ToChecked()) {
872+
Local<Value> accum =
873+
result->Get(env->context(), v8_name).ToLocalChecked();
874+
if (!accum->IsArray()) {
875+
accum = Array::New(env->isolate(), &accum, 1);
876+
result->Set(env->context(), v8_name, accum).Check();
877+
}
878+
Local<Array> array = accum.As<Array>();
879+
array->Set(env->context(), array->Length(), v8_value).Check();
880+
} else {
881+
result->Set(env->context(), v8_name, v8_value).Check();
882+
}
883+
}
884+
885+
return result;
886+
}
887+
799888
MaybeLocal<Value> GetFingerprintDigest(
800889
Environment* env,
801890
const EVP_MD* method,
@@ -1159,22 +1248,44 @@ MaybeLocal<Value> GetPeerCert(
11591248
return result;
11601249
}
11611250

1162-
MaybeLocal<Object> X509ToObject(Environment* env, X509* cert) {
1251+
MaybeLocal<Object> X509ToObject(
1252+
Environment* env,
1253+
X509* cert,
1254+
bool names_as_string) {
11631255
EscapableHandleScope scope(env->isolate());
11641256
Local<Context> context = env->context();
11651257
Local<Object> info = Object::New(env->isolate());
11661258

11671259
BIOPointer bio(BIO_new(BIO_s_mem()));
11681260

1261+
if (names_as_string) {
1262+
// TODO(tniessen): this branch should not have to exist. It is only here
1263+
// because toLegacyObject() does not actually return a legacy object, and
1264+
// instead represents subject and issuer as strings.
1265+
if (!Set<Value>(context,
1266+
info,
1267+
env->subject_string(),
1268+
GetSubject(env, bio, cert)) ||
1269+
!Set<Value>(context,
1270+
info,
1271+
env->issuer_string(),
1272+
GetIssuerString(env, bio, cert))) {
1273+
return MaybeLocal<Object>();
1274+
}
1275+
} else {
1276+
if (!Set<Value>(context,
1277+
info,
1278+
env->subject_string(),
1279+
GetX509NameObject<X509_get_subject_name>(env, cert)) ||
1280+
!Set<Value>(context,
1281+
info,
1282+
env->issuer_string(),
1283+
GetX509NameObject<X509_get_issuer_name>(env, cert))) {
1284+
return MaybeLocal<Object>();
1285+
}
1286+
}
1287+
11691288
if (!Set<Value>(context,
1170-
info,
1171-
env->subject_string(),
1172-
GetSubject(env, bio, cert)) ||
1173-
!Set<Value>(context,
1174-
info,
1175-
env->issuer_string(),
1176-
GetIssuerString(env, bio, cert)) ||
1177-
!Set<Value>(context,
11781289
info,
11791290
env->subjectaltname_string(),
11801291
GetSubjectAltNameString(env, bio, cert)) ||

‎src/node_crypto_common.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,8 @@ v8::MaybeLocal<v8::Object> ECPointToBuffer(
122122

123123
v8::MaybeLocal<v8::Object> X509ToObject(
124124
Environment* env,
125-
X509* cert);
125+
X509* cert,
126+
bool names_as_string = false);
126127

127128
} // namespace crypto
128129
} // namespace node

‎test/fixtures/x509-escaping/create-certs.js

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -500,3 +500,144 @@ for (let i = 0; i < infoAccessExtensions.length; i++) {
500500
});
501501
writeFileSync(`./info-${i}-cert.pem`, `${pem}\n`);
502502
}
503+
504+
const subjects = [
505+
[
506+
[
507+
{ type: oid.localityName, value: UTF8String.encode('Somewhere') }
508+
],
509+
[
510+
{ type: oid.commonName, value: UTF8String.encode('evil.example.com') }
511+
]
512+
],
513+
[
514+
[
515+
{
516+
type: oid.localityName,
517+
value: UTF8String.encode('Somewhere\0evil.example.com'),
518+
}
519+
]
520+
],
521+
[
522+
[
523+
{
524+
type: oid.localityName,
525+
value: UTF8String.encode('Somewhere\nCN=evil.example.com')
526+
}
527+
]
528+
],
529+
[
530+
[
531+
{
532+
type: oid.localityName,
533+
value: UTF8String.encode('Somewhere, CN = evil.example.com')
534+
}
535+
]
536+
],
537+
[
538+
[
539+
{
540+
type: oid.localityName,
541+
value: UTF8String.encode('Somewhere/CN=evil.example.com')
542+
}
543+
]
544+
],
545+
[
546+
[
547+
{
548+
type: oid.localityName,
549+
value: UTF8String.encode('M\u00fcnchen\\\nCN=evil.example.com')
550+
}
551+
]
552+
],
553+
[
554+
[
555+
{ type: oid.localityName, value: UTF8String.encode('Somewhere') },
556+
{ type: oid.commonName, value: UTF8String.encode('evil.example.com') },
557+
]
558+
],
559+
[
560+
[
561+
{
562+
type: oid.localityName,
563+
value: UTF8String.encode('Somewhere + CN=evil.example.com'),
564+
}
565+
]
566+
],
567+
[
568+
[
569+
{ type: oid.localityName, value: UTF8String.encode('L1') },
570+
{ type: oid.localityName, value: UTF8String.encode('L2') },
571+
],
572+
[
573+
{ type: oid.localityName, value: UTF8String.encode('L3') },
574+
]
575+
],
576+
[
577+
[
578+
{ type: oid.localityName, value: UTF8String.encode('L1') },
579+
],
580+
[
581+
{ type: oid.localityName, value: UTF8String.encode('L2') },
582+
],
583+
[
584+
{ type: oid.localityName, value: UTF8String.encode('L3') },
585+
],
586+
],
587+
];
588+
589+
for (let i = 0; i < subjects.length; i++) {
590+
const tbs = {
591+
version: 'v3',
592+
serialNumber: new BN('01', 16),
593+
signature: {
594+
algorithm: oid.sha256WithRSAEncryption,
595+
parameters: null_
596+
},
597+
issuer: {
598+
type: 'rdnSequence',
599+
value: subjects[i]
600+
},
601+
validity: {
602+
notBefore: { type: 'utcTime', value: now },
603+
notAfter: { type: 'utcTime', value: now + days * 86400000 }
604+
},
605+
subject: {
606+
type: 'rdnSequence',
607+
value: subjects[i]
608+
},
609+
subjectPublicKeyInfo: {
610+
algorithm: {
611+
algorithm: oid.rsaEncryption,
612+
parameters: null_
613+
},
614+
subjectPublicKey: {
615+
unused: 0,
616+
data: publicKey
617+
}
618+
}
619+
};
620+
621+
// Self-sign the certificate.
622+
const tbsDer = rfc5280.TBSCertificate.encode(tbs, 'der');
623+
const signature = crypto.createSign(digest).update(tbsDer).sign(privateKey);
624+
625+
// Construct the signed certificate.
626+
const cert = {
627+
tbsCertificate: tbs,
628+
signatureAlgorithm: {
629+
algorithm: oid.sha256WithRSAEncryption,
630+
parameters: null_
631+
},
632+
signature: {
633+
unused: 0,
634+
data: signature
635+
}
636+
};
637+
638+
// Store the signed certificate.
639+
const pem = rfc5280.Certificate.encode(cert, 'pem', {
640+
label: 'CERTIFICATE'
641+
});
642+
writeFileSync(`./subj-${i}-cert.pem`, `${pem}\n`);
643+
}

‎test/fixtures/x509-escaping/subj-0-cert.pem

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
-----BEGIN CERTIFICATE-----
2+
MIIE1zCCAr+gAwIBAgIBATANBgkqhkiG9w0BAQsFADAvMRIwEAYDVQQHDAlTb21l
3+
d2hlcmUxGTAXBgNVBAMMEGV2aWwuZXhhbXBsZS5jb20wHhcNMjExMjIwMTQ1NzM1
4+
WhcNMzExMjE4MTQ1NzM1WjAvMRIwEAYDVQQHDAlTb21ld2hlcmUxGTAXBgNVBAMM
5+
EGV2aWwuZXhhbXBsZS5jb20wggIiMA0GCSqGSIb3DQEBAQUAA4ICDwAwggIKAoIC
6+
AQCxEWd00u9E9T/ko6WcCKjhZ7tjnfVylnA7M0EHOwvdivgD46eAb1omsonLagiV
7+
rZrG7EpYuMhtz+g3Yv1d0nvFvv8ge9UIdnN8EDTDzLpJ3KbNqHURraiXuBDqa3rd
8+
Y4JBakCcuYHl1bj1OTew7xl1FWc1je04rBTQGTFIRdmJZYyc9bIw9WkY6msod0w1
9+
PDcLhZS3emh/eYaL4zAQWrVhQfWzf4rZzFaI/a5n0o75aUkTuvxDDQ51V2d6WkSU
10+
3KbOnf2JW+QJXfzsNOeiYA9AnfY59evr4GEeG8VZdGuUG39uDCIWmAUT8elhXXqV
11+
q+NdBqc6VUNLDJbqCmMx/Ecp48EHO6X5uXm0xViZIVPNIzqiiRhVt4nFfwPQZrTg
12+
aq2+tD7/zD1yED4O1FhlDl5twH2N7+oG06HsEluQdLPrj7IedpneGVKMs078Ddov
13+
7j6icYv/RZHVetDlrzDDHjLJWwxyAWzdGdkhtMGPd6B9i4TtF/PU3J3nbpLn5XfE
14+
BFu4jJ+w+5Wvk5a60gF1ERy/OLBM/e8sro2sEBIpp1tN1wJVBZOtTIi4VVDhwDRQ
15+
Uiwb2d1Re7GQ7+mcz5D/01qxW6S+w0IKrpwJUjR3mpa0OU98KfKVJkeyeEBLkEhD
16+
dnGTDqZ9E/ickGosrW2gAAYKgzXk725dpxTdpLEosfDbpwIDAQABMA0GCSqGSIb3
17+
DQEBCwUAA4ICAQAnszSuVqfEmpjf2VMvk9TUuiop0tejHP+hB30IURJqA9K51edx
18+
IRszXXU4Sj8uHT88RpKxgDm/GcfEA0l2rWZ6Mal6pmUyjteJJPMVA6fgeNM8XvtJ
19+
eoxi2wm8FzxXJrPK7fOMG5/fLb7ENUZYFRHVFJ+Gk290DP7x81Gzb5tcsolrVqW+
20+
TZdV2aBZya28NjgXncjinIlD61I6LzoQbDInab5nEPKMRuRTXMLfbAypXrPAbsfz
21+
+Z6ZKhfNEo0/5cI4iG8MQXM1HgbFCkWOTPPeR53lo+1f9dN3IZ+1PYUjkOJzuxUZ
22+
HIA+Dy+S1ocfK582LqohexhjeC5AL74rJJcgns9ORxz2GN1buIRTzi9XL2egp7cd
23+
+XgZ3phpY4mIM0bH+DJ7eIqkM17WkEwJ3vazu7tEmIldc06Pmt2vFEcQB3T0bsw7
24+
lBZdwSEkqTb+IexaQerSyztuxKc2DhOLTqZfVPCd2LWhasNSHzGmanI3vmBy98MN
25+
LZzo7+G1BDMyMsl3DwEiwOGYARXJklU1LxCj6nVCTymNToLXtF2xHcZuK94Pqol9
26+
n8zMCUYNOr7USWA25GwfpN65UHN7YXsOl9XIMWl+iVA5QepAI9sL0n3CyFW0ZXgn
27+
DsZkfikYa+xhQSUANV4zDx1X8FxZmT0Op/+mhkvwL1+YKUHJy3WdXrIFgw==
28+
-----END CERTIFICATE-----

‎test/fixtures/x509-escaping/subj-1-cert.pem

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
-----BEGIN CERTIFICATE-----
2+
MIIEwzCCAqugAwIBAgIBATANBgkqhkiG9w0BAQsFADAlMSMwIQYDVQQHDBpTb21l
3+
d2hlcmUAZXZpbC5leGFtcGxlLmNvbTAeFw0yMTEyMjAxNDU3MzVaFw0zMTEyMTgx
4+
NDU3MzVaMCUxIzAhBgNVBAcMGlNvbWV3aGVyZQBldmlsLmV4YW1wbGUuY29tMIIC
5+
IjANBgkqhkiG9w0BAQEFAAOCAg8AMIICCgKCAgEAsRFndNLvRPU/5KOlnAio4We7
6+
Y531cpZwOzNBBzsL3Yr4A+OngG9aJrKJy2oIla2axuxKWLjIbc/oN2L9XdJ7xb7/
7+
IHvVCHZzfBA0w8y6Sdymzah1Ea2ol7gQ6mt63WOCQWpAnLmB5dW49Tk3sO8ZdRVn
8+
NY3tOKwU0BkxSEXZiWWMnPWyMPVpGOprKHdMNTw3C4WUt3pof3mGi+MwEFq1YUH1
9+
s3+K2cxWiP2uZ9KO+WlJE7r8Qw0OdVdnelpElNymzp39iVvkCV387DTnomAPQJ32
10+
OfXr6+BhHhvFWXRrlBt/bgwiFpgFE/HpYV16lavjXQanOlVDSwyW6gpjMfxHKePB
11+
Bzul+bl5tMVYmSFTzSM6ookYVbeJxX8D0Ga04GqtvrQ+/8w9chA+DtRYZQ5ebcB9
12+
je/qBtOh7BJbkHSz64+yHnaZ3hlSjLNO/A3aL+4+onGL/0WR1XrQ5a8wwx4yyVsM
13+
cgFs3RnZIbTBj3egfYuE7Rfz1Nyd526S5+V3xARbuIyfsPuVr5OWutIBdREcvziw
14+
TP3vLK6NrBASKadbTdcCVQWTrUyIuFVQ4cA0UFIsG9ndUXuxkO/pnM+Q/9NasVuk
15+
vsNCCq6cCVI0d5qWtDlPfCnylSZHsnhAS5BIQ3Zxkw6mfRP4nJBqLK1toAAGCoM1
16+
5O9uXacU3aSxKLHw26cCAwEAATANBgkqhkiG9w0BAQsFAAOCAgEAmjKOoKxLwPY4
17+
e65pYTUSBctPZ2juW5uNs8UvH5O32OC9RhENJBIIKn3B9Z/wkexR2zcvaQmJObLW
18+
6mkR7O0tNgsXVYJFzLRBfjM/nyP6nafiCUekmoh9Kojq6x5IQQgEsK+Uw123kkoI
19+
w/h3hBYBq8+CFPnYtBLZBVVFMNGaATXrYJPCcjVrtAHYxIWaDN2R+1DWLRIV72sF
20+
hu4xGz0kmUbzforl/FA3gdgM7mwfZMF4+EoQZi5mShdWnyfzAHIbtahnA4lPNtx9
21+
vBqYIZ/a2ITsXmWc2KGs/rRG+SDLzg+H1Xudvu/y2d1ULpZQfT6bg6Ro855FiU9h
22+
TyHHQGGqlC9/DjHy//wERsFEJZh5/j21LGyalEjgfOYtzPkjZlIweYr8LlHTrauo
23+
/gWihriaaWAkD+2fwQ09CUHdvOG6yoT+j/E50FsekfqV3tKMwoZoph6dF1TWQg32
24+
JXV0akpd5ff1cca8sZgJfUksDfSkrwG7fl3tje30vQTlvNrhu2MCKFGQwyXed3qg
25+
86lx+sTZjxMYvqWWysKTx8aIJ95XAK2jJ2OEVI2X6cdgoAp6aMkycbttik4hDoPJ
26+
eAWaZo2UFs2MGoUbX9m4RzPqPuBHNFqoV6yRyS1K/3KWyxVVvamZY0Qgzmoi4coB
27+
hRlTO6GDkF7u1YQ7eZi7pP7U8OcklfE=
28+
-----END CERTIFICATE-----

0 commit comments

Comments
 (0)