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>
This commit is contained in:
David Benjamin
Boringssl LUCI CQ
parent
2a52444f9d
commit
8ebfea76db
@ -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) {
|
||||
|
||||
@ -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<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>
|
||||
|
||||
@ -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));
|
||||
|
||||
@ -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);
|
||||
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user