diff options
author | David Benjamin <davidben@google.com> | 2023-03-13 11:58:08 -0400 |
---|---|---|
committer | Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> | 2023-03-21 16:28:49 +0000 |
commit | 8c8629bfd89436e5019b6bd3c65cff4bf1a76b76 (patch) | |
tree | 635b5235bcddf91d25a32ea1bad6aff56c3bb6cc /crypto/asn1 | |
parent | 92de195169d26d9f5cec7ef34df9194e614e50f8 (diff) | |
download | boringssl-8c8629bfd89436e5019b6bd3c65cff4bf1a76b76.zip boringssl-8c8629bfd89436e5019b6bd3c65cff4bf1a76b76.tar.gz boringssl-8c8629bfd89436e5019b6bd3c65cff4bf1a76b76.tar.bz2 |
Represent unknown universal types with V_ASN1_OTHER
OpenSSL's ASN1_STRING representation has many cases. There's a grab-bag
V_ASN1_OTHER cases that can represent any element. But it is currently
only used for non-universal tags. Unknown universal tags go into the
type field directly.
This has a few problems:
- Certain high values, V_ASN1_NEG_INTEGER and V_ASN1_NEG_ENUMERATED,
are treated special. This was one of the two causes behind
CVE-2016-2108 and had to be worked around with V_ASN1_MAX_UNIVERSAL.
- OpenSSL can never compatibly support a new universal type in a
non-ASN1_STRING form. Otherwise ASN1_TYPE's union changes its
in-memory representation.
- It is a bit ambiguous when OpenSSL does or doesn't know the type.
- This is broadly implemented by having a default in all the
switch/cases, which is a little awkward.
- It's yet another "unknown tag" case when V_ASN1_OTHER covers such
cases just fine.
Remove this representation and use V_ASN1_OTHER. This more unambiguously
resolves CVE-2016-2108. ASN1_STRING's and ASN1_TYPE's respective type
fields are now a closed set. Update the documenthation accordingly.
Formally allowing universal types in ASN1_STRING also opens the door to
clearing the ASN1_PRINTABLE mess (https://crbug.com/boringssl/412).
BoringSSL currently rejects X.509 names that are actually valid, because
the OpenSSL X509_NAME representation cannot represent them. This allows
us to introduce an ASN1_STRING-based ANY representation, which just
represents all non-ASN1_STRING types in an V_ASN1_OTHER.
The implementation is a little clumsy (the way things tasn_dec.c is
written, I had to introduce yet another check), but I'm hoping that,
when the parser is rewritten with CBS, this can be integrated into a
single type dispatch.
Update-Note: This does not change the set of inputs accepted or rejected
by the ASN.1 parser. It does, however, change the in-memory
representation in edge cases. Unless the application was specifically
inspecting the in-memory representation for these unknown types, we
expect this to have no impact.
Fixed: 561
Change-Id: Ibf9550e285ce50b11c7609d28b139354b9dd41dc
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/58148
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Diffstat (limited to 'crypto/asn1')
-rw-r--r-- | crypto/asn1/asn1_test.cc | 100 | ||||
-rw-r--r-- | crypto/asn1/tasn_dec.c | 25 | ||||
-rw-r--r-- | crypto/asn1/tasn_enc.c | 8 |
3 files changed, 89 insertions, 44 deletions
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc index 640b726..1421462 100644 --- a/crypto/asn1/asn1_test.cc +++ b/crypto/asn1/asn1_test.cc @@ -40,45 +40,6 @@ #endif -// kTag128 is an ASN.1 structure with a universal tag with number 128. -static const uint8_t kTag128[] = { - 0x1f, 0x81, 0x00, 0x01, 0x00, -}; - -// kTag258 is an ASN.1 structure with a universal tag with number 258. -static const uint8_t kTag258[] = { - 0x1f, 0x82, 0x02, 0x01, 0x00, -}; - -static_assert(V_ASN1_NEG_INTEGER == 258, - "V_ASN1_NEG_INTEGER changed. Update kTag258 to collide with it."); - -// kTagOverflow is an ASN.1 structure with a universal tag with number 2^35-1, -// which will not fit in an int. -static const uint8_t kTagOverflow[] = { - 0x1f, 0xff, 0xff, 0xff, 0xff, 0x7f, 0x01, 0x00, -}; - -TEST(ASN1Test, LargeTags) { - const uint8_t *p = kTag258; - bssl::UniquePtr<ASN1_TYPE> obj(d2i_ASN1_TYPE(NULL, &p, sizeof(kTag258))); - EXPECT_FALSE(obj) << "Parsed value with illegal tag" << obj->type; - ERR_clear_error(); - - p = kTagOverflow; - obj.reset(d2i_ASN1_TYPE(NULL, &p, sizeof(kTagOverflow))); - EXPECT_FALSE(obj) << "Parsed value with tag overflow" << obj->type; - ERR_clear_error(); - - p = kTag128; - obj.reset(d2i_ASN1_TYPE(NULL, &p, sizeof(kTag128))); - ASSERT_TRUE(obj); - EXPECT_EQ(128, obj->type); - const uint8_t kZero = 0; - EXPECT_EQ(Bytes(&kZero, 1), Bytes(obj->value.asn1_string->data, - obj->value.asn1_string->length)); -} - // |obj| and |i2d_func| require different template parameters because C++ may // deduce, say, |ASN1_STRING*| via |obj| and |const ASN1_STRING*| via // |i2d_func|. Template argument deduction then fails. The language is not able @@ -107,6 +68,67 @@ void TestSerialize(T obj, int (*i2d_func)(U a, uint8_t **pp), EXPECT_EQ(Bytes(expected), Bytes(buf)); } +// Historically, unknown universal tags were represented in |ASN1_TYPE| as +// |ASN1_STRING|s with the type matching the tag number. This can collide with +// |V_ASN_NEG|, which was one of the causes of CVE-2016-2108. We now represent +// unsupported values with |V_ASN1_OTHER|, but retain the |V_ASN1_MAX_UNIVERSAL| +// limit. +TEST(ASN1Test, UnknownTags) { + // kTag258 is an ASN.1 structure with a universal tag with number 258. + static const uint8_t kTag258[] = {0x1f, 0x82, 0x02, 0x01, 0x00}; + static_assert( + V_ASN1_NEG_INTEGER == 258, + "V_ASN1_NEG_INTEGER changed. Update kTag258 to collide with it."); + const uint8_t *p = kTag258; + bssl::UniquePtr<ASN1_TYPE> obj(d2i_ASN1_TYPE(NULL, &p, sizeof(kTag258))); + EXPECT_FALSE(obj) << "Parsed value with illegal tag" << obj->type; + ERR_clear_error(); + + // kTagOverflow is an ASN.1 structure with a universal tag with number 2^35-1, + // which will not fit in an int. + static const uint8_t kTagOverflow[] = {0x1f, 0xff, 0xff, 0xff, + 0xff, 0x7f, 0x01, 0x00}; + p = kTagOverflow; + obj.reset(d2i_ASN1_TYPE(NULL, &p, sizeof(kTagOverflow))); + EXPECT_FALSE(obj) << "Parsed value with tag overflow" << obj->type; + ERR_clear_error(); + + // kTag128 is an ASN.1 structure with a universal tag with number 128. It + // should be parsed as |V_ASN1_OTHER|. + static const uint8_t kTag128[] = {0x1f, 0x81, 0x00, 0x01, 0x00}; + p = kTag128; + obj.reset(d2i_ASN1_TYPE(NULL, &p, sizeof(kTag128))); + ASSERT_TRUE(obj); + EXPECT_EQ(V_ASN1_OTHER, obj->type); + EXPECT_EQ(Bytes(kTag128), Bytes(obj->value.asn1_string->data, + obj->value.asn1_string->length)); + TestSerialize(obj.get(), i2d_ASN1_TYPE, kTag128); + + // The historical in-memory representation of |kTag128| was for both + // |obj->type| and |obj->value.asn1_string->type| to be NULL. This is no + // longer used and should be rejected by the encoder. + obj.reset(ASN1_TYPE_new()); + ASSERT_TRUE(obj); + obj->type = 128; + obj->value.asn1_string = ASN1_STRING_type_new(128); + ASSERT_TRUE(obj->value.asn1_string); + const uint8_t zero = 0; + ASSERT_TRUE(ASN1_STRING_set(obj->value.asn1_string, &zero, sizeof(zero))); + EXPECT_EQ(-1, i2d_ASN1_TYPE(obj.get(), nullptr)); + + // If a tag is known, but has the wrong constructed bit, it should be + // rejected, not placed in |V_ASN1_OTHER|. + static const uint8_t kConstructedOctetString[] = {0x24, 0x00}; + p = kConstructedOctetString; + obj.reset(d2i_ASN1_TYPE(nullptr, &p, sizeof(kConstructedOctetString))); + EXPECT_FALSE(obj); + + static const uint8_t kPrimitiveSequence[] = {0x10, 0x00}; + p = kPrimitiveSequence; + obj.reset(d2i_ASN1_TYPE(nullptr, &p, sizeof(kPrimitiveSequence))); + EXPECT_FALSE(obj); +} + static bssl::UniquePtr<BIGNUM> BIGNUMPow2(unsigned bit) { bssl::UniquePtr<BIGNUM> bn(BN_new()); if (!bn || diff --git a/crypto/asn1/tasn_dec.c b/crypto/asn1/tasn_dec.c index 4f25fbb..23c526e 100644 --- a/crypto/asn1/tasn_dec.c +++ b/crypto/asn1/tasn_dec.c @@ -136,6 +136,23 @@ unsigned long ASN1_tag2bit(int tag) { return tag2bit[tag]; } +static int is_supported_universal_type(int tag, int aclass) { + if (aclass != V_ASN1_UNIVERSAL) { + return 0; + } + return tag == V_ASN1_OBJECT || tag == V_ASN1_NULL || tag == V_ASN1_BOOLEAN || + tag == V_ASN1_BIT_STRING || tag == V_ASN1_INTEGER || + tag == V_ASN1_ENUMERATED || tag == V_ASN1_OCTET_STRING || + tag == V_ASN1_NUMERICSTRING || tag == V_ASN1_PRINTABLESTRING || + tag == V_ASN1_T61STRING || tag == V_ASN1_VIDEOTEXSTRING || + tag == V_ASN1_IA5STRING || tag == V_ASN1_UTCTIME || + tag == V_ASN1_GENERALIZEDTIME || tag == V_ASN1_GRAPHICSTRING || + tag == V_ASN1_VISIBLESTRING || tag == V_ASN1_GENERALSTRING || + tag == V_ASN1_UNIVERSALSTRING || tag == V_ASN1_BMPSTRING || + tag == V_ASN1_UTF8STRING || tag == V_ASN1_SET || + tag == V_ASN1_SEQUENCE; +} + // Macro to initialize and invalidate the cache // Decode an ASN1 item, this currently behaves just like a standard 'd2i' @@ -677,7 +694,7 @@ static int asn1_d2i_ex_primitive(ASN1_VALUE **pval, const unsigned char **in, OPENSSL_PUT_ERROR(ASN1, ASN1_R_NESTED_ASN1_ERROR); return 0; } - if (oclass != V_ASN1_UNIVERSAL) { + if (!is_supported_universal_type(utype, oclass)) { utype = V_ASN1_OTHER; } } @@ -820,8 +837,7 @@ static int asn1_ex_c2i(ASN1_VALUE **pval, const unsigned char *cont, int len, case V_ASN1_UTF8STRING: case V_ASN1_OTHER: case V_ASN1_SET: - case V_ASN1_SEQUENCE: - default: { + case V_ASN1_SEQUENCE: { CBS cbs; CBS_init(&cbs, cont, (size_t)len); if (utype == V_ASN1_BMPSTRING) { @@ -884,6 +900,9 @@ static int asn1_ex_c2i(ASN1_VALUE **pval, const unsigned char *cont, int len, } break; } + default: + OPENSSL_PUT_ERROR(ASN1, ASN1_R_BAD_TEMPLATE); + goto err; } // If ASN1_ANY and NULL type fix up value if (typ && (utype == V_ASN1_NULL)) { diff --git a/crypto/asn1/tasn_enc.c b/crypto/asn1/tasn_enc.c index ca3b3fc..b0d72ce 100644 --- a/crypto/asn1/tasn_enc.c +++ b/crypto/asn1/tasn_enc.c @@ -691,13 +691,17 @@ static int asn1_ex_i2c(ASN1_VALUE **pval, unsigned char *cout, int *out_omit, case V_ASN1_UTF8STRING: case V_ASN1_SEQUENCE: case V_ASN1_SET: - default: + // This is not a valid |ASN1_ITEM| type, but it appears in |ASN1_TYPE|. + case V_ASN1_OTHER: // All based on ASN1_STRING and handled the same strtmp = (ASN1_STRING *)*pval; cont = strtmp->data; len = strtmp->length; - break; + + default: + OPENSSL_PUT_ERROR(ASN1, ASN1_R_BAD_TEMPLATE); + return -1; } if (cout && len) { OPENSSL_memcpy(cout, cont, len); |