Reject non-minimal lengths in ASN1_get_object
Now that the preceding CL has isolated the X.509 signature hack, we can
apply the strictness across the legacy parser. This is particularly
important for the TBSCertificate parser, where it is ambiguous which
value one checks the signature over. (Officially, you're supposed to
re-encode as DER. In practice, people don't do this.)
This change means many of our primitive types are actually parsed as
DER. I've removed the bug references in the comment in the documentation
where I believe they're finally correct.
Update-Note: Non-minimal lengths in certificates are no longer accepted,
as required for standards compliance. The one exception is the signature
field, where we still carry an exception. Some of this was already
enforced by libssl's parser.
Bug: 354
Change-Id: I57cfa7df9e1ec5707390e9b32fe1ec6b5d8172f9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58186
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/asn1/asn1_lib.c b/crypto/asn1/asn1_lib.c
index 3b97b38..dd56c98 100644
--- a/crypto/asn1/asn1_lib.c
+++ b/crypto/asn1/asn1_lib.c
@@ -111,20 +111,10 @@
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 1421462..cd26528 100644
--- a/crypto/asn1/asn1_test.cc
+++ b/crypto/asn1/asn1_test.cc
@@ -2235,10 +2235,24 @@
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<uint8_t> 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 <typename T>
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 8adba6c..653af39 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -4148,8 +4148,8 @@
-----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 @@
-----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 @@
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 030306b..5df6816 100644
--- a/include/openssl/asn1.h
+++ b/include/openssl/asn1.h
@@ -631,9 +631,6 @@
// 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 @@
// 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 @@
// 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 @@
// 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 @@
// 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 @@
// 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 @@
// 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 @@
// 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 @@
// |*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);
