Bug 1059924, Part 1: Centralize tag and length decoding in mozilla::pkix's DER decoder, r=keeler
authorBrian Smith <brian@briansmith.org>
Tue, 02 Sep 2014 22:03:30 -0700
changeset 14671 956856eaf246f26c26d6c874d7b510a17990e2d7
parent 14670 1c6057955f8a787f571df5c3523787d2b036f348
child 14672 cf0ddad16d73713fe3bf4c06415b7fc27932ee14
push id3202
push userfranziskuskiefer@gmail.com
push dateMon, 01 Oct 2018 08:30:12 +0000
reviewerskeeler
bugs1059924
Bug 1059924, Part 1: Centralize tag and length decoding in mozilla::pkix's DER decoder, r=keeler
lib/mozpkix/include/pkix/Input.h
lib/mozpkix/lib/pkixder.cpp
lib/mozpkix/lib/pkixder.h
lib/mozpkix/lib/pkixocsp.cpp
lib/mozpkix/test/gtest/pkixder_input_tests.cpp
--- a/lib/mozpkix/include/pkix/Input.h
+++ b/lib/mozpkix/include/pkix/Input.h
@@ -150,16 +150,26 @@ public:
   }
 
   explicit Reader(Input input)
     : input(input.UnsafeGetData())
     , end(input.UnsafeGetData() + input.GetLength())
   {
   }
 
+  Result Init(Input input)
+  {
+    if (this->input) {
+      return Result::FATAL_ERROR_INVALID_ARGS;
+    }
+    this->input = input.UnsafeGetData();
+    this->end = input.UnsafeGetData() + input.GetLength();
+    return Success;
+  }
+
   bool Peek(uint8_t expectedByte) const
   {
     return input < end && *input == expectedByte;
   }
 
   Result Read(uint8_t& out)
   {
     Result rv = EnsureLength(1);
--- a/lib/mozpkix/lib/pkixder.cpp
+++ b/lib/mozpkix/lib/pkixder.cpp
@@ -24,34 +24,31 @@
 
 #include "pkixder.h"
 
 #include "pkix/bind.h"
 #include "pkixutil.h"
 
 namespace mozilla { namespace pkix { namespace der {
 
-namespace internal {
-
 // Too complicated to be inline
 Result
-ExpectTagAndGetLength(Reader& input, uint8_t expectedTag, uint16_t& length)
+ReadTagAndGetValue(Reader& input, /*out*/ uint8_t& tag, /*out*/ Input& value)
 {
-  assert((expectedTag & 0x1F) != 0x1F); // high tag number form not allowed
+  Result rv;
 
-  uint8_t tag;
-  Result rv;
   rv = input.Read(tag);
   if (rv != Success) {
     return rv;
   }
+  if ((tag & 0x1F) == 0x1F) {
+    return Result::ERROR_BAD_DER; // high tag number form not allowed
+  }
 
-  if (tag != expectedTag) {
-    return Result::ERROR_BAD_DER;
-  }
+  uint16_t length;
 
   // The short form of length is a single byte with the high order bit set
   // to zero. The long form of length is one byte with the high order bit
   // set, followed by N bytes, where N is encoded in the lowest 7 bits of
   // the first byte.
   uint8_t length1;
   rv = input.Read(length1);
   if (rv != Success) {
@@ -79,22 +76,19 @@ ExpectTagAndGetLength(Reader& input, uin
       // Not shortest possible encoding
       return Result::ERROR_BAD_DER;
     }
   } else {
     // We don't support lengths larger than 2^16 - 1.
     return Result::ERROR_BAD_DER;
   }
 
-  // Ensure the input is long enough for the length it says it has.
-  return input.EnsureLength(length);
+  return input.Skip(length, value);
 }
 
-} // namespace internal
-
 static Result
 OptionalNull(Reader& input)
 {
   if (input.Peek(NULLTag)) {
     return Null(input);
   }
   return Success;
 }
--- a/lib/mozpkix/lib/pkixder.h
+++ b/lib/mozpkix/lib/pkixder.h
@@ -66,90 +66,70 @@ enum Tag
   ENUMERATED = UNIVERSAL | 0x0a,
   SEQUENCE = UNIVERSAL | CONSTRUCTED | 0x10, // 0x30
   UTCTime = UNIVERSAL | 0x17,
   GENERALIZED_TIME = UNIVERSAL | 0x18,
 };
 
 MOZILLA_PKIX_ENUM_CLASS EmptyAllowed { No = 0, Yes = 1 };
 
-inline Result
-ExpectTagAndLength(Reader& input, uint8_t expectedTag, uint8_t expectedLength)
-{
-  assert((expectedTag & 0x1F) != 0x1F); // high tag number form not allowed
-  assert(expectedLength < 128); // must be a single-byte length
-
-  uint16_t tagAndLength;
-  Result rv = input.Read(tagAndLength);
-  if (rv != Success) {
-    return rv;
-  }
-
-  uint16_t expectedTagAndLength = static_cast<uint16_t>(expectedTag << 8);
-  expectedTagAndLength |= expectedLength;
-
-  if (tagAndLength != expectedTagAndLength) {
-    return Result::ERROR_BAD_DER;
-  }
-
-  return Success;
-}
-
-namespace internal {
-
-Result
-ExpectTagAndGetLength(Reader& input, uint8_t expectedTag, uint16_t& length);
-
-} // namespace internal
-
-inline Result
-ExpectTagAndSkipValue(Reader& input, uint8_t tag)
-{
-  uint16_t length;
-  Result rv = internal::ExpectTagAndGetLength(input, tag, length);
-  if (rv != Success) {
-    return rv;
-  }
-  return input.Skip(length);
-}
+Result ReadTagAndGetValue(Reader& input, /*out*/ uint8_t& tag,
+                          /*out*/ Input& value);
+Result End(Reader& input);
 
 inline Result
 ExpectTagAndGetValue(Reader& input, uint8_t tag, /*out*/ Input& value)
 {
-  uint16_t length;
-  Result rv = internal::ExpectTagAndGetLength(input, tag, length);
+  uint8_t actualTag;
+  Result rv = ReadTagAndGetValue(input, actualTag, value);
   if (rv != Success) {
     return rv;
   }
-  return input.Skip(length, value);
+  if (tag != actualTag) {
+    return Result::ERROR_BAD_DER;
+  }
+  return Success;
 }
 
 inline Result
 ExpectTagAndGetValue(Reader& input, uint8_t tag, /*out*/ Reader& value)
 {
-  uint16_t length;
-  Result rv = internal::ExpectTagAndGetLength(input, tag, length);
+  Input valueInput;
+  Result rv = ExpectTagAndGetValue(input, tag, valueInput);
   if (rv != Success) {
     return rv;
   }
-  return input.Skip(length, value);
+  return value.Init(valueInput);
+}
+
+inline Result
+ExpectTagAndEmptyValue(Reader& input, uint8_t tag)
+{
+  Reader value;
+  Result rv = ExpectTagAndGetValue(input, tag, value);
+  if (rv != Success) {
+    return rv;
+  }
+  return End(value);
+}
+
+inline Result
+ExpectTagAndSkipValue(Reader& input, uint8_t tag)
+{
+  Input ignoredValue;
+  return ExpectTagAndGetValue(input, tag, ignoredValue);
 }
 
 // Like ExpectTagAndGetValue, except the output Input will contain the
 // encoded tag and length along with the value.
 inline Result
 ExpectTagAndGetTLV(Reader& input, uint8_t tag, /*out*/ Input& tlv)
 {
   Reader::Mark mark(input.GetMark());
-  uint16_t length;
-  Result rv = internal::ExpectTagAndGetLength(input, tag, length);
-  if (rv != Success) {
-    return rv;
-  }
-  rv = input.Skip(length);
+  Result rv = ExpectTagAndSkipValue(input, tag);
   if (rv != Success) {
     return rv;
   }
   return input.GetInput(mark, tlv);
 }
 
 inline Result
 End(Reader& input)
@@ -248,47 +228,53 @@ namespace internal {
 // This parser will only parse values between 0..127. If this range is
 // increased then callers will need to be changed.
 template <typename T> inline Result
 IntegralValue(Reader& input, uint8_t tag, T& value)
 {
   // Conveniently, all the Integers that we actually have to be able to parse
   // are positive and very small. Consequently, this parser is *much* simpler
   // than a general Integer parser would need to be.
-  Result rv = ExpectTagAndLength(input, tag, 1);
+  Reader valueReader;
+  Result rv = ExpectTagAndGetValue(input, tag, valueReader);
   if (rv != Success) {
     return rv;
   }
   uint8_t valueByte;
-  rv = input.Read(valueByte);
+  rv = valueReader.Read(valueByte);
   if (rv != Success) {
     return rv;
   }
   if (valueByte & 0x80) { // negative
     return Result::ERROR_BAD_DER;
   }
   value = valueByte;
-  return Success;
+  return End(valueReader);
 }
 
 } // namespace internal
 
 Result
 BitStringWithNoUnusedBits(Reader& input, /*out*/ Input& value);
 
 inline Result
 Boolean(Reader& input, /*out*/ bool& value)
 {
-  Result rv = ExpectTagAndLength(input, BOOLEAN, 1);
+  Reader valueReader;
+  Result rv = ExpectTagAndGetValue(input, BOOLEAN, valueReader);
   if (rv != Success) {
     return rv;
   }
 
   uint8_t intValue;
-  rv = input.Read(intValue);
+  rv = valueReader.Read(intValue);
+  if (rv != Success) {
+    return rv;
+  }
+  rv = End(valueReader);
   if (rv != Success) {
     return rv;
   }
   switch (intValue) {
     case 0: value = false; return Success;
     case 0xFF: value = true; return Success;
     default:
       return Result::ERROR_BAD_DER;
@@ -388,17 +374,17 @@ OptionalInteger(Reader& input, long defa
   }
   value = parsedValue;
   return Success;
 }
 
 inline Result
 Null(Reader& input)
 {
-  return ExpectTagAndLength(input, NULLTag, 0);
+  return ExpectTagAndEmptyValue(input, NULLTag);
 }
 
 template <uint8_t Len>
 Result
 OID(Reader& input, const uint8_t (&expectedOid)[Len])
 {
   Reader value;
   Result rv = ExpectTagAndGetValue(input, OIDTag, value);
--- a/lib/mozpkix/lib/pkixocsp.cpp
+++ b/lib/mozpkix/lib/pkixocsp.cpp
@@ -547,18 +547,18 @@ SingleResponse(Reader& input, Context& c
   //     unknown     [2]     IMPLICIT UnknownInfo }
   //
   // In the event of multiple SingleResponses for a cert that have conflicting
   // statuses, we use the following precedence rules:
   //
   // * revoked overrides good and unknown
   // * good overrides unknown
   if (input.Peek(static_cast<uint8_t>(CertStatus::Good))) {
-    rv = der::ExpectTagAndLength(input, static_cast<uint8_t>(CertStatus::Good),
-                                 0);
+    rv = der::ExpectTagAndEmptyValue(input,
+                                     static_cast<uint8_t>(CertStatus::Good));
     if (rv != Success) {
       return rv;
     }
     if (context.certStatus != CertStatus::Revoked) {
       context.certStatus = CertStatus::Good;
     }
   } else if (input.Peek(static_cast<uint8_t>(CertStatus::Revoked))) {
     // We don't need any info from the RevokedInfo structure, so we don't even
@@ -567,18 +567,18 @@ SingleResponse(Reader& input, Context& c
     // stapling.
     rv = der::ExpectTagAndSkipValue(input,
                                     static_cast<uint8_t>(CertStatus::Revoked));
     if (rv != Success) {
       return rv;
     }
     context.certStatus = CertStatus::Revoked;
   } else {
-    rv = der::ExpectTagAndLength(input,
-                                 static_cast<uint8_t>(CertStatus::Unknown), 0);
+    rv = der::ExpectTagAndEmptyValue(input,
+                                     static_cast<uint8_t>(CertStatus::Unknown));
     if (rv != Success) {
       return rv;
     }
   }
 
   // http://tools.ietf.org/html/rfc6960#section-3.2
   // 5. The time at which the status being indicated is known to be
   //    correct (thisUpdate) is sufficiently recent;
--- a/lib/mozpkix/test/gtest/pkixder_input_tests.cpp
+++ b/lib/mozpkix/test/gtest/pkixder_input_tests.cpp
@@ -81,16 +81,18 @@ const uint8_t DER_OVERRUN_SEQUENCE_OF_IN
 };
 
 const uint8_t DER_INT16[] = {
   0x02,                       // INTEGER
   0x02,                       // length
   0x12, 0x34                  // 0x1234
 };
 
+static const Input EMPTY_INPUT;
+
 TEST_F(pkixder_input_tests, InputInit)
 {
   Input buf;
   ASSERT_EQ(Success,
             buf.Init(DER_SEQUENCE_OF_INT8, sizeof DER_SEQUENCE_OF_INT8));
 }
 
 TEST_F(pkixder_input_tests, InputInitWithNullPointerOrZeroLength)
@@ -454,74 +456,77 @@ TEST_F(pkixder_input_tests, MarkAndGetIn
 
   ASSERT_EQ(Success, input.Skip(3));
 
   Input item;
   ASSERT_EQ(Result::FATAL_ERROR_INVALID_ARGS, input.GetInput(mark, item));
 }
 #endif
 
-TEST_F(pkixder_input_tests, ExpectTagAndLength)
+TEST_F(pkixder_input_tests, ReadTagAndGetValue_Input_AtEnd)
 {
-  Input buf(DER_SEQUENCE_OF_INT8);
-  Reader input(buf);
-
-  ASSERT_EQ(Success, ExpectTagAndLength(input, SEQUENCE,
-                                        sizeof DER_SEQUENCE_OF_INT8 - 2));
-}
-
-TEST_F(pkixder_input_tests, ExpectTagAndLengthWithWrongLength)
-{
-  Input buf(DER_INT16);
-  Reader input(buf);
-
-  // Wrong length
-  ASSERT_EQ(Result::ERROR_BAD_DER, ExpectTagAndLength(input, INTEGER, 4));
+  Reader input(EMPTY_INPUT);
+  uint8_t tag;
+  Input value;
+  ASSERT_EQ(Result::ERROR_BAD_DER, ReadTagAndGetValue(input, tag, value));
 }
 
-TEST_F(pkixder_input_tests, ExpectTagAndLengthWithWrongTag)
+TEST_F(pkixder_input_tests, ReadTagAndGetValue_Input_TruncatedAfterTag)
 {
-  Input buf(DER_INT16);
+  static const uint8_t DER[] = { SEQUENCE };
+  Input buf(DER);
   Reader input(buf);
-
-  // Wrong type
-  ASSERT_EQ(Result::ERROR_BAD_DER, ExpectTagAndLength(input, OCTET_STRING, 2));
+  uint8_t tag;
+  Input value;
+  ASSERT_EQ(Result::ERROR_BAD_DER, ReadTagAndGetValue(input, tag, value));
 }
 
-TEST_F(pkixder_input_tests, ExpectTagAndGetLength)
+TEST_F(pkixder_input_tests, ReadTagAndGetValue_Input_ValidEmpty)
 {
-  Input buf(DER_SEQUENCE_OF_INT8);
+  Input buf(DER_SEQUENCE_EMPTY);
   Reader input(buf);
-
-  uint16_t length = 0;
-  ASSERT_EQ(Success,
-            der::internal::ExpectTagAndGetLength(input, SEQUENCE, length));
-  ASSERT_EQ(sizeof DER_SEQUENCE_OF_INT8 - 2, length);
-  ASSERT_EQ(Success, input.Skip(length));
+  uint8_t tag = 0;
+  Input value;
+  ASSERT_EQ(Success, ReadTagAndGetValue(input, tag, value));
+  ASSERT_EQ(SEQUENCE, tag);
+  ASSERT_EQ(0u, value.GetLength());
   ASSERT_TRUE(input.AtEnd());
 }
 
-TEST_F(pkixder_input_tests, ExpectTagAndGetLengthWithWrongTag)
+TEST_F(pkixder_input_tests, ReadTagAndGetValue_Input_ValidNotEmpty)
 {
-  Input buf(DER_SEQUENCE_OF_INT8);
+  Input buf(DER_SEQUENCE_NOT_EMPTY);
   Reader input(buf);
-
-  uint16_t length = 0;
-  ASSERT_EQ(Result::ERROR_BAD_DER,
-            der::internal::ExpectTagAndGetLength(input, INTEGER, length));
+  uint8_t tag = 0;
+  Input value;
+  ASSERT_EQ(Success, ReadTagAndGetValue(input, tag, value));
+  ASSERT_EQ(SEQUENCE, tag);
+  Input expected(DER_SEQUENCE_NOT_EMPTY_VALUE);
+  ASSERT_TRUE(InputsAreEqual(expected, value));
+  ASSERT_TRUE(input.AtEnd());
 }
 
-TEST_F(pkixder_input_tests, ExpectTagAndGetLengthWithWrongLength)
+TEST_F(pkixder_input_tests,
+       ReadTagAndGetValue_Input_InvalidNotEmptyValueTruncated)
+{
+  Input buf(DER_SEQUENCE_NOT_EMPTY_VALUE_TRUNCATED);
+  Reader input(buf);
+  uint8_t tag;
+  Input value;
+  ASSERT_EQ(Result::ERROR_BAD_DER, ReadTagAndGetValue(input, tag, value));
+}
+
+TEST_F(pkixder_input_tests, ReadTagAndGetValue_Input_InvalidWrongLength)
 {
   Input buf(DER_TRUNCATED_SEQUENCE_OF_INT8);
   Reader input(buf);
-
-  uint16_t length = 0;
+  uint8_t tag;
+  Input value;
   ASSERT_EQ(Result::ERROR_BAD_DER,
-            der::internal::ExpectTagAndGetLength(input, SEQUENCE, length));
+            ReadTagAndGetValue(input, tag, value));
 }
 
 TEST_F(pkixder_input_tests, ExpectTagAndGetValue_Reader_ValidEmpty)
 {
   Input buf(DER_SEQUENCE_EMPTY);
   Reader input(buf);
   Reader value;
   ASSERT_EQ(Success, ExpectTagAndGetValue(input, SEQUENCE, value));
@@ -553,17 +558,17 @@ TEST_F(pkixder_input_tests, ExpectTagAnd
 {
   Input buf(DER_TRUNCATED_SEQUENCE_OF_INT8);
   Reader input(buf);
   Reader value;
   ASSERT_EQ(Result::ERROR_BAD_DER,
             ExpectTagAndGetValue(input, SEQUENCE, value));
 }
 
-TEST_F(pkixder_input_tests, ExpectTagAndGetLength_Reader_InvalidWrongTag)
+TEST_F(pkixder_input_tests, ExpectTagAndGetValue_Reader_InvalidWrongTag)
 {
   Input buf(DER_SEQUENCE_NOT_EMPTY);
   Reader input(buf);
   Reader value;
   ASSERT_EQ(Result::ERROR_BAD_DER,
             ExpectTagAndGetValue(input, INTEGER, value));
 }
 
@@ -602,25 +607,62 @@ TEST_F(pkixder_input_tests, ExpectTagAnd
 {
   Input buf(DER_TRUNCATED_SEQUENCE_OF_INT8);
   Reader input(buf);
   Input value;
   ASSERT_EQ(Result::ERROR_BAD_DER,
             ExpectTagAndGetValue(input, SEQUENCE, value));
 }
 
-TEST_F(pkixder_input_tests, ExpectTagAndGetLength_Input_InvalidWrongTag)
+TEST_F(pkixder_input_tests, ExpectTagAndGetValue_Input_InvalidWrongTag)
 {
   Input buf(DER_SEQUENCE_NOT_EMPTY);
   Reader input(buf);
   Input value;
   ASSERT_EQ(Result::ERROR_BAD_DER,
             ExpectTagAndGetValue(input, INTEGER, value));
 }
 
+TEST_F(pkixder_input_tests, ExpectTagAndEmptyValue_ValidEmpty)
+{
+  Input buf(DER_SEQUENCE_EMPTY);
+  Reader input(buf);
+  ASSERT_EQ(Success, ExpectTagAndEmptyValue(input, SEQUENCE));
+  ASSERT_TRUE(input.AtEnd());
+}
+
+TEST_F(pkixder_input_tests, ExpectTagAndEmptyValue_InValidNotEmpty)
+{
+  Input buf(DER_SEQUENCE_NOT_EMPTY);
+  Reader input(buf);
+  ASSERT_EQ(Result::ERROR_BAD_DER, ExpectTagAndEmptyValue(input, SEQUENCE));
+}
+
+TEST_F(pkixder_input_tests,
+       ExpectTagAndEmptyValue_Input_InvalidNotEmptyValueTruncated)
+{
+  Input buf(DER_SEQUENCE_NOT_EMPTY_VALUE_TRUNCATED);
+  Reader input(buf);
+  ASSERT_EQ(Result::ERROR_BAD_DER, ExpectTagAndEmptyValue(input, SEQUENCE));
+}
+
+TEST_F(pkixder_input_tests, ExpectTagAndEmptyValue_InvalidWrongLength)
+{
+  Input buf(DER_TRUNCATED_SEQUENCE_OF_INT8);
+  Reader input(buf);
+  ASSERT_EQ(Result::ERROR_BAD_DER, ExpectTagAndEmptyValue(input, SEQUENCE));
+}
+
+TEST_F(pkixder_input_tests, ExpectTagAndEmptyValue_InvalidWrongTag)
+{
+  Input buf(DER_SEQUENCE_NOT_EMPTY);
+  Reader input(buf);
+  ASSERT_EQ(Result::ERROR_BAD_DER, ExpectTagAndEmptyValue(input, INTEGER));
+}
+
 TEST_F(pkixder_input_tests, ExpectTagAndGetTLV_Input_ValidEmpty)
 {
   Input buf(DER_SEQUENCE_EMPTY);
   Reader input(buf);
   Input tlv;
   ASSERT_EQ(Success, ExpectTagAndGetTLV(input, SEQUENCE, tlv));
   Input expected(DER_SEQUENCE_EMPTY);
   ASSERT_TRUE(InputsAreEqual(expected, tlv));