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);
