--- 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));
