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>

2 files changed, 49 insertions, 17 deletions

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 @@ int i2a_ASN1_OBJECT(BIO *bp, const ASN1_OBJECT *a)
 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 (inf & V_ASN1_CONSTRUCTED) {
+        OPENSSL_PUT_ERROR(ASN1, ASN1_R_TYPE_NOT_PRIMITIVE);
+        return NULL;
     }
 
-    if (tag != V_ASN1_OBJECT) {
-        i = ASN1_R_EXPECTING_AN_OBJECT;
-        goto err;
+    if (tag != V_ASN1_OBJECT || xclass != V_ASN1_UNIVERSAL) {
+        OPENSSL_PUT_ERROR(ASN1, ASN1_R_EXPECTING_AN_OBJECT);
+        return NULL;
     }
-    ret = c2i_ASN1_OBJECT(a, &p, len);
-    if (ret)
+    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 @@ TEST(ASN1Test, UnusedBooleanBits) {
   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 @@ TEST(ASN1Test, ASN1ObjectReuse) {
   // 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) {