diff --git a/src/crypto/asn1/asn1_test.cc b/src/crypto/asn1/asn1_test.cc index 640b72681..142146200 100644 --- a/src/crypto/asn1/asn1_test.cc +++ b/src/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 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 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 BIGNUMPow2(unsigned bit) { bssl::UniquePtr bn(BN_new()); if (!bn || diff --git a/src/crypto/asn1/tasn_dec.c b/src/crypto/asn1/tasn_dec.c index 4f25fbbac..23c526e0f 100644 --- a/src/crypto/asn1/tasn_dec.c +++ b/src/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/src/crypto/asn1/tasn_enc.c b/src/crypto/asn1/tasn_enc.c index ca3b3fc50..b0d72ce86 100644 --- a/src/crypto/asn1/tasn_enc.c +++ b/src/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); diff --git a/src/include/openssl/asn1.h b/src/include/openssl/asn1.h index 59996b66f..030306b54 100644 --- a/src/include/openssl/asn1.h +++ b/src/include/openssl/asn1.h @@ -467,31 +467,39 @@ DECLARE_ASN1_ITEM(ASN1_FBOOLEAN) // |ASN1_STRING|, to represent most values. // An asn1_string_st (aka |ASN1_STRING|) represents a value of a string-like -// ASN.1 type. It contains a type field, and a byte string data field with a +// ASN.1 type. It contains a |type| field, and a byte string |data| field with a // type-specific representation. // -// When representing a string value, the type field is one of -// |V_ASN1_OCTET_STRING|, |V_ASN1_UTF8STRING|, |V_ASN1_NUMERICSTRING|, -// |V_ASN1_PRINTABLESTRING|, |V_ASN1_T61STRING|, |V_ASN1_VIDEOTEXSTRING|, -// |V_ASN1_IA5STRING|, |V_ASN1_GRAPHICSTRING|, |V_ASN1_ISO64STRING|, -// |V_ASN1_VISIBLESTRING|, |V_ASN1_GENERALSTRING|, |V_ASN1_UNIVERSALSTRING|, or -// |V_ASN1_BMPSTRING|. The data contains the byte representation of of the +// If |type| is one of |V_ASN1_OCTET_STRING|, |V_ASN1_UTF8STRING|, +// |V_ASN1_NUMERICSTRING|, |V_ASN1_PRINTABLESTRING|, |V_ASN1_T61STRING|, +// |V_ASN1_VIDEOTEXSTRING|, |V_ASN1_IA5STRING|, |V_ASN1_GRAPHICSTRING|, +// |V_ASN1_ISO64STRING|, |V_ASN1_VISIBLESTRING|, |V_ASN1_GENERALSTRING|, +// |V_ASN1_UNIVERSALSTRING|, or |V_ASN1_BMPSTRING|, the object represents an +// ASN.1 string type. The data contains the byte representation of the // string. // -// When representing a BIT STRING value, the type field is |V_ASN1_BIT_STRING|. -// See bit string documentation below for how the data and flags are used. +// If |type| is |V_ASN1_BIT_STRING|, the object represents a BIT STRING value. +// See bit string documentation below for the data and flags. // -// When representing an INTEGER or ENUMERATED value, the type field is one of -// |V_ASN1_INTEGER|, |V_ASN1_NEG_INTEGER|, |V_ASN1_ENUMERATED|, or -// |V_ASN1_NEG_ENUMERATED|. See integer documentation below for details. +// If |type| is one of |V_ASN1_INTEGER|, |V_ASN1_NEG_INTEGER|, +// |V_ASN1_ENUMERATED|, or |V_ASN1_NEG_ENUMERATED|, the object represents an +// INTEGER or ENUMERATED value. See integer documentation below for details. // -// When representing a GeneralizedTime or UTCTime value, the type field is -// |V_ASN1_GENERALIZEDTIME| or |V_ASN1_UTCTIME|, respectively. The data contains -// the DER encoding of the value. For example, the UNIX epoch would be +// If |type| is |V_ASN1_GENERALIZEDTIME| or |V_ASN1_UTCTIME|, the object +// represents a GeneralizedTime or UTCTime value, respectively. The data +// contains the DER encoding of the value. For example, the UNIX epoch would be // "19700101000000Z" for a GeneralizedTime and "700101000000Z" for a UTCTime. // -// |ASN1_STRING|, when stored in an |ASN1_TYPE|, may also represent an element -// with tag not directly supported by this library. See |ASN1_TYPE| for details. +// If |type| is |V_ASN1_SEQUENCE|, |V_ASN1_SET|, or |V_ASN1_OTHER|, the object +// represents a SEQUENCE, SET, or arbitrary ASN.1 value, respectively. Unlike +// the above cases, the data contains the DER encoding of the entire structure, +// including the header. If the value is explicitly or implicitly tagged, this +// too will be reflected in the data field. As this case handles unknown types, +// the contents are not checked when parsing or serializing. +// +// Other values of |type| do not represent a valid ASN.1 value, though +// default-constructed objects may set |type| to -1. Such objects cannot be +// serialized. // // |ASN1_STRING| additionally has the following typedefs: |ASN1_BIT_STRING|, // |ASN1_BMPSTRING|, |ASN1_ENUMERATED|, |ASN1_GENERALIZEDTIME|, @@ -508,15 +516,14 @@ DECLARE_ASN1_ITEM(ASN1_FBOOLEAN) // |ASN1_STRING_length|. // // If a function returns an |ASN1_STRING| where the typedef or ASN.1 structure -// implies constraints on the type field, callers may assume that the type field -// is correct. However, if a function takes an |ASN1_STRING| as input, callers -// must ensure the type field matches. These invariants are not captured by the -// C type system and may not be checked at runtime. For example, callers may -// assume the output of |X509_get0_serialNumber| has type |V_ASN1_INTEGER| or -// |V_ASN1_NEG_INTEGER|. Callers must not pass a string of type -// |V_ASN1_OCTET_STRING| to |X509_set_serialNumber|. Doing so may break -// invariants on the |X509| object and break the |X509_get0_serialNumber| -// invariant. +// implies constraints on |type|, callers may assume that |type| is correct. +// However, if a function takes an |ASN1_STRING| as input, callers must ensure +// |type| matches. These invariants are not captured by the C type system and +// may not be checked at runtime. For example, callers may assume the output of +// |X509_get0_serialNumber| has type |V_ASN1_INTEGER| or |V_ASN1_NEG_INTEGER|. +// Callers must not pass a string of type |V_ASN1_OCTET_STRING| to +// |X509_set_serialNumber|. Doing so may break invariants on the |X509| object +// and break the |X509_get0_serialNumber| invariant. // // TODO(https://crbug.com/boringssl/445): This is very unfriendly. Getting the // type field wrong should not cause memory errors, but it may do strange @@ -1492,15 +1499,14 @@ DECLARE_ASN1_ITEM(ASN1_OBJECT) // |ASN1_BOOLEAN|. // // If |type| is |V_ASN1_SEQUENCE|, |V_ASN1_SET|, or |V_ASN1_OTHER|, the tag is -// SEQUENCE, SET, or some non-universal tag, respectively. |value| is an -// |ASN1_STRING| containing the entire element, including the tag and length. -// The |ASN1_STRING|'s |type| field matches the containing |ASN1_TYPE|'s |type|. +// SEQUENCE, SET, or some arbitrary tag, respectively. |value| uses the +// corresponding |ASN1_STRING| representation. Although any type may be +// represented in |V_ASN1_OTHER|, the parser will always return the more +// specific encoding when available. // -// Other positive values of |type|, up to |V_ASN1_MAX_UNIVERSAL|, correspond to -// universal primitive tags not directly supported by this library. |value| is -// an |ASN1_STRING| containing the body of the element, excluding the tag -// and length. The |ASN1_STRING|'s |type| field matches the containing -// |ASN1_TYPE|'s |type|. +// Other values of |type| do not represent a valid ASN.1 value, though +// default-constructed objects may set |type| to -1. Such objects cannot be +// serialized. struct asn1_type_st { int type; union { diff --git a/src/rust/bssl-crypto/src/lib.rs b/src/rust/bssl-crypto/src/lib.rs index 840033752..9024ed4b9 100644 --- a/src/rust/bssl-crypto/src/lib.rs +++ b/src/rust/bssl-crypto/src/lib.rs @@ -37,6 +37,9 @@ pub mod hkdf; /// BoringSSL implemented hmac operations. pub mod hmac; +/// BoringSSL implemented cryptographically secure pseudo-random number generation. +pub mod rand; + #[cfg(test)] mod test_helpers; diff --git a/src/rust/bssl-crypto/src/rand.rs b/src/rust/bssl-crypto/src/rand.rs new file mode 100644 index 000000000..9fdbe0a7e --- /dev/null +++ b/src/rust/bssl-crypto/src/rand.rs @@ -0,0 +1,41 @@ +/* Copyright (c) 2023, Google Inc. + * + * Permission to use, copy, modify, and/or distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY + * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION + * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. + */ + +use crate::CSliceMut; + +/// Fills buf with random bytes. In the event that sufficient random data can not be obtained, +/// BoringSSL will abort, so the assert will never be hit. +pub fn rand_bytes(buf: &mut [u8]) { + let mut ffi_buf = CSliceMut::from(buf); + let result = unsafe { bssl_sys::RAND_bytes(ffi_buf.as_mut_ptr(), ffi_buf.len()) }; + assert_eq!(result, 1, "BoringSSL RAND_bytes API failed unexpectedly"); +} + +#[cfg(test)] +mod tests { + use super::rand_bytes; + + #[test] + fn test_rand_bytes() { + let mut buf = [0; 32]; + rand_bytes(&mut buf); + } + + #[test] + fn test_rand_bytes_empty() { + let mut buf = []; + rand_bytes(&mut buf); + } +}