Add a function for validating a DER-encoded INTEGER.

This also modifies der::ParseUint64() to support the full uint64_t range.

BUG=410574

Review URL: https://codereview.chromium.org/1295943002

Cr-Commit-Position: refs/heads/master@{#344081}

3 files changed, 128 insertions, 36 deletions

diff --git a/net/der/parse_values.cc b/net/der/parse_values.cc
index 4459032..7920d9f 100644
--- a/net/der/parse_values.cc
+++ b/net/der/parse_values.cc
@@ -111,6 +111,26 @@ bool ValidateGeneralizedTime(const GeneralizedTime& time) {
   return true;
 }
 
+// Returns the number of bytes of numeric precision in a DER encoded INTEGER
+// value. |in| must be a valid DER encoding of an INTEGER for this to work.
+//
+// Normally the precision of the number is exactly in.Length(). However when
+// encoding positive numbers using DER it is possible to have a leading zero
+// (to prevent number from being interpreted as negative).
+//
+// For instance a 160-bit positive number might take 21 bytes to encode. This
+// function will return 20 in such a case.
+size_t GetUnsignedIntegerLength(const Input& in) {
+  der::ByteReader reader(in);
+  uint8_t first_byte;
+  if (!reader.ReadByte(&first_byte))
+    return 0;  // Not valid DER  as |in| was empty.
+
+  if (first_byte == 0 && in.Length() > 1)
+    return in.Length() - 1;
+  return in.Length();
+}
+
 }  // namespace
 
 bool ParseBool(const Input& in, bool* out) {
@@ -125,39 +145,47 @@ bool ParseBoolRelaxed(const Input& in, bool* out) {
   return ParseBoolInternal(in, out, true /* relaxed */);
 }
 
+// ITU-T X.690 section 8.3.2 specifies that an integer value must be encoded
+// in the smallest number of octets. If the encoding consists of more than
+// one octet, then the bits of the first octet and the most significant bit
+// of the second octet must not be all zeroes or all ones.
+bool IsValidInteger(const Input& in, bool* negative) {
+  der::ByteReader reader(in);
+  uint8_t first_byte;
+
+  if (!reader.ReadByte(&first_byte))
+    return false;  // Empty inputs are not allowed.
+
+  uint8_t second_byte;
+  if (reader.ReadByte(&second_byte)) {
+    if ((first_byte == 0x00 || first_byte == 0xFF) &&
+        (first_byte & 0x80) == (second_byte & 0x80)) {
+      // Not a minimal encoding.
+      return false;
+    }
+  }
+
+  *negative = (first_byte & 0x80) == 0x80;
+  return true;
+}
+
 bool ParseUint64(const Input& in, uint64_t* out) {
+  // Reject non-minimally encoded numbers and negative numbers.
+  bool negative;
+  if (!IsValidInteger(in, &negative) || negative)
+    return false;
+
+  // Reject (non-negative) integers whose value would overflow the output type.
+  if (GetUnsignedIntegerLength(in) > sizeof(*out))
+    return false;
+
   ByteReader reader(in);
-  size_t bytes_read = 0;
   uint8_t data;
   uint64_t value = 0;
-  // Note that for simplicity, this check only admits integers up to 2^63-1.
-  if (in.Length() > sizeof(uint64_t) || in.Length() == 0)
-    return false;
+
   while (reader.ReadByte(&data)) {
-    if (bytes_read == 0 && (data & 0x80)) {
-      return false;
-    }
     value <<= 8;
     value |= data;
-    bytes_read++;
-  }
-  // ITU-T X.690 section 8.3.2 specifies that an integer value must be encoded
-  // in the smallest number of octets. If the encoding consists of more than
-  // one octet, then the bits of the first octet and the most significant bit
-  // of the second octet must not be all zeroes or all ones.
-  // Because this function only parses unsigned integers, there's no need to
-  // check for the all ones case.
-  if (bytes_read > 1) {
-    ByteReader first_bytes_reader(in);
-    uint8_t first_byte;
-    uint8_t second_byte;
-    if (!first_bytes_reader.ReadByte(&first_byte) ||
-        !first_bytes_reader.ReadByte(&second_byte)) {
-      return false;
-    }
-    if (first_byte == 0 && !(second_byte & 0x80)) {
-      return false;
-    }
   }
   *out = value;
   return true;
diff --git a/net/der/parse_values.h b/net/der/parse_values.h
index 39c3c02..27a6f5f 100644
--- a/net/der/parse_values.h
+++ b/net/der/parse_values.h
@@ -22,12 +22,23 @@ NET_EXPORT bool ParseBool(const Input& in, bool* out) WARN_UNUSED_RESULT;
 // value that is a valid BER encoding will be parsed successfully.
 NET_EXPORT bool ParseBoolRelaxed(const Input& in, bool* out) WARN_UNUSED_RESULT;
 
+// Checks the validity of a DER-encoded ASN.1 INTEGER value from |in|, and
+// determines the sign of the number. Returns true on success and
+// fills |negative|. Otherwise returns false and does not modify the out
+// parameter.
+//
+//    in: The value portion of an INTEGER.
+//    negative: Out parameter that is set to true if the number is negative
+//        and false otherwise (zero is non-negative).
+NET_EXPORT bool IsValidInteger(const Input& in,
+                               bool* negative) WARN_UNUSED_RESULT;
+
 // Reads a DER-encoded ASN.1 INTEGER value from |in| and puts the resulting
 // value in |out|. ASN.1 INTEGERs are arbitrary precision; this function is
 // provided as a convenience when the caller knows that the value is unsigned
-// and is between 0 and 2^63-1. This function does not support the full range of
-// uint64_t. This function returns false if the value is too big to fit in a
-// uint64_t, is negative, or if there is an error reading the integer.
+// and is between 0 and 2^64-1. This function returns false if the value is too
+// big to fit in a uint64_t, is negative, or if there is an error reading the
+// integer.
 NET_EXPORT bool ParseUint64(const Input& in, uint64_t* out) WARN_UNUSED_RESULT;
 
 // The BitString class is a helper for representing a valid parsed BIT STRING.
diff --git a/net/der/parse_values_unittest.cc b/net/der/parse_values_unittest.cc
index ff0aafc..b76c4c9 100644
--- a/net/der/parse_values_unittest.cc
+++ b/net/der/parse_values_unittest.cc
@@ -184,19 +184,26 @@ struct Uint64TestData {
 
 const Uint64TestData kUint64TestData[] = {
     {true, {0x00}, 1, 0},
+    // This number fails because it is not a minimal representation.
+    {false, {0x00, 0x00}, 2},
     {true, {0x01}, 1, 1},
-    {false, {0xFF}, 1, 0},
+    {false, {0xFF}, 1},
     {true, {0x7F, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF}, 8, INT64_MAX},
-    {false, {0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}, 8, 0},
-    {false, {0x00, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF}, 9, 0},
-    {false, {0x00, 0x01}, 2, 1},
-    {false, {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09}, 9, 0},
-    {false, {0}, 0, 0},
+    {true,
+     {0x00, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF},
+     9,
+     UINT64_MAX},
+    // This number fails because it is negative.
+    {false, {0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF}, 8},
+    {false, {0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}, 8},
+    {false, {0x00, 0x01}, 2},
+    {false, {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09}, 9},
+    {false, {0}, 0},
 };
 
 TEST(ParseValuesTest, ParseUint64) {
   for (size_t i = 0; i < arraysize(kUint64TestData); i++) {
-    Uint64TestData test_case = kUint64TestData[i];
+    const Uint64TestData& test_case = kUint64TestData[i];
     SCOPED_TRACE(i);
 
     uint64_t result;
@@ -207,6 +214,52 @@ TEST(ParseValuesTest, ParseUint64) {
   }
 }
 
+struct IsValidIntegerTestData {
+  bool should_pass;
+  const uint8_t input[2];
+  size_t length;
+  bool negative;
+};
+
+const IsValidIntegerTestData kIsValidIntegerTestData[] = {
+    // Empty input (invalid DER).
+    {false, {0x00}, 0},
+
+    // The correct encoding for zero.
+    {true, {0x00}, 1, false},
+
+    // Invalid representation of zero (not minimal)
+    {false, {0x00, 0x00}, 2},
+
+    // Valid single byte negative numbers.
+    {true, {0x80}, 1, true},
+    {true, {0xFF}, 1, true},
+
+    // Non-minimal negative number.
+    {false, {0xFF, 0x80}, 2},
+
+    // Positive number with a legitimate leading zero.
+    {true, {0x00, 0x80}, 2, false},
+
+    // A legitimate negative number that starts with FF (MSB of second byte is
+    // 0 so OK).
+    {true, {0xFF, 0x7F}, 2, true},
+};
+
+TEST(ParseValuesTest, IsValidInteger) {
+  for (size_t i = 0; i < arraysize(kIsValidIntegerTestData); i++) {
+    const auto& test_case = kIsValidIntegerTestData[i];
+    SCOPED_TRACE(i);
+
+    bool negative;
+    EXPECT_EQ(
+        test_case.should_pass,
+        IsValidInteger(Input(test_case.input, test_case.length), &negative));
+    if (test_case.should_pass)
+      EXPECT_EQ(test_case.negative, negative);
+  }
+}
+
 // Tests parsing an empty BIT STRING.
 TEST(ParseValuesTest, ParseBitStringEmptyNoUnusedBits) {
   const uint8_t kData[] = {0x00};