Correctly handle LONG_MIN in ASN1_INTEGER_get. Along the way, add ASN1_INTEGER_get_uint64 from upstream, which has much better error-handling. Also fold the IntegerSetting test into the main integer test as the test data is largely redundant. Change-Id: I7ec84629264ebf405bea4bce59a13c4495d81ed7 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/51634 Reviewed-by: Adam Langley <agl@google.com> Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/asn1/a_int.c b/crypto/asn1/a_int.c index 63deded..512472a 100644 --- a/crypto/asn1/a_int.c +++ b/crypto/asn1/a_int.c
@@ -62,6 +62,7 @@ #include <openssl/bytestring.h> #include <openssl/err.h> #include <openssl/mem.h> +#include <openssl/type_check.h> #include "../internal.h" @@ -309,43 +310,76 @@ return asn1_string_set_uint64(out, v, V_ASN1_ENUMERATED); } +static int asn1_string_get_abs_uint64(uint64_t *out, const ASN1_STRING *a, + int type) +{ + if ((a->type & ~V_ASN1_NEG) != type) { + OPENSSL_PUT_ERROR(ASN1, ASN1_R_WRONG_INTEGER_TYPE); + return 0; + } + uint8_t buf[sizeof(uint64_t)] = {0}; + if (a->length > (int)sizeof(buf)) { + OPENSSL_PUT_ERROR(ASN1, ASN1_R_INVALID_INTEGER); + return 0; + } + OPENSSL_memcpy(buf + sizeof(buf) - a->length, a->data, a->length); + *out = CRYPTO_load_u64_be(buf); + return 1; +} + +static int asn1_string_get_uint64(uint64_t *out, const ASN1_STRING *a, int type) +{ + if (!asn1_string_get_abs_uint64(out, a, type)) { + return 0; + } + if (a->type & V_ASN1_NEG) { + OPENSSL_PUT_ERROR(ASN1, ASN1_R_INVALID_INTEGER); + return 0; + } + return 1; +} + +int ASN1_INTEGER_get_uint64(uint64_t *out, const ASN1_INTEGER *a) +{ + return asn1_string_get_uint64(out, a, V_ASN1_INTEGER); +} + +int ASN1_ENUMERATED_get_uint64(uint64_t *out, const ASN1_ENUMERATED *a) +{ + return asn1_string_get_uint64(out, a, V_ASN1_ENUMERATED); +} + static long asn1_string_get_long(const ASN1_STRING *a, int type) { - int neg = 0, i; - - if (a == NULL) - return (0L); - i = a->type; - if (i == (type | V_ASN1_NEG)) - neg = 1; - else if (i != type) - return -1; - - OPENSSL_STATIC_ASSERT(sizeof(uint64_t) >= sizeof(long), - "long larger than uint64_t"); - - if (a->length > (int)sizeof(uint64_t)) { - /* hmm... a bit ugly, return all ones */ - return -1; + if (a == NULL) { + return 0; } - uint64_t r64 = 0; - if (a->data != NULL) { - for (i = 0; i < a->length; i++) { - r64 <<= 8; - r64 |= (unsigned char)a->data[i]; - } - - if (r64 > LONG_MAX) { - return -1; - } + uint64_t v; + if (!asn1_string_get_abs_uint64(&v, a, type)) { + goto err; } - long r = (long) r64; - if (neg) - r = -r; + int64_t i64; + int fits_in_i64; + /* Check |v != 0| to handle manually-constructed negative zeros. */ + if ((a->type & V_ASN1_NEG) && v != 0) { + i64 = (int64_t)(0u - v); + fits_in_i64 = i64 < 0; + } else { + i64 = (int64_t)v; + fits_in_i64 = i64 >= 0; + } + OPENSSL_STATIC_ASSERT(sizeof(long) <= sizeof(int64_t), "long is too big"); - return r; + if (fits_in_i64 && LONG_MIN <= i64 && i64 <= LONG_MAX) { + return (long)i64; + } + +err: + /* This function's return value does not distinguish overflow from -1. */ + ERR_clear_error(); + return -1; } long ASN1_INTEGER_get(const ASN1_INTEGER *a)
diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc index ae3ddc2..7d69889 100644 --- a/crypto/asn1/asn1_test.cc +++ b/crypto/asn1/asn1_test.cc
@@ -328,14 +328,17 @@ objs["der"] = std::move(by_der); // Construct |ASN1_INTEGER| from |long| or |uint64_t|, if it fits. - bool fits_in_long = false; + bool fits_in_long = false, fits_in_u64 = false; + uint64_t u64 = 0; long l = 0; uint64_t abs_u64; if (BN_get_u64(bn.get(), &abs_u64)) { - if (!BN_is_negative(bn.get())) { + fits_in_u64 = !BN_is_negative(bn.get()); + if (fits_in_u64) { + u64 = abs_u64; bssl::UniquePtr<ASN1_INTEGER> by_u64(ASN1_INTEGER_new()); ASSERT_TRUE(by_u64); - ASSERT_TRUE(ASN1_INTEGER_set_uint64(by_u64.get(), abs_u64)); + ASSERT_TRUE(ASN1_INTEGER_set_uint64(by_u64.get(), u64)); objs["u64"] = std::move(by_u64); } @@ -383,8 +386,16 @@ ASSERT_TRUE(bn2); EXPECT_EQ(0, BN_cmp(bn.get(), bn2.get())); - // TODO(davidben): Fix |ASN1_INTEGER_get| to correctly handle |LONG_MIN|. - if (fits_in_long && l != LONG_MIN) { + if (fits_in_u64) { + uint64_t v; + ASSERT_TRUE(ASN1_INTEGER_get_uint64(&v, obj)); + EXPECT_EQ(v, u64); + } else { + uint64_t v; + EXPECT_FALSE(ASN1_INTEGER_get_uint64(&v, obj)); + } + + if (fits_in_long) { EXPECT_EQ(l, ASN1_INTEGER_get(obj)); } else { EXPECT_EQ(-1, ASN1_INTEGER_get(obj));
diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h index d9a9ca9..d6fa2f7 100644 --- a/include/openssl/asn1.h +++ b/include/openssl/asn1.h
@@ -1076,16 +1076,25 @@ // |ASN1_INTEGER*|. DECLARE_ASN1_ITEM(ASN1_INTEGER) -// ASN1_INTEGER_set sets |a| to an INTEGER with value |v|. It returns one on -// success and zero on error. -OPENSSL_EXPORT int ASN1_INTEGER_set(ASN1_INTEGER *a, long v); - // ASN1_INTEGER_set_uint64 sets |a| to an INTEGER with value |v|. It returns one // on success and zero on error. OPENSSL_EXPORT int ASN1_INTEGER_set_uint64(ASN1_INTEGER *out, uint64_t v); +// ASN1_INTEGER_set sets |a| to an INTEGER with value |v|. It returns one on +// success and zero on error. +OPENSSL_EXPORT int ASN1_INTEGER_set(ASN1_INTEGER *a, long v); + +// ASN1_INTEGER_get_uint64 converts |a| to a |uint64_t|. On success, it returns +// one and sets |*out| to the result. If |a| did not fit or has the wrong type, +// it returns zero. +OPENSSL_EXPORT int ASN1_INTEGER_get_uint64(uint64_t *out, + const ASN1_INTEGER *a); + // ASN1_INTEGER_get returns the value of |a| as a |long|, or -1 if |a| is out of // range or the wrong type. +// +// WARNING: This function's return value cannot distinguish errors from -1. +// Prefer |ASN1_INTEGER_get_uint64|. OPENSSL_EXPORT long ASN1_INTEGER_get(const ASN1_INTEGER *a); // BN_to_ASN1_INTEGER sets |ai| to an INTEGER with value |bn| and returns |ai| @@ -1131,22 +1140,31 @@ // |ASN1_ENUMERATED*|. DECLARE_ASN1_ITEM(ASN1_ENUMERATED) -// ASN1_ENUMERATED_set sets |a| to an ENUMERATED with value |v|. It returns one -// on success and zero on error. -OPENSSL_EXPORT int ASN1_ENUMERATED_set(ASN1_ENUMERATED *a, long v); - // ASN1_ENUMERATED_set_uint64 sets |a| to an ENUMERATED with value |v|. It // returns one on success and zero on error. OPENSSL_EXPORT int ASN1_ENUMERATED_set_uint64(ASN1_ENUMERATED *out, uint64_t v); +// ASN1_ENUMERATED_set sets |a| to an ENUMERATED with value |v|. It returns one +// on success and zero on error. +OPENSSL_EXPORT int ASN1_ENUMERATED_set(ASN1_ENUMERATED *a, long v); + +// ASN1_ENUMERATED_get_uint64 converts |a| to a |uint64_t|. On success, it +// returns one and sets |*out| to the result. If |a| did not fit or has the +// wrong type, it returns zero. +OPENSSL_EXPORT int ASN1_ENUMERATED_get_uint64(uint64_t *out, + const ASN1_ENUMERATED *a); + // ASN1_ENUMERATED_get returns the value of |a| as a |long|, or -1 if |a| is out // of range or the wrong type. +// +// WARNING: This function's return value cannot distinguish errors from -1. +// Prefer |ASN1_ENUMERATED_get_uint64|. OPENSSL_EXPORT long ASN1_ENUMERATED_get(const ASN1_ENUMERATED *a); // BN_to_ASN1_ENUMERATED sets |ai| to an ENUMERATED with value |bn| and returns // |ai| on success or NULL or error. If |ai| is NULL, it returns a -// newly-allocated |ASN1_INTEGER| on success instead, which the caller must -// release with |ASN1_INTEGER_free|. +// newly-allocated |ASN1_ENUMERATED| on success instead, which the caller must +// release with |ASN1_ENUMERATED_free|. OPENSSL_EXPORT ASN1_ENUMERATED *BN_to_ASN1_ENUMERATED(const BIGNUM *bn, ASN1_ENUMERATED *ai);