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>
diff --git a/crypto/asn1/asn1_lib.c b/crypto/asn1/asn1_lib.c
index fbf4d68..edf5e7c 100644
--- a/crypto/asn1/asn1_lib.c
+++ b/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_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;
- }
-
- /* 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);
+ if (in_len < 0) {
+ 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;
+ /* 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;
+ }
- if (max-- < 1) {
- return 0;
+ /* 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;
+
+ /* 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;
}
- 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;
- }
- /*
- * 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;
}
/*
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 38414e9..48b83da 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -3524,6 +3524,20 @@
-----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 @@
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) {
