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>

2 files changed, 77 insertions, 32 deletions

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 @@ int ASN1_ENUMERATED_set_uint64(ASN1_ENUMERATED *out, uint64_t v)
     return asn1_string_set_uint64(out, v, V_ASN1_ENUMERATED);
 }
 
-static long asn1_string_get_long(const ASN1_STRING *a, int type)
+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)
 {
-    int neg = 0, i;
+    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;
+}
 
-    if (a == NULL)
-        return (0L);
-    i = a->type;
-    if (i == (type | V_ASN1_NEG))
-        neg = 1;
-    else if (i != type)
-        return -1;
+int ASN1_INTEGER_get_uint64(uint64_t *out, const ASN1_INTEGER *a)
+{
+    return asn1_string_get_uint64(out, a, V_ASN1_INTEGER);
+}
 
-    OPENSSL_STATIC_ASSERT(sizeof(uint64_t) >= sizeof(long),
-                          "long larger than uint64_t");
+int ASN1_ENUMERATED_get_uint64(uint64_t *out, const ASN1_ENUMERATED *a)
+{
+    return asn1_string_get_uint64(out, a, V_ASN1_ENUMERATED);
+}
 
-    if (a->length > (int)sizeof(uint64_t)) {
-        /* hmm... a bit ugly, return all ones */
-        return -1;
+static long asn1_string_get_long(const ASN1_STRING *a, int type)
+{
+    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];
-      }
+    uint64_t v;
+    if (!asn1_string_get_abs_uint64(&v, a, type)) {
+        goto err;
+    }
 
-      if (r64 > LONG_MAX) {
-          return -1;
-      }
+    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");
 
-    long r = (long) r64;
-    if (neg)
-        r = -r;
+    if (fits_in_i64 && LONG_MIN <= i64 && i64 <= LONG_MAX) {
+        return (long)i64;
+    }
 
-    return r;
+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 @@ TEST(ASN1Test, Integer) {
     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 @@ TEST(ASN1Test, Integer) {
       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));