diff --git a/crypto/asn1/asn1_lib.c b/crypto/asn1/asn1_lib.c index 3b97b38e0..dd56c9890 100644 --- a/crypto/asn1/asn1_lib.c +++ b/crypto/asn1/asn1_lib.c @@ -111,20 +111,10 @@ int ASN1_get_object(const unsigned char **inp, long *out_len, int *out_tag, return 0x80; } - // 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. CBS_ASN1_TAG 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) || + if (!CBS_get_any_asn1(&cbs, &body, &tag) || // 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) { diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc index 142146200..cd26528db 100644 --- a/crypto/asn1/asn1_test.cc +++ b/crypto/asn1/asn1_test.cc @@ -2235,10 +2235,24 @@ TEST(ASN1Test, GetObject) { EXPECT_EQ(0x80, ASN1_get_object(&ptr, &length, &tag, &tag_class, sizeof(kTruncated))); + // Indefinite-length encoding is not allowed in DER. static const uint8_t kIndefinite[] = {0x30, 0x80, 0x00, 0x00}; ptr = kIndefinite; EXPECT_EQ(0x80, ASN1_get_object(&ptr, &length, &tag, &tag_class, sizeof(kIndefinite))); + + // DER requires lengths be minimally-encoded. This should be {0x30, 0x00}. + static const uint8_t kNonMinimal[] = {0x30, 0x81, 0x00}; + ptr = kNonMinimal; + EXPECT_EQ(0x80, ASN1_get_object(&ptr, &length, &tag, &tag_class, + sizeof(kNonMinimal))); + + // This should be {0x04, 0x81, 0x80, ...}. + std::vector non_minimal = {0x04, 0x82, 0x00, 0x80}; + non_minimal.resize(non_minimal.size() + 0x80); + ptr = non_minimal.data(); + EXPECT_EQ(0x80, ASN1_get_object(&ptr, &length, &tag, &tag_class, + non_minimal.size())); } template diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 8adba6c68..653af3911 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -4148,8 +4148,8 @@ soBsxWI= -----END CERTIFICATE----- )"; -// kNonMinimalLengthSignature is an X.509 certificate where the signature -// has a non-minimal length. +// kNonMinimalLengthSignature is an X.509 certificate where the signature has a +// non-minimal length. static const char kNonMinimalLengthSignature[] = R"( -----BEGIN CERTIFICATE----- MIIBITCBxqADAgECAgIE0jAKBggqhkjOPQQDAjAPMQ0wCwYDVQQDEwRUZXN0MCAX @@ -4162,6 +4162,20 @@ soBsxWI= -----END CERTIFICATE----- )"; +// kNonMinimalLengthSerial is an X.509 certificate where the serial number has a +// non-minimal length. +static const char kNonMinimalLengthSerial[] = R"( +-----BEGIN CERTIFICATE----- +MIIBITCBx6ADAgECAoECBNIwCgYIKoZIzj0EAwIwDzENMAsGA1UEAxMEVGVzdDAg +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)); @@ -4175,6 +4189,7 @@ TEST(X509Test, BER) { EXPECT_FALSE(CertFromPEM(kHighTagNumber)); // Lengths must be minimal in DER. EXPECT_FALSE(CertFromPEM(kNonMinimalLengthOuter)); + EXPECT_FALSE(CertFromPEM(kNonMinimalLengthSerial)); // We, for now, accept a non-minimal length in the signature field. See // b/18228011. EXPECT_TRUE(CertFromPEM(kNonMinimalLengthSignature)); diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h index 030306b54..5df681603 100644 --- a/include/openssl/asn1.h +++ b/include/openssl/asn1.h @@ -631,9 +631,6 @@ OPENSSL_EXPORT void ASN1_VISIBLESTRING_free(ASN1_VISIBLESTRING *str); // The following functions parse up to |len| bytes from |*inp| as a // DER-encoded ASN.1 value of the corresponding type, as described in // |d2i_SAMPLE|. -// -// TODO(https://crbug.com/boringssl/354): This function currently also accepts -// BER, but this will be removed in the future. OPENSSL_EXPORT ASN1_BMPSTRING *d2i_ASN1_BMPSTRING(ASN1_BMPSTRING **out, const uint8_t **inp, long len); @@ -917,9 +914,6 @@ OPENSSL_EXPORT void ASN1_BIT_STRING_free(ASN1_BIT_STRING *str); // d2i_ASN1_BIT_STRING parses up to |len| bytes from |*inp| as a DER-encoded // ASN.1 BIT STRING, as described in |d2i_SAMPLE|. -// -// TODO(https://crbug.com/boringssl/354): This function currently also accepts -// BER, but this will be removed in the future. OPENSSL_EXPORT ASN1_BIT_STRING *d2i_ASN1_BIT_STRING(ASN1_BIT_STRING **out, const uint8_t **inp, long len); @@ -932,9 +926,6 @@ OPENSSL_EXPORT int i2d_ASN1_BIT_STRING(const ASN1_BIT_STRING *in, // c2i_ASN1_BIT_STRING decodes |len| bytes from |*inp| as the contents of a // DER-encoded BIT STRING, excluding the tag and length. It behaves like // |d2i_SAMPLE| except, on success, it always consumes all |len| bytes. -// -// TODO(https://crbug.com/boringssl/354): This function currently also accepts -// BER, but this will be removed in the future. OPENSSL_EXPORT ASN1_BIT_STRING *c2i_ASN1_BIT_STRING(ASN1_BIT_STRING **out, const uint8_t **inp, long len); @@ -1027,9 +1018,6 @@ OPENSSL_EXPORT ASN1_INTEGER *ASN1_INTEGER_dup(const ASN1_INTEGER *x); // d2i_ASN1_INTEGER parses up to |len| bytes from |*inp| as a DER-encoded // ASN.1 INTEGER, as described in |d2i_SAMPLE|. -// -// TODO(https://crbug.com/boringssl/354): This function currently also accepts -// BER, but this will be removed in the future. OPENSSL_EXPORT ASN1_INTEGER *d2i_ASN1_INTEGER(ASN1_INTEGER **out, const uint8_t **inp, long len); @@ -1040,9 +1028,6 @@ OPENSSL_EXPORT int i2d_ASN1_INTEGER(const ASN1_INTEGER *in, uint8_t **outp); // c2i_ASN1_INTEGER decodes |len| bytes from |*inp| as the contents of a // DER-encoded INTEGER, excluding the tag and length. It behaves like // |d2i_SAMPLE| except, on success, it always consumes all |len| bytes. -// -// TODO(https://crbug.com/boringssl/354): This function currently also accepts -// some invalid inputs, but this will be removed in the future. OPENSSL_EXPORT ASN1_INTEGER *c2i_ASN1_INTEGER(ASN1_INTEGER **in, const uint8_t **outp, long len); @@ -1111,9 +1096,6 @@ OPENSSL_EXPORT void ASN1_ENUMERATED_free(ASN1_ENUMERATED *str); // d2i_ASN1_ENUMERATED parses up to |len| bytes from |*inp| as a DER-encoded // ASN.1 ENUMERATED, as described in |d2i_SAMPLE|. -// -// TODO(https://crbug.com/boringssl/354): This function currently also accepts -// BER, but this will be removed in the future. OPENSSL_EXPORT ASN1_ENUMERATED *d2i_ASN1_ENUMERATED(ASN1_ENUMERATED **out, const uint8_t **inp, long len); @@ -1246,9 +1228,6 @@ OPENSSL_EXPORT void ASN1_GENERALIZEDTIME_free(ASN1_GENERALIZEDTIME *str); // d2i_ASN1_GENERALIZEDTIME parses up to |len| bytes from |*inp| as a // DER-encoded ASN.1 GeneralizedTime, as described in |d2i_SAMPLE|. -// -// TODO(https://crbug.com/boringssl/354): This function currently also accepts -// BER, but this will be removed in the future. OPENSSL_EXPORT ASN1_GENERALIZEDTIME *d2i_ASN1_GENERALIZEDTIME( ASN1_GENERALIZEDTIME **out, const uint8_t **inp, long len); @@ -1402,9 +1381,6 @@ OPENSSL_EXPORT void ASN1_NULL_free(ASN1_NULL *null); // d2i_ASN1_NULL parses a DER-encoded ASN.1 NULL value from up to |len| bytes // at |*inp|, as described in |d2i_SAMPLE|. -// -// TODO(https://crbug.com/boringssl/354): This function currently also accepts -// BER, but this will be removed in the future. OPENSSL_EXPORT ASN1_NULL *d2i_ASN1_NULL(ASN1_NULL **out, const uint8_t **inp, long len); @@ -1758,13 +1734,11 @@ OPENSSL_EXPORT int i2t_ASN1_OBJECT(char *buf, int buf_len, // |*out_length|, |*out_tag|, and |*out_class| to the element's length, tag // number, and tag class, respectively, // -// Unlike OpenSSL, this function does not support indefinite-length elements. +// Unlike OpenSSL, this function only supports DER. Indefinite and non-minimal +// lengths are rejected. // // This function is difficult to use correctly. Use |CBS_get_asn1| and related // functions from bytestring.h. -// -// TODO(https://crbug.com/boringssl/354): Remove support for non-minimal -// lengths. OPENSSL_EXPORT int ASN1_get_object(const unsigned char **inp, long *out_length, int *out_tag, int *out_class, long max_len);