Skip to content

Commit

Permalink
Bug 1010581: Document Expect/Match/Skip terminology in mozilla::pkix:…
Browse files Browse the repository at this point in the history 
…:der and make that code more consistent, r=keeler

--HG--
extra : rebase_source : 12aa2e1e9eed4f32a75732a65cbfaba9789d5d39
  • Loading branch information
@briansmith
briansmith committed May 15, 2014
1 parent 9c1cb9b commit 5b29501
Show file tree
Hide file tree
Showing 5 changed files with 93 additions and 107 deletions.
4 changes: 4 additions & 0 deletions lib/mozpkix/lib/pkixder.cpp
View file
Expand Up @@ -34,6 +34,8 @@ Fail(PRErrorCode errorCode)
return Failure;
}

namespace internal {

// Too complicated to be inline
Result
ExpectTagAndGetLength(Input& input, uint8_t expectedTag, uint16_t& length)
Expand Down Expand Up @@ -86,4 +88,6 @@ ExpectTagAndGetLength(Input& input, uint8_t expectedTag, uint16_t& length)
return input.EnsureLength(length);
}

} // namespace internal

} } } // namespace mozilla::pkix::der
109 changes: 54 additions & 55 deletions lib/mozpkix/lib/pkixder.h
View file
Expand Up @@ -25,6 +25,18 @@
#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"

Expand Down Expand Up @@ -252,14 +264,48 @@ ExpectTagAndLength(Input& input, uint8_t expectedTag, uint8_t expectedLength)
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
Expand All @@ -276,20 +322,13 @@ template <typename Decoder>
inline Result
Nested(Input& input, uint8_t tag, Decoder decoder)
{
uint16_t length;
if (ExpectTagAndGetLength(input, tag, length) != Success) {
return Failure;
}

Input nested;
if (input.Skip(length, nested) != Success) {
if (ExpectTagAndGetValue(input, tag, nested) != Success) {
return Failure;
}

if (decoder(nested) != Success) {
return Failure;
}

return End(nested);
}

Expand All @@ -300,18 +339,13 @@ 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);
}

Expand All @@ -337,13 +371,8 @@ 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;
}

Expand All @@ -363,26 +392,6 @@ NestedOf(Input& input, uint8_t outerTag, uint8_t innerTag,
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 {
Expand Down Expand Up @@ -461,18 +470,13 @@ Enumerated(Input& input, uint8_t& 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;
}

Expand Down Expand Up @@ -538,7 +542,7 @@ OID(Input& input, const uint8_t (&expectedOid)[Len])
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;
Expand All @@ -563,12 +567,7 @@ CertificateSerialNumber(Input& input, /*out*/ SECItem& value)
// 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;
}

Expand Down
56 changes: 21 additions & 35 deletions lib/mozpkix/lib/pkixocsp.cpp
View file
Expand Up @@ -267,7 +267,8 @@ GetOCSPSignerCertificate(TrustDomain& trustDomain,
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) {
Expand Down Expand Up @@ -448,20 +449,14 @@ 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;
}

Expand All @@ -477,7 +472,8 @@ BasicResponse(der::Input& input, Context& context)
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) {
Expand Down Expand Up @@ -506,14 +502,14 @@ BasicResponse(der::Input& input, Context& context)
// 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;
}

Expand All @@ -526,7 +522,7 @@ BasicResponse(der::Input& input, Context& context)
// 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;
}

Expand Down Expand Up @@ -564,18 +560,12 @@ ResponseData(der::Input& input, Context& context,
// 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;
}

Expand Down Expand Up @@ -663,7 +653,8 @@ SingleResponse(der::Input& input, Context& context)
// 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;
}
Expand Down Expand Up @@ -761,12 +752,14 @@ CertID(der::Input& input, const Context& context, /*out*/ bool& match)
}

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

Expand Down Expand Up @@ -851,13 +844,8 @@ MatchKeyHash(const SECItem& keyHash, const CERTCertificate& cert,
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;
}

Expand All @@ -867,11 +855,9 @@ CheckExtensionForCriticality(der::Input& input)
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
Expand Down

0 comments on commit 5b29501

Please sign in to comment.