Skip to content

Commit

Permalink
tls: fix handling of x509 subject and issuer
Browse files Browse the repository at this point in the history
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.

CVE-ID: CVE-2021-44533
PR-URL: nodejs-private/node-private#300
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
  • Loading branch information
tniessen authored and richardlau committed Jan 10, 2022
1 parent 50439b4 commit a336444
Show file tree
Hide file tree
Showing 16 changed files with 656 additions and 13 deletions.
6 changes: 4 additions & 2 deletions lib/_tls_common.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
128 changes: 119 additions & 9 deletions src/crypto/crypto_common.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down Expand Up @@ -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()));
}
Expand Down Expand Up @@ -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)) ||
Expand Down
3 changes: 2 additions & 1 deletion src/crypto/crypto_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion src/crypto/crypto_x509.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
141 changes: 141 additions & 0 deletions test/fixtures/x509-escaping/create-certs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`);
}
28 changes: 28 additions & 0 deletions test/fixtures/x509-escaping/subj-0-cert.pem
Original file line number Diff line number Diff line change
@@ -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-----
28 changes: 28 additions & 0 deletions test/fixtures/x509-escaping/subj-1-cert.pem
Original file line number Diff line number Diff line change
@@ -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-----
Loading

0 comments on commit a336444

Please sign in to comment.