Add APIs for checking ASN.1 INTEGERs.

We have several implementations of this internally, so consolidate them.
Chromium also has a copy in net/der/parse_values.cc which could call
into this.

(I'm also hoping we can make c2i_ASN1_INTEGER call this and
further tighten up crypto/asn1's parser, but I see Chromium still has an
allow_invalid_serial_numbers option, so perhaps not quite yet.)

Update-Note: This CL does not change behavior, but I'm leaving a note to
myself to make net/der/parse_values.cc call the new functions.

Change-Id: If2aae6574ba6a30e343e1308da6af543616156ec
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/44051
Reviewed-by: Adam Langley <agl@google.com>

2 files changed, 92 insertions, 61 deletions

diff --git a/crypto/bytestring/bytestring_test.cc b/crypto/bytestring/bytestring_test.cc
index 93593e9..6445842 100644
--- a/crypto/bytestring/bytestring_test.cc
+++ b/crypto/bytestring/bytestring_test.cc
@@ -720,50 +720,65 @@ static const ASN1Uint64Test kASN1Uint64Tests[] = {
 struct ASN1InvalidUint64Test {
   const char *encoding;
   size_t encoding_len;
+  bool overflow;
 };
 
 static const ASN1InvalidUint64Test kASN1InvalidUint64Tests[] = {
     // Bad tag.
-    {"\x03\x01\x00", 3},
+    {"\x03\x01\x00", 3, false},
     // Empty contents.
-    {"\x02\x00", 2},
+    {"\x02\x00", 2, false},
     // Negative number.
-    {"\x02\x01\x80", 3},
+    {"\x02\x01\x80", 3, false},
     // Overflow.
-    {"\x02\x09\x01\x00\x00\x00\x00\x00\x00\x00\x00", 11},
+    {"\x02\x09\x01\x00\x00\x00\x00\x00\x00\x00\x00", 11, true},
     // Leading zeros.
-    {"\x02\x02\x00\x01", 4},
+    {"\x02\x02\x00\x01", 4, false},
 };
 
 TEST(CBSTest, ASN1Uint64) {
-  for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(kASN1Uint64Tests); i++) {
-    SCOPED_TRACE(i);
-    const ASN1Uint64Test *test = &kASN1Uint64Tests[i];
+  for (const ASN1Uint64Test &test : kASN1Uint64Tests) {
+    SCOPED_TRACE(Bytes(test.encoding, test.encoding_len));
+    SCOPED_TRACE(test.value);
     CBS cbs;
     uint64_t value;
     uint8_t *out;
     size_t len;
 
-    CBS_init(&cbs, (const uint8_t *)test->encoding, test->encoding_len);
+    CBS_init(&cbs, (const uint8_t *)test.encoding, test.encoding_len);
     ASSERT_TRUE(CBS_get_asn1_uint64(&cbs, &value));
     EXPECT_EQ(0u, CBS_len(&cbs));
-    EXPECT_EQ(test->value, value);
+    EXPECT_EQ(test.value, value);
+
+    CBS child;
+    int is_negative;
+    CBS_init(&cbs, (const uint8_t *)test.encoding, test.encoding_len);
+    ASSERT_TRUE(CBS_get_asn1(&cbs, &child, CBS_ASN1_INTEGER));
+    EXPECT_TRUE(CBS_is_valid_asn1_integer(&child, &is_negative));
+    EXPECT_EQ(0, is_negative);
+    EXPECT_TRUE(CBS_is_unsigned_asn1_integer(&child));
 
     bssl::ScopedCBB cbb;
     ASSERT_TRUE(CBB_init(cbb.get(), 0));
-    ASSERT_TRUE(CBB_add_asn1_uint64(cbb.get(), test->value));
+    ASSERT_TRUE(CBB_add_asn1_uint64(cbb.get(), test.value));
     ASSERT_TRUE(CBB_finish(cbb.get(), &out, &len));
     bssl::UniquePtr<uint8_t> scoper(out);
-    EXPECT_EQ(Bytes(test->encoding, test->encoding_len), Bytes(out, len));
+    EXPECT_EQ(Bytes(test.encoding, test.encoding_len), Bytes(out, len));
   }
 
-  for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(kASN1InvalidUint64Tests); i++) {
-    const ASN1InvalidUint64Test *test = &kASN1InvalidUint64Tests[i];
+  for (const ASN1InvalidUint64Test &test : kASN1InvalidUint64Tests) {
+    SCOPED_TRACE(Bytes(test.encoding, test.encoding_len));
     CBS cbs;
     uint64_t value;
 
-    CBS_init(&cbs, (const uint8_t *)test->encoding, test->encoding_len);
+    CBS_init(&cbs, (const uint8_t *)test.encoding, test.encoding_len);
     EXPECT_FALSE(CBS_get_asn1_uint64(&cbs, &value));
+
+    CBS_init(&cbs, (const uint8_t *)test.encoding, test.encoding_len);
+    CBS child;
+    if (CBS_get_asn1(&cbs, &child, CBS_ASN1_INTEGER)) {
+      EXPECT_EQ(test.overflow, !!CBS_is_unsigned_asn1_integer(&child));
+    }
   }
 }
 
@@ -793,50 +808,67 @@ static const ASN1Int64Test kASN1Int64Tests[] = {
 struct ASN1InvalidInt64Test {
   const char *encoding;
   size_t encoding_len;
+  bool overflow;
 };
 
 static const ASN1InvalidInt64Test kASN1InvalidInt64Tests[] = {
     // Bad tag.
-    {"\x03\x01\x00", 3},
+    {"\x03\x01\x00", 3, false},
     // Empty contents.
-    {"\x02\x00", 2},
+    {"\x02\x00", 2, false},
     // Overflow.
-    {"\x02\x09\x01\x00\x00\x00\x00\x00\x00\x00\x00", 11},
+    {"\x02\x09\x01\x00\x00\x00\x00\x00\x00\x00\x00", 11, true},
+    // Underflow.
+    {"\x02\x09\x08\xff\xff\xff\xff\xff\xff\xff\xff", 11, true},
     // Leading zeros.
-    {"\x02\x02\x00\x01", 4},
+    {"\x02\x02\x00\x01", 4, false},
     // Leading 0xff.
-    {"\x02\x02\xff\xff", 4},
+    {"\x02\x02\xff\xff", 4, false},
 };
 
 TEST(CBSTest, ASN1Int64) {
-  for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(kASN1Int64Tests); i++) {
-    SCOPED_TRACE(i);
-    const ASN1Int64Test *test = &kASN1Int64Tests[i];
+  for (const ASN1Int64Test &test : kASN1Int64Tests) {
+    SCOPED_TRACE(Bytes(test.encoding, test.encoding_len));
+    SCOPED_TRACE(test.value);
     CBS cbs;
     int64_t value;
     uint8_t *out;
     size_t len;
 
-    CBS_init(&cbs, (const uint8_t *)test->encoding, test->encoding_len);
+    CBS_init(&cbs, (const uint8_t *)test.encoding, test.encoding_len);
     ASSERT_TRUE(CBS_get_asn1_int64(&cbs, &value));
     EXPECT_EQ(0u, CBS_len(&cbs));
-    EXPECT_EQ(test->value, value);
+    EXPECT_EQ(test.value, value);
+
+    CBS child;
+    int is_negative;
+    CBS_init(&cbs, (const uint8_t *)test.encoding, test.encoding_len);
+    ASSERT_TRUE(CBS_get_asn1(&cbs, &child, CBS_ASN1_INTEGER));
+    EXPECT_TRUE(CBS_is_valid_asn1_integer(&child, &is_negative));
+    EXPECT_EQ(test.value < 0, !!is_negative);
+    EXPECT_EQ(test.value >= 0, !!CBS_is_unsigned_asn1_integer(&child));
 
     bssl::ScopedCBB cbb;
     ASSERT_TRUE(CBB_init(cbb.get(), 0));
-    ASSERT_TRUE(CBB_add_asn1_int64(cbb.get(), test->value));
+    ASSERT_TRUE(CBB_add_asn1_int64(cbb.get(), test.value));
     ASSERT_TRUE(CBB_finish(cbb.get(), &out, &len));
     bssl::UniquePtr<uint8_t> scoper(out);
-    EXPECT_EQ(Bytes(test->encoding, test->encoding_len), Bytes(out, len));
+    EXPECT_EQ(Bytes(test.encoding, test.encoding_len), Bytes(out, len));
   }
 
-  for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(kASN1InvalidInt64Tests); i++) {
-    const ASN1InvalidInt64Test *test = &kASN1InvalidInt64Tests[i];
+  for (const ASN1InvalidInt64Test &test : kASN1InvalidInt64Tests) {
+    SCOPED_TRACE(Bytes(test.encoding, test.encoding_len));
     CBS cbs;
     int64_t value;
 
-    CBS_init(&cbs, (const uint8_t *)test->encoding, test->encoding_len);
+    CBS_init(&cbs, (const uint8_t *)test.encoding, test.encoding_len);
     EXPECT_FALSE(CBS_get_asn1_int64(&cbs, &value));
+
+    CBS_init(&cbs, (const uint8_t *)test.encoding, test.encoding_len);
+    CBS child;
+    if (CBS_get_asn1(&cbs, &child, CBS_ASN1_INTEGER)) {
+      EXPECT_EQ(test.overflow, !!CBS_is_valid_asn1_integer(&child, NULL));
+    }
   }
 }
 
diff --git a/crypto/bytestring/cbs.c b/crypto/bytestring/cbs.c
index 49d7003..d7b2af7 100644
--- a/crypto/bytestring/cbs.c
+++ b/crypto/bytestring/cbs.c
@@ -426,29 +426,14 @@ int CBS_peek_asn1_tag(const CBS *cbs, unsigned tag_value) {
 
 int CBS_get_asn1_uint64(CBS *cbs, uint64_t *out) {
   CBS bytes;
-  if (!CBS_get_asn1(cbs, &bytes, CBS_ASN1_INTEGER)) {
+  if (!CBS_get_asn1(cbs, &bytes, CBS_ASN1_INTEGER) ||
+      !CBS_is_unsigned_asn1_integer(&bytes)) {
     return 0;
   }
 
   *out = 0;
   const uint8_t *data = CBS_data(&bytes);
   size_t len = CBS_len(&bytes);
-
-  if (len == 0) {
-    // An INTEGER is encoded with at least one octet.
-    return 0;
-  }
-
-  if ((data[0] & 0x80) != 0) {
-    // Negative number.
-    return 0;
-  }
-
-  if (data[0] == 0 && len > 1 && (data[1] & 0x80) == 0) {
-    // Extra leading zeros.
-    return 0;
-  }
-
   for (size_t i = 0; i < len; i++) {
     if ((*out >> 56) != 0) {
       // Too large to represent as a uint64_t.
@@ -462,31 +447,21 @@ int CBS_get_asn1_uint64(CBS *cbs, uint64_t *out) {
 }
 
 int CBS_get_asn1_int64(CBS *cbs, int64_t *out) {
+  int is_negative;
   CBS bytes;
-  if (!CBS_get_asn1(cbs, &bytes, CBS_ASN1_INTEGER)) {
+  if (!CBS_get_asn1(cbs, &bytes, CBS_ASN1_INTEGER) ||
+      !CBS_is_valid_asn1_integer(&bytes, &is_negative)) {
     return 0;
   }
   const uint8_t *data = CBS_data(&bytes);
   const size_t len = CBS_len(&bytes);
-
-  if (len == 0 || len > sizeof(int64_t)) {
-    // An INTEGER is encoded with at least one octet.
+  if (len > sizeof(int64_t)) {
     return 0;
   }
-  if (len > 1) {
-    if (data[0] == 0 && (data[1] & 0x80) == 0) {
-      return 0;  // Extra leading zeros.
-    }
-    if (data[0] == 0xff && (data[1] & 0x80) != 0) {
-      return 0;  // Extra leading 0xff.
-    }
-  }
-
   union {
     int64_t i;
     uint8_t bytes[sizeof(int64_t)];
   } u;
-  const int is_negative = (data[0] & 0x80);
   memset(u.bytes, is_negative ? 0xff : 0, sizeof(u.bytes));  // Sign-extend.
   for (size_t i = 0; i < len; i++) {
     u.bytes[i] = data[len - i - 1];
@@ -635,6 +610,30 @@ int CBS_asn1_bitstring_has_bit(const CBS *cbs, unsigned bit) {
          (CBS_data(cbs)[byte_num] & (1 << bit_num)) != 0;
 }
 
+int CBS_is_valid_asn1_integer(const CBS *cbs, int *out_is_negative) {
+  CBS copy = *cbs;
+  uint8_t first_byte, second_byte;
+  if (!CBS_get_u8(&copy, &first_byte)) {
+    return 0;  // INTEGERs may not be empty.
+  }
+  if (out_is_negative != NULL) {
+    *out_is_negative = (first_byte & 0x80) != 0;
+  }
+  if (!CBS_get_u8(&copy, &second_byte)) {
+    return 1;  // One byte INTEGERs are always minimal.
+  }
+  if ((first_byte == 0x00 && (second_byte & 0x80) == 0) ||
+      (first_byte == 0xff && (second_byte & 0x80) != 0)) {
+    return 0;  // The value is minimal iff the first 9 bits are not all equal.
+  }
+  return 1;
+}
+
+int CBS_is_unsigned_asn1_integer(const CBS *cbs) {
+  int is_negative;
+  return CBS_is_valid_asn1_integer(cbs, &is_negative) && !is_negative;
+}
+
 static int add_decimal(CBB *out, uint64_t v) {
   char buf[DECIMAL_SIZE(uint64_t) + 1];
   BIO_snprintf(buf, sizeof(buf), "%" PRIu64, v);