diff --git a/crypto/asn1/asn1_lib.c b/crypto/asn1/asn1_lib.c index fbf4d6866..edf5e7c50 100644 --- a/crypto/asn1/asn1_lib.c +++ b/crypto/asn1/asn1_lib.c @@ -59,7 +59,7 @@ #include #include -#include +#include #include #include @@ -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; } /* diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 38414e99f..48b83da3a 100644 --- a/crypto/x509/x509_test.cc +++ b/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) {