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>

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)

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

include/openssl/asn1.h

@@ -1076,16 +1076,25 @@ OPENSSL_EXPORT int i2c_ASN1_INTEGER(const ASN1_INTEGER *in, uint8_t **outp);
 // |ASN1_INTEGER*|.
 DECLARE_ASN1_ITEM(ASN1_INTEGER)
 
+// 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_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_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 @@ OPENSSL_EXPORT int i2d_ASN1_ENUMERATED(const ASN1_ENUMERATED *in,
 // |ASN1_ENUMERATED*|.
 DECLARE_ASN1_ITEM(ASN1_ENUMERATED)
 
+// 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_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_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);