Bug 1122841, Part 1: Add PositiveInteger parser, r=keeler
authorBrian Smith <brian@briansmith.org>
Fri, 06 Feb 2015 18:21:20 -0800
changeset 255645 75c440d6b2fff1191f4a48807243e5be1783c12a
parent 255644 80a2f25792fc84d2137ed485d07e8afefefe06c8
child 255646 3fe8d7d7f9f7373d0d3a3341d1a46347c06c85c7
push id4610
push userjlund@mozilla.com
push dateMon, 30 Mar 2015 18:32:55 +0000
treeherdermozilla-beta@4df54044d9ef [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1122841
milestone38.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1122841, Part 1: Add PositiveInteger parser, r=keeler
security/pkix/lib/pkixder.cpp
security/pkix/lib/pkixder.h
security/pkix/test/gtest/pkixder_universal_types_tests.cpp
security/pkix/test/lib/pkixtestutil.cpp
security/pkix/test/lib/pkixtestutil.h
--- a/security/pkix/lib/pkixder.cpp
+++ b/security/pkix/lib/pkixder.cpp
@@ -543,11 +543,77 @@ TimeChoice(Reader& tagged, uint8_t expec
                           (static_cast<uint64_t>(hours)      * 60u * 60u) +
                           (static_cast<uint64_t>(minutes)          * 60u) +
                           seconds;
 
   time = TimeFromElapsedSecondsAD(totalSeconds);
   return Success;
 }
 
+Result
+IntegralBytes(Reader& input, uint8_t tag,
+              IntegralValueRestriction valueRestriction,
+              /*out*/ Input& value,
+              /*optional out*/ Input::size_type* significantBytes)
+{
+  Result rv = ExpectTagAndGetValue(input, tag, value);
+  if (rv != Success) {
+    return rv;
+  }
+  Reader reader(value);
+
+  // There must be at least one byte in the value. (Zero is encoded with a
+  // single 0x00 value byte.)
+  uint8_t firstByte;
+  rv = reader.Read(firstByte);
+  if (rv != Success) {
+    return rv;
+  }
+
+  // If there is a byte after an initial 0x00/0xFF, then the initial byte
+  // indicates a positive/negative integer value with its high bit set/unset.
+  bool prefixed = !reader.AtEnd() && (firstByte == 0 || firstByte == 0xff);
+
+  if (prefixed) {
+    uint8_t nextByte;
+    if (reader.Read(nextByte) != Success) {
+      return NotReached("Read of one byte failed but not at end.",
+                        Result::FATAL_ERROR_LIBRARY_FAILURE);
+    }
+    if ((firstByte & 0x80) == (nextByte & 0x80)) {
+      return Result::ERROR_BAD_DER;
+    }
+  }
+
+  switch (valueRestriction) {
+    case IntegralValueRestriction::MustBe0To127:
+      if (value.GetLength() != 1 || (firstByte & 0x80) != 0) {
+        return Result::ERROR_BAD_DER;
+      }
+      break;
+
+    case IntegralValueRestriction::MustBePositive:
+      if ((value.GetLength() == 1 && firstByte == 0) ||
+          (firstByte & 0x80) != 0) {
+        return Result::ERROR_BAD_DER;
+      }
+      break;
+
+    case IntegralValueRestriction::NoRestriction:
+      break;
+  }
+
+  if (significantBytes) {
+    *significantBytes = value.GetLength();
+    if (prefixed) {
+      assert(*significantBytes > 1);
+      --*significantBytes;
+    }
+
+    assert(*significantBytes > 0);
+  }
+
+  return Success;
+}
+
 } // namespace internal
 
 } } } // namespace mozilla::pkix::der
--- a/security/pkix/lib/pkixder.h
+++ b/security/pkix/lib/pkixder.h
@@ -256,39 +256,52 @@ ExpectTagAndGetValueAtEnd(Input outer, u
   Reader outerReader(outer);
   return ExpectTagAndGetValueAtEnd(outerReader, expectedTag, inner);
 }
 
 // Universal types
 
 namespace internal {
 
+enum class IntegralValueRestriction
+{
+  NoRestriction,
+  MustBePositive,
+  MustBe0To127,
+};
+
+Result IntegralBytes(Reader& input, uint8_t tag,
+                     IntegralValueRestriction valueRestriction,
+             /*out*/ Input& value,
+    /*optional out*/ Input::size_type* significantBytes = nullptr);
+
 // 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.
-  Reader valueReader;
-  Result rv = ExpectTagAndGetValue(input, tag, valueReader);
+  Input valueBytes;
+  Result rv = IntegralBytes(input, tag, IntegralValueRestriction::MustBe0To127,
+                            valueBytes, nullptr);
   if (rv != Success) {
     return rv;
   }
+  Reader valueReader(valueBytes);
   uint8_t valueByte;
   rv = valueReader.Read(valueByte);
   if (rv != Success) {
-    return rv;
-  }
-  if (valueByte & 0x80) { // negative
-    return Result::ERROR_BAD_DER;
+    return NotReached("IntegralBytes already validated the value.", rv);
   }
   value = valueByte;
-  return End(valueReader);
+  rv = End(valueReader);
+  assert(rv == Success); // guaranteed by IntegralBytes's range checks.
+  return rv;
 }
 
 } // namespace internal
 
 Result
 BitStringWithNoUnusedBits(Reader& input, /*out*/ Input& value);
 
 inline Result
@@ -370,16 +383,29 @@ GeneralizedTime(Reader& input, /*out*/ T
 // time formats that start at 1970.
 inline Result
 TimeChoice(Reader& input, /*out*/ Time& time)
 {
   uint8_t expectedTag = input.Peek(UTCTime) ? UTCTime : GENERALIZED_TIME;
   return internal::TimeChoice(input, expectedTag, time);
 }
 
+// Parse a DER integer value into value. Empty values, negative values, and
+// zero are rejected. If significantBytes is not null, then it will be set to
+// the number of significant bytes in the value (the length of the value, less
+// the length of any leading padding), which is useful for key size checks.
+inline Result
+PositiveInteger(Reader& input, /*out*/ Input& value,
+                /*optional out*/ Input::size_type* significantBytes = nullptr)
+{
+  return internal::IntegralBytes(
+           input, INTEGER, internal::IntegralValueRestriction::MustBePositive,
+           value, significantBytes);
+}
+
 // This parser will only parse values between 0..127. If this range is
 // increased then callers will need to be changed.
 inline Result
 Integer(Reader& input, /*out*/ uint8_t& value)
 {
   return internal::IntegralValue(input, INTEGER, value);
 }
 
@@ -441,50 +467,19 @@ CertificateSerialNumber(Reader& input, /
   // * "The serial number MUST be a positive integer assigned by the CA to
   //   each certificate."
   // * "Certificate users MUST be able to handle serialNumber values up to 20
   //   octets. Conforming CAs MUST NOT use serialNumber values longer than 20
   //   octets."
   // * "Note: Non-conforming CAs may issue certificates with serial numbers
   //   that are negative or zero.  Certificate users SHOULD be prepared to
   //   gracefully handle such certificates."
-
-  Result rv = ExpectTagAndGetValue(input, INTEGER, value);
-  if (rv != Success) {
-    return rv;
-  }
-
-  if (value.GetLength() == 0) {
-    return Result::ERROR_BAD_DER;
-  }
-
-  // Check for overly-long encodings. If the first byte is 0x00 then the high
-  // bit on the second byte must be 1; otherwise the same *positive* value
-  // could be encoded without the leading 0x00 byte. If the first byte is 0xFF
-  // then the second byte must NOT have its high bit set; otherwise the same
-  // *negative* value could be encoded without the leading 0xFF byte.
-  if (value.GetLength() > 1) {
-    Reader valueInput(value);
-    uint8_t firstByte;
-    rv = valueInput.Read(firstByte);
-    if (rv != Success) {
-      return rv;
-    }
-    uint8_t secondByte;
-    rv = valueInput.Read(secondByte);
-    if (rv != Success) {
-      return rv;
-    }
-    if ((firstByte == 0x00 && (secondByte & 0x80) == 0) ||
-        (firstByte == 0xff && (secondByte & 0x80) != 0)) {
-      return Result::ERROR_BAD_DER;
-    }
-  }
-
-  return Success;
+  return internal::IntegralBytes(
+           input, INTEGER, internal::IntegralValueRestriction::NoRestriction,
+           value);
 }
 
 // x.509 and OCSP both use this same version numbering scheme, though OCSP
 // only supports v1.
 enum class Version { v1 = 0, v2 = 1, v3 = 2, v4 = 3 };
 
 // X.509 Certificate and OCSP ResponseData both use this
 // "[0] EXPLICIT Version DEFAULT <defaultVersion>" construct, but with
--- a/security/pkix/test/gtest/pkixder_universal_types_tests.cpp
+++ b/security/pkix/test/gtest/pkixder_universal_types_tests.cpp
@@ -890,170 +890,188 @@ TEST_F(pkixder_universal_types_tests, Ti
     0x18,                           // Generalized Time
     17,                             // Length = 17
     '1', '9', '9', '1', '0', '1', '0', '1', // YYYYMMDD (1991-01-01)
     '1', '6', '4', '5', '4', '0', '.', '3', 'Z' // HHMMSS.FFF (16:45:40.3Z)
   };
   ExpectBadTime(DER_GENERALIZED_TIME_INVALID_FRACTIONAL_SECONDS);
 }
 
-TEST_F(pkixder_universal_types_tests, Integer_0_127)
+struct IntegerTestParams
+{
+  ByteString encoded;
+  struct PositiveIntegerParams
+  {
+    bool isValid;
+    Input::size_type significantBytesIfValid;
+  } positiveInteger;
+  uint8_t smallNonnegativeIntegerValue;
+};
+
+class pkixder_universal_types_tests_Integer
+  : public ::testing::Test
+  , public ::testing::WithParamInterface<IntegerTestParams>
+{
+};
+
+#define INVALID 0xFF
+
+static const IntegerTestParams INTEGER_TEST_PARAMS[] =
 {
-  for (uint8_t i = 0; i <= 127; ++i) {
-    const uint8_t DER[] = {
-      0x02, // INTEGER
-      0x01, // length
-      i,    // value
-    };
-    Input input(DER);
-    Reader reader(input);
+  // Zero is encoded with one value byte of 0x00.
+  { TLV(2, ByteString()), { false, INVALID }, INVALID },
+  { TLV(2, "\x00"), { false, INVALID }, 0 },
+
+  // Positive single-byte values
+  { TLV(2, "\x01"), { true, 1 }, 1 },
+  { TLV(2, "\x02"), { true, 1 }, 2 },
+  { TLV(2, "\x7e"), { true, 1 }, 0x7e },
+  { TLV(2, "\x7f"), { true, 1 }, 0x7f },
+
+  // Negative single-byte values
+  { TLV(2, "\x80"), { false, INVALID }, INVALID },
+  { TLV(2, "\x81"), { false, INVALID }, INVALID },
+  { TLV(2, "\xFE"), { false, INVALID }, INVALID },
+  { TLV(2, "\xFF"), { false, INVALID }, INVALID },
+
+  // Positive two-byte values not starting with 0x00
+  { TLV(2, "\x7F\x00"), { true, 2 }, INVALID },
+  { TLV(2, "\x01\x00"), { true, 2 }, INVALID },
+  { TLV(2, "\x01\x02"), { true, 2 }, INVALID },
+
+  // Negative two-byte values not starting with 0xFF
+  { TLV(2, "\x80\x00"), { false, INVALID }, INVALID },
+  { TLV(2, "\x80\x7F"), { false, INVALID }, INVALID },
+  { TLV(2, "\x80\x80"), { false, INVALID }, INVALID },
+  { TLV(2, "\x80\xFF"), { false, INVALID }, INVALID },
+
+  // The leading zero is necessary.
+  { TLV(2, "\x00\x80"), { true, 1}, INVALID },
+  { TLV(2, "\x00\x81"), { true, 1}, INVALID },
+  { TLV(2, "\x00\xFF"), { true, 1}, INVALID },
+
+  // The leading zero is unnecessary.
+  { TLV(2, "\x00\x01"), { false, INVALID }, INVALID },
+  { TLV(2, "\x00\x7F"), { false, INVALID }, INVALID },
+
+  // The leading 0xFF is necessary.
+  { TLV(2, "\xFF\x00"), { false, INVALID }, INVALID },
+  { TLV(2, "\xFF\x7F"), { false, INVALID }, INVALID },
+
+  // The leading 0xFF is unnecessary.
+  { TLV(2, "\xFF\x80"), { false, INVALID }, INVALID },
+  { TLV(2, "\xFF\xFF"), { false, INVALID }, INVALID },
 
-    uint8_t value = i + 1u; // initialize with a value that is NOT i.
-    ASSERT_EQ(Success, Integer(reader, value));
-    ASSERT_EQ(i, value);
+  // Truncated values
+  { TLV(2, 1, ByteString(/*missing value*/)), { false, INVALID }, INVALID },
+  { TLV(2, 3, "\x11\x22" /*truncated*/), { false, INVALID }, INVALID },
+  { TLV(2, 4, "\x11\x22" /*truncated*/), { false, INVALID }, INVALID },
+  { TLV(2, 2, "\x00" /*truncated*/), { false, INVALID }, INVALID },
+  { TLV(2, 2, "\xFF" /*truncated*/), { false, INVALID }, INVALID },
+  { TLV(2, 3, "\x00\x80" /*truncated*/), { false, INVALID }, INVALID },
+  { TLV(2, 3, "\xFF\x00" /*truncated*/), { false, INVALID }, INVALID },
+
+  // Misc. larger values
+  { TLV(2, 4, "\x11\x22\x33\x44"), { true, 4 }, INVALID },
+  { TLV(2,
+        "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x00"
+        "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x00"
+        "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x00"
+        "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x00"
+        "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x00"
+        "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x00"
+        "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x00"
+        "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x00"
+
+        "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x00"
+        "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x00"
+        "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x00"
+        "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x00"
+        "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x00"
+        "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x00"
+        "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x00"
+        "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f\x00"),
+    { true, 256 }, INVALID },
+};
+
+TEST_P(pkixder_universal_types_tests_Integer, Integer)
+{
+  const IntegerTestParams& params(GetParam());
+  Input input;
+  ASSERT_EQ(Success, input.Init(params.encoded.data(),
+                                params.encoded.length()));
+  Reader reader(input);
+  Result expectedResult = params.smallNonnegativeIntegerValue != INVALID
+                        ? Success
+                        : Result::ERROR_BAD_DER;
+  uint8_t value;
+  ASSERT_EQ(expectedResult, der::Integer(reader, value));
+  if (expectedResult == Success) {
+    ASSERT_EQ(params.smallNonnegativeIntegerValue, value);
+    ASSERT_TRUE(reader.AtEnd());
   }
 }
 
-TEST_F(pkixder_universal_types_tests, Integer_Negative1)
+TEST_P(pkixder_universal_types_tests_Integer,
+       PositiveInteger_without_significantBytes)
 {
-  // This is a valid integer value but our integer parser cannot parse
-  // negative values.
-
-  static const uint8_t DER[] = {
-    0x02, // INTEGER
-    0x01, // length
-    0xff, // -1 (two's complement)
-  };
-  Input input(DER);
-  Reader reader(input);
-
-  uint8_t value;
-  ASSERT_EQ(Result::ERROR_BAD_DER, Integer(reader, value));
-}
-
-TEST_F(pkixder_universal_types_tests, Integer_Negative128)
-{
-  // This is a valid integer value but our integer parser cannot parse
-  // negative values.
-
-  static const uint8_t DER[] = {
-    0x02, // INTEGER
-    0x01, // length
-    0x80, // -128 (two's complement)
-  };
-  Input input(DER);
+  const IntegerTestParams& params(GetParam());
+  Input input;
+  ASSERT_EQ(Success, input.Init(params.encoded.data(),
+                                params.encoded.length()));
   Reader reader(input);
-
-  uint8_t value;
-  ASSERT_EQ(Result::ERROR_BAD_DER, Integer(reader, value));
-}
-
-TEST_F(pkixder_universal_types_tests, Integer_128)
-{
-  // This is a valid integer value but our integer parser cannot parse
-  // values that require more than one byte to encode.
-
-  static const uint8_t DER[] = {
-    0x02, // INTEGER
-    0x02, // length
-    0x00, 0x80 // 128
-  };
-  Input input(DER);
-  Reader reader(input);
-
-  uint8_t value;
-  ASSERT_EQ(Result::ERROR_BAD_DER, Integer(reader, value));
-}
-
-TEST_F(pkixder_universal_types_tests, Integer11223344)
-{
-  // This is a valid integer value but our integer parser cannot parse
-  // values that require more than one byte to be encoded.
-
-  static const uint8_t DER[] = {
-    0x02,                       // INTEGER
-    0x04,                       // length
-    0x11, 0x22, 0x33, 0x44      // 0x11223344
-  };
-  Input input(DER);
-  Reader reader(input);
-
-  uint8_t value;
-  ASSERT_EQ(Result::ERROR_BAD_DER, Integer(reader, value));
+  Result expectedResult = params.positiveInteger.isValid
+                        ? Success
+                        : Result::ERROR_BAD_DER;
+  Input value;
+  ASSERT_EQ(expectedResult, der::PositiveInteger(reader, value));
+  if (expectedResult == Success) {
+    Reader anotherReader(input);
+    Input expectedValue;
+    ASSERT_EQ(Success, ExpectTagAndGetValue(anotherReader,
+                                            der::INTEGER, expectedValue));
+    ASSERT_TRUE(InputsAreEqual(expectedValue, value));
+    ASSERT_TRUE(reader.AtEnd());
+  }
 }
 
-TEST_F(pkixder_universal_types_tests, IntegerTruncatedOneByte)
+TEST_P(pkixder_universal_types_tests_Integer,
+       PositiveInteger_with_significantBytes)
 {
-  const uint8_t DER_INTEGER_TRUNCATED[] = {
-    0x02,                       // INTEGER
-    0x01,                       // length
-    // MISSING DATA HERE
-  };
-  Input input(DER_INTEGER_TRUNCATED);
+  const IntegerTestParams& params(GetParam());
+  Input input;
+  ASSERT_EQ(Success, input.Init(params.encoded.data(),
+                                params.encoded.length()));
   Reader reader(input);
-
-  uint8_t value;
-  ASSERT_EQ(Result::ERROR_BAD_DER, Integer(reader, value));
-}
+  Result expectedResult = params.positiveInteger.isValid
+                        ? Success
+                        : Result::ERROR_BAD_DER;
+  Input value;
+  Input::size_type significantBytes = INVALID;
+  ASSERT_EQ(expectedResult, der::PositiveInteger(reader, value,
+                                                 &significantBytes));
+  if (expectedResult == Success) {
+    ASSERT_NE(INVALID, params.positiveInteger.significantBytesIfValid);
+    ASSERT_EQ(params.positiveInteger.significantBytesIfValid,
+              significantBytes);
 
-TEST_F(pkixder_universal_types_tests, IntegerTruncatedLarge)
-{
-  const uint8_t DER_INTEGER_TRUNCATED[] = {
-    0x02,                       // INTEGER
-    0x04,                       // length
-    0x11, 0x22                  // 0x1122
-    // MISSING DATA HERE
-  };
-  Input input(DER_INTEGER_TRUNCATED);
-  Reader reader(input);
-
-  uint8_t value;
-  ASSERT_EQ(Result::ERROR_BAD_DER, Integer(reader, value));
+    Reader anotherReader(input);
+    Input expectedValue;
+    ASSERT_EQ(Success, ExpectTagAndGetValue(anotherReader,
+                                            der::INTEGER, expectedValue));
+    ASSERT_TRUE(InputsAreEqual(expectedValue, value));
+    ASSERT_TRUE(reader.AtEnd());
+  }
 }
 
-TEST_F(pkixder_universal_types_tests, IntegerZeroLength)
-{
-  const uint8_t DER_INTEGER_ZERO_LENGTH[] = {
-    0x02,                       // INTEGER
-    0x00                        // length
-  };
-  Input input(DER_INTEGER_ZERO_LENGTH);
-  Reader reader(input);
-
-  uint8_t value;
-  ASSERT_EQ(Result::ERROR_BAD_DER, Integer(reader, value));
-}
+#undef INVALID
 
-TEST_F(pkixder_universal_types_tests, IntegerOverlyLong1)
-{
-  const uint8_t DER_INTEGER_OVERLY_LONG1[] = {
-    0x02,                       // INTEGER
-    0x02,                       // length
-    0x00, 0x01                  //
-  };
-  Input input(DER_INTEGER_OVERLY_LONG1);
-  Reader reader(input);
-
-  uint8_t value;
-  ASSERT_EQ(Result::ERROR_BAD_DER, Integer(reader, value));
-}
-
-TEST_F(pkixder_universal_types_tests, IntegerOverlyLong2)
-{
-  const uint8_t DER_INTEGER_OVERLY_LONG2[] = {
-    0x02,                       // INTEGER
-    0x02,                       // length
-    0xff, 0x80                  //
-  };
-  Input input(DER_INTEGER_OVERLY_LONG2);
-  Reader reader(input);
-
-  uint8_t value;
-  ASSERT_EQ(Result::ERROR_BAD_DER, Integer(reader, value));
-}
+INSTANTIATE_TEST_CASE_P(pkixder_universal_types_tests_Integer,
+                        pkixder_universal_types_tests_Integer,
+                        testing::ValuesIn(INTEGER_TEST_PARAMS));
 
 TEST_F(pkixder_universal_types_tests, OptionalIntegerSupportedDefault)
 {
   // The input is a BOOLEAN and not INTEGER for the input so we'll not parse
   // anything and instead use the default value.
   Input input(DER_BOOLEAN_TRUE);
   Reader reader(input);
 
--- a/security/pkix/test/lib/pkixtestutil.cpp
+++ b/security/pkix/test/lib/pkixtestutil.cpp
@@ -116,30 +116,30 @@ TamperOnce(/*in/out*/ ByteString& item, 
     return Result::FATAL_ERROR_INVALID_ARGS; // More than once match.
   }
   item.replace(pos, from.length(), to);
   return Success;
 }
 
 // Given a tag and a value, generates a DER-encoded tag-length-value item.
 ByteString
-TLV(uint8_t tag, const ByteString& value)
+TLV(uint8_t tag, size_t length, const ByteString& value)
 {
   ByteString result;
   result.push_back(tag);
 
   if (value.length() < 128) {
-    result.push_back(static_cast<uint8_t>(value.length()));
+    result.push_back(static_cast<uint8_t>(length));
   } else if (value.length() < 256) {
     result.push_back(0x81u);
-    result.push_back(static_cast<uint8_t>(value.length()));
+    result.push_back(static_cast<uint8_t>(length));
   } else if (value.length() < 65536) {
     result.push_back(0x82u);
-    result.push_back(static_cast<uint8_t>(value.length() / 256));
-    result.push_back(static_cast<uint8_t>(value.length() % 256));
+    result.push_back(static_cast<uint8_t>(length / 256));
+    result.push_back(static_cast<uint8_t>(length % 256));
   } else {
     // It is MUCH more convenient for TLV to be infallible than for it to have
     // "proper" error handling.
     abort();
   }
   result.append(value);
   return result;
 }
--- a/security/pkix/test/lib/pkixtestutil.h
+++ b/security/pkix/test/lib/pkixtestutil.h
@@ -91,17 +91,47 @@ const uint8_t alg_md2WithRSAEncryption[]
 
 const ByteString md2WithRSAEncryption(alg_md2WithRSAEncryption,
   MOZILLA_PKIX_ARRAY_LENGTH(alg_md2WithRSAEncryption));
 
 // e.g. YMDHMS(2016, 12, 31, 1, 23, 45) => 2016-12-31:01:23:45 (GMT)
 mozilla::pkix::Time YMDHMS(uint16_t year, uint16_t month, uint16_t day,
                            uint16_t hour, uint16_t minutes, uint16_t seconds);
 
-ByteString TLV(uint8_t tag, const ByteString& value);
+ByteString TLV(uint8_t tag, size_t length, const ByteString& value);
+
+inline ByteString
+TLV(uint8_t tag, const ByteString& value)
+{
+  return TLV(tag, value.length(), value);
+}
+
+// Although we can't enforce it without relying on Cuser-defined literals,
+// which aren't supported by all of our compilers yet, you should only pass
+// string literals as the last parameter to the following two functions.
+
+template <size_t N>
+inline ByteString
+TLV(uint8_t tag, const char(&value)[N])
+{
+  static_assert(N > 0, "cannot have string literal of size 0");
+  assert(value[N - 1] == 0);
+  return TLV(tag, ByteString(reinterpret_cast<const uint8_t*>(&value), N - 1));
+}
+
+template <size_t N>
+inline ByteString
+TLV(uint8_t tag, size_t length, const char(&value)[N])
+{
+  static_assert(N > 0, "cannot have string literal of size 0");
+  assert(value[N - 1] == 0);
+  return TLV(tag, length,
+             ByteString(reinterpret_cast<const uint8_t*>(&value), N - 1));
+}
+
 ByteString Boolean(bool value);
 ByteString Integer(long value);
 
 ByteString CN(const ByteString&, uint8_t encodingTag = 0x0c /*UTF8String*/);
 
 inline ByteString
 CN(const char* value, uint8_t encodingTag = 0x0c /*UTF8String*/)
 {