Bug 1010581: Document Expect/Match/Skip terminology in mozilla::pkix::der and make that code more consistent, r=keeler
authorBrian Smith <brian@briansmith.org>
Wed, 14 May 2014 19:30:09 -0700
changeset 183489 fe7bffe6bb06fbe0cf047f1c979acb14875d3d1c
parent 183488 b9eff37173e1219027e2b5cb6822cb33504106b6
child 183490 a4ae7060f43ac1a4e49b30dfd7a95c5212940d4b
push idunknown
push userunknown
push dateunknown
reviewerskeeler
bugs1010581
milestone32.0a1
Bug 1010581: Document Expect/Match/Skip terminology in mozilla::pkix::der and make that code more consistent, r=keeler
security/pkix/lib/pkixder.cpp
security/pkix/lib/pkixder.h
security/pkix/lib/pkixocsp.cpp
security/pkix/test/gtest/pkixder_input_tests.cpp
security/pkix/test/gtest/pkixder_pki_types_tests.cpp
--- a/security/pkix/lib/pkixder.cpp
+++ b/security/pkix/lib/pkixder.cpp
@@ -29,16 +29,18 @@ namespace mozilla { namespace pkix { nam
 // not inline
 Result
 Fail(PRErrorCode errorCode)
 {
   PR_SetError(errorCode, 0);
   return Failure;
 }
 
+namespace internal {
+
 // Too complicated to be inline
 Result
 ExpectTagAndGetLength(Input& input, uint8_t expectedTag, uint16_t& length)
 {
   PR_ASSERT((expectedTag & 0x1F) != 0x1F); // high tag number form not allowed
 
   uint8_t tag;
   if (input.Read(tag) != Success) {
@@ -81,9 +83,11 @@ ExpectTagAndGetLength(Input& input, uint
     // We don't support lengths larger than 2^16 - 1.
     return Fail(SEC_ERROR_BAD_DER);
   }
 
   // Ensure the input is long enough for the length it says it has.
   return input.EnsureLength(length);
 }
 
+} // namespace internal
+
 } } } // namespace mozilla::pkix::der
--- a/security/pkix/lib/pkixder.h
+++ b/security/pkix/lib/pkixder.h
@@ -20,16 +20,28 @@
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
 
 #ifndef mozilla_pkix__pkixder_h
 #define mozilla_pkix__pkixder_h
 
+// Expect* functions advance the input mark and return Success if the input
+// matches the given criteria; they return Failure with the input mark in an
+// undefined state if the input does not match the criteria.
+//
+// Match* functions advance the input mark and return true if the input matches
+// the given criteria; they return false without changing the input mark if the
+// input does not match the criteria.
+//
+// Skip* functions unconditionally advance the input mark and return Success if
+// they are able to do so; otherwise they return Failure with the input mark in
+// an undefined state.
+
 #include "pkix/enumclass.h"
 #include "pkix/nullptr.h"
 
 #include "prerror.h"
 #include "prlog.h"
 #include "secder.h"
 #include "secerr.h"
 #include "secoidt.h"
@@ -247,76 +259,98 @@ ExpectTagAndLength(Input& input, uint8_t
 
   if (tagAndLength != expectedTagAndLength) {
     return Fail(SEC_ERROR_BAD_DER);
   }
 
   return Success;
 }
 
+namespace internal {
+
 Result
 ExpectTagAndGetLength(Input& input, uint8_t expectedTag, uint16_t& length);
 
+} // namespace internal
+
 inline Result
-ExpectTagAndIgnoreLength(Input& input, uint8_t expectedTag)
+ExpectTagAndSkipLength(Input& input, uint8_t expectedTag)
 {
   uint16_t ignored;
-  return ExpectTagAndGetLength(input, expectedTag, ignored);
+  return internal::ExpectTagAndGetLength(input, expectedTag, ignored);
+}
+
+inline Result
+ExpectTagAndSkipValue(Input& input, uint8_t tag)
+{
+  uint16_t length;
+  if (internal::ExpectTagAndGetLength(input, tag, length) != Success) {
+    return Failure;
+  }
+  return input.Skip(length);
+}
+
+inline Result
+ExpectTagAndGetValue(Input& input, uint8_t tag, /*out*/ SECItem& value)
+{
+  uint16_t length;
+  if (internal::ExpectTagAndGetLength(input, tag, length) != Success) {
+    return Failure;
+  }
+  return input.Skip(length, value);
+}
+
+inline Result
+ExpectTagAndGetValue(Input& input, uint8_t tag, /*out*/ Input& value)
+{
+  uint16_t length;
+  if (internal::ExpectTagAndGetLength(input, tag, length) != Success) {
+    return Failure;
+  }
+  return input.Skip(length, value);
 }
 
 inline Result
 End(Input& input)
 {
   if (!input.AtEnd()) {
     return Fail(SEC_ERROR_BAD_DER);
   }
 
   return Success;
 }
 
 template <typename Decoder>
 inline Result
 Nested(Input& input, uint8_t tag, Decoder decoder)
 {
-  uint16_t length;
-  if (ExpectTagAndGetLength(input, tag, length) != Success) {
+  Input nested;
+  if (ExpectTagAndGetValue(input, tag, nested) != Success) {
     return Failure;
   }
-
-  Input nested;
-  if (input.Skip(length, nested) != Success) {
-    return Failure;
-  }
-
   if (decoder(nested) != Success) {
     return Failure;
   }
-
   return End(nested);
 }
 
 template <typename Decoder>
 inline Result
 Nested(Input& input, uint8_t outerTag, uint8_t innerTag, Decoder decoder)
 {
   // XXX: This doesn't work (in VS2010):
   // return Nested(input, outerTag, bind(Nested, _1, innerTag, decoder));
 
-  uint16_t length;
-  if (ExpectTagAndGetLength(input, outerTag, length) != Success) {
-    return Failure;
-  }
   Input nestedInput;
-  if (input.Skip(length, nestedInput) != Success) {
+  if (ExpectTagAndGetValue(input, outerTag, nestedInput) != Success) {
     return Failure;
   }
   if (Nested(nestedInput, innerTag, decoder) != Success) {
     return Failure;
   }
-
   return End(nestedInput);
 }
 
 // This can be used to decode constructs like this:
 //
 //     ...
 //     foos SEQUENCE OF Foo,
 //     ...
@@ -332,23 +366,18 @@ Nested(Input& input, uint8_t outerTag, u
 //
 // In this example, Foo will get called once for each element of foos.
 //
 template <typename Decoder>
 inline Result
 NestedOf(Input& input, uint8_t outerTag, uint8_t innerTag,
          EmptyAllowed mayBeEmpty, Decoder decoder)
 {
-  uint16_t responsesLength;
-  if (ExpectTagAndGetLength(input, outerTag, responsesLength) != Success) {
-    return Failure;
-  }
-
   Input inner;
-  if (input.Skip(responsesLength, inner) != Success) {
+  if (ExpectTagAndGetValue(input, outerTag, inner) != Success) {
     return Failure;
   }
 
   if (inner.AtEnd()) {
     if (mayBeEmpty != EmptyAllowed::Yes) {
       return Fail(SEC_ERROR_BAD_DER);
     }
     return Success;
@@ -358,36 +387,16 @@ NestedOf(Input& input, uint8_t outerTag,
     if (Nested(inner, innerTag, decoder) != Success) {
       return Failure;
     }
   } while (!inner.AtEnd());
 
   return Success;
 }
 
-inline Result
-Skip(Input& input, uint8_t tag)
-{
-  uint16_t length;
-  if (ExpectTagAndGetLength(input, tag, length) != Success) {
-    return Failure;
-  }
-  return input.Skip(length);
-}
-
-inline Result
-Skip(Input& input, uint8_t tag, /*out*/ SECItem& value)
-{
-  uint16_t length;
-  if (ExpectTagAndGetLength(input, tag, length) != Success) {
-    return Failure;
-  }
-  return input.Skip(length, value);
-}
-
 // Universal types
 
 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(Input& input, uint8_t tag, T& value)
@@ -456,28 +465,23 @@ inline Result
 Enumerated(Input& input, uint8_t& value)
 {
   return internal::IntegralValue(input, ENUMERATED | 0, value);
 }
 
 inline Result
 GeneralizedTime(Input& input, PRTime& time)
 {
-  uint16_t length;
   SECItem encoded;
-  if (ExpectTagAndGetLength(input, GENERALIZED_TIME, length) != Success) {
-    return Failure;
-  }
-  if (input.Skip(length, encoded) != Success) {
+  if (ExpectTagAndGetValue(input, GENERALIZED_TIME, encoded) != Success) {
     return Failure;
   }
   if (DER_GeneralizedTimeToTime(&time, &encoded) != SECSuccess) {
     return Failure;
   }
-
   return Success;
 }
 
 // This parser will only parse values between 0..127. If this range is
 // increased then callers will need to be changed.
 inline Result
 Integer(Input& input, /*out*/ uint8_t& value)
 {
@@ -533,17 +537,17 @@ OID(Input& input, const uint8_t (&expect
 // PKI-specific types
 
 // AlgorithmIdentifier  ::=  SEQUENCE  {
 //         algorithm               OBJECT IDENTIFIER,
 //         parameters              ANY DEFINED BY algorithm OPTIONAL  }
 inline Result
 AlgorithmIdentifier(Input& input, SECAlgorithmID& algorithmID)
 {
-  if (Skip(input, OIDTag, algorithmID.algorithm) != Success) {
+  if (ExpectTagAndGetValue(input, OIDTag, algorithmID.algorithm) != Success) {
     return Failure;
   }
   algorithmID.parameters.data = nullptr;
   algorithmID.parameters.len = 0;
   if (input.AtEnd()) {
     return Success;
   }
   return Null(input);
@@ -558,22 +562,17 @@ CertificateSerialNumber(Input& input, /*
   //   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."
 
-  uint16_t length;
-  if (ExpectTagAndGetLength(input, INTEGER, length) != Success) {
-    return Failure;
-  }
-
-  if (input.Skip(length, value) != Success) {
+  if (ExpectTagAndGetValue(input, INTEGER, value) != Success) {
     return Failure;
   }
 
   if (value.len == 0) {
     return Fail(SEC_ERROR_BAD_DER);
   }
 
   // Check for overly-long encodings. If the first byte is 0x00 then the high
--- a/security/pkix/lib/pkixocsp.cpp
+++ b/security/pkix/lib/pkixocsp.cpp
@@ -262,17 +262,18 @@ GetOCSPSignerCertificate(TrustDomain& tr
       case ResponderIDType::byKey:
       {
         der::Input responderID;
         if (responderID.Init(responderIDItem.data, responderIDItem.len)
               != der::Success) {
           return nullptr;
         }
         SECItem keyHash;
-        if (der::Skip(responderID, der::OCTET_STRING, keyHash) != der::Success) {
+        if (der::ExpectTagAndGetValue(responderID, der::OCTET_STRING, keyHash)
+              != der::Success) {
           return nullptr;
         }
         if (MatchKeyHash(keyHash, *potentialSigner.get(), match) != der::Success) {
           return nullptr;
         }
         break;
       }
 
@@ -443,46 +444,41 @@ ResponseBytes(der::Input& input, Context
 //    signatureAlgorithm   AlgorithmIdentifier,
 //    signature            BIT STRING,
 //    certs            [0] EXPLICIT SEQUENCE OF Certificate OPTIONAL }
 der::Result
 BasicResponse(der::Input& input, Context& context)
 {
   der::Input::Mark mark(input.GetMark());
 
-  uint16_t length;
-  if (der::ExpectTagAndGetLength(input, der::SEQUENCE, length)
-        != der::Success) {
-    return der::Failure;
-  }
-
   // The signature covers the entire DER encoding of tbsResponseData, including
   // the beginning tag and length. However, when we're parsing tbsResponseData,
   // we want to strip off the tag and length because we don't need it after
   // we've confirmed it's there and figured out what length it is.
 
   der::Input tbsResponseData;
-
-  if (input.Skip(length, tbsResponseData) != der::Success) {
+  if (der::ExpectTagAndGetValue(input, der::SEQUENCE, tbsResponseData)
+        != der::Success) {
     return der::Failure;
   }
 
   CERTSignedData signedData;
 
   if (input.GetSECItem(siBuffer, mark, signedData.data) != der::Success) {
     return der::Failure;
   }
 
   if (der::Nested(input, der::SEQUENCE,
                   bind(der::AlgorithmIdentifier, _1,
                        ref(signedData.signatureAlgorithm))) != der::Success) {
     return der::Failure;
   }
 
-  if (der::Skip(input, der::BIT_STRING, signedData.signature) != der::Success) {
+  if (der::ExpectTagAndGetValue(input, der::BIT_STRING, signedData.signature)
+        != der::Success) {
     return der::Failure;
   }
   if (signedData.signature.len == 0) {
     return der::Fail(SEC_ERROR_OCSP_BAD_SIGNATURE);
   }
   unsigned int unusedBitsAtEnd = signedData.signature.data[0];
   // XXX: Really the constraint should be that unusedBitsAtEnd must be less
   // than 7. But, we suspect there are no valid OCSP response signatures with
@@ -501,37 +497,37 @@ BasicResponse(der::Input& input, Context
   size_t numCerts = 0;
 
   if (!input.AtEnd()) {
     // We ignore the lengths of the wrappers because we'll detect bad lengths
     // during parsing--too short and we'll run out of input for parsing a cert,
     // and too long and we'll have leftover data that won't parse as a cert.
 
     // [0] wrapper
-    if (der::ExpectTagAndIgnoreLength(
+    if (der::ExpectTagAndSkipLength(
           input, der::CONSTRUCTED | der::CONTEXT_SPECIFIC | 0)
         != der::Success) {
       return der::Failure;
     }
 
     // SEQUENCE wrapper
-    if (der::ExpectTagAndIgnoreLength(input, der::SEQUENCE) != der::Success) {
+    if (der::ExpectTagAndSkipLength(input, der::SEQUENCE) != der::Success) {
       return der::Failure;
     }
 
     // sequence of certificates
     while (!input.AtEnd()) {
       if (numCerts == PR_ARRAY_SIZE(certs)) {
         return der::Fail(SEC_ERROR_BAD_DER);
       }
 
       // Unwrap the SEQUENCE that contains the certificate, which is itself a
       // SEQUENCE.
       der::Input::Mark mark(input.GetMark());
-      if (der::Skip(input, der::SEQUENCE) != der::Success) {
+      if (der::ExpectTagAndSkipValue(input, der::SEQUENCE) != der::Success) {
         return der::Failure;
       }
 
       if (input.GetSECItem(siBuffer, mark, certs[numCerts]) != der::Success) {
         return der::Failure;
       }
       ++numCerts;
     }
@@ -559,28 +555,22 @@ ResponseData(der::Input& input, Context&
     // TODO: more specific error code for bad version?
     return der::Fail(SEC_ERROR_BAD_DER);
   }
 
   // ResponderID ::= CHOICE {
   //    byName              [1] Name,
   //    byKey               [2] KeyHash }
   SECItem responderID;
-  uint16_t responderIDLength;
   ResponderIDType responderIDType
     = input.Peek(static_cast<uint8_t>(ResponderIDType::byName))
     ? ResponderIDType::byName
     : ResponderIDType::byKey;
-  if (ExpectTagAndGetLength(input, static_cast<uint8_t>(responderIDType),
-                            responderIDLength) != der::Success) {
-    return der::Failure;
-  }
-  // TODO: responderID probably needs to have another level of ASN1 tag/length
-  // checked and stripped.
-  if (input.Skip(responderIDLength, responderID) != der::Success) {
+  if (ExpectTagAndGetValue(input, static_cast<uint8_t>(responderIDType),
+                           responderID) != der::Success) {
     return der::Failure;
   }
 
   // This is the soonest we can verify the signature. We verify the signature
   // right away to follow the principal of minimizing the processing of data
   // before verifying its signature.
   if (VerifySignature(context, responderIDType, responderID, certs, numCerts,
                       signedResponseData) != SECSuccess) {
@@ -658,17 +648,18 @@ SingleResponse(der::Input& input, Contex
     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
     // parse it. TODO: We should mention issues like this in the explanation of
     // why we treat invalid OCSP responses equivalently to revoked for OCSP
     // stapling.
-    if (der::Skip(input, static_cast<uint8_t>(CertStatus::Revoked))
+    if (der::ExpectTagAndSkipValue(input,
+                                   static_cast<uint8_t>(CertStatus::Revoked))
           != der::Success) {
       return der::Failure;
     }
     context.certStatus = CertStatus::Revoked;
   } else if (ExpectTagAndLength(input,
                                 static_cast<uint8_t>(CertStatus::Unknown),
                                 0) != der::Success) {
     return der::Failure;
@@ -756,22 +747,24 @@ CertID(der::Input& input, const Context&
   SECAlgorithmID hashAlgorithm;
   if (der::Nested(input, der::SEQUENCE,
                   bind(der::AlgorithmIdentifier, _1, ref(hashAlgorithm)))
          != der::Success) {
     return der::Failure;
   }
 
   SECItem issuerNameHash;
-  if (der::Skip(input, der::OCTET_STRING, issuerNameHash) != der::Success) {
+  if (der::ExpectTagAndGetValue(input, der::OCTET_STRING, issuerNameHash)
+        != der::Success) {
     return der::Failure;
   }
 
   SECItem issuerKeyHash;
-  if (der::Skip(input, der::OCTET_STRING, issuerKeyHash) != der::Success) {
+  if (der::ExpectTagAndGetValue(input, der::OCTET_STRING, issuerKeyHash)
+        != der::Success) {
     return der::Failure;
   }
 
   SECItem serialNumber;
   if (der::CertificateSerialNumber(input, serialNumber) != der::Success) {
     return der::Failure;
   }
 
@@ -846,37 +839,30 @@ MatchKeyHash(const SECItem& keyHash, con
 // Extension  ::=  SEQUENCE  {
 //      extnID      OBJECT IDENTIFIER,
 //      critical    BOOLEAN DEFAULT FALSE,
 //      extnValue   OCTET STRING
 //      }
 static der::Result
 CheckExtensionForCriticality(der::Input& input)
 {
-  uint16_t toSkip;
-  if (ExpectTagAndGetLength(input, der::OIDTag, toSkip) != der::Success) {
-    return der::Failure;
-  }
-
   // TODO: maybe we should check the syntax of the OID value
-  if (input.Skip(toSkip) != der::Success) {
+  if (ExpectTagAndSkipValue(input, der::OIDTag) != der::Success) {
     return der::Failure;
   }
 
   // The only valid explicit encoding of the value is TRUE, so don't even
   // bother parsing it, since we're going to fail either way.
   if (input.Peek(der::BOOLEAN)) {
     return der::Fail(SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION);
   }
 
-  if (ExpectTagAndGetLength(input, der::OCTET_STRING, toSkip)
-        != der::Success) {
-    return der::Failure;
-  }
-  return input.Skip(toSkip);
+  input.SkipToEnd();
+
+  return der::Success;
 }
 
 // Extensions ::= SEQUENCE SIZE (1..MAX) OF Extension
 static der::Result
 CheckExtensionsForCriticality(der::Input& input)
 {
   // TODO(bug 997994): some responders include an empty SEQUENCE OF
   // Extension, which is invalid (der::MayBeEmpty should really be
--- a/security/pkix/test/gtest/pkixder_input_tests.cpp
+++ b/security/pkix/test/gtest/pkixder_input_tests.cpp
@@ -357,41 +357,41 @@ TEST_F(pkixder_input_tests, SkipToSECIte
   const uint8_t der[] = { 0x11, 0x22, 0x33, 0x44 };
   ASSERT_EQ(Success, input.Init(der, sizeof der));
 
   SECItem skippedSECItem;
   ASSERT_EQ(Failure, input.Skip(sizeof der + 1, skippedSECItem));
   ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());
 }
 
-TEST_F(pkixder_input_tests, Skip)
+TEST_F(pkixder_input_tests, ExpectTagAndSkipValue)
 {
   Input input;
   ASSERT_EQ(Success,
             input.Init(DER_SEQUENCE_OF_INT8, sizeof DER_SEQUENCE_OF_INT8));
 
-  ASSERT_EQ(Success, Skip(input, SEQUENCE));
+  ASSERT_EQ(Success, ExpectTagAndSkipValue(input, SEQUENCE));
   ASSERT_EQ(Success, End(input));
 }
 
-TEST_F(pkixder_input_tests, SkipWithTruncatedData)
+TEST_F(pkixder_input_tests, ExpectTagAndSkipValueWithTruncatedData)
 {
   Input input;
   ASSERT_EQ(Success, input.Init(DER_TRUNCATED_SEQUENCE_OF_INT8,
                                 sizeof DER_TRUNCATED_SEQUENCE_OF_INT8));
 
-  ASSERT_EQ(Failure, Skip(input, SEQUENCE));
+  ASSERT_EQ(Failure, ExpectTagAndSkipValue(input, SEQUENCE));
 }
 
-TEST_F(pkixder_input_tests, SkipWithOverrunData)
+TEST_F(pkixder_input_tests, ExpectTagAndSkipValueWithOverrunData)
 {
   Input input;
   ASSERT_EQ(Success, input.Init(DER_OVERRUN_SEQUENCE_OF_INT8,
                                 sizeof DER_OVERRUN_SEQUENCE_OF_INT8));
-  ASSERT_EQ(Success, Skip(input, SEQUENCE));
+  ASSERT_EQ(Success, ExpectTagAndSkipValue(input, SEQUENCE));
   ASSERT_EQ(Failure, End(input));
 }
 
 TEST_F(pkixder_input_tests, AtEndOnUnInitializedInput)
 {
   Input input;
   ASSERT_TRUE(input.AtEnd());
 }
@@ -486,57 +486,57 @@ TEST_F(pkixder_input_tests, ExpectTagAnd
 
 TEST_F(pkixder_input_tests, ExpectTagAndGetLength)
 {
   Input input;
   ASSERT_EQ(Success,
             input.Init(DER_SEQUENCE_OF_INT8, sizeof DER_SEQUENCE_OF_INT8));
 
   uint16_t length = 0;
-  ASSERT_EQ(Success, ExpectTagAndGetLength(input, SEQUENCE, length));
+  ASSERT_EQ(Success, internal::ExpectTagAndGetLength(input, SEQUENCE, length));
   ASSERT_EQ(sizeof DER_SEQUENCE_OF_INT8 - 2, length);
   ASSERT_EQ(Success, input.Skip(length));
   ASSERT_TRUE(input.AtEnd());
 }
 
 TEST_F(pkixder_input_tests, ExpectTagAndGetLengthWithWrongTag)
 {
   Input input;
   ASSERT_EQ(Success,
             input.Init(DER_SEQUENCE_OF_INT8, sizeof DER_SEQUENCE_OF_INT8));
 
   uint16_t length = 0;
-  ASSERT_EQ(Failure, ExpectTagAndGetLength(input, INTEGER, length));
+  ASSERT_EQ(Failure, internal::ExpectTagAndGetLength(input, INTEGER, length));
   ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());
 }
 
 TEST_F(pkixder_input_tests, ExpectTagAndGetLengthWithWrongLength)
 {
   Input input;
   ASSERT_EQ(Success, input.Init(DER_TRUNCATED_SEQUENCE_OF_INT8,
                                 sizeof DER_TRUNCATED_SEQUENCE_OF_INT8));
 
   uint16_t length = 0;
-  ASSERT_EQ(Failure, ExpectTagAndGetLength(input, SEQUENCE, length));
+  ASSERT_EQ(Failure, internal::ExpectTagAndGetLength(input, SEQUENCE, length));
   ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());
 }
 
-TEST_F(pkixder_input_tests, ExpectTagAndIgnoreLength)
+TEST_F(pkixder_input_tests, ExpectTagAndSkipLength)
 {
   Input input;
   ASSERT_EQ(Success, input.Init(DER_INT16, sizeof DER_INT16));
-  ASSERT_EQ(Success, ExpectTagAndIgnoreLength(input, INTEGER));
+  ASSERT_EQ(Success, ExpectTagAndSkipLength(input, INTEGER));
 }
 
-TEST_F(pkixder_input_tests, ExpectTagAndIgnoreLengthWithWrongTag)
+TEST_F(pkixder_input_tests, ExpectTagAndSkipLengthWithWrongTag)
 {
   Input input;
   ASSERT_EQ(Success, input.Init(DER_INT16, sizeof DER_INT16));
 
-  ASSERT_EQ(Failure, ExpectTagAndIgnoreLength(input, OCTET_STRING));
+  ASSERT_EQ(Failure, ExpectTagAndSkipLength(input, OCTET_STRING));
   ASSERT_EQ(SEC_ERROR_BAD_DER, PR_GetError());
 }
 
 TEST_F(pkixder_input_tests, EndAtEnd)
 {
   Input input;
   ASSERT_EQ(Success, input.Init(DER_INT16, sizeof DER_INT16));
   ASSERT_EQ(Success, input.Skip(4));
--- a/security/pkix/test/gtest/pkixder_pki_types_tests.cpp
+++ b/security/pkix/test/gtest/pkixder_pki_types_tests.cpp
@@ -74,21 +74,18 @@ TEST_F(pkixder_pki_types_tests, Algorith
     0x06, 0x04, 0xde, 0xad, 0xbe, 0xef, // OID
     0x05, 0x00                          // NULL
   };
 
   Input input;
   ASSERT_EQ(Success, input.Init(DER_ALGORITHM_IDENTIFIER_NULL_PARAMS,
                                 sizeof DER_ALGORITHM_IDENTIFIER_NULL_PARAMS));
 
-  uint16_t length;
-  ASSERT_EQ(Success, ExpectTagAndGetLength(input, SEQUENCE, length));
-
   Input nested;
-  ASSERT_EQ(Success, input.Skip(length, nested));
+  ASSERT_EQ(Success, ExpectTagAndGetValue(input, SEQUENCE, nested));
 
   const uint8_t expectedAlgorithmID[] = {
     0xde, 0xad, 0xbe, 0xef
   };
 
   SECAlgorithmID algorithmID;
   ASSERT_EQ(Success, AlgorithmIdentifier(nested, algorithmID));