Reimplement ASN1_get_object with CBS.

Now we only have one BER/DER TLV parser. Annoyingly, this uses the CBS
BER function, not the DER one. This is because Android sometimes needs
allow a non-minimal length in certificate signature fields (see
b/18228011).

For now, this CL calls CBS_get_any_ber_asn1_element. This is still an
improvement over the old parser because we'll reject non-minimal tags
(which are actually even forbidden in BER). Later, we should move the
special case to just the signature field, and ultimately to a
preprocessing step specific to that part of Android.

Update-Note: Invalid certificates (and the few external structures using
asn1t.h) with incorrectly-encoded tags will now be rejected.

Bug: 354
Change-Id: I56a7faa1ffd51ee38cc315ebaddaef98079fd90e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51626
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

crypto/asn1/asn1_lib.c

@@ -59,7 +59,7 @@
 #include <limits.h>
 #include <string.h>
 
-#include <openssl/asn1_mac.h>
+#include <openssl/bytestring.h>
 #include <openssl/err.h>
 #include <openssl/mem.h>
 
@@ -104,101 +104,54 @@ OPENSSL_DECLARE_ERROR_REASON(ASN1, UNKNOWN_FORMAT)
 OPENSSL_DECLARE_ERROR_REASON(ASN1, UNKNOWN_TAG)
 OPENSSL_DECLARE_ERROR_REASON(ASN1, UNSUPPORTED_TYPE)
 
-static int asn1_get_length(const unsigned char **pp, long *rl, long max);
 static void asn1_put_length(unsigned char **pp, int length);
 
-int ASN1_get_object(const unsigned char **pp, long *plength, int *ptag,
-                    int *pclass, long omax)
+int ASN1_get_object(const unsigned char **inp, long *out_len, int *out_tag,
+                    int *out_class, long in_len)
 {
-    int i, ret;
-    long l;
-    const unsigned char *p = *pp;
-    int tag, xclass;
-    long max = omax;
-
-    if (!max)
-        goto err;
-    ret = (*p & V_ASN1_CONSTRUCTED);
-    xclass = (*p & V_ASN1_PRIVATE);
-    i = *p & V_ASN1_PRIMITIVE_TAG;
-    if (i == V_ASN1_PRIMITIVE_TAG) { /* high-tag */
-        p++;
-        if (--max == 0)
-            goto err;
-        l = 0;
-        while (*p & 0x80) {
-            l <<= 7L;
-            l |= *(p++) & 0x7f;
-            if (--max == 0)
-                goto err;
-            if (l > (INT_MAX >> 7L))
-                goto err;
-        }
-        l <<= 7L;
-        l |= *(p++) & 0x7f;
-        tag = (int)l;
-        if (--max == 0)
-            goto err;
-    } else {
-        tag = i;
-        p++;
-        if (--max == 0)
-            goto err;
+    if (in_len < 0) {
+        OPENSSL_PUT_ERROR(ASN1, ASN1_R_HEADER_TOO_LONG);
+        return 0x80;
     }
 
-    /* To avoid ambiguity with V_ASN1_NEG, impose a limit on universal tags. */
-    if (xclass == V_ASN1_UNIVERSAL && tag > V_ASN1_MAX_UNIVERSAL)
-        goto err;
-
-    *ptag = tag;
-    *pclass = xclass;
-    if (!asn1_get_length(&p, plength, max))
-        goto err;
-
-    if (*plength > (omax - (p - *pp))) {
-        OPENSSL_PUT_ERROR(ASN1, ASN1_R_TOO_LONG);
+    /* TODO(https://crbug.com/boringssl/354): This should use |CBS_get_asn1| to
+     * reject non-minimal lengths, which are only allowed in BER. However,
+     * Android sometimes needs allow a non-minimal length in certificate
+     * signature fields (see b/18228011). Make this only apply to that field,
+     * while requiring DER elsewhere. Better yet, it should be limited to an
+     * preprocessing step in that part of Android. */
+    unsigned tag;
+    size_t header_len;
+    int indefinite;
+    CBS cbs, body;
+    CBS_init(&cbs, *inp, (size_t)in_len);
+    if (!CBS_get_any_ber_asn1_element(&cbs, &body, &tag, &header_len,
+        /*out_ber_found=*/NULL, &indefinite) ||
+        indefinite ||
+        !CBS_skip(&body, header_len) ||
+        /* Bound the length to comfortably fit in an int. Lengths in this
+         * module often switch between int and long without overflow checks. */
+        CBS_len(&body) > INT_MAX / 2) {
+        OPENSSL_PUT_ERROR(ASN1, ASN1_R_HEADER_TOO_LONG);
         return 0x80;
     }
-    *pp = p;
-    return ret;
- err:
-    OPENSSL_PUT_ERROR(ASN1, ASN1_R_HEADER_TOO_LONG);
-    return 0x80;
-}
 
-static int asn1_get_length(const unsigned char **pp, long *rl, long max)
-{
-    const unsigned char *p = *pp;
-    unsigned long ret = 0;
-    unsigned long i;
+    /* Convert between tag representations. */
+    int tag_class = (tag & CBS_ASN1_CLASS_MASK) >> CBS_ASN1_TAG_SHIFT;
+    int constructed = (tag & CBS_ASN1_CONSTRUCTED) >> CBS_ASN1_TAG_SHIFT;
+    int tag_number = tag & CBS_ASN1_TAG_NUMBER_MASK;
 
-    if (max-- < 1) {
-        return 0;
-    }
-    if (*p == 0x80) {
-        /* We do not support BER indefinite-length encoding. */
-        return 0;
-    }
-    i = *p & 0x7f;
-    if (*(p++) & 0x80) {
-        if (i > sizeof(ret) || max < (long)i)
-            return 0;
-        while (i-- > 0) {
-            ret <<= 8L;
-            ret |= *(p++);
-        }
-    } else {
-        ret = i;
+    /* To avoid ambiguity with V_ASN1_NEG, impose a limit on universal tags. */
+    if (tag_class == V_ASN1_UNIVERSAL && tag_number > V_ASN1_MAX_UNIVERSAL) {
+        OPENSSL_PUT_ERROR(ASN1, ASN1_R_HEADER_TOO_LONG);
+        return 0x80;
     }
-    /*
-     * Bound the length to comfortably fit in an int. Lengths in this module
-     * often switch between int and long without overflow checks.
-     */
-    if (ret > INT_MAX / 2)
-        return 0;
-    *pp = p;
-    *rl = (long)ret;
-    return 1;
+
+    *inp = CBS_data(&body);
+    *out_len = CBS_len(&body);
+    *out_tag = tag_number;
+    *out_class = tag_class;
+    return constructed;
 }
 
 /*

crypto/x509/x509_test.cc

@@ -3524,6 +3524,20 @@ BwIgfB55FGohg/B6dGh5XxSZmmi08cueFV7mHzJSYV51yRQB
 -----END CERTIFICATE-----
 )";
 
+// kHighTagNumber is an X.509 certificate where the outermost SEQUENCE tag uses
+// high tag number form.
+static const char kHighTagNumber[] = R"(
+-----BEGIN CERTIFICATE-----
+PxCCASAwgcagAwIBAgICBNIwCgYIKoZIzj0EAwIwDzENMAsGA1UEAxMEVGVzdDAg
+Fw0wMDAxMDEwMDAwMDBaGA8yMTAwMDEwMTAwMDAwMFowDzENMAsGA1UEAxMEVGVz
+dDBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABOYraeK/ZZ+Xvi8eDZSKTNWXa7ep
+Hg1G+92pqR6d3LpaAefWl6gKGPnDxKMeVuJ8g0jbFhoc9R1+8ZQtS89yIsGjEDAO
+MAwGA1UdEwQFMAMBAf8wCgYIKoZIzj0EAwIDSQAwRgIhAKnSIhfmzfQpeOKFHiAq
+cml3ex6oaVVGoJWCsPQoZjVAAiEAqTHS9HzZBTQ20cMPXUpf8u5AXZP7adeh4qnk
+soBsxWI=
+-----END CERTIFICATE-----
+)";
+
 TEST(X509Test, BER) {
   // Constructed strings are forbidden in DER.
   EXPECT_FALSE(CertFromPEM(kConstructedBitString));
@@ -3532,6 +3546,9 @@ TEST(X509Test, BER) {
   EXPECT_FALSE(CertFromPEM(kIndefiniteLength));
   // Padding bits in BIT STRINGs must be zero in BER.
   EXPECT_FALSE(CertFromPEM(kNonZeroPadding));
+  // Tags must be minimal in both BER and DER, though many BER decoders
+  // incorrectly support non-minimal tags.
+  EXPECT_FALSE(CertFromPEM(kHighTagNumber));
 }
 
 TEST(X509Test, Names) {