Check tag class and constructed bit in d2i_ASN1_OBJECT.
d2i_ASN1_OBJECT had a similar set of bugs in as in
https://boringssl-review.googlesource.com/c/boringssl/+/49866.
This does not affect any other d2i functions. Those already go through
the ASN1_ITEM machinery.
Update-Note: d2i_ASN1_OBJECT will now notice more incorrect tags. It was
already checking for tag number 6, so it is unlikely anyone was relying
on this as a non-tag-checking parser.
Change-Id: I30f9ad28e3859aeb7a38c0ea299cd2e30002abce
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50290
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/asn1/a_object.c b/crypto/asn1/a_object.c
index 0de3141..30a656d 100644
--- a/crypto/asn1/a_object.c
+++ b/crypto/asn1/a_object.c
@@ -146,29 +146,29 @@
ASN1_OBJECT *d2i_ASN1_OBJECT(ASN1_OBJECT **a, const unsigned char **pp,
long length)
{
- const unsigned char *p;
long len;
int tag, xclass;
- int inf, i;
- ASN1_OBJECT *ret = NULL;
- p = *pp;
- inf = ASN1_get_object(&p, &len, &tag, &xclass, length);
+ const unsigned char *p = *pp;
+ int inf = ASN1_get_object(&p, &len, &tag, &xclass, length);
if (inf & 0x80) {
- i = ASN1_R_BAD_OBJECT_HEADER;
- goto err;
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_BAD_OBJECT_HEADER);
+ return NULL;
}
- if (tag != V_ASN1_OBJECT) {
- i = ASN1_R_EXPECTING_AN_OBJECT;
- goto err;
+ if (inf & V_ASN1_CONSTRUCTED) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_TYPE_NOT_PRIMITIVE);
+ return NULL;
}
- ret = c2i_ASN1_OBJECT(a, &p, len);
- if (ret)
+
+ if (tag != V_ASN1_OBJECT || xclass != V_ASN1_UNIVERSAL) {
+ OPENSSL_PUT_ERROR(ASN1, ASN1_R_EXPECTING_AN_OBJECT);
+ return NULL;
+ }
+ ASN1_OBJECT *ret = c2i_ASN1_OBJECT(a, &p, len);
+ if (ret) {
*pp = p;
+ }
return ret;
- err:
- OPENSSL_PUT_ERROR(ASN1, i);
- return (NULL);
}
ASN1_OBJECT *c2i_ASN1_OBJECT(ASN1_OBJECT **a, const unsigned char **pp,
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc
index 1b0c11c..ab9cb01 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -266,7 +266,7 @@
EXPECT_FALSE(val->value.ptr);
}
-TEST(ASN1Test, ASN1ObjectReuse) {
+TEST(ASN1Test, ParseASN1Object) {
// 1.2.840.113554.4.1.72585.2, an arbitrary unknown OID.
static const uint8_t kOID[] = {0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12,
0x04, 0x01, 0x84, 0xb7, 0x09, 0x02};
@@ -277,16 +277,48 @@
// OBJECT_IDENTIFIER { 1.3.101.112 }
static const uint8_t kDER[] = {0x06, 0x03, 0x2b, 0x65, 0x70};
const uint8_t *ptr = kDER;
+ // Parse an |ASN1_OBJECT| with object reuse.
EXPECT_TRUE(d2i_ASN1_OBJECT(&obj, &ptr, sizeof(kDER)));
EXPECT_EQ(NID_ED25519, OBJ_obj2nid(obj));
ASN1_OBJECT_free(obj);
- // Repeat the test, this time overriding a static |ASN1_OBJECT|.
+ // Repeat the test, this time overriding a static |ASN1_OBJECT|. It should
+ // detect this and construct a new one.
obj = OBJ_nid2obj(NID_rsaEncryption);
ptr = kDER;
EXPECT_TRUE(d2i_ASN1_OBJECT(&obj, &ptr, sizeof(kDER)));
EXPECT_EQ(NID_ED25519, OBJ_obj2nid(obj));
ASN1_OBJECT_free(obj);
+
+ const std::vector<uint8_t> kInvalidObjects[] = {
+ // No tag header.
+ {},
+ // No length.
+ {0x06},
+ // Truncated contents.
+ {0x06, 0x01},
+ // An OID may not be empty.
+ {0x06, 0x00},
+ // The last byte may not be a continuation byte (high bit set).
+ {0x06, 0x03, 0x2b, 0x65, 0xf0},
+ // Each component must be minimally-encoded.
+ {0x06, 0x03, 0x2b, 0x65, 0x80, 0x70},
+ {0x06, 0x03, 0x80, 0x2b, 0x65, 0x70},
+ // Wrong tag number.
+ {0x01, 0x03, 0x2b, 0x65, 0x70},
+ // Wrong tag class.
+ {0x86, 0x03, 0x2b, 0x65, 0x70},
+ // Element is constructed.
+ {0x26, 0x03, 0x2b, 0x65, 0x70},
+ };
+ for (const auto &invalid : kInvalidObjects) {
+ SCOPED_TRACE(Bytes(invalid));
+ ptr = invalid.data();
+ obj = d2i_ASN1_OBJECT(nullptr, &ptr, invalid.size());
+ EXPECT_FALSE(obj);
+ ASN1_OBJECT_free(obj);
+ ERR_clear_error();
+ }
}
TEST(ASN1Test, BitString) {
