Fix EDIPartyName parsing and GENERAL_NAME_cmp.
Cherry pick note: Fix for rvc-qpr-dev is required (see bug
for details). Note that this branch is code-frozen due to
FIPS certification, but my understanding is that security
fixes trump that, but that's why I've included the minimal
fix from BoringSSL rather than patching the roll-up CL
from master in aosp/1553538.
See also CVE-2020-1971, f960d81215ebf3f65e03d4d5d857fb9b666d6920, and
aa0ad2011d3e7ad8a611da274ef7d9c7706e289b from upstream OpenSSL.
Unlike upstream's version, this CL opts for a simpler edipartyname_cmp.
GENERAL_NAME_cmp is already unsuitable for ordering, just equality,
which means there's no need to preserve return values from
ASN1_STRING_cmp. Additionally, the ASN.1 structure implies most fields
cannot be NULL.
(The change from other to x400Address is a no-op. They're the same type.
Just x400Address is a little clearer. Historical quirks of the
GENERAL_NAME structure.)
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44404
Bug: 175147055
Test: atest boringssl_crypto_test boringssl_ssl_test
Change-Id: Ieffd0cde14d1f93f9ad6a884609ed631b891599b
Merged-In: I1fb4105341a73be9d5f978301f7318e16027f37d
diff --git a/src/crypto/x509/x509_test.cc b/src/crypto/x509/x509_test.cc
index 521d757..5a31c56 100644
--- a/src/crypto/x509/x509_test.cc
+++ b/src/crypto/x509/x509_test.cc
@@ -2226,3 +2226,182 @@
EXPECT_EQ(X509_V_OK, verify_cert(leaf));
}
}
+
+TEST(X509Test, GeneralName) {
+ const std::vector<uint8_t> kNames[] = {
+ // [0] {
+ // OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.2.1 }
+ // [0] {
+ // SEQUENCE {}
+ // }
+ // }
+ {0xa0, 0x13, 0x06, 0x0d, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04,
+ 0x01, 0x84, 0xb7, 0x09, 0x02, 0x01, 0xa0, 0x02, 0x30, 0x00},
+ // [0] {
+ // OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.2.1 }
+ // [0] {
+ // [APPLICATION 0] {}
+ // }
+ // }
+ {0xa0, 0x13, 0x06, 0x0d, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04,
+ 0x01, 0x84, 0xb7, 0x09, 0x02, 0x01, 0xa0, 0x02, 0x60, 0x00},
+ // [0] {
+ // OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.2.1 }
+ // [0] {
+ // UTF8String { "a" }
+ // }
+ // }
+ {0xa0, 0x14, 0x06, 0x0d, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04,
+ 0x01, 0x84, 0xb7, 0x09, 0x02, 0x01, 0xa0, 0x03, 0x0c, 0x01, 0x61},
+ // [0] {
+ // OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.2.2 }
+ // [0] {
+ // UTF8String { "a" }
+ // }
+ // }
+ {0xa0, 0x14, 0x06, 0x0d, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04,
+ 0x01, 0x84, 0xb7, 0x09, 0x02, 0x02, 0xa0, 0x03, 0x0c, 0x01, 0x61},
+ // [0] {
+ // OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.2.1 }
+ // [0] {
+ // UTF8String { "b" }
+ // }
+ // }
+ {0xa0, 0x14, 0x06, 0x0d, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04,
+ 0x01, 0x84, 0xb7, 0x09, 0x02, 0x01, 0xa0, 0x03, 0x0c, 0x01, 0x62},
+ // [0] {
+ // OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.2.1 }
+ // [0] {
+ // BOOLEAN { TRUE }
+ // }
+ // }
+ {0xa0, 0x14, 0x06, 0x0d, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04,
+ 0x01, 0x84, 0xb7, 0x09, 0x02, 0x01, 0xa0, 0x03, 0x01, 0x01, 0xff},
+ // [0] {
+ // OBJECT_IDENTIFIER { 1.2.840.113554.4.1.72585.2.1 }
+ // [0] {
+ // BOOLEAN { FALSE }
+ // }
+ // }
+ {0xa0, 0x14, 0x06, 0x0d, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04,
+ 0x01, 0x84, 0xb7, 0x09, 0x02, 0x01, 0xa0, 0x03, 0x01, 0x01, 0x00},
+ // [1 PRIMITIVE] { "a" }
+ {0x81, 0x01, 0x61},
+ // [1 PRIMITIVE] { "b" }
+ {0x81, 0x01, 0x62},
+ // [2 PRIMITIVE] { "a" }
+ {0x82, 0x01, 0x61},
+ // [2 PRIMITIVE] { "b" }
+ {0x82, 0x01, 0x62},
+ // [4] {
+ // SEQUENCE {
+ // SET {
+ // SEQUENCE {
+ // # commonName
+ // OBJECT_IDENTIFIER { 2.5.4.3 }
+ // UTF8String { "a" }
+ // }
+ // }
+ // }
+ // }
+ {0xa4, 0x0e, 0x30, 0x0c, 0x31, 0x0a, 0x30, 0x08, 0x06, 0x03, 0x55, 0x04,
+ 0x03, 0x0c, 0x01, 0x61},
+ // [4] {
+ // SEQUENCE {
+ // SET {
+ // SEQUENCE {
+ // # commonName
+ // OBJECT_IDENTIFIER { 2.5.4.3 }
+ // UTF8String { "b" }
+ // }
+ // }
+ // }
+ // }
+ {0xa4, 0x0e, 0x30, 0x0c, 0x31, 0x0a, 0x30, 0x08, 0x06, 0x03, 0x55, 0x04,
+ 0x03, 0x0c, 0x01, 0x62},
+ // [5] {
+ // [1] {
+ // UTF8String { "a" }
+ // }
+ // }
+ {0xa5, 0x05, 0xa1, 0x03, 0x0c, 0x01, 0x61},
+ // [5] {
+ // [1] {
+ // UTF8String { "b" }
+ // }
+ // }
+ {0xa5, 0x05, 0xa1, 0x03, 0x0c, 0x01, 0x62},
+ // [5] {
+ // [0] {
+ // UTF8String {}
+ // }
+ // [1] {
+ // UTF8String { "a" }
+ // }
+ // }
+ {0xa5, 0x09, 0xa0, 0x02, 0x0c, 0x00, 0xa1, 0x03, 0x0c, 0x01, 0x61},
+ // [5] {
+ // [0] {
+ // UTF8String { "a" }
+ // }
+ // [1] {
+ // UTF8String { "a" }
+ // }
+ // }
+ {0xa5, 0x0a, 0xa0, 0x03, 0x0c, 0x01, 0x61, 0xa1, 0x03, 0x0c, 0x01, 0x61},
+ // [5] {
+ // [0] {
+ // UTF8String { "b" }
+ // }
+ // [1] {
+ // UTF8String { "a" }
+ // }
+ // }
+ {0xa5, 0x0a, 0xa0, 0x03, 0x0c, 0x01, 0x62, 0xa1, 0x03, 0x0c, 0x01, 0x61},
+ // [6 PRIMITIVE] { "a" }
+ {0x86, 0x01, 0x61},
+ // [6 PRIMITIVE] { "b" }
+ {0x86, 0x01, 0x62},
+ // [7 PRIMITIVE] { `11111111` }
+ {0x87, 0x04, 0x11, 0x11, 0x11, 0x11},
+ // [7 PRIMITIVE] { `22222222`}
+ {0x87, 0x04, 0x22, 0x22, 0x22, 0x22},
+ // [7 PRIMITIVE] { `11111111111111111111111111111111` }
+ {0x87, 0x10, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11,
+ 0x11, 0x11, 0x11, 0x11, 0x11, 0x11},
+ // [7 PRIMITIVE] { `22222222222222222222222222222222` }
+ {0x87, 0x10, 0x22, 0x22, 0x22, 0x22, 0x22, 0x22, 0x22, 0x22, 0x22, 0x22,
+ 0x22, 0x22, 0x22, 0x22, 0x22, 0x22},
+ // [8 PRIMITIVE] { 1.2.840.113554.4.1.72585.2.1 }
+ {0x88, 0x0d, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04, 0x01, 0x84, 0xb7,
+ 0x09, 0x02, 0x01},
+ // [8 PRIMITIVE] { 1.2.840.113554.4.1.72585.2.2 }
+ {0x88, 0x0d, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x04, 0x01, 0x84, 0xb7,
+ 0x09, 0x02, 0x02},
+ };
+
+ // Every name should be equal to itself and not equal to any others.
+ for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(kNames); i++) {
+ SCOPED_TRACE(Bytes(kNames[i]));
+
+ const uint8_t *ptr = kNames[i].data();
+ bssl::UniquePtr<GENERAL_NAME> a(
+ d2i_GENERAL_NAME(nullptr, &ptr, kNames[i].size()));
+ ASSERT_TRUE(a);
+
+ for (size_t j = 0; j < OPENSSL_ARRAY_SIZE(kNames); j++) {
+ SCOPED_TRACE(Bytes(kNames[j]));
+
+ ptr = kNames[j].data();
+ bssl::UniquePtr<GENERAL_NAME> b(
+ d2i_GENERAL_NAME(nullptr, &ptr, kNames[j].size()));
+ ASSERT_TRUE(b);
+
+ if (i == j) {
+ EXPECT_EQ(GENERAL_NAME_cmp(a.get(), b.get()), 0);
+ } else {
+ EXPECT_NE(GENERAL_NAME_cmp(a.get(), b.get()), 0);
+ }
+ }
+ }
+}
diff --git a/src/crypto/x509v3/v3_genn.c b/src/crypto/x509v3/v3_genn.c
index 552a524..8d6fbe2 100644
--- a/src/crypto/x509v3/v3_genn.c
+++ b/src/crypto/x509v3/v3_genn.c
@@ -72,8 +72,9 @@
IMPLEMENT_ASN1_FUNCTIONS(OTHERNAME)
ASN1_SEQUENCE(EDIPARTYNAME) = {
- ASN1_IMP_OPT(EDIPARTYNAME, nameAssigner, DIRECTORYSTRING, 0),
- ASN1_IMP_OPT(EDIPARTYNAME, partyName, DIRECTORYSTRING, 1)
+ /* DirectoryString is a CHOICE type, so use explicit tagging. */
+ ASN1_EXP_OPT(EDIPARTYNAME, nameAssigner, DIRECTORYSTRING, 0),
+ ASN1_EXP(EDIPARTYNAME, partyName, DIRECTORYSTRING, 1)
} ASN1_SEQUENCE_END(EDIPARTYNAME)
IMPLEMENT_ASN1_FUNCTIONS(EDIPARTYNAME)
@@ -102,6 +103,24 @@
IMPLEMENT_ASN1_DUP_FUNCTION(GENERAL_NAME)
+static int edipartyname_cmp(const EDIPARTYNAME *a, const EDIPARTYNAME *b)
+{
+ /* nameAssigner is optional and may be NULL. */
+ if (a->nameAssigner == NULL) {
+ if (b->nameAssigner != NULL) {
+ return -1;
+ }
+ } else {
+ if (b->nameAssigner == NULL ||
+ ASN1_STRING_cmp(a->nameAssigner, b->nameAssigner) != 0) {
+ return -1;
+ }
+ }
+
+ /* partyName may not be NULL. */
+ return ASN1_STRING_cmp(a->partyName, b->partyName);
+}
+
/* Returns 0 if they are equal, != 0 otherwise. */
int GENERAL_NAME_cmp(GENERAL_NAME *a, GENERAL_NAME *b)
{
@@ -111,8 +130,11 @@
return -1;
switch (a->type) {
case GEN_X400:
+ result = ASN1_TYPE_cmp(a->d.x400Address, b->d.x400Address);
+ break;
+
case GEN_EDIPARTY:
- result = ASN1_TYPE_cmp(a->d.other, b->d.other);
+ result = edipartyname_cmp(a->d.ediPartyName, b->d.ediPartyName);
break;
case GEN_OTHERNAME:
@@ -159,8 +181,11 @@
{
switch (type) {
case GEN_X400:
+ a->d.x400Address = value;
+ break;
+
case GEN_EDIPARTY:
- a->d.other = value;
+ a->d.ediPartyName = value;
break;
case GEN_OTHERNAME:
@@ -194,8 +219,10 @@
*ptype = a->type;
switch (a->type) {
case GEN_X400:
+ return a->d.x400Address;
+
case GEN_EDIPARTY:
- return a->d.other;
+ return a->d.ediPartyName;
case GEN_OTHERNAME:
return a->d.otherName;
diff --git a/src/include/openssl/x509v3.h b/src/include/openssl/x509v3.h
index b5db715..4b0ae2f 100644
--- a/src/include/openssl/x509v3.h
+++ b/src/include/openssl/x509v3.h
@@ -534,6 +534,12 @@
DECLARE_ASN1_FUNCTIONS(GENERAL_NAME)
OPENSSL_EXPORT GENERAL_NAME *GENERAL_NAME_dup(GENERAL_NAME *a);
+
+// GENERAL_NAME_cmp returns zero if |a| and |b| are equal and a non-zero
+// value otherwise. Note this function does not provide a comparison suitable
+// for sorting.
+//
+// TODO(davidben): Const-correct this function.
OPENSSL_EXPORT int GENERAL_NAME_cmp(GENERAL_NAME *a, GENERAL_NAME *b);
