Index: net/cert/internal/verify_certificate_chain.cc |
diff --git a/net/cert/internal/verify_certificate_chain.cc b/net/cert/internal/verify_certificate_chain.cc |
index 617b8511f17deb57995c5d63e35ca4d4639ef190..34d1422385fc340aa43c43aee052fb4e821421ce 100644 |
--- a/net/cert/internal/verify_certificate_chain.cc |
+++ b/net/cert/internal/verify_certificate_chain.cc |
@@ -9,7 +9,6 @@ |
#include "base/logging.h" |
#include "base/memory/ptr_util.h" |
#include "net/cert/internal/cert_error_params.h" |
-#include "net/cert/internal/cert_error_scoper.h" |
#include "net/cert/internal/cert_errors.h" |
#include "net/cert/internal/name_constraints.h" |
#include "net/cert/internal/parse_certificate.h" |
@@ -57,45 +56,17 @@ DEFINE_CERT_ERROR_ID(kSignatureAlgorithmsDifferentEncoding, |
"Certificate.signatureAlgorithm is encoded differently " |
"than TBSCertificate.signature"); |
-DEFINE_CERT_ERROR_ID(kContextTrustAnchor, "Processing Trust Anchor"); |
-DEFINE_CERT_ERROR_ID(kContextCertificate, "Processing Certificate"); |
- |
-// This class changes the error scope to indicate which certificate in the |
-// chain is currently being processed. |
-class CertErrorScoperForCert : public CertErrorScoper { |
- public: |
- CertErrorScoperForCert(CertErrors* parent_errors, size_t index) |
- : CertErrorScoper(parent_errors), index_(index) {} |
- |
- std::unique_ptr<CertErrorNode> BuildRootNode() override { |
- return base::MakeUnique<CertErrorNode>( |
- CertErrorNodeType::TYPE_CONTEXT, kContextCertificate, |
- CreateCertErrorParams1SizeT("index", index_)); |
- } |
- |
- private: |
- size_t index_; |
- |
- DISALLOW_COPY_AND_ASSIGN(CertErrorScoperForCert); |
-}; |
- |
-// Returns true if the certificate does not contain any unconsumed _critical_ |
+// Adds errors to |errors| if the certificate contains unconsumed _critical_ |
// extensions. |
-WARN_UNUSED_RESULT bool VerifyNoUnconsumedCriticalExtensions( |
- const ParsedCertificate& cert, |
- CertErrors* errors) { |
- bool has_unconsumed_critical_extensions = false; |
- |
+void VerifyNoUnconsumedCriticalExtensions(const ParsedCertificate& cert, |
+ CertErrors* errors) { |
for (const auto& entry : cert.unparsed_extensions()) { |
if (entry.second.critical) { |
- has_unconsumed_critical_extensions = true; |
errors->AddError(kUnconsumedCriticalExtension, |
CreateCertErrorParams2Der("oid", entry.second.oid, |
"value", entry.second.value)); |
} |
} |
- |
- return !has_unconsumed_critical_extensions; |
} |
// Returns true if |cert| was self-issued. The definition of self-issuance |
@@ -113,30 +84,25 @@ WARN_UNUSED_RESULT bool IsSelfIssued(const ParsedCertificate& cert) { |
return cert.normalized_subject() == cert.normalized_issuer(); |
} |
-// Returns true if |cert| is valid at time |time|. |
+// Adds errors to |errors| if |cert| is not valid at time |time|. |
// |
// The certificate's validity requirements are described by RFC 5280 section |
// 4.1.2.5: |
// |
// The validity period for a certificate is the period of time from |
// notBefore through notAfter, inclusive. |
-WARN_UNUSED_RESULT bool VerifyTimeValidity(const ParsedCertificate& cert, |
- const der::GeneralizedTime time, |
- CertErrors* errors) { |
- if (time < cert.tbs().validity_not_before) { |
+void VerifyTimeValidity(const ParsedCertificate& cert, |
+ const der::GeneralizedTime time, |
+ CertErrors* errors) { |
+ if (time < cert.tbs().validity_not_before) |
errors->AddError(kValidityFailedNotBefore); |
- return false; |
- } |
- if (cert.tbs().validity_not_after < time) { |
+ if (cert.tbs().validity_not_after < time) |
errors->AddError(kValidityFailedNotAfter); |
- return false; |
- } |
- |
- return true; |
} |
-// Returns true if |cert| has internally consistent signature algorithms. |
+// Adds errors to |errors| if |cert| has internally inconsistent signature |
+// algorithms. |
// |
// X.509 certificates contain two different signature algorithms: |
// (1) The signatureAlgorithm field of Certificate |
@@ -155,16 +121,15 @@ WARN_UNUSED_RESULT bool VerifyTimeValidity(const ParsedCertificate& cert, |
// In practice however there are certificates which use different encodings for |
// specifying RSA with SHA1 (different OIDs). This is special-cased for |
// compatibility sake. |
-WARN_UNUSED_RESULT bool VerifySignatureAlgorithmsMatch( |
- const ParsedCertificate& cert, |
- CertErrors* errors) { |
+void VerifySignatureAlgorithmsMatch(const ParsedCertificate& cert, |
+ CertErrors* errors) { |
const der::Input& alg1_tlv = cert.signature_algorithm_tlv(); |
const der::Input& alg2_tlv = cert.tbs().signature_algorithm_tlv; |
// Ensure that the two DER-encoded signature algorithms are byte-for-byte |
// equal. |
if (alg1_tlv == alg2_tlv) |
- return true; |
+ return; |
// But make a compatibility concession if alternate encodings are used |
// TODO(eroman): Turn this warning into an error. |
@@ -174,20 +139,18 @@ WARN_UNUSED_RESULT bool VerifySignatureAlgorithmsMatch( |
kSignatureAlgorithmsDifferentEncoding, |
CreateCertErrorParams2Der("Certificate.algorithm", alg1_tlv, |
"TBSCertificate.signature", alg2_tlv)); |
- return true; |
+ return; |
} |
errors->AddError( |
kSignatureAlgorithmMismatch, |
CreateCertErrorParams2Der("Certificate.algorithm", alg1_tlv, |
"TBSCertificate.signature", alg2_tlv)); |
- |
- return false; |
} |
// This function corresponds to RFC 5280 section 6.1.3's "Basic Certificate |
// Processing" procedure. |
-WARN_UNUSED_RESULT bool BasicCertificateProcessing( |
+void BasicCertificateProcessing( |
const ParsedCertificate& cert, |
bool is_target_cert, |
const SignaturePolicy* signature_policy, |
@@ -199,8 +162,7 @@ WARN_UNUSED_RESULT bool BasicCertificateProcessing( |
// Check that the signature algorithms in Certificate vs TBSCertificate |
// match. This isn't part of RFC 5280 section 6.1.3, but is mandated by |
// sections 4.1.1.2 and 4.1.2.3. |
- if (!VerifySignatureAlgorithmsMatch(cert, errors)) |
- return false; |
+ VerifySignatureAlgorithmsMatch(cert, errors); |
// Verify the digital signature using the previous certificate's key (RFC |
// 5280 section 6.1.3 step a.1). |
@@ -208,30 +170,25 @@ WARN_UNUSED_RESULT bool BasicCertificateProcessing( |
errors->AddError( |
kInvalidOrUnsupportedSignatureAlgorithm, |
CreateCertErrorParams1Der("algorithm", cert.signature_algorithm_tlv())); |
- return false; |
- } |
- |
- if (!VerifySignedData(cert.signature_algorithm(), cert.tbs_certificate_tlv(), |
- cert.signature_value(), working_spki, signature_policy, |
- errors)) { |
- errors->AddError(kVerifySignedDataFailed); |
- return false; |
+ } else { |
+ if (!VerifySignedData(cert.signature_algorithm(), |
+ cert.tbs_certificate_tlv(), cert.signature_value(), |
+ working_spki, signature_policy, errors)) { |
+ errors->AddError(kVerifySignedDataFailed); |
+ } |
} |
// Check the time range for the certificate's validity, ensuring it is valid |
// at |time|. |
// (RFC 5280 section 6.1.3 step a.2) |
- if (!VerifyTimeValidity(cert, time, errors)) |
- return false; |
+ VerifyTimeValidity(cert, time, errors); |
// TODO(eroman): Check revocation (RFC 5280 section 6.1.3 step a.3) |
// Verify the certificate's issuer name matches the issuing certificate's |
// subject name. (RFC 5280 section 6.1.3 step a.4) |
- if (cert.normalized_issuer() != working_normalized_issuer_name) { |
+ if (cert.normalized_issuer() != working_normalized_issuer_name) |
errors->AddError(kSubjectDoesNotMatchIssuer); |
- return false; |
- } |
// Name constraints (RFC 5280 section 6.1.3 step b & c) |
// If certificate i is self-issued and it is not the final certificate in the |
@@ -242,20 +199,17 @@ WARN_UNUSED_RESULT bool BasicCertificateProcessing( |
if (!nc->IsPermittedCert(cert.normalized_subject(), |
cert.subject_alt_names())) { |
errors->AddError(kNotPermittedByNameConstraints); |
- return false; |
} |
} |
} |
// TODO(eroman): Steps d-f are omitted, as policy constraints are not yet |
// implemented. |
- |
- return true; |
} |
// This function corresponds to RFC 5280 section 6.1.4's "Preparation for |
// Certificate i+1" procedure. |cert| is expected to be an intermediate. |
-WARN_UNUSED_RESULT bool PrepareForNextCertificate( |
+void PrepareForNextCertificate( |
const ParsedCertificate& cert, |
size_t* max_path_length_ptr, |
der::Input* working_spki, |
@@ -301,12 +255,8 @@ WARN_UNUSED_RESULT bool PrepareForNextCertificate( |
// can't contain a BasicConstraints extension. |
if (!cert.has_basic_constraints()) { |
errors->AddError(kMissingBasicConstraints); |
- return false; |
- } |
- |
- if (!cert.basic_constraints().is_ca) { |
+ } else if (!cert.basic_constraints().is_ca) { |
errors->AddError(kBasicConstraintsIndicatesNotCa); |
- return false; |
} |
// From RFC 5280 section 6.1.4 step l: |
@@ -317,9 +267,9 @@ WARN_UNUSED_RESULT bool PrepareForNextCertificate( |
if (!IsSelfIssued(cert)) { |
if (*max_path_length_ptr == 0) { |
errors->AddError(kMaxPathLengthViolated); |
- return false; |
+ } else { |
+ --(*max_path_length_ptr); |
} |
- --(*max_path_length_ptr); |
} |
// From RFC 5280 section 6.1.4 step m: |
@@ -327,7 +277,7 @@ WARN_UNUSED_RESULT bool PrepareForNextCertificate( |
// If pathLenConstraint is present in the certificate and is |
// less than max_path_length, set max_path_length to the value |
// of pathLenConstraint. |
- if (cert.basic_constraints().has_path_len && |
+ if (cert.has_basic_constraints() && cert.basic_constraints().has_path_len && |
cert.basic_constraints().path_len < *max_path_length_ptr) { |
*max_path_length_ptr = cert.basic_constraints().path_len; |
} |
@@ -339,7 +289,6 @@ WARN_UNUSED_RESULT bool PrepareForNextCertificate( |
if (cert.has_key_usage() && |
!cert.key_usage().AssertsBit(KEY_USAGE_BIT_KEY_CERT_SIGN)) { |
errors->AddError(kKeyCertSignBitNotSet); |
- return false; |
} |
// From RFC 5280 section 6.1.4 step o: |
@@ -348,15 +297,12 @@ WARN_UNUSED_RESULT bool PrepareForNextCertificate( |
// the certificate. Process any other recognized non-critical |
// extension present in the certificate that is relevant to path |
// processing. |
- if (!VerifyNoUnconsumedCriticalExtensions(cert, errors)) |
- return false; |
- |
- return true; |
+ VerifyNoUnconsumedCriticalExtensions(cert, errors); |
} |
// Checks that if the target certificate has properties that only a CA should |
// have (keyCertSign, CA=true, pathLenConstraint), then its other properties |
-// are consistent with being a CA. |
+// are consistent with being a CA. If it does, adds errors to |errors|. |
// |
// This follows from some requirements in RFC 5280 section 4.2.1.9. In |
// particular: |
@@ -376,9 +322,8 @@ WARN_UNUSED_RESULT bool PrepareForNextCertificate( |
// TODO(eroman): I don't believe Firefox enforces the keyCertSign restriction |
// for compatibility reasons. Investigate if we need to similarly relax this |
// constraint. |
-WARN_UNUSED_RESULT bool VerifyTargetCertHasConsistentCaBits( |
- const ParsedCertificate& cert, |
- CertErrors* errors) { |
+void VerifyTargetCertHasConsistentCaBits(const ParsedCertificate& cert, |
+ CertErrors* errors) { |
// Check if the certificate contains any property specific to CAs. |
bool has_ca_property = |
(cert.has_basic_constraints() && |
@@ -398,17 +343,12 @@ WARN_UNUSED_RESULT bool VerifyTargetCertHasConsistentCaBits( |
// TODO(eroman): Add DER for basic constraints and key usage. |
errors->AddError(kTargetCertInconsistentCaBits); |
} |
- |
- return success; |
} |
- |
- return true; |
} |
// This function corresponds with RFC 5280 section 6.1.5's "Wrap-Up Procedure". |
// It does processing for the final certificate (the target cert). |
-WARN_UNUSED_RESULT bool WrapUp(const ParsedCertificate& cert, |
- CertErrors* errors) { |
+void WrapUp(const ParsedCertificate& cert, CertErrors* errors) { |
// TODO(crbug.com/634452): Steps a-b are omitted as policy constraints are not |
// yet implemented. |
@@ -424,36 +364,29 @@ WARN_UNUSED_RESULT bool WrapUp(const ParsedCertificate& cert, |
// |
// Note that this is duplicated by PrepareForNextCertificate() so as to |
// directly match the procedures in RFC 5280's section 6.1. |
- if (!VerifyNoUnconsumedCriticalExtensions(cert, errors)) |
- return false; |
+ VerifyNoUnconsumedCriticalExtensions(cert, errors); |
// TODO(eroman): Step g is omitted, as policy constraints are not yet |
// implemented. |
// The following check is NOT part of RFC 5280 6.1.5's "Wrap-Up Procedure", |
// however is implied by RFC 5280 section 4.2.1.9. |
- if (!VerifyTargetCertHasConsistentCaBits(cert, errors)) |
- return false; |
- |
- return true; |
+ VerifyTargetCertHasConsistentCaBits(cert, errors); |
} |
// Initializes the path validation algorithm given anchor constraints. This |
// follows the description in RFC 5937 |
-WARN_UNUSED_RESULT bool ProcessTrustAnchorConstraints( |
+void ProcessTrustAnchorConstraints( |
const TrustAnchor& trust_anchor, |
size_t* max_path_length_ptr, |
std::vector<const NameConstraints*>* name_constraints_list, |
CertErrors* errors) { |
- // Set the trust anchor as the current context for any subsequent errors. |
- CertErrorScoperNoParams error_context(errors, kContextTrustAnchor); |
- |
// In RFC 5937 the enforcement of anchor constraints is governed by the input |
// enforceTrustAnchorConstraints to path validation. In our implementation |
// this is always on, and enforcement is controlled solely by whether or not |
// the trust anchor specified constraints. |
if (!trust_anchor.enforces_constraints()) |
- return true; |
+ return; |
// Anchor constraints are encoded via the attached certificate. |
const ParsedCertificate& cert = *trust_anchor.cert(); |
@@ -492,29 +425,25 @@ WARN_UNUSED_RESULT bool ProcessTrustAnchorConstraints( |
// Extensions may be marked critical or not critical. When trust anchor |
// constraints are enforced, clients MUST reject certification paths |
// containing a trust anchor with unrecognized critical extensions. |
- if (!VerifyNoUnconsumedCriticalExtensions(cert, errors)) |
- return false; |
- |
- return true; |
+ VerifyNoUnconsumedCriticalExtensions(cert, errors); |
} |
-} // namespace |
- |
// This implementation is structured to mimic the description of certificate |
// path verification given by RFC 5280 section 6.1. |
-bool VerifyCertificateChain(const ParsedCertificateList& certs, |
- const TrustAnchor* trust_anchor, |
- const SignaturePolicy* signature_policy, |
- const der::GeneralizedTime& time, |
- CertErrors* errors) { |
+void VerifyCertificateChainNoReturnValue( |
+ const ParsedCertificateList& certs, |
+ const TrustAnchor* trust_anchor, |
+ const SignaturePolicy* signature_policy, |
+ const der::GeneralizedTime& time, |
+ CertPathErrors* errors) { |
DCHECK(trust_anchor); |
DCHECK(signature_policy); |
DCHECK(errors); |
// An empty chain is necessarily invalid. |
if (certs.empty()) { |
- errors->AddError(kChainIsEmpty); |
- return false; |
+ errors->GetOtherErrors()->AddError(kChainIsEmpty); |
+ return; |
} |
// Will contain a NameConstraints for each previous cert in the chain which |
@@ -557,10 +486,13 @@ bool VerifyCertificateChain(const ParsedCertificateList& certs, |
size_t max_path_length = certs.size(); |
// Apply any trust anchor constraints per RFC 5937. |
- if (!ProcessTrustAnchorConstraints(*trust_anchor, &max_path_length, |
- &name_constraints_list, errors)) { |
- return false; |
- } |
+ // |
+ // TODO(eroman): Errors on the trust anchor are put into a certificate bucket |
+ // GetErrorsForCert(certs.size()). This is a bit magical, and |
+ // has some integration issues. |
+ ProcessTrustAnchorConstraints(*trust_anchor, &max_path_length, |
+ &name_constraints_list, |
+ errors->GetErrorsForCert(certs.size())); |
// Iterate over all the certificates in the reverse direction: starting from |
// the certificate signed by trust anchor and progressing towards the target |
@@ -580,28 +512,24 @@ bool VerifyCertificateChain(const ParsedCertificateList& certs, |
const ParsedCertificate& cert = *certs[index_into_certs]; |
- // Set the current certificate as the context for any subsequent errors. |
- CertErrorScoperForCert error_context(errors, i); |
+ // Output errors for the current certificate into an error bucket that is |
+ // associated with that certificate. |
+ CertErrors* cert_errors = errors->GetErrorsForCert(index_into_certs); |
// Per RFC 5280 section 6.1: |
// * Do basic processing for each certificate |
// * If it is the last certificate in the path (target certificate) |
// - Then run "Wrap up" |
// - Otherwise run "Prepare for Next cert" |
- if (!BasicCertificateProcessing( |
- cert, is_target_cert, signature_policy, time, working_spki, |
- working_normalized_issuer_name, name_constraints_list, errors)) { |
- return false; |
- } |
+ BasicCertificateProcessing(cert, is_target_cert, signature_policy, time, |
+ working_spki, working_normalized_issuer_name, |
+ name_constraints_list, cert_errors); |
if (!is_target_cert) { |
- if (!PrepareForNextCertificate(cert, &max_path_length, &working_spki, |
- &working_normalized_issuer_name, |
- &name_constraints_list, errors)) { |
- return false; |
- } |
+ PrepareForNextCertificate(cert, &max_path_length, &working_spki, |
+ &working_normalized_issuer_name, |
+ &name_constraints_list, cert_errors); |
} else { |
- if (!WrapUp(cert, errors)) |
- return false; |
+ WrapUp(cert, cert_errors); |
} |
} |
@@ -609,8 +537,21 @@ bool VerifyCertificateChain(const ParsedCertificateList& certs, |
// |
// A certificate MUST NOT appear more than once in a prospective |
// certification path. |
+} |
- return true; |
+} // namespace |
+ |
+bool VerifyCertificateChain(const ParsedCertificateList& certs, |
+ const TrustAnchor* trust_anchor, |
+ const SignaturePolicy* signature_policy, |
+ const der::GeneralizedTime& time, |
+ CertPathErrors* errors) { |
+ // TODO(eroman): This function requires that |errors| is empty upon entry, |
+ // which is not part of the API contract. |
+ DCHECK(!errors->ContainsHighSeverityErrors()); |
+ VerifyCertificateChainNoReturnValue(certs, trust_anchor, signature_policy, |
+ time, errors); |
+ return !errors->ContainsHighSeverityErrors(); |
} |
} // namespace net |