Use InsertBraces - and reformat pki as such Bug: 659 Change-Id: I48eeda0bcd0de45d70644c321138225f83cb6c60 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64107 Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: Bob Beck <bbe@google.com> Auto-Submit: Bob Beck <bbe@google.com>
diff --git a/.clang-format b/.clang-format index 33b77f9..4ab7f98 100644 --- a/.clang-format +++ b/.clang-format
@@ -8,6 +8,7 @@ # IncludeCategories does not recognize <openssl/header.h>. We should # reconfigure IncludeCategories to match. For now, keep it at Preserve. IncludeBlocks: Preserve +InsertBraces: true TypenameMacros: ['LHASH_OF', 'STACK_OF'] StatementMacros: - "DECLARE_ASN1_ALLOC_FUNCTIONS"
diff --git a/pki/asn1_util.cc b/pki/asn1_util.cc index 4e34412..3c9cef7 100644 --- a/pki/asn1_util.cc +++ b/pki/asn1_util.cc
@@ -36,15 +36,18 @@ der::Parser parser(in); der::Parser certificate; - if (!parser.ReadSequence(&certificate)) + if (!parser.ReadSequence(&certificate)) { return false; + } // We don't allow junk after the certificate. - if (parser.HasMore()) + if (parser.HasMore()) { return false; + } - if (!certificate.ReadSequence(tbs_certificate)) + if (!certificate.ReadSequence(tbs_certificate)) { return false; + } bool unused; if (!tbs_certificate->SkipOptionalTag( @@ -53,17 +56,21 @@ } // serialNumber - if (!tbs_certificate->SkipTag(der::kInteger)) + if (!tbs_certificate->SkipTag(der::kInteger)) { return false; + } // signature - if (!tbs_certificate->SkipTag(der::kSequence)) + if (!tbs_certificate->SkipTag(der::kSequence)) { return false; + } // issuer - if (!tbs_certificate->SkipTag(der::kSequence)) + if (!tbs_certificate->SkipTag(der::kSequence)) { return false; + } // validity - if (!tbs_certificate->SkipTag(der::kSequence)) + if (!tbs_certificate->SkipTag(der::kSequence)) { return false; + } return true; } @@ -89,8 +96,9 @@ der::Parser *extensions_parser) { bool present; der::Parser tbs_cert_parser; - if (!SeekToSPKI(in, &tbs_cert_parser)) + if (!SeekToSPKI(in, &tbs_cert_parser)) { return false; + } // From RFC 5280, section 4.1 // TBSCertificate ::= SEQUENCE { @@ -101,8 +109,9 @@ // extensions [3] EXPLICIT Extensions OPTIONAL } // subjectPublicKeyInfo - if (!tbs_cert_parser.SkipTag(der::kSequence)) + if (!tbs_cert_parser.SkipTag(der::kSequence)) { return false; + } // issuerUniqueID if (!tbs_cert_parser.SkipOptionalTag(der::kTagContextSpecific | 1, &present)) { @@ -134,11 +143,13 @@ // |extensions| was EXPLICITly tagged, so we still need to remove the // ASN.1 SEQUENCE header. der::Parser explicit_extensions_parser(extensions.value()); - if (!explicit_extensions_parser.ReadSequence(extensions_parser)) + if (!explicit_extensions_parser.ReadSequence(extensions_parser)) { return false; + } - if (explicit_extensions_parser.HasMore()) + if (explicit_extensions_parser.HasMore()) { return false; + } *extensions_present = true; return true; @@ -154,8 +165,9 @@ ParsedExtension *out_extension) { der::Parser extensions; bool extensions_present; - if (!SeekToExtensions(der::Input(cert), &extensions_present, &extensions)) + if (!SeekToExtensions(der::Input(cert), &extensions_present, &extensions)) { return false; + } if (!extensions_present) { *out_extension_present = false; return true; @@ -183,22 +195,26 @@ bool ExtractSubjectFromDERCert(std::string_view cert, std::string_view *subject_out) { der::Parser parser; - if (!SeekToSubject(der::Input(cert), &parser)) + if (!SeekToSubject(der::Input(cert), &parser)) { return false; + } der::Input subject; - if (!parser.ReadRawTLV(&subject)) + if (!parser.ReadRawTLV(&subject)) { return false; + } *subject_out = subject.AsStringView(); return true; } bool ExtractSPKIFromDERCert(std::string_view cert, std::string_view *spki_out) { der::Parser parser; - if (!SeekToSPKI(der::Input(cert), &parser)) + if (!SeekToSPKI(der::Input(cert), &parser)) { return false; + } der::Input spki; - if (!parser.ReadRawTLV(&spki)) + if (!parser.ReadRawTLV(&spki)) { return false; + } *spki_out = spki.AsStringView(); return true; } @@ -217,17 +233,20 @@ // Step into SubjectPublicKeyInfo sequence. der::Parser parser((der::Input(spki))); der::Parser spki_parser; - if (!parser.ReadSequence(&spki_parser)) + if (!parser.ReadSequence(&spki_parser)) { return false; + } // Step over algorithm field (a SEQUENCE). - if (!spki_parser.SkipTag(der::kSequence)) + if (!spki_parser.SkipTag(der::kSequence)) { return false; + } // Extract the subjectPublicKey field. der::Input spk; - if (!spki_parser.ReadTag(der::kBitString, &spk)) + if (!spki_parser.ReadTag(der::kBitString, &spk)) { return false; + } *spk_out = spk.AsStringView(); return true; } @@ -273,12 +292,14 @@ der::Parser parser((der::Input(cert))); der::Parser certificate; - if (!parser.ReadSequence(&certificate)) + if (!parser.ReadSequence(&certificate)) { return false; + } der::Parser tbs_certificate; - if (!certificate.ReadSequence(&tbs_certificate)) + if (!certificate.ReadSequence(&tbs_certificate)) { return false; + } bool unused; if (!tbs_certificate.SkipOptionalTag( @@ -287,16 +308,19 @@ } // serialNumber - if (!tbs_certificate.SkipTag(der::kInteger)) + if (!tbs_certificate.SkipTag(der::kInteger)) { return false; + } // signature der::Input tbs_algorithm; - if (!tbs_certificate.ReadRawTLV(&tbs_algorithm)) + if (!tbs_certificate.ReadRawTLV(&tbs_algorithm)) { return false; + } der::Input cert_algorithm; - if (!certificate.ReadRawTLV(&cert_algorithm)) + if (!certificate.ReadRawTLV(&cert_algorithm)) { return false; + } *cert_signature_algorithm_sequence = cert_algorithm.AsStringView(); *tbs_signature_algorithm_sequence = tbs_algorithm.AsStringView(); @@ -314,10 +338,12 @@ ParsedExtension extension; if (!ExtractExtensionWithOID(cert, der::Input(extension_oid), - out_extension_present, &extension)) + out_extension_present, &extension)) { return false; - if (!*out_extension_present) + } + if (!*out_extension_present) { return true; + } *out_extension_critical = extension.critical; *out_contents = extension.value.AsStringView();
diff --git a/pki/cert_errors.cc b/pki/cert_errors.cc index 80f28d0..da95b49 100644 --- a/pki/cert_errors.cc +++ b/pki/cert_errors.cc
@@ -52,8 +52,9 @@ result += CertErrorIdToDebugString(id); result += +"\n"; - if (params) + if (params) { AppendLinesWithIndentation(params->ToDebugString(), " ", &result); + } return result; } @@ -84,16 +85,18 @@ std::string CertErrors::ToDebugString() const { std::string result; - for (const CertError &node : nodes_) + for (const CertError &node : nodes_) { result += node.ToDebugString(); + } return result; } bool CertErrors::ContainsError(CertErrorId id) const { for (const CertError &node : nodes_) { - if (node.id == id) + if (node.id == id) { return true; + } } return false; } @@ -101,8 +104,9 @@ bool CertErrors::ContainsAnyErrorWithSeverity( CertError::Severity severity) const { for (const CertError &node : nodes_) { - if (node.severity == severity) + if (node.severity == severity) { return true; + } } return false; } @@ -115,14 +119,16 @@ CertPathErrors::~CertPathErrors() = default; CertErrors *CertPathErrors::GetErrorsForCert(size_t cert_index) { - if (cert_index >= cert_errors_.size()) + if (cert_index >= cert_errors_.size()) { cert_errors_.resize(cert_index + 1); + } return &cert_errors_[cert_index]; } const CertErrors *CertPathErrors::GetErrorsForCert(size_t cert_index) const { - if (cert_index >= cert_errors_.size()) + if (cert_index >= cert_errors_.size()) { return nullptr; + } return &cert_errors_[cert_index]; } @@ -130,12 +136,14 @@ bool CertPathErrors::ContainsError(CertErrorId id) const { for (const CertErrors &errors : cert_errors_) { - if (errors.ContainsError(id)) + if (errors.ContainsError(id)) { return true; + } } - if (other_errors_.ContainsError(id)) + if (other_errors_.ContainsError(id)) { return true; + } return false; } @@ -143,12 +151,14 @@ bool CertPathErrors::ContainsAnyErrorWithSeverity( CertError::Severity severity) const { for (const CertErrors &errors : cert_errors_) { - if (errors.ContainsAnyErrorWithSeverity(severity)) + if (errors.ContainsAnyErrorWithSeverity(severity)) { return true; + } } - if (other_errors_.ContainsAnyErrorWithSeverity(severity)) + if (other_errors_.ContainsAnyErrorWithSeverity(severity)) { return true; + } return false; } @@ -162,8 +172,9 @@ // then continue. const CertErrors &errors = cert_errors_[i]; std::string cert_errors_string = errors.ToDebugString(); - if (cert_errors_string.empty()) + if (cert_errors_string.empty()) { continue; + } // Add a header that identifies which certificate this CertErrors pertains // to.
diff --git a/pki/cert_issuer_source_static.cc b/pki/cert_issuer_source_static.cc index 9d395d3..bbf967f 100644 --- a/pki/cert_issuer_source_static.cc +++ b/pki/cert_issuer_source_static.cc
@@ -21,8 +21,9 @@ ParsedCertificateList *issuers) { auto range = intermediates_.equal_range(cert->normalized_issuer().AsStringView()); - for (auto it = range.first; it != range.second; ++it) + for (auto it = range.first; it != range.second; ++it) { issuers->push_back(it->second); + } } void CertIssuerSourceStatic::AsyncGetIssuersOf(
diff --git a/pki/cert_issuer_source_sync_unittest.h b/pki/cert_issuer_source_sync_unittest.h index 12b44e7..c9e25c1 100644 --- a/pki/cert_issuer_source_sync_unittest.h +++ b/pki/cert_issuer_source_sync_unittest.h
@@ -34,8 +34,9 @@ ::testing::AssertionResult r = ReadTestPem("testdata/cert_issuer_source_static_unittest/" + file_name, "CERTIFICATE", &der); - if (!r) + if (!r) { return r; + } CertErrors errors; *result = ParsedCertificate::Create( bssl::UniquePtr<CRYPTO_BUFFER>(CRYPTO_BUFFER_new( @@ -95,17 +96,20 @@ source().SyncGetIssuersOf(cert.get(), &matches); std::vector<der::Input> der_result_matches; - for (const auto &it : matches) + for (const auto &it : matches) { der_result_matches.push_back(it->der_cert()); + } std::sort(der_result_matches.begin(), der_result_matches.end()); std::vector<der::Input> der_expected_matches; - for (const auto &it : expected_matches) + for (const auto &it : expected_matches) { der_expected_matches.push_back(it->der_cert()); + } std::sort(der_expected_matches.begin(), der_expected_matches.end()); - if (der_expected_matches == der_result_matches) + if (der_expected_matches == der_result_matches) { return true; + } // Print some extra information for debugging. EXPECT_EQ(der_expected_matches, der_result_matches);
diff --git a/pki/certificate_policies.cc b/pki/certificate_policies.cc index 69de164..9ae941d 100644 --- a/pki/certificate_policies.cc +++ b/pki/certificate_policies.cc
@@ -64,8 +64,9 @@ } // policyQualifierId PolicyQualifierId, der::Input qualifier_oid; - if (!policy_information_parser.ReadTag(der::kOid, &qualifier_oid)) + if (!policy_information_parser.ReadTag(der::kOid, &qualifier_oid)) { return false; + } if (restrict_to_known_qualifiers && qualifier_oid != der::Input(kCpsPointerId) && qualifier_oid != der::Input(kUserNoticeId)) { @@ -85,8 +86,9 @@ return false; } - if (policy_qualifiers) + if (policy_qualifiers) { policy_qualifiers->push_back({qualifier_oid, qualifier_tlv}); + } } return true; } @@ -136,11 +138,13 @@ // certificatePolicies ::= SEQUENCE SIZE (1..MAX) OF PolicyInformation der::Parser extension_parser(extension_value); der::Parser policies_sequence_parser; - if (!extension_parser.ReadSequence(&policies_sequence_parser)) + if (!extension_parser.ReadSequence(&policies_sequence_parser)) { return false; + } // Should not have trailing data after certificatePolicies sequence. - if (extension_parser.HasMore()) + if (extension_parser.HasMore()) { return false; + } // The certificatePolicies sequence should have at least 1 element. if (!policies_sequence_parser.HasMore()) { errors->AddError(kPoliciesEmptySequence); @@ -148,18 +152,21 @@ } policy_oids->clear(); - if (policy_informations) + if (policy_informations) { policy_informations->clear(); + } while (policies_sequence_parser.HasMore()) { // PolicyInformation ::= SEQUENCE { der::Parser policy_information_parser; - if (!policies_sequence_parser.ReadSequence(&policy_information_parser)) + if (!policies_sequence_parser.ReadSequence(&policy_information_parser)) { return false; + } // policyIdentifier CertPolicyId, der::Input policy_oid; - if (!policy_information_parser.ReadTag(der::kOid, &policy_oid)) + if (!policy_information_parser.ReadTag(der::kOid, &policy_oid)) { return false; + } policy_oids->push_back(policy_oid); @@ -170,8 +177,9 @@ policy_qualifiers = &policy_informations->back().policy_qualifiers; } - if (!policy_information_parser.HasMore()) + if (!policy_information_parser.HasMore()) { continue; + } // policyQualifiers SEQUENCE SIZE (1..MAX) OF // PolicyQualifierInfo OPTIONAL } @@ -249,8 +257,9 @@ // PolicyConstraints ::= SEQUENCE { der::Parser sequence_parser; - if (!parser.ReadSequence(&sequence_parser)) + if (!parser.ReadSequence(&sequence_parser)) { return false; + } // RFC 5280 prohibits CAs from issuing PolicyConstraints as an empty sequence: // @@ -259,8 +268,9 @@ // or the requireExplicitPolicy field MUST be present. The behavior of // clients that encounter an empty policy constraints field is not // addressed in this profile. - if (!sequence_parser.HasMore()) + if (!sequence_parser.HasMore()) { return false; + } std::optional<der::Input> require_value; if (!sequence_parser.ReadOptionalTag(der::ContextSpecificPrimitive(0), @@ -295,8 +305,9 @@ } // There should be no remaining data. - if (sequence_parser.HasMore() || parser.HasMore()) + if (sequence_parser.HasMore() || parser.HasMore()) { return false; + } return true; } @@ -337,34 +348,41 @@ // PolicyMappings ::= SEQUENCE SIZE (1..MAX) OF SEQUENCE { der::Parser sequence_parser; - if (!parser.ReadSequence(&sequence_parser)) + if (!parser.ReadSequence(&sequence_parser)) { return false; + } // Must be at least 1 mapping. - if (!sequence_parser.HasMore()) + if (!sequence_parser.HasMore()) { return false; + } while (sequence_parser.HasMore()) { der::Parser mapping_parser; - if (!sequence_parser.ReadSequence(&mapping_parser)) + if (!sequence_parser.ReadSequence(&mapping_parser)) { return false; + } ParsedPolicyMapping mapping; - if (!mapping_parser.ReadTag(der::kOid, &mapping.issuer_domain_policy)) + if (!mapping_parser.ReadTag(der::kOid, &mapping.issuer_domain_policy)) { return false; - if (!mapping_parser.ReadTag(der::kOid, &mapping.subject_domain_policy)) + } + if (!mapping_parser.ReadTag(der::kOid, &mapping.subject_domain_policy)) { return false; + } // There shouldn't be extra unconsumed data. - if (mapping_parser.HasMore()) + if (mapping_parser.HasMore()) { return false; + } mappings->push_back(mapping); } // There shouldn't be extra unconsumed data. - if (parser.HasMore()) + if (parser.HasMore()) { return false; + } return true; }
diff --git a/pki/crl.cc b/pki/crl.cc index 2ac7948..187a034 100644 --- a/pki/crl.cc +++ b/pki/crl.cc
@@ -56,32 +56,38 @@ // CertificateList ::= SEQUENCE { der::Parser certificate_list_parser; - if (!parser.ReadSequence(&certificate_list_parser)) + if (!parser.ReadSequence(&certificate_list_parser)) { return false; + } // tbsCertList TBSCertList - if (!certificate_list_parser.ReadRawTLV(out_tbs_cert_list_tlv)) + if (!certificate_list_parser.ReadRawTLV(out_tbs_cert_list_tlv)) { return false; + } // signatureAlgorithm AlgorithmIdentifier, - if (!certificate_list_parser.ReadRawTLV(out_signature_algorithm_tlv)) + if (!certificate_list_parser.ReadRawTLV(out_signature_algorithm_tlv)) { return false; + } // signatureValue BIT STRING } std::optional<der::BitString> signature_value = certificate_list_parser.ReadBitString(); - if (!signature_value) + if (!signature_value) { return false; + } *out_signature_value = signature_value.value(); // There isn't an extension point at the end of CertificateList. - if (certificate_list_parser.HasMore()) + if (certificate_list_parser.HasMore()) { return false; + } // By definition the input was a single CertificateList, so there shouldn't be // unconsumed data. - if (parser.HasMore()) + if (parser.HasMore()) { return false; + } return true; } @@ -91,21 +97,25 @@ // TBSCertList ::= SEQUENCE { der::Parser tbs_parser; - if (!parser.ReadSequence(&tbs_parser)) + if (!parser.ReadSequence(&tbs_parser)) { return false; + } // version Version OPTIONAL, // -- if present, MUST be v2 std::optional<der::Input> version_der; - if (!tbs_parser.ReadOptionalTag(der::kInteger, &version_der)) + if (!tbs_parser.ReadOptionalTag(der::kInteger, &version_der)) { return false; + } if (version_der.has_value()) { uint64_t version64; - if (!der::ParseUint64(*version_der, &version64)) + if (!der::ParseUint64(*version_der, &version64)) { return false; + } // If version is present, it MUST be v2(1). - if (version64 != 1) + if (version64 != 1) { return false; + } out->version = CrlVersion::V2; } else { // Uh, RFC 5280 doesn't actually say it anywhere, but presumably if version @@ -114,16 +124,19 @@ } // signature AlgorithmIdentifier, - if (!tbs_parser.ReadRawTLV(&out->signature_algorithm_tlv)) + if (!tbs_parser.ReadRawTLV(&out->signature_algorithm_tlv)) { return false; + } // issuer Name, - if (!tbs_parser.ReadRawTLV(&out->issuer_tlv)) + if (!tbs_parser.ReadRawTLV(&out->issuer_tlv)) { return false; + } // thisUpdate Time, - if (!ReadUTCOrGeneralizedTime(&tbs_parser, &out->this_update)) + if (!ReadUTCOrGeneralizedTime(&tbs_parser, &out->this_update)) { return false; + } // nextUpdate Time OPTIONAL, der::Tag maybe_next_update_tag; @@ -133,8 +146,9 @@ (maybe_next_update_tag == der::kUtcTime || maybe_next_update_tag == der::kGeneralizedTime)) { der::GeneralizedTime next_update_time; - if (!ReadUTCOrGeneralizedTime(&tbs_parser, &next_update_time)) + if (!ReadUTCOrGeneralizedTime(&tbs_parser, &next_update_time)) { return false; + } out->next_update = next_update_time; } else { out->next_update = std::nullopt; @@ -147,8 +161,9 @@ &unused_revoked_certificates) && maybe_revoked_certifigates_tag == der::kSequence) { der::Input revoked_certificates_tlv; - if (!tbs_parser.ReadRawTLV(&revoked_certificates_tlv)) + if (!tbs_parser.ReadRawTLV(&revoked_certificates_tlv)) { return false; + } out->revoked_certificates_tlv = revoked_certificates_tlv; } else { out->revoked_certificates_tlv = std::nullopt; @@ -161,8 +176,9 @@ return false; } if (out->crl_extensions_tlv.has_value()) { - if (out->version != CrlVersion::V2) + if (out->version != CrlVersion::V2) { return false; + } } if (tbs_parser.HasMore()) { @@ -172,8 +188,9 @@ // By definition the input was a single sequence, so there shouldn't be // unconsumed data. - if (parser.HasMore()) + if (parser.HasMore()) { return false; + } return true; } @@ -185,14 +202,16 @@ der::Parser idp_extension_value_parser(extension_value); // IssuingDistributionPoint ::= SEQUENCE { der::Parser idp_parser; - if (!idp_extension_value_parser.ReadSequence(&idp_parser)) + if (!idp_extension_value_parser.ReadSequence(&idp_parser)) { return false; + } // 5.2.5. Conforming CRLs issuers MUST NOT issue CRLs where the DER // encoding of the issuing distribution point extension is an empty // sequence. - if (!idp_parser.HasMore()) + if (!idp_parser.HasMore()) { return false; + } // distributionPoint [0] DistributionPointName OPTIONAL, std::optional<der::Input> distribution_point; @@ -220,8 +239,9 @@ CertErrors errors; *out_distribution_point_names = GeneralNames::CreateFromValue(*der_full_name, &errors); - if (!*out_distribution_point_names) + if (!*out_distribution_point_names) { return false; + } if (dp_name_parser.HasMore()) { // CHOICE represents a single value. @@ -239,10 +259,12 @@ } if (only_contains_user_certs.has_value()) { bool bool_value; - if (!der::ParseBool(*only_contains_user_certs, &bool_value)) + if (!der::ParseBool(*only_contains_user_certs, &bool_value)) { return false; - if (!bool_value) + } + if (!bool_value) { return false; // DER-encoding requires DEFAULT values be omitted. + } *out_only_contains_cert_type = ContainedCertsType::USER_CERTS; } @@ -254,10 +276,12 @@ } if (only_contains_ca_certs.has_value()) { bool bool_value; - if (!der::ParseBool(*only_contains_ca_certs, &bool_value)) + if (!der::ParseBool(*only_contains_ca_certs, &bool_value)) { return false; - if (!bool_value) + } + if (!bool_value) { return false; // DER-encoding requires DEFAULT values be omitted. + } if (*out_only_contains_cert_type != ContainedCertsType::ANY_CERTS) { // 5.2.5. at most one of onlyContainsUserCerts, onlyContainsCACerts, // and onlyContainsAttributeCerts may be set to TRUE. @@ -271,8 +295,9 @@ // onlyContainsAttributeCerts [5] BOOLEAN DEFAULT FALSE } // onlySomeReasons, indirectCRL, and onlyContainsAttributeCerts are not // supported, fail parsing if they are present. - if (idp_parser.HasMore()) + if (idp_parser.HasMore()) { return false; + } return true; } @@ -291,62 +316,73 @@ // revokedCertificates SEQUENCE OF SEQUENCE { der::Parser revoked_certificates_parser; - if (!parser.ReadSequence(&revoked_certificates_parser)) + if (!parser.ReadSequence(&revoked_certificates_parser)) { return CRLRevocationStatus::UNKNOWN; + } // RFC 5280 Section 5.1.2.6: "When there are no revoked certificates, the // revoked certificates list MUST be absent." - if (!revoked_certificates_parser.HasMore()) + if (!revoked_certificates_parser.HasMore()) { return CRLRevocationStatus::UNKNOWN; + } // By definition the input was a single Extensions sequence, so there // shouldn't be unconsumed data. - if (parser.HasMore()) + if (parser.HasMore()) { return CRLRevocationStatus::UNKNOWN; + } bool found_matching_serial = false; while (revoked_certificates_parser.HasMore()) { // revokedCertificates SEQUENCE OF SEQUENCE { der::Parser crl_entry_parser; - if (!revoked_certificates_parser.ReadSequence(&crl_entry_parser)) + if (!revoked_certificates_parser.ReadSequence(&crl_entry_parser)) { return CRLRevocationStatus::UNKNOWN; + } der::Input revoked_cert_serial_number; // userCertificate CertificateSerialNumber, - if (!crl_entry_parser.ReadTag(der::kInteger, &revoked_cert_serial_number)) + if (!crl_entry_parser.ReadTag(der::kInteger, &revoked_cert_serial_number)) { return CRLRevocationStatus::UNKNOWN; + } // revocationDate Time, der::GeneralizedTime unused_revocation_date; - if (!ReadUTCOrGeneralizedTime(&crl_entry_parser, &unused_revocation_date)) + if (!ReadUTCOrGeneralizedTime(&crl_entry_parser, &unused_revocation_date)) { return CRLRevocationStatus::UNKNOWN; + } // crlEntryExtensions Extensions OPTIONAL if (crl_entry_parser.HasMore()) { // -- if present, version MUST be v2 - if (crl_version != CrlVersion::V2) + if (crl_version != CrlVersion::V2) { return CRLRevocationStatus::UNKNOWN; + } der::Input crl_entry_extensions_tlv; - if (!crl_entry_parser.ReadRawTLV(&crl_entry_extensions_tlv)) + if (!crl_entry_parser.ReadRawTLV(&crl_entry_extensions_tlv)) { return CRLRevocationStatus::UNKNOWN; + } std::map<der::Input, ParsedExtension> extensions; - if (!ParseExtensions(crl_entry_extensions_tlv, &extensions)) + if (!ParseExtensions(crl_entry_extensions_tlv, &extensions)) { return CRLRevocationStatus::UNKNOWN; + } // RFC 5280 Section 5.3: "If a CRL contains a critical CRL entry // extension that the application cannot process, then the application // MUST NOT use that CRL to determine the status of any certificates." for (const auto &ext : extensions) { - if (ext.second.critical) + if (ext.second.critical) { return CRLRevocationStatus::UNKNOWN; + } } } - if (crl_entry_parser.HasMore()) + if (crl_entry_parser.HasMore()) { return CRLRevocationStatus::UNKNOWN; + } if (revoked_cert_serial_number == cert_serial) { // Cert is revoked, but can't return yet since there might be critical @@ -355,8 +391,9 @@ } } - if (found_matching_serial) + if (found_matching_serial) { return CRLRevocationStatus::REVOKED; + } // |cert| is not present in the revokedCertificates list. return CRLRevocationStatus::GOOD; @@ -401,8 +438,9 @@ } ParsedCrlTbsCertList tbs_cert_list; - if (!ParseCrlTbsCertList(tbs_cert_list_tlv, &tbs_cert_list)) + if (!ParseCrlTbsCertList(tbs_cert_list_tlv, &tbs_cert_list)) { return CRLRevocationStatus::UNKNOWN; + } // 5.1.1.2 signatureAlgorithm // @@ -466,16 +504,18 @@ // TODO(https://crbug.com/749276): could do exact comparison first and only // fall back to normalizing if that fails. std::string normalized_crl_issuer; - if (!NormalizeNameTLV(tbs_cert_list.issuer_tlv, &normalized_crl_issuer)) + if (!NormalizeNameTLV(tbs_cert_list.issuer_tlv, &normalized_crl_issuer)) { return CRLRevocationStatus::UNKNOWN; + } if (der::Input(normalized_crl_issuer) != target_cert->normalized_issuer()) { return CRLRevocationStatus::UNKNOWN; } if (tbs_cert_list.crl_extensions_tlv.has_value()) { std::map<der::Input, ParsedExtension> extensions; - if (!ParseExtensions(*tbs_cert_list.crl_extensions_tlv, &extensions)) + if (!ParseExtensions(*tbs_cert_list.crl_extensions_tlv, &extensions)) { return CRLRevocationStatus::UNKNOWN; + } // 6.3.3 (b) (2) If the complete CRL includes an issuing distribution point // (IDP) CRL extension, check the following: @@ -559,8 +599,9 @@ for (const auto &ext : extensions) { // Fail if any unhandled critical CRL extensions are present. - if (ext.second.critical) + if (ext.second.critical) { return CRLRevocationStatus::UNKNOWN; + } } }
diff --git a/pki/encode_values.cc b/pki/encode_values.cc index 4d4a0e9..25dc677 100644 --- a/pki/encode_values.cc +++ b/pki/encode_values.cc
@@ -13,8 +13,9 @@ namespace { bool WriteFourDigit(uint16_t value, uint8_t out[4]) { - if (value >= 10000) + if (value >= 10000) { return false; + } out[3] = '0' + (value % 10); value /= 10; out[2] = '0' + (value % 10); @@ -26,8 +27,9 @@ } bool WriteTwoDigit(uint8_t value, uint8_t out[2]) { - if (value >= 100) + if (value >= 100) { return false; + } out[0] = '0' + (value / 10); out[1] = '0' + (value % 10); return true; @@ -82,12 +84,14 @@ } bool EncodeUTCTime(const GeneralizedTime &time, uint8_t out[kUTCTimeLength]) { - if (!time.InUTCTimeRange()) + if (!time.InUTCTimeRange()) { return false; + } uint16_t year = time.year - 1900; - if (year >= 100) + if (year >= 100) { year -= 100; + } if (!WriteTwoDigit(year, out) || !WriteTwoDigit(time.month, out + 2) || !WriteTwoDigit(time.day, out + 4) ||
diff --git a/pki/extended_key_usage.cc b/pki/extended_key_usage.cc index d81599a..caf2514 100644 --- a/pki/extended_key_usage.cc +++ b/pki/extended_key_usage.cc
@@ -14,26 +14,30 @@ std::vector<der::Input> *eku_oids) { der::Parser extension_parser(extension_value); der::Parser sequence_parser; - if (!extension_parser.ReadSequence(&sequence_parser)) + if (!extension_parser.ReadSequence(&sequence_parser)) { return false; + } // Section 4.2.1.12 of RFC 5280 defines ExtKeyUsageSyntax as: // ExtKeyUsageSyntax ::= SEQUENCE SIZE (1..MAX) OF KeyPurposeId // // Therefore, the sequence must contain at least one KeyPurposeId. - if (!sequence_parser.HasMore()) + if (!sequence_parser.HasMore()) { return false; + } while (sequence_parser.HasMore()) { der::Input eku_oid; - if (!sequence_parser.ReadTag(der::kOid, &eku_oid)) + if (!sequence_parser.ReadTag(der::kOid, &eku_oid)) { // The SEQUENCE OF must contain only KeyPurposeIds (OIDs). return false; + } eku_oids->push_back(eku_oid); } - if (extension_parser.HasMore()) + if (extension_parser.HasMore()) { // The extension value must follow ExtKeyUsageSyntax - there is no way that // it could be extended to allow for something after the SEQUENCE OF. return false; + } return true; }
diff --git a/pki/extended_key_usage_unittest.cc b/pki/extended_key_usage_unittest.cc index f764e52..e617dce 100644 --- a/pki/extended_key_usage_unittest.cc +++ b/pki/extended_key_usage_unittest.cc
@@ -15,8 +15,9 @@ // Helper method to check if an EKU is present in a std::vector of EKUs. bool HasEKU(const std::vector<der::Input> &list, const der::Input &eku) { for (const auto &oid : list) { - if (oid == eku) + if (oid == eku) { return true; + } } return false; }
diff --git a/pki/general_names.cc b/pki/general_names.cc index 253e54e..a881c11 100644 --- a/pki/general_names.cc +++ b/pki/general_names.cc
@@ -103,8 +103,9 @@ der::Parser parser(input); der::Tag tag; der::Input value; - if (!parser.ReadTagAndValue(&tag, &value)) + if (!parser.ReadTagAndValue(&tag, &value)) { return false; + } GeneralNameTypes name_type = GENERAL_NAME_NONE; if (tag == der::ContextSpecificConstructed(0)) { // otherName [0] OtherName, @@ -140,8 +141,9 @@ // only the value portion. der::Parser name_parser(value); der::Input name_value; - if (!name_parser.ReadTag(der::kSequence, &name_value) || parser.HasMore()) + if (!name_parser.ReadTag(der::kSequence, &name_value) || parser.HasMore()) { return false; + } subtrees->directory_names.push_back(name_value); } else if (tag == der::ContextSpecificConstructed(5)) { // ediPartyName [5] EDIPartyName,
diff --git a/pki/input.cc b/pki/input.cc index 274055b..a0d8af6 100644 --- a/pki/input.cc +++ b/pki/input.cc
@@ -34,16 +34,18 @@ : data_(in.UnsafeData()), len_(in.Length()) {} bool ByteReader::ReadByte(uint8_t *byte_p) { - if (!HasMore()) + if (!HasMore()) { return false; + } *byte_p = *data_; Advance(1); return true; } bool ByteReader::ReadBytes(size_t len, Input *out) { - if (len > len_) + if (len > len_) { return false; + } *out = Input(data_, len); Advance(len); return true;
diff --git a/pki/name_constraints.cc b/pki/name_constraints.cc index 6dea412..6de5b50 100644 --- a/pki/name_constraints.cc +++ b/pki/name_constraints.cc
@@ -56,14 +56,17 @@ bool DNSNameMatches(std::string_view name, std::string_view dns_constraint, WildcardMatchType wildcard_matching) { // Everything matches the empty DNS name constraint. - if (dns_constraint.empty()) + if (dns_constraint.empty()) { return true; + } // Normalize absolute DNS names by removing the trailing dot, if any. - if (!name.empty() && *name.rbegin() == '.') + if (!name.empty() && *name.rbegin() == '.') { name.remove_suffix(1); - if (!dns_constraint.empty() && *dns_constraint.rbegin() == '.') + } + if (!dns_constraint.empty() && *dns_constraint.rbegin() == '.') { dns_constraint.remove_suffix(1); + } // Wildcard partial-match handling ("*.bar.com" matching name constraint // "foo.bar.com"). This only handles the case where the the dnsname and the @@ -89,13 +92,15 @@ } // Exact match. - if (name.size() == dns_constraint.size()) + if (name.size() == dns_constraint.size()) { return true; + } // If dNSName constraint starts with a dot, only subdomains should match. // (e.g., "foo.bar.com" matches constraint ".bar.com", but "bar.com" doesn't.) // RFC 5280 is ambiguous, but this matches the behavior of other platforms. - if (!dns_constraint.empty() && dns_constraint[0] == '.') + if (!dns_constraint.empty() && dns_constraint[0] == '.') { dns_constraint.remove_prefix(1); + } // Subtree match. if (name.size() > dns_constraint.size() && name[name.size() - dns_constraint.size() - 1] == '.') { @@ -127,16 +132,19 @@ // BaseDistance ::= INTEGER (0..MAX) der::Parser sequence_parser(value); // The GeneralSubtrees sequence should have at least 1 element. - if (!sequence_parser.HasMore()) + if (!sequence_parser.HasMore()) { return false; + } while (sequence_parser.HasMore()) { der::Parser subtree_sequence; - if (!sequence_parser.ReadSequence(&subtree_sequence)) + if (!sequence_parser.ReadSequence(&subtree_sequence)) { return false; + } der::Input raw_general_name; - if (!subtree_sequence.ReadRawTLV(&raw_general_name)) + if (!subtree_sequence.ReadRawTLV(&raw_general_name)) { return false; + } if (!ParseGeneralName(raw_general_name, GeneralNames::IP_ADDRESS_AND_NETMASK, subtrees, @@ -157,8 +165,9 @@ // fail if a name of this type actually appears in a subsequent cert and // this extension was marked critical. However the minimum and maximum // fields appear uncommon enough that implementing that isn't useful. - if (subtree_sequence.HasMore()) + if (subtree_sequence.HasMore()) { return false; + } } return true; } @@ -273,8 +282,9 @@ BSSL_CHECK(errors); auto name_constraints = std::make_unique<NameConstraints>(); - if (!name_constraints->Parse(extension_value, is_critical, errors)) + if (!name_constraints->Parse(extension_value, is_critical, errors)) { return nullptr; + } return name_constraints; } @@ -288,10 +298,12 @@ // NameConstraints ::= SEQUENCE { // permittedSubtrees [0] GeneralSubtrees OPTIONAL, // excludedSubtrees [1] GeneralSubtrees OPTIONAL } - if (!extension_parser.ReadSequence(&sequence_parser)) + if (!extension_parser.ReadSequence(&sequence_parser)) { return false; - if (extension_parser.HasMore()) + } + if (extension_parser.HasMore()) { return false; + } std::optional<der::Input> permitted_subtrees_value; if (!sequence_parser.ReadOptionalTag(der::ContextSpecificConstructed(0), @@ -325,11 +337,13 @@ // Conforming CAs MUST NOT issue certificates where name constraints is an // empty sequence. That is, either the permittedSubtrees field or the // excludedSubtrees MUST be present. - if (!permitted_subtrees_value && !excluded_subtrees_value) + if (!permitted_subtrees_value && !excluded_subtrees_value) { return false; + } - if (sequence_parser.HasMore()) + if (sequence_parser.HasMore()) { return false; + } return true; } @@ -488,8 +502,9 @@ // This code assumes that criticality condition is checked by the caller, and // therefore only needs to avoid the IsPermittedDirectoryName check against an // empty subject in such a case. - if (subject_alt_names && subject_rdn_sequence.Length() == 0) + if (subject_alt_names && subject_rdn_sequence.Length() == 0) { return; + } if (!IsPermittedDirectoryName(subject_rdn_sequence)) { errors->AddError(cert_errors::kNotPermittedByNameConstraints); @@ -608,21 +623,24 @@ // When matching wildcard hosts against excluded subtrees, consider it a // match if the constraint would match any expansion of the wildcard. Eg, // *.bar.com should match a constraint of foo.bar.com. - if (DNSNameMatches(name, excluded_name, WILDCARD_PARTIAL_MATCH)) + if (DNSNameMatches(name, excluded_name, WILDCARD_PARTIAL_MATCH)) { return false; + } } // If permitted subtrees are not constrained, any name that is not excluded is // allowed. - if (!(permitted_subtrees_.present_name_types & GENERAL_NAME_DNS_NAME)) + if (!(permitted_subtrees_.present_name_types & GENERAL_NAME_DNS_NAME)) { return true; + } for (const auto &permitted_name : permitted_subtrees_.dns_names) { // When matching wildcard hosts against permitted subtrees, consider it a // match only if the constraint would match all expansions of the wildcard. // Eg, *.bar.com should match a constraint of bar.com, but not foo.bar.com. - if (DNSNameMatches(name, permitted_name, WILDCARD_FULL_MATCH)) + if (DNSNameMatches(name, permitted_name, WILDCARD_FULL_MATCH)) { return true; + } } return false; @@ -631,18 +649,21 @@ bool NameConstraints::IsPermittedDirectoryName( const der::Input &name_rdn_sequence) const { for (const auto &excluded_name : excluded_subtrees_.directory_names) { - if (VerifyNameInSubtree(name_rdn_sequence, excluded_name)) + if (VerifyNameInSubtree(name_rdn_sequence, excluded_name)) { return false; + } } // If permitted subtrees are not constrained, any name that is not excluded is // allowed. - if (!(permitted_subtrees_.present_name_types & GENERAL_NAME_DIRECTORY_NAME)) + if (!(permitted_subtrees_.present_name_types & GENERAL_NAME_DIRECTORY_NAME)) { return true; + } for (const auto &permitted_name : permitted_subtrees_.directory_names) { - if (VerifyNameInSubtree(name_rdn_sequence, permitted_name)) + if (VerifyNameInSubtree(name_rdn_sequence, permitted_name)) { return true; + } } return false;
diff --git a/pki/name_constraints_unittest.cc b/pki/name_constraints_unittest.cc index 4121b6f..569bbec 100644 --- a/pki/name_constraints_unittest.cc +++ b/pki/name_constraints_unittest.cc
@@ -64,10 +64,12 @@ CertErrors errors; name_constraints->IsPermittedCert(subject_rdn_sequence, subject_alt_names, &errors); - if (!errors.ContainsAnyErrorWithSeverity(CertError::SEVERITY_HIGH)) + if (!errors.ContainsAnyErrorWithSeverity(CertError::SEVERITY_HIGH)) { return ::testing::AssertionSuccess(); - if (!errors.ContainsError(cert_errors::kNotPermittedByNameConstraints)) + } + if (!errors.ContainsError(cert_errors::kNotPermittedByNameConstraints)) { ADD_FAILURE() << "unexpected error " << errors.ToDebugString(); + } return ::testing::AssertionFailure(); }
diff --git a/pki/ocsp.cc b/pki/ocsp.cc index ed0d2ce..54afab5 100644 --- a/pki/ocsp.cc +++ b/pki/ocsp.cc
@@ -40,25 +40,34 @@ bool ParseOCSPCertID(const der::Input &raw_tlv, OCSPCertID *out) { der::Parser outer_parser(raw_tlv); der::Parser parser; - if (!outer_parser.ReadSequence(&parser)) + if (!outer_parser.ReadSequence(&parser)) { return false; - if (outer_parser.HasMore()) + } + if (outer_parser.HasMore()) { return false; + } der::Input sigalg_tlv; - if (!parser.ReadRawTLV(&sigalg_tlv)) + if (!parser.ReadRawTLV(&sigalg_tlv)) { return false; - if (!ParseHashAlgorithm(sigalg_tlv, &(out->hash_algorithm))) + } + if (!ParseHashAlgorithm(sigalg_tlv, &(out->hash_algorithm))) { return false; - if (!parser.ReadTag(der::kOctetString, &(out->issuer_name_hash))) + } + if (!parser.ReadTag(der::kOctetString, &(out->issuer_name_hash))) { return false; - if (!parser.ReadTag(der::kOctetString, &(out->issuer_key_hash))) + } + if (!parser.ReadTag(der::kOctetString, &(out->issuer_key_hash))) { return false; - if (!parser.ReadTag(der::kInteger, &(out->serial_number))) + } + if (!parser.ReadTag(der::kInteger, &(out->serial_number))) { return false; + } CertErrors errors; - if (!VerifySerialNumber(out->serial_number, false /*warnings_only*/, &errors)) + if (!VerifySerialNumber(out->serial_number, false /*warnings_only*/, + &errors)) { return false; + } return !parser.HasMore(); } @@ -75,8 +84,9 @@ // } bool ParseRevokedInfo(const der::Input &raw_tlv, OCSPCertStatus *out) { der::Parser parser(raw_tlv); - if (!parser.ReadGeneralizedTime(&(out->revocation_time))) + if (!parser.ReadGeneralizedTime(&(out->revocation_time))) { return false; + } der::Input reason_input; if (!parser.ReadOptionalTag(der::ContextSpecificConstructed(0), &reason_input, @@ -87,20 +97,24 @@ der::Parser reason_parser(reason_input); der::Input reason_value_input; uint8_t reason_value; - if (!reason_parser.ReadTag(der::kEnumerated, &reason_value_input)) + if (!reason_parser.ReadTag(der::kEnumerated, &reason_value_input)) { return false; - if (!der::ParseUint8(reason_value_input, &reason_value)) + } + if (!der::ParseUint8(reason_value_input, &reason_value)) { return false; + } if (reason_value > static_cast<uint8_t>(OCSPCertStatus::RevocationReason::LAST)) { return false; } out->revocation_reason = static_cast<OCSPCertStatus::RevocationReason>(reason_value); - if (out->revocation_reason == OCSPCertStatus::RevocationReason::UNUSED) + if (out->revocation_reason == OCSPCertStatus::RevocationReason::UNUSED) { return false; - if (reason_parser.HasMore()) + } + if (reason_parser.HasMore()) { return false; + } } return !parser.HasMore(); } @@ -120,16 +134,18 @@ der::Parser parser(raw_tlv); der::Tag status_tag; der::Input status; - if (!parser.ReadTagAndValue(&status_tag, &status)) + if (!parser.ReadTagAndValue(&status_tag, &status)) { return false; + } out->has_reason = false; if (status_tag == der::ContextSpecificPrimitive(0)) { out->status = OCSPRevocationStatus::GOOD; } else if (status_tag == der::ContextSpecificConstructed(1)) { out->status = OCSPRevocationStatus::REVOKED; - if (!ParseRevokedInfo(status, out)) + if (!ParseRevokedInfo(status, out)) { return false; + } } else if (status_tag == der::ContextSpecificPrimitive(2)) { out->status = OCSPRevocationStatus::UNKNOWN; } else { @@ -166,20 +182,26 @@ OCSPSingleResponse *out) { der::Parser outer_parser(raw_tlv); der::Parser parser; - if (!outer_parser.ReadSequence(&parser)) + if (!outer_parser.ReadSequence(&parser)) { return false; - if (outer_parser.HasMore()) + } + if (outer_parser.HasMore()) { return false; + } - if (!parser.ReadRawTLV(&(out->cert_id_tlv))) + if (!parser.ReadRawTLV(&(out->cert_id_tlv))) { return false; + } der::Input status_tlv; - if (!parser.ReadRawTLV(&status_tlv)) + if (!parser.ReadRawTLV(&status_tlv)) { return false; - if (!ParseCertStatus(status_tlv, &(out->cert_status))) + } + if (!ParseCertStatus(status_tlv, &(out->cert_status))) { return false; - if (!parser.ReadGeneralizedTime(&(out->this_update))) + } + if (!parser.ReadGeneralizedTime(&(out->this_update))) { return false; + } der::Input next_update_input; if (!parser.ReadOptionalTag(der::ContextSpecificConstructed(0), @@ -188,10 +210,12 @@ } if (out->has_next_update) { der::Parser next_update_parser(next_update_input); - if (!next_update_parser.ReadGeneralizedTime(&(out->next_update))) + if (!next_update_parser.ReadGeneralizedTime(&(out->next_update))) { return false; - if (next_update_parser.HasMore()) + } + if (next_update_parser.HasMore()) { return false; + } } if (!parser.ReadOptionalTag(der::ContextSpecificConstructed(1), @@ -216,8 +240,9 @@ der::Parser parser(raw_tlv); der::Tag id_tag; der::Input id_input; - if (!parser.ReadTagAndValue(&id_tag, &id_input)) + if (!parser.ReadTagAndValue(&id_tag, &id_input)) { return false; + } if (id_tag == der::ContextSpecificConstructed(1)) { out->type = OCSPResponseData::ResponderType::NAME; @@ -225,12 +250,15 @@ } else if (id_tag == der::ContextSpecificConstructed(2)) { der::Parser key_parser(id_input); der::Input key_hash; - if (!key_parser.ReadTag(der::kOctetString, &key_hash)) + if (!key_parser.ReadTag(der::kOctetString, &key_hash)) { return false; - if (key_parser.HasMore()) + } + if (key_parser.HasMore()) { return false; - if (key_hash.Length() != SHA_DIGEST_LENGTH) + } + if (key_hash.Length() != SHA_DIGEST_LENGTH) { return false; + } out->type = OCSPResponseData::ResponderType::KEY_HASH; out->key_hash = key_hash; @@ -252,10 +280,12 @@ bool ParseOCSPResponseData(const der::Input &raw_tlv, OCSPResponseData *out) { der::Parser outer_parser(raw_tlv); der::Parser parser; - if (!outer_parser.ReadSequence(&parser)) + if (!outer_parser.ReadSequence(&parser)) { return false; - if (outer_parser.HasMore()) + } + if (outer_parser.HasMore()) { return false; + } der::Input version_input; bool version_present; @@ -269,33 +299,41 @@ // TODO: Add warning about non-strict parsing. if (version_present) { der::Parser version_parser(version_input); - if (!version_parser.ReadUint8(&(out->version))) + if (!version_parser.ReadUint8(&(out->version))) { return false; - if (version_parser.HasMore()) + } + if (version_parser.HasMore()) { return false; + } } else { out->version = 0; } - if (out->version != 0) + if (out->version != 0) { return false; + } der::Input responder_input; - if (!parser.ReadRawTLV(&responder_input)) + if (!parser.ReadRawTLV(&responder_input)) { return false; - if (!ParseResponderID(responder_input, &(out->responder_id))) + } + if (!ParseResponderID(responder_input, &(out->responder_id))) { return false; - if (!parser.ReadGeneralizedTime(&(out->produced_at))) + } + if (!parser.ReadGeneralizedTime(&(out->produced_at))) { return false; + } der::Parser responses_parser; - if (!parser.ReadSequence(&responses_parser)) + if (!parser.ReadSequence(&responses_parser)) { return false; + } out->responses.clear(); while (responses_parser.HasMore()) { der::Input single_response; - if (!responses_parser.ReadRawTLV(&single_response)) + if (!responses_parser.ReadRawTLV(&single_response)) { return false; + } out->responses.push_back(single_response); } @@ -322,25 +360,31 @@ bool ParseBasicOCSPResponse(const der::Input &raw_tlv, OCSPResponse *out) { der::Parser outer_parser(raw_tlv); der::Parser parser; - if (!outer_parser.ReadSequence(&parser)) + if (!outer_parser.ReadSequence(&parser)) { return false; - if (outer_parser.HasMore()) + } + if (outer_parser.HasMore()) { return false; + } - if (!parser.ReadRawTLV(&(out->data))) + if (!parser.ReadRawTLV(&(out->data))) { return false; + } der::Input sigalg_tlv; - if (!parser.ReadRawTLV(&sigalg_tlv)) + if (!parser.ReadRawTLV(&sigalg_tlv)) { return false; + } // TODO(crbug.com/634443): Propagate the errors. std::optional<SignatureAlgorithm> sigalg = ParseSignatureAlgorithm(sigalg_tlv); - if (!sigalg) + if (!sigalg) { return false; + } out->signature_algorithm = sigalg.value(); std::optional<der::BitString> signature = parser.ReadBitString(); - if (!signature) + if (!signature) { return false; + } out->signature = signature.value(); der::Input certs_input; if (!parser.ReadOptionalTag(der::ContextSpecificConstructed(0), &certs_input, @@ -352,14 +396,17 @@ if (out->has_certs) { der::Parser certs_seq_parser(certs_input); der::Parser certs_parser; - if (!certs_seq_parser.ReadSequence(&certs_parser)) + if (!certs_seq_parser.ReadSequence(&certs_parser)) { return false; - if (certs_seq_parser.HasMore()) + } + if (certs_seq_parser.HasMore()) { return false; + } while (certs_parser.HasMore()) { der::Input cert_tlv; - if (!certs_parser.ReadRawTLV(&cert_tlv)) + if (!certs_parser.ReadRawTLV(&cert_tlv)) { return false; + } out->certs.push_back(cert_tlv); } } @@ -381,24 +428,29 @@ bool ParseOCSPResponse(const der::Input &raw_tlv, OCSPResponse *out) { der::Parser outer_parser(raw_tlv); der::Parser parser; - if (!outer_parser.ReadSequence(&parser)) + if (!outer_parser.ReadSequence(&parser)) { return false; - if (outer_parser.HasMore()) + } + if (outer_parser.HasMore()) { return false; + } der::Input response_status_input; uint8_t response_status; - if (!parser.ReadTag(der::kEnumerated, &response_status_input)) + if (!parser.ReadTag(der::kEnumerated, &response_status_input)) { return false; - if (!der::ParseUint8(response_status_input, &response_status)) + } + if (!der::ParseUint8(response_status_input, &response_status)) { return false; + } if (response_status > static_cast<uint8_t>(OCSPResponse::ResponseStatus::LAST)) { return false; } out->status = static_cast<OCSPResponse::ResponseStatus>(response_status); - if (out->status == OCSPResponse::ResponseStatus::UNUSED) + if (out->status == OCSPResponse::ResponseStatus::UNUSED) { return false; + } if (out->status == OCSPResponse::ResponseStatus::SUCCESSFUL) { der::Parser outer_bytes_parser; @@ -407,26 +459,33 @@ &outer_bytes_parser)) { return false; } - if (!outer_bytes_parser.ReadSequence(&bytes_parser)) + if (!outer_bytes_parser.ReadSequence(&bytes_parser)) { return false; - if (outer_bytes_parser.HasMore()) + } + if (outer_bytes_parser.HasMore()) { return false; + } der::Input type_oid; - if (!bytes_parser.ReadTag(der::kOid, &type_oid)) + if (!bytes_parser.ReadTag(der::kOid, &type_oid)) { return false; - if (type_oid != der::Input(kBasicOCSPResponseOid)) + } + if (type_oid != der::Input(kBasicOCSPResponseOid)) { return false; + } // As per RFC 6960 Section 4.2.1, the value of |response| SHALL be the DER // encoding of BasicOCSPResponse. der::Input response; - if (!bytes_parser.ReadTag(der::kOctetString, &response)) + if (!bytes_parser.ReadTag(der::kOctetString, &response)) { return false; - if (!ParseBasicOCSPResponse(response, out)) + } + if (!ParseBasicOCSPResponse(response, out)) { return false; - if (bytes_parser.HasMore()) + } + if (bytes_parser.HasMore()) { return false; + } } return !parser.HasMore(); @@ -505,15 +564,18 @@ break; } - if (!VerifyHash(type, id.issuer_name_hash, certificate->tbs().issuer_tlv)) + if (!VerifyHash(type, id.issuer_name_hash, certificate->tbs().issuer_tlv)) { return false; + } der::Input key_tlv; - if (!GetSubjectPublicKeyBytes(issuer_certificate->tbs().spki_tlv, &key_tlv)) + if (!GetSubjectPublicKeyBytes(issuer_certificate->tbs().spki_tlv, &key_tlv)) { return false; + } - if (!VerifyHash(type, id.issuer_key_hash, key_tlv)) + if (!VerifyHash(type, id.issuer_key_hash, key_tlv)) { return false; + } return id.serial_number == certificate->tbs().serial_number; } @@ -550,14 +612,16 @@ der::Input cert_rdn; if (!der::Parser(id.name).ReadTag(der::kSequence, &name_rdn) || !der::Parser(cert->tbs().subject_tlv) - .ReadTag(der::kSequence, &cert_rdn)) + .ReadTag(der::kSequence, &cert_rdn)) { return false; + } return VerifyNameMatch(name_rdn, cert_rdn); } case OCSPResponseData::ResponderType::KEY_HASH: { der::Input key; - if (!GetSubjectPublicKeyBytes(cert->tbs().spki_tlv, &key)) + if (!GetSubjectPublicKeyBytes(cert->tbs().spki_tlv, &key)) { return false; + } return VerifyHash(EVP_sha1(), id.key_hash, key); } } @@ -591,13 +655,15 @@ // The Authorized Responder must include the value id-kp-OCSPSigning as // part of the extended key usage extension. - if (!responder_certificate->has_extended_key_usage()) + if (!responder_certificate->has_extended_key_usage()) { return false; + } for (const auto &key_purpose_oid : responder_certificate->extended_key_usage()) { - if (key_purpose_oid == der::Input(kOCSPSigning)) + if (key_purpose_oid == der::Input(kOCSPSigning)) { return true; + } } return false; } @@ -636,8 +702,9 @@ OCSPParseCertificate(responder_cert_tlv.AsStringView()); // If failed parsing the certificate, keep looking. - if (!cur_responder_certificate) + if (!cur_responder_certificate) { continue; + } // If the certificate doesn't match the OCSP's responder ID, keep looking. if (!CheckResponderIDMatchesCertificate(response_data.responder_id, @@ -710,8 +777,9 @@ for (const auto &ext : extensions) { // The CT OCSP extension is handled in ct::ExtractSCTListFromOCSPResponse - if (ext.second.oid == ct_ext_oid) + if (ext.second.oid == ct_ext_oid) { continue; + } // TODO: handle SingleResponse extensions @@ -742,8 +810,9 @@ // certificate. A SingleResponse matches a certificate if it has the same // serial number, issuer name (hash), and issuer public key (hash). OCSPSingleResponse single_response; - if (!ParseOCSPSingleResponse(single_response_der, &single_response)) + if (!ParseOCSPSingleResponse(single_response_der, &single_response)) { return OCSPRevocationStatus::UNKNOWN; + } // Reject unhandled critical extensions in SingleResponse if (single_response.has_extensions && @@ -753,10 +822,12 @@ } OCSPCertID cert_id; - if (!ParseOCSPCertID(single_response.cert_id_tlv, &cert_id)) + if (!ParseOCSPCertID(single_response.cert_id_tlv, &cert_id)) { return OCSPRevocationStatus::UNKNOWN; - if (!CheckCertIDMatchesCertificate(cert_id, cert, issuer_certificate)) + } + if (!CheckCertIDMatchesCertificate(cert_id, cert, issuer_certificate)) { continue; + } // The SingleResponse matches the certificate, but may be out of date. Out // of date responses are noted seperate from responses with mismatched @@ -768,8 +839,9 @@ ? &single_response.next_update : nullptr, verify_time_epoch_seconds, max_age_seconds)) { - if (*response_details != OCSPVerifyResult::PROVIDED) + if (*response_details != OCSPVerifyResult::PROVIDED) { *response_details = OCSPVerifyResult::INVALID_DATE; + } continue; } @@ -921,8 +993,9 @@ // number doesn't matter for correctness. const size_t kInitialBufferSize = 100; - if (!CBB_init(cbb.get(), kInitialBufferSize)) + if (!CBB_init(cbb.get(), kInitialBufferSize)) { return false; + } // OCSPRequest ::= SEQUENCE { // tbsRequest TBSRequest, @@ -934,29 +1007,34 @@ // requestList SEQUENCE OF Request, // requestExtensions [2] EXPLICIT Extensions OPTIONAL } CBB ocsp_request; - if (!CBB_add_asn1(cbb.get(), &ocsp_request, CBS_ASN1_SEQUENCE)) + if (!CBB_add_asn1(cbb.get(), &ocsp_request, CBS_ASN1_SEQUENCE)) { return false; + } CBB tbs_request; - if (!CBB_add_asn1(&ocsp_request, &tbs_request, CBS_ASN1_SEQUENCE)) + if (!CBB_add_asn1(&ocsp_request, &tbs_request, CBS_ASN1_SEQUENCE)) { return false; + } // "version", "requestorName", and "requestExtensions" are omitted. CBB request_list; - if (!CBB_add_asn1(&tbs_request, &request_list, CBS_ASN1_SEQUENCE)) + if (!CBB_add_asn1(&tbs_request, &request_list, CBS_ASN1_SEQUENCE)) { return false; + } CBB request; - if (!CBB_add_asn1(&request_list, &request, CBS_ASN1_SEQUENCE)) + if (!CBB_add_asn1(&request_list, &request, CBS_ASN1_SEQUENCE)) { return false; + } // Request ::= SEQUENCE { // reqCert CertID, // singleRequestExtensions [0] EXPLICIT Extensions OPTIONAL } CBB req_cert; - if (!CBB_add_asn1(&request, &req_cert, CBS_ASN1_SEQUENCE)) + if (!CBB_add_asn1(&request, &req_cert, CBS_ASN1_SEQUENCE)) { return false; + } // CertID ::= SEQUENCE { // hashAlgorithm AlgorithmIdentifier, @@ -966,19 +1044,22 @@ // TODO(eroman): Don't use SHA1. const EVP_MD *md = EVP_sha1(); - if (!EVP_marshal_digest_algorithm(&req_cert, md)) + if (!EVP_marshal_digest_algorithm(&req_cert, md)) { return false; + } AppendHashAsOctetString(md, &req_cert, issuer->tbs().subject_tlv); der::Input key_tlv; - if (!GetSubjectPublicKeyBytes(issuer->tbs().spki_tlv, &key_tlv)) + if (!GetSubjectPublicKeyBytes(issuer->tbs().spki_tlv, &key_tlv)) { return false; + } AppendHashAsOctetString(md, &req_cert, key_tlv); CBB serial_number; - if (!CBB_add_asn1(&req_cert, &serial_number, CBS_ASN1_INTEGER)) + if (!CBB_add_asn1(&req_cert, &serial_number, CBS_ASN1_INTEGER)) { return false; + } if (!CBB_add_bytes(&serial_number, cert->tbs().serial_number.UnsafeData(), cert->tbs().serial_number.Length())) { return false; @@ -986,8 +1067,9 @@ uint8_t *result_bytes; size_t result_bytes_length; - if (!CBB_finish(cbb.get(), &result_bytes, &result_bytes_length)) + if (!CBB_finish(cbb.get(), &result_bytes, &result_bytes_length)) { return false; + } bssl::UniquePtr<uint8_t> delete_tbs_cert_bytes(result_bytes); request_der->assign(result_bytes, result_bytes + result_bytes_length);
diff --git a/pki/ocsp_verify_result.cc b/pki/ocsp_verify_result.cc index 30ab6ca..f343f8e 100644 --- a/pki/ocsp_verify_result.cc +++ b/pki/ocsp_verify_result.cc
@@ -11,8 +11,9 @@ OCSPVerifyResult::~OCSPVerifyResult() = default; bool OCSPVerifyResult::operator==(const OCSPVerifyResult &other) const { - if (response_status != other.response_status) + if (response_status != other.response_status) { return false; + } if (response_status == PROVIDED) { // |revocation_status| is only defined when |response_status| is PROVIDED.
diff --git a/pki/parse_certificate.cc b/pki/parse_certificate.cc index 6894b23..ab06c93 100644 --- a/pki/parse_certificate.cc +++ b/pki/parse_certificate.cc
@@ -76,8 +76,9 @@ [[nodiscard]] bool IsSequenceTLV(const der::Input &input) { der::Parser parser(input); der::Parser unused_sequence_parser; - if (!parser.ReadSequence(&unused_sequence_parser)) + if (!parser.ReadSequence(&unused_sequence_parser)) { return false; + } // Should by a single SEQUENCE by definition of the function. return !parser.HasMore(); } @@ -103,8 +104,9 @@ CertificateVersion *version) { der::Parser parser(in); uint64_t version64; - if (!parser.ReadUint64(&version64)) + if (!parser.ReadUint64(&version64)) { return false; + } switch (version64) { case 0: @@ -159,8 +161,9 @@ CertErrors errors; distribution_point->distribution_point_fullname = GeneralNames::CreateFromValue(*der_full_name, &errors); - if (!distribution_point->distribution_point_fullname) + if (!distribution_point->distribution_point_fullname) { return false; + } return !parser.HasMore(); } @@ -191,8 +194,9 @@ // DistributionPoint ::= SEQUENCE { der::Parser distrib_point_parser; - if (!parser->ReadSequence(&distrib_point_parser)) + if (!parser->ReadSequence(&distrib_point_parser)) { return false; + } // distributionPoint [0] DistributionPointName OPTIONAL, std::optional<der::Input> distribution_point_name; @@ -224,11 +228,13 @@ // RFC 5280, section 4.2.1.13: // either distributionPoint or cRLIssuer MUST be present. - if (!distribution_point_name && !distribution_point.crl_issuer) + if (!distribution_point_name && !distribution_point.crl_issuer) { return false; + } - if (distrib_point_parser.HasMore()) + if (distrib_point_parser.HasMore()) { return false; + } distribution_points->push_back(std::move(distribution_point)); return true; @@ -261,8 +267,9 @@ // 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. - if (negative) + if (negative) { errors->AddWarning(kSerialNumberIsNegative); + } if (value.Length() == 1 && value[0] == 0) { errors->AddWarning(kSerialNumberIsZero); } @@ -285,14 +292,17 @@ der::Input value; der::Tag tag; - if (!parser->ReadTagAndValue(&tag, &value)) + if (!parser->ReadTagAndValue(&tag, &value)) { return false; + } - if (tag == der::kUtcTime) + if (tag == der::kUtcTime) { return der::ParseUTCTime(value, out); + } - if (tag == der::kGeneralizedTime) + if (tag == der::kGeneralizedTime) { return der::ParseGeneralizedTime(value, out); + } // Unrecognized tag. return false; @@ -305,25 +315,30 @@ // Validity ::= SEQUENCE { der::Parser validity_parser; - if (!parser.ReadSequence(&validity_parser)) + if (!parser.ReadSequence(&validity_parser)) { return false; + } // notBefore Time, - if (!ReadUTCOrGeneralizedTime(&validity_parser, not_before)) + if (!ReadUTCOrGeneralizedTime(&validity_parser, not_before)) { return false; + } // notAfter Time } - if (!ReadUTCOrGeneralizedTime(&validity_parser, not_after)) + if (!ReadUTCOrGeneralizedTime(&validity_parser, not_after)) { return false; + } // By definition the input was a single Validity sequence, so there shouldn't // be unconsumed data. - if (parser.HasMore()) + if (parser.HasMore()) { return false; + } // The Validity type does not have an extension point. - if (validity_parser.HasMore()) + if (validity_parser.HasMore()) { return false; + } // Note that RFC 5280 doesn't require notBefore to be <= // notAfter, so that will not be considered a "parsing" error here. Instead it @@ -340,8 +355,9 @@ // |out_errors| is optional. But ensure it is non-null for the remainder of // this function. CertErrors unused_errors; - if (!out_errors) + if (!out_errors) { out_errors = &unused_errors; + } der::Parser parser(certificate_tlv); @@ -411,8 +427,9 @@ ParsedTbsCertificate *out, CertErrors *errors) { // The rest of this function assumes that |errors| is non-null. CertErrors unused_errors; - if (!errors) + if (!errors) { errors = &unused_errors; + } // TODO(crbug.com/634443): Add useful error information to |errors|. @@ -456,8 +473,9 @@ options.allow_invalid_serial_numbers, errors)) { // Invalid serial numbers are only considered fatal failures if // |!allow_invalid_serial_numbers|. - if (!options.allow_invalid_serial_numbers) + if (!options.allow_invalid_serial_numbers) { return false; + } } // signature AlgorithmIdentifier, @@ -572,8 +590,9 @@ // By definition the input was a single TBSCertificate, so there shouldn't be // unconsumed data. - if (parser.HasMore()) + if (parser.HasMore()) { return false; + } return true; } @@ -593,39 +612,47 @@ // Extension ::= SEQUENCE { der::Parser extension_parser; - if (!parser.ReadSequence(&extension_parser)) + if (!parser.ReadSequence(&extension_parser)) { return false; + } // extnID OBJECT IDENTIFIER, - if (!extension_parser.ReadTag(der::kOid, &out->oid)) + if (!extension_parser.ReadTag(der::kOid, &out->oid)) { return false; + } // critical BOOLEAN DEFAULT FALSE, out->critical = false; bool has_critical; der::Input critical; - if (!extension_parser.ReadOptionalTag(der::kBool, &critical, &has_critical)) + if (!extension_parser.ReadOptionalTag(der::kBool, &critical, &has_critical)) { return false; + } if (has_critical) { - if (!der::ParseBool(critical, &out->critical)) + if (!der::ParseBool(critical, &out->critical)) { return false; - if (!out->critical) + } + if (!out->critical) { return false; // DER-encoding requires DEFAULT values be omitted. + } } // extnValue OCTET STRING - if (!extension_parser.ReadTag(der::kOctetString, &out->value)) + if (!extension_parser.ReadTag(der::kOctetString, &out->value)) { return false; + } // The Extension type does not have an extension point (everything goes in // extnValue). - if (extension_parser.HasMore()) + if (extension_parser.HasMore()) { return false; + } // By definition the input was a single Extension sequence, so there shouldn't // be unconsumed data. - if (parser.HasMore()) + if (parser.HasMore()) { return false; + } return true; } @@ -637,13 +664,15 @@ // Extensions ::= SEQUENCE SIZE (1..MAX) OF Extension der::Parser extensions_parser; - if (!parser.ReadSequence(&extensions_parser)) + if (!parser.ReadSequence(&extensions_parser)) { return false; + } // The Extensions SEQUENCE must contains at least 1 element (otherwise it // should have been omitted). - if (!extensions_parser.HasMore()) + if (!extensions_parser.HasMore()) { return false; + } extensions->clear(); @@ -651,24 +680,28 @@ ParsedExtension extension; der::Input extension_tlv; - if (!extensions_parser.ReadRawTLV(&extension_tlv)) + if (!extensions_parser.ReadRawTLV(&extension_tlv)) { return false; + } - if (!ParseExtension(extension_tlv, &extension)) + if (!ParseExtension(extension_tlv, &extension)) { return false; + } bool is_duplicate = !extensions->insert(std::make_pair(extension.oid, extension)).second; // RFC 5280 says that an extension should not appear more than once. - if (is_duplicate) + if (is_duplicate) { return false; + } } // By definition the input was a single Extensions sequence, so there // shouldn't be unconsumed data. - if (parser.HasMore()) + if (parser.HasMore()) { return false; + } return true; } @@ -678,8 +711,9 @@ std::map<der::Input, ParsedExtension> *unconsumed_extensions, ParsedExtension *extension) { auto it = unconsumed_extensions->find(oid); - if (it == unconsumed_extensions->end()) + if (it == unconsumed_extensions->end()) { return false; + } *extension = it->second; unconsumed_extensions->erase(it); @@ -692,18 +726,21 @@ // BasicConstraints ::= SEQUENCE { der::Parser sequence_parser; - if (!parser.ReadSequence(&sequence_parser)) + if (!parser.ReadSequence(&sequence_parser)) { return false; + } // cA BOOLEAN DEFAULT FALSE, out->is_ca = false; bool has_ca; der::Input ca; - if (!sequence_parser.ReadOptionalTag(der::kBool, &ca, &has_ca)) + if (!sequence_parser.ReadOptionalTag(der::kBool, &ca, &has_ca)) { return false; + } if (has_ca) { - if (!der::ParseBool(ca, &out->is_ca)) + if (!der::ParseBool(ca, &out->is_ca)) { return false; + } // TODO(eroman): Should reject if CA was set to false, since // DER-encoding requires DEFAULT values be omitted. In // practice however there are a lot of certificates that use @@ -718,21 +755,24 @@ } if (out->has_path_len) { // TODO(eroman): Surface reason for failure if length was longer than uint8. - if (!der::ParseUint8(encoded_path_len, &out->path_len)) + if (!der::ParseUint8(encoded_path_len, &out->path_len)) { return false; + } } else { // Default initialize to 0 as a precaution. out->path_len = 0; } // There shouldn't be any unconsumed data in the extension. - if (sequence_parser.HasMore()) + if (sequence_parser.HasMore()) { return false; + } // By definition the input was a single BasicConstraints sequence, so there // shouldn't be unconsumed data. - if (parser.HasMore()) + if (parser.HasMore()) { return false; + } return true; } @@ -742,19 +782,22 @@ bool ParseKeyUsage(const der::Input &key_usage_tlv, der::BitString *key_usage) { der::Parser parser(key_usage_tlv); std::optional<der::BitString> key_usage_internal = parser.ReadBitString(); - if (!key_usage_internal) + if (!key_usage_internal) { return false; + } // By definition the input was a single BIT STRING. - if (parser.HasMore()) + if (parser.HasMore()) { return false; + } // RFC 5280 section 4.2.1.3: // // When the keyUsage extension appears in a certificate, at least // one of the bits MUST be set to 1. - if (BitStringIsAllZeros(key_usage_internal.value())) + if (BitStringIsAllZeros(key_usage_internal.value())) { return false; + } *key_usage = key_usage_internal.value(); return true; @@ -770,18 +813,21 @@ // AuthorityInfoAccessSyntax ::= // SEQUENCE SIZE (1..MAX) OF AccessDescription der::Parser sequence_parser; - if (!parser.ReadSequence(&sequence_parser)) + if (!parser.ReadSequence(&sequence_parser)) { return false; - if (!sequence_parser.HasMore()) + } + if (!sequence_parser.HasMore()) { return false; + } while (sequence_parser.HasMore()) { AuthorityInfoAccessDescription access_description; // AccessDescription ::= SEQUENCE { der::Parser access_description_sequence_parser; - if (!sequence_parser.ReadSequence(&access_description_sequence_parser)) + if (!sequence_parser.ReadSequence(&access_description_sequence_parser)) { return false; + } // accessMethod OBJECT IDENTIFIER, if (!access_description_sequence_parser.ReadTag( @@ -795,8 +841,9 @@ return false; } - if (access_description_sequence_parser.HasMore()) + if (access_description_sequence_parser.HasMore()) { return false; + } out_access_descriptions->push_back(access_description); } @@ -827,13 +874,16 @@ if (access_location_tag == der::ContextSpecificPrimitive(6)) { // uniformResourceIdentifier [6] IA5String, std::string_view uri = access_location_value.AsStringView(); - if (!bssl::string_util::IsAscii(uri)) + if (!bssl::string_util::IsAscii(uri)) { return false; + } - if (access_description.access_method_oid == der::Input(kAdCaIssuersOid)) + if (access_description.access_method_oid == der::Input(kAdCaIssuersOid)) { out_ca_issuers_uris->push_back(uri); - else if (access_description.access_method_oid == der::Input(kAdOcspOid)) + } else if (access_description.access_method_oid == + der::Input(kAdOcspOid)) { out_ocsp_uris->push_back(uri); + } } } return true; @@ -854,19 +904,23 @@ // CRLDistributionPoints ::= SEQUENCE SIZE (1..MAX) OF DistributionPoint der::Parser extension_value_parser(extension_value); der::Parser distribution_points_parser; - if (!extension_value_parser.ReadSequence(&distribution_points_parser)) + if (!extension_value_parser.ReadSequence(&distribution_points_parser)) { return false; - if (extension_value_parser.HasMore()) + } + if (extension_value_parser.HasMore()) { return false; + } // Sequence must have a minimum of 1 item. - if (!distribution_points_parser.HasMore()) + if (!distribution_points_parser.HasMore()) { return false; + } while (distribution_points_parser.HasMore()) { if (!ParseAndAddDistributionPoint(&distribution_points_parser, - distribution_points)) + distribution_points)) { return false; + } } return true; @@ -892,10 +946,12 @@ der::Parser extension_value_parser(extension_value); der::Parser aki_parser; - if (!extension_value_parser.ReadSequence(&aki_parser)) + if (!extension_value_parser.ReadSequence(&aki_parser)) { return false; - if (extension_value_parser.HasMore()) + } + if (extension_value_parser.HasMore()) { return false; + } // TODO(mattm): Should having an empty AuthorityKeyIdentifier SEQUENCE be an // error? RFC 5280 doesn't explicitly say it. @@ -929,8 +985,9 @@ // There shouldn't be any unconsumed data in the AuthorityKeyIdentifier // SEQUENCE. - if (aki_parser.HasMore()) + if (aki_parser.HasMore()) { return false; + } return true; } @@ -947,8 +1004,9 @@ } // There shouldn't be any unconsumed data in the extension SEQUENCE. - if (extension_value_parser.HasMore()) + if (extension_value_parser.HasMore()) { return false; + } return true; }
diff --git a/pki/parse_certificate_unittest.cc b/pki/parse_certificate_unittest.cc index 6537e76..ec3c642 100644 --- a/pki/parse_certificate_unittest.cc +++ b/pki/parse_certificate_unittest.cc
@@ -172,8 +172,9 @@ EXPECT_EQ(expected_result, actual_result); VerifyCertErrors(expected_errors, errors, test_file_path); - if (!expected_result || !actual_result) + if (!expected_result || !actual_result) { return; + } // Ensure that the ParsedTbsCertificate matches expectations. EXPECT_EQ(expected_version, parsed.version); @@ -673,12 +674,14 @@ cert_bytes.size(), nullptr)), {}, &errors); - if (!cert) + if (!cert) { return false; + } auto it = cert->extensions().find(der::Input(kCrlDistributionPointsOid)); - if (it == cert->extensions().end()) + if (it == cert->extensions().end()) { return false; + } der::Input crl_dp_tlv = it->second.value;
diff --git a/pki/parse_name.cc b/pki/parse_name.cc index c961867..443bc09 100644 --- a/pki/parse_name.cc +++ b/pki/parse_name.cc
@@ -21,8 +21,9 @@ CBS cbs; CBS_init(&cbs, oid.UnsafeData(), oid.Length()); bssl::UniquePtr<char> text(CBS_asn1_oid_to_text(&cbs)); - if (!text) + if (!text) { return std::string(); + } return text.get(); } @@ -100,16 +101,18 @@ type_string = "emailAddress"; } else { type_string = OidToString(type); - if (type_string.empty()) + if (type_string.empty()) { return false; + } value_string = "#" + bssl::string_util::HexEncode(value.UnsafeData(), value.Length()); } if (value_string.empty()) { std::string unescaped; - if (!ValueAsStringUnsafe(&unescaped)) + if (!ValueAsStringUnsafe(&unescaped)) { return false; + } bool nonprintable = false; for (unsigned int i = 0; i < unescaped.length(); ++i) { @@ -138,9 +141,10 @@ // If we have non-printable characters in a TeletexString, we hex encode // since we don't handle Teletex control codes. - if (nonprintable && value_tag == der::kTeletexString) + if (nonprintable && value_tag == der::kTeletexString) { value_string = "#" + bssl::string_util::HexEncode(value.UnsafeData(), value.Length()); + } } *out = type_string + "=" + value_string; @@ -150,23 +154,27 @@ bool ReadRdn(der::Parser *parser, RelativeDistinguishedName *out) { while (parser->HasMore()) { der::Parser attr_type_and_value; - if (!parser->ReadSequence(&attr_type_and_value)) + if (!parser->ReadSequence(&attr_type_and_value)) { return false; + } // Read the attribute type, which must be an OBJECT IDENTIFIER. der::Input type; - if (!attr_type_and_value.ReadTag(der::kOid, &type)) + if (!attr_type_and_value.ReadTag(der::kOid, &type)) { return false; + } // Read the attribute value. der::Tag tag; der::Input value; - if (!attr_type_and_value.ReadTagAndValue(&tag, &value)) + if (!attr_type_and_value.ReadTagAndValue(&tag, &value)) { return false; + } // There should be no more elements in the sequence after reading the // attribute type and value. - if (attr_type_and_value.HasMore()) + if (attr_type_and_value.HasMore()) { return false; + } out->push_back(X509NameAttribute(type, tag, value)); } @@ -179,8 +187,9 @@ bool ParseName(const der::Input &name_tlv, RDNSequence *out) { der::Parser name_parser(name_tlv); der::Input name_value; - if (!name_parser.ReadTag(der::kSequence, &name_value)) + if (!name_parser.ReadTag(der::kSequence, &name_value)) { return false; + } return ParseNameValue(name_value, out); } @@ -188,11 +197,13 @@ der::Parser rdn_sequence_parser(name_value); while (rdn_sequence_parser.HasMore()) { der::Parser rdn_parser; - if (!rdn_sequence_parser.ReadConstructed(der::kSet, &rdn_parser)) + if (!rdn_sequence_parser.ReadConstructed(der::kSet, &rdn_parser)) { return false; + } RelativeDistinguishedName type_and_values; - if (!ReadRdn(&rdn_parser, &type_and_values)) + if (!ReadRdn(&rdn_parser, &type_and_values)) { return false; + } out->push_back(type_and_values); } @@ -206,15 +217,18 @@ RelativeDistinguishedName rdn = rdn_sequence[size - i - 1]; std::string rdn_string; for (const auto &atv : rdn) { - if (!rdn_string.empty()) + if (!rdn_string.empty()) { rdn_string += "+"; + } std::string atv_string; - if (!atv.AsRFC2253String(&atv_string)) + if (!atv.AsRFC2253String(&atv_string)) { return false; + } rdn_string += atv_string; } - if (!rdns_string.empty()) + if (!rdns_string.empty()) { rdns_string += ","; + } rdns_string += rdn_string; }
diff --git a/pki/parse_values.cc b/pki/parse_values.cc index 13e2c20..cc7a072 100644 --- a/pki/parse_values.cc +++ b/pki/parse_values.cc
@@ -20,12 +20,14 @@ // According to ITU-T X.690 section 8.2, a bool is encoded as a single octet // where the octet of all zeroes is FALSE and a non-zero value for the octet // is TRUE. - if (in.Length() != 1) + if (in.Length() != 1) { return false; + } ByteReader data(in); uint8_t byte; - if (!data.ReadByte(&byte)) + if (!data.ReadByte(&byte)) { return false; + } if (byte == 0) { *out = false; return true; @@ -67,10 +69,12 @@ // 0 and 60 (to allow for leap seconds; no validation is done that a leap // second is on a day that could be a leap second). bool ValidateGeneralizedTime(const GeneralizedTime &time) { - if (time.month < 1 || time.month > 12) + if (time.month < 1 || time.month > 12) { return false; - if (time.day < 1) + } + if (time.day < 1) { return false; + } if (time.hours > 23) { return false; } @@ -88,8 +92,9 @@ case 6: case 9: case 11: - if (time.day > 30) + if (time.day > 30) { return false; + } break; case 1: case 3: @@ -98,17 +103,20 @@ case 8: case 10: case 12: - if (time.day > 31) + if (time.day > 31) { return false; + } break; case 2: if (time.year % 4 == 0 && (time.year % 100 != 0 || time.year % 400 == 0)) { - if (time.day > 29) + if (time.day > 29) { return false; + } } else { - if (time.day > 28) + if (time.day > 28) { return false; + } } break; default: @@ -129,11 +137,13 @@ size_t GetUnsignedIntegerLength(const Input &in) { der::ByteReader reader(in); uint8_t first_byte; - if (!reader.ReadByte(&first_byte)) + if (!reader.ReadByte(&first_byte)) { return 0; // Not valid DER as |in| was empty. + } - if (first_byte == 0 && in.Length() > 1) + if (first_byte == 0 && in.Length() > 1) { return in.Length() - 1; + } return in.Length(); } @@ -170,12 +180,14 @@ bool ParseUint64(const Input &in, uint64_t *out) { // Reject non-minimally encoded numbers and negative numbers. bool negative; - if (!IsValidInteger(in, &negative) || negative) + if (!IsValidInteger(in, &negative) || negative) { return false; + } // Reject (non-negative) integers whose value would overflow the output type. - if (GetUnsignedIntegerLength(in) > sizeof(*out)) + if (GetUnsignedIntegerLength(in) > sizeof(*out)) { return false; + } ByteReader reader(in); uint8_t data; @@ -192,11 +204,13 @@ bool ParseUint8(const Input &in, uint8_t *out) { // TODO(eroman): Implement this more directly. uint64_t value; - if (!ParseUint64(in, &value)) + if (!ParseUint64(in, &value)) { return false; + } - if (value > 0xFF) + if (value > 0xFF) { return false; + } *out = static_cast<uint8_t>(value); return true; @@ -217,8 +231,9 @@ // If the bit is outside of the bitstring, by definition it is not // asserted. - if (byte_index >= bytes_.Length()) + if (byte_index >= bytes_.Length()) { return false; + } // Within a byte, bits are ordered from most significant to least significant. // Convert |bit_index| to an index within the |byte_index| byte, measured from @@ -241,14 +256,17 @@ // bit 1 as the least significant bit, the number of unused bits in the final // subsequent octet. The number shall be in the range zero to seven. uint8_t unused_bits; - if (!reader.ReadByte(&unused_bits)) + if (!reader.ReadByte(&unused_bits)) { return std::nullopt; - if (unused_bits > 7) + } + if (unused_bits > 7) { return std::nullopt; + } Input bytes; - if (!reader.ReadBytes(reader.BytesLeft(), &bytes)) + if (!reader.ReadBytes(reader.BytesLeft(), &bytes)) { return std::nullopt; // Not reachable. + } // Ensure that unused bits in the last byte are set to 0. if (unused_bits > 0) { @@ -256,8 +274,9 @@ // // If the bitstring is empty, there shall be no subsequent octets, // and the initial octet shall be zero. - if (bytes.Length() == 0) + if (bytes.Length() == 0) { return std::nullopt; + } uint8_t last_byte = bytes[bytes.Length() - 1]; // From ITU-T X.690, section 11.2.1 (applies to CER and DER, but not BER): @@ -265,8 +284,9 @@ // Each unused bit in the final octet of the encoding of a bit string value // shall be set to zero. uint8_t mask = 0xFF >> (8 - unused_bits); - if ((mask & last_byte) != 0) + if ((mask & last_byte) != 0) { return std::nullopt; + } } return BitString(bytes, unused_bits); @@ -306,15 +326,17 @@ return false; } uint8_t zulu; - if (!reader.ReadByte(&zulu) || zulu != 'Z' || reader.HasMore()) + if (!reader.ReadByte(&zulu) || zulu != 'Z' || reader.HasMore()) { return false; + } if (time.year < 50) { time.year += 2000; } else { time.year += 1900; } - if (!ValidateGeneralizedTime(time)) + if (!ValidateGeneralizedTime(time)) { return false; + } *value = time; return true; } @@ -331,18 +353,21 @@ return false; } uint8_t zulu; - if (!reader.ReadByte(&zulu) || zulu != 'Z' || reader.HasMore()) + if (!reader.ReadByte(&zulu) || zulu != 'Z' || reader.HasMore()) { return false; - if (!ValidateGeneralizedTime(time)) + } + if (!ValidateGeneralizedTime(time)) { return false; + } *value = time; return true; } bool ParseIA5String(Input in, std::string *out) { for (char c : in.AsStringView()) { - if (static_cast<uint8_t>(c) > 127) + if (static_cast<uint8_t>(c) > 127) { return false; + } } *out = in.AsString(); return true; @@ -356,8 +381,9 @@ // "for VisibleString [the range] is 32 to 126 ... For VisibleString .. all // the values in the range are present." for (char c : in.AsStringView()) { - if (static_cast<uint8_t>(c) < 32 || static_cast<uint8_t>(c) > 126) + if (static_cast<uint8_t>(c) < 32 || static_cast<uint8_t>(c) > 126) { return false; + } } *out = in.AsString(); return true;
diff --git a/pki/parsed_certificate.cc b/pki/parsed_certificate.cc index 5f2406d..0e8146c 100644 --- a/pki/parsed_certificate.cc +++ b/pki/parsed_certificate.cc
@@ -58,8 +58,9 @@ bool ParsedCertificate::GetExtension(const der::Input &extension_oid, ParsedExtension *parsed_extension) const { - if (!tbs_.extensions_tlv) + if (!tbs_.extensions_tlv) { return false; + } auto it = extensions_.find(extension_oid); if (it == extensions_.end()) { @@ -81,8 +82,9 @@ // |errors| is an optional parameter, but to keep the code simpler, use a // dummy object when one wasn't provided. CertErrors unused_errors; - if (!errors) + if (!errors) { errors = &unused_errors; + } auto result = std::make_shared<ParsedCertificate>(PrivateConstructor{}); result->cert_data_ = std::move(backing_data); @@ -284,8 +286,9 @@ CertErrors *errors) { std::shared_ptr<const ParsedCertificate> cert( Create(std::move(cert_data), options, errors)); - if (!cert) + if (!cert) { return false; + } chain->push_back(std::move(cert)); return true; }
diff --git a/pki/parsed_certificate_unittest.cc b/pki/parsed_certificate_unittest.cc index 95f4433..33cd078 100644 --- a/pki/parsed_certificate_unittest.cc +++ b/pki/parsed_certificate_unittest.cc
@@ -48,8 +48,9 @@ // The errors are baselined for |!allow_invalid_serial_numbers|. So if // requesting a non-default option skip the error checks. // TODO(eroman): This is ugly. - if (!options.allow_invalid_serial_numbers) + if (!options.allow_invalid_serial_numbers) { VerifyCertErrors(expected_errors, errors, test_file_path); + } // Every parse failure being tested should emit error information. if (!cert) {
diff --git a/pki/parser.cc b/pki/parser.cc index ae6a201..327c239 100644 --- a/pki/parser.cc +++ b/pki/parser.cc
@@ -31,8 +31,9 @@ } bool Parser::Advance() { - if (advance_len_ == 0) + if (advance_len_ == 0) { return false; + } bool ret = !!CBS_skip(&cbs_, advance_len_); advance_len_ = 0; return ret; @@ -42,15 +43,17 @@ bool Parser::ReadRawTLV(Input *out) { CBS tmp_out; - if (!CBS_get_any_asn1_element(&cbs_, &tmp_out, nullptr, nullptr)) + if (!CBS_get_any_asn1_element(&cbs_, &tmp_out, nullptr, nullptr)) { return false; + } *out = Input(CBS_data(&tmp_out), CBS_len(&tmp_out)); return true; } bool Parser::ReadTagAndValue(Tag *tag, Input *out) { - if (!PeekTagAndValue(tag, out)) + if (!PeekTagAndValue(tag, out)) { return false; + } BSSL_CHECK(Advance()); return true; } @@ -77,8 +80,9 @@ bool Parser::ReadOptionalTag(Tag tag, Input *out, bool *present) { std::optional<Input> tmp_out; - if (!ReadOptionalTag(tag, &tmp_out)) + if (!ReadOptionalTag(tag, &tmp_out)) { return false; + } *present = tmp_out.has_value(); *out = tmp_out.value_or(der::Input()); return true; @@ -108,11 +112,13 @@ // Type-specific variants of ReadTag bool Parser::ReadConstructed(Tag tag, Parser *out) { - if (!IsConstructed(tag)) + if (!IsConstructed(tag)) { return false; + } Input data; - if (!ReadTag(tag, &data)) + if (!ReadTag(tag, &data)) { return false; + } *out = Parser(data); return true; } @@ -123,29 +129,33 @@ bool Parser::ReadUint8(uint8_t *out) { Input encoded_int; - if (!ReadTag(kInteger, &encoded_int)) + if (!ReadTag(kInteger, &encoded_int)) { return false; + } return ParseUint8(encoded_int, out); } bool Parser::ReadUint64(uint64_t *out) { Input encoded_int; - if (!ReadTag(kInteger, &encoded_int)) + if (!ReadTag(kInteger, &encoded_int)) { return false; + } return ParseUint64(encoded_int, out); } std::optional<BitString> Parser::ReadBitString() { Input value; - if (!ReadTag(kBitString, &value)) + if (!ReadTag(kBitString, &value)) { return std::nullopt; + } return ParseBitString(value); } bool Parser::ReadGeneralizedTime(GeneralizedTime *out) { Input value; - if (!ReadTag(kGeneralizedTime, &value)) + if (!ReadTag(kGeneralizedTime, &value)) { return false; + } return ParseGeneralizedTime(value, out); }
diff --git a/pki/path_builder.cc b/pki/path_builder.cc index f0e74e8..ad69ecd 100644 --- a/pki/path_builder.cc +++ b/pki/path_builder.cc
@@ -43,8 +43,9 @@ RDNSequence subject; std::string subject_str; if (!ParseName(cert->tbs().subject_tlv, &subject) || - !ConvertToRFC2253(subject, &subject_str)) + !ConvertToRFC2253(subject, &subject_str)) { subject_str = "???"; + } return FingerPrintParsedCertificate(cert) + " " + subject_str; } @@ -52,8 +53,9 @@ std::string PathDebugString(const ParsedCertificateList &certs) { std::string s; for (const auto &cert : certs) { - if (!s.empty()) + if (!s.empty()) { s += "\n"; + } s += " " + CertDebugString(cert.get()); } return s; @@ -84,8 +86,9 @@ // values indicate higer priority. KeyIdentifierMatch CalculateKeyIdentifierMatch( const ParsedCertificate *target, const ParsedCertificate *issuer) { - if (!target->authority_key_identifier()) + if (!target->authority_key_identifier()) { return kNoData; + } // TODO(crbug.com/635205): If issuer does not have a subjectKeyIdentifier, // could try synthesizing one using the standard SHA-1 method. Ideally in a @@ -301,8 +304,9 @@ void CertIssuersIter::AddIssuers(ParsedCertificateList new_issuers) { for (std::shared_ptr<const ParsedCertificate> &issuer : new_issuers) { if (present_issuers_.find(issuer->der_cert().AsStringView()) != - present_issuers_.end()) + present_issuers_.end()) { continue; + } present_issuers_.insert(issuer->der_cert().AsStringView()); // Look up the trust for this issuer. @@ -332,8 +336,9 @@ } void CertIssuersIter::SortRemainingIssuers() { - if (!issuers_needs_sort_) + if (!issuers_needs_sort_) { return; + } std::stable_sort( issuers_.begin() + cur_issuer_, issuers_.end(), @@ -392,8 +397,9 @@ // Copies the ParsedCertificate elements of the current path to |*out_path|. void CopyPath(ParsedCertificateList *out_path) { out_path->clear(); - for (const auto &node : cur_path_) + for (const auto &node : cur_path_) { out_path->push_back(node->reference_cert()); + } } // Returns true if the path is empty. @@ -408,8 +414,9 @@ std::string PathDebugString() { std::string s; for (const auto &node : cur_path_) { - if (!s.empty()) + if (!s.empty()) { s += "\n"; + } s += " " + CertDebugString(node->cert()); } return s; @@ -440,8 +447,9 @@ } // namespace const ParsedCertificate *CertPathBuilderResultPath::GetTrustedCert() const { - if (certs.empty()) + if (certs.empty()) { return nullptr; + } switch (last_cert_trust.type) { case CertificateTrustType::TRUSTED_ANCHOR: @@ -531,8 +539,9 @@ // If the deadline is already expired before the first call to // GetNextPath, cur_path_ will be empty. Return the leaf cert in that // case. - if (next_issuer_.cert) + if (next_issuer_.cert) { out_certs->push_back(next_issuer_.cert); + } } else { cur_path_.CopyPath(out_certs); } @@ -698,8 +707,9 @@ bool CertPathBuilder::Result::AnyPathContainsError(CertErrorId error_id) const { for (const auto &path : paths) { - if (path->errors.ContainsError(error_id)) + if (path->errors.ContainsError(error_id)) { return true; + } } return false; @@ -709,8 +719,9 @@ const { const CertPathBuilderResultPath *result_path = GetBestPathPossiblyInvalid(); - if (result_path && result_path->IsValid()) + if (result_path && result_path->IsValid()) { return result_path; + } return nullptr; } @@ -720,8 +731,9 @@ BSSL_CHECK((paths.empty() && best_result_index == 0) || best_result_index < paths.size()); - if (best_result_index >= paths.size()) + if (best_result_index >= paths.size()) { return nullptr; + } return paths[best_result_index].get(); }
diff --git a/pki/path_builder_pkits_unittest.cc b/pki/path_builder_pkits_unittest.cc index 6f3573f..e912e46 100644 --- a/pki/path_builder_pkits_unittest.cc +++ b/pki/path_builder_pkits_unittest.cc
@@ -43,8 +43,9 @@ CertPathBuilderResultPath *path) override { SimplePathBuilderDelegate::CheckPathAfterVerification(path_builder, path); - if (!path->IsValid()) + if (!path->IsValid()) { return; + } // It would be preferable if this test could use // CheckValidatedChainRevocation somehow, but that only supports getting @@ -56,8 +57,9 @@ // Trust anchors bypass OCSP/CRL revocation checks. (The only way to // revoke trust anchors is via CRLSet or the built-in SPKI block list). - if (reverse_i == 0 && path->last_cert_trust.IsTrustAnchor()) + if (reverse_i == 0 && path->last_cert_trust.IsTrustAnchor()) { continue; + } // RFC 5280 6.3.3. [If the CRL was not specified in a distribution // point], assume a DP with both the reasons and the @@ -91,8 +93,9 @@ } // If there were only DistributionPoints with reasons, just use the // first one. - if (cert_dp == &fake_cert_dp && !distribution_points.empty()) + if (cert_dp == &fake_cert_dp && !distribution_points.empty()) { cert_dp = &distribution_points[0]; + } } bool cert_good = false; @@ -154,8 +157,9 @@ // TODO(mattm): test with other irrelevant certs in cert_issuer_sources? CertIssuerSourceStatic cert_issuer_source; - for (size_t i = 1; i < cert_ders.size() - 1; ++i) + for (size_t i = 1; i < cert_ders.size() - 1; ++i) { cert_issuer_source.AddCert(certs[i]); + } std::shared_ptr<const ParsedCertificate> target_cert(certs.back());
diff --git a/pki/path_builder_unittest.cc b/pki/path_builder_unittest.cc index dfe5a50..ebb940a 100644 --- a/pki/path_builder_unittest.cc +++ b/pki/path_builder_unittest.cc
@@ -86,8 +86,9 @@ ~StaticAsyncRequest() override = default; void GetNext(ParsedCertificateList *out_certs) override { - if (issuers_iter_ != issuers_.end()) + if (issuers_iter_ != issuers_.end()) { out_certs->push_back(std::move(*issuers_iter_++)); + } } ParsedCertificateList issuers_; @@ -142,8 +143,9 @@ std::string der; ::testing::AssertionResult r = ReadTestPem( "testdata/ssl/certificates/" + file_name, "CERTIFICATE", &der); - if (!r) + if (!r) { return r; + } CertErrors errors; *result = ParsedCertificate::Create( bssl::UniquePtr<CRYPTO_BUFFER>(CRYPTO_BUFFER_new( @@ -594,11 +596,13 @@ b_by_c_, b_by_f_, f_by_e_, c_by_d_, c_by_e_}; CertIssuerSourceStatic sync_certs; if (reverse_order) { - for (auto it = certs.rbegin(); it != certs.rend(); ++it) + for (auto it = certs.rbegin(); it != certs.rend(); ++it) { sync_certs.AddCert(*it); + } } else { - for (const auto &cert : certs) + for (const auto &cert : certs) { sync_certs.AddCert(cert); + } } CertPathBuilder path_builder( @@ -1756,10 +1760,12 @@ // Set up the trust store such that |distrusted_cert| is blocked, and // the root is trusted (except if it was |distrusted_cert|). TrustStoreInMemory trust_store; - if (distrusted_cert != test_.chain.back()) + if (distrusted_cert != test_.chain.back()) { trust_store.AddTrustAnchor(test_.chain.back()); - if (distrusted_cert) + } + if (distrusted_cert) { trust_store.AddDistrustedCertificateForTest(distrusted_cert); + } // Add the single intermediate. CertIssuerSourceStatic intermediates; @@ -1817,8 +1823,9 @@ // The built path should be identical the the one read from disk. const auto &path = *result.GetBestValidPath(); ASSERT_EQ(test_.chain.size(), path.certs.size()); - for (size_t i = 0; i < test_.chain.size(); ++i) + for (size_t i = 0; i < test_.chain.size(); ++i) { EXPECT_EQ(test_.chain[i], path.certs[i]); + } } // Try path building when only the target is blocked - should fail.
diff --git a/pki/path_builder_verify_certificate_chain_unittest.cc b/pki/path_builder_verify_certificate_chain_unittest.cc index bb0cab9..a5aeef7 100644 --- a/pki/path_builder_verify_certificate_chain_unittest.cc +++ b/pki/path_builder_verify_certificate_chain_unittest.cc
@@ -24,8 +24,9 @@ trust_store.AddCertificate(test.chain.back(), test.last_cert_trust); CertIssuerSourceStatic intermediate_cert_issuer_source; - for (size_t i = 1; i < test.chain.size(); ++i) + for (size_t i = 1; i < test.chain.size(); ++i) { intermediate_cert_issuer_source.AddCert(test.chain[i]); + } // First cert in the |chain| is the target. CertPathBuilder path_builder(
diff --git a/pki/pem.cc b/pki/pem.cc index 57068d4..0bff537 100644 --- a/pki/pem.cc +++ b/pki/pem.cc
@@ -39,14 +39,16 @@ while (pos_ != std::string_view::npos) { // Scan for the beginning of the next PEM encoded block. pos_ = str_.find(kPEMHeaderBeginBlock, pos_); - if (pos_ == std::string_view::npos) + if (pos_ == std::string_view::npos) { return false; // No more PEM blocks + } std::vector<PEMType>::const_iterator it; // Check to see if it is of an acceptable block type. for (it = block_types_.begin(); it != block_types_.end(); ++it) { - if (!bssl::string_util::StartsWith(str_.substr(pos_), it->header)) + if (!bssl::string_util::StartsWith(str_.substr(pos_), it->header)) { continue; + } // Look for a footer matching the header. If none is found, then all // data following this point is invalid and should not be parsed. @@ -77,8 +79,9 @@ // continue the search. Otherwise, |pos_| has been updated to the most // appropriate search position to continue searching from and should not // be adjusted. - if (it == block_types_.end()) + if (it == block_types_.end()) { pos_ += kPEMHeaderBeginBlock.size(); + } } return false;
diff --git a/pki/signature_algorithm.cc b/pki/signature_algorithm.cc index 83f65d1..341cae5 100644 --- a/pki/signature_algorithm.cc +++ b/pki/signature_algorithm.cc
@@ -131,12 +131,14 @@ [[nodiscard]] bool IsNull(const der::Input &input) { der::Parser parser(input); der::Input null_value; - if (!parser.ReadTag(der::kNull, &null_value)) + if (!parser.ReadTag(der::kNull, &null_value)) { return false; + } // NULL values are TLV encoded; the value is expected to be empty. - if (!IsEmpty(null_value)) + if (!IsEmpty(null_value)) { return false; + } // By definition of this function, the entire input must be a NULL. return !parser.HasMore(); @@ -175,12 +177,14 @@ DigestAlgorithm *mgf1_hash) { der::Input oid; der::Input params; - if (!ParseAlgorithmIdentifier(input, &oid, ¶ms)) + if (!ParseAlgorithmIdentifier(input, &oid, ¶ms)) { return false; + } // MGF1 is the only supported mask generation algorithm. - if (oid != der::Input(kOidMgf1)) + if (oid != der::Input(kOidMgf1)) { return false; + } return ParseHashAlgorithm(params, mgf1_hash); } @@ -275,17 +279,20 @@ der::Parser parser(input); der::Parser algorithm_identifier_parser; - if (!parser.ReadSequence(&algorithm_identifier_parser)) + if (!parser.ReadSequence(&algorithm_identifier_parser)) { return false; + } // There shouldn't be anything after the sequence. This is by definition, // as the input to this function is expected to be a single // AlgorithmIdentifier. - if (parser.HasMore()) + if (parser.HasMore()) { return false; + } - if (!algorithm_identifier_parser.ReadTag(der::kOid, algorithm)) + if (!algorithm_identifier_parser.ReadTag(der::kOid, algorithm)) { return false; + } // Read the optional parameters to a der::Input. The parameters can be at // most one TLV (for instance NULL or a sequence).
diff --git a/pki/simple_path_builder_delegate.cc b/pki/simple_path_builder_delegate.cc index 4dfaea8..248d640 100644 --- a/pki/simple_path_builder_delegate.cc +++ b/pki/simple_path_builder_delegate.cc
@@ -84,8 +84,9 @@ if (pkey_id == EVP_PKEY_RSA) { // Extract the modulus length from the key. RSA *rsa = EVP_PKEY_get0_RSA(public_key); - if (!rsa) + if (!rsa) { return false; + } unsigned int modulus_length_bits = RSA_bits(rsa); if (modulus_length_bits < min_rsa_modulus_length_bits_) { @@ -102,8 +103,9 @@ if (pkey_id == EVP_PKEY_EC) { // Extract the curve name. EC_KEY *ec = EVP_PKEY_get0_EC_KEY(public_key); - if (!ec) + if (!ec) { return false; // Unexpected. + } int curve_nid = EC_GROUP_get_curve_name(EC_KEY_get0_group(ec)); if (!IsAcceptableCurveForEcdsa(curve_nid)) {
diff --git a/pki/test_helpers.cc b/pki/test_helpers.cc index f149d32..b7369d9 100644 --- a/pki/test_helpers.cc +++ b/pki/test_helpers.cc
@@ -28,8 +28,9 @@ bool GetValue(std::string_view prefix, std::string_view line, std::string *value, bool *has_value) { - if (!bssl::string_util::StartsWith(line, prefix)) + if (!bssl::string_util::StartsWith(line, prefix)) { return false; + } if (*has_value) { ADD_FAILURE() << "Duplicated " << prefix; @@ -195,8 +196,9 @@ *chain = ParsedCertificateList(); std::string file_data = ReadTestFileToString(file_path_ascii); - if (file_data.empty()) + if (file_data.empty()) { return false; + } std::vector<std::string> pem_headers = {"CERTIFICATE"}; @@ -221,10 +223,12 @@ std::shared_ptr<const ParsedCertificate> ReadCertFromFile( const std::string &file_path_ascii) { ParsedCertificateList chain; - if (!ReadCertChainFromFile(file_path_ascii, &chain)) + if (!ReadCertChainFromFile(file_path_ascii, &chain)) { return nullptr; - if (chain.size() != 1) + } + if (chain.size() != 1) { return nullptr; + } return chain[0]; } @@ -234,8 +238,9 @@ *test = {}; std::string file_data = ReadTestFileToString(file_path_ascii); - if (file_data.empty()) + if (file_data.empty()) { return false; + } bool has_chain = false; bool has_trust = false;
diff --git a/pki/trust_store_collection.cc b/pki/trust_store_collection.cc index 804b1c8..4c607b5 100644 --- a/pki/trust_store_collection.cc +++ b/pki/trust_store_collection.cc
@@ -35,8 +35,9 @@ // last one if (!cur_trust.HasUnspecifiedTrust()) { result = cur_trust; - if (result.IsDistrusted()) + if (result.IsDistrusted()) { break; + } } }
diff --git a/pki/trust_store_in_memory.cc b/pki/trust_store_in_memory.cc index 40aaa1f..47fe6d4 100644 --- a/pki/trust_store_in_memory.cc +++ b/pki/trust_store_in_memory.cc
@@ -44,8 +44,9 @@ void TrustStoreInMemory::SyncGetIssuersOf(const ParsedCertificate *cert, ParsedCertificateList *issuers) { auto range = entries_.equal_range(cert->normalized_issuer().AsStringView()); - for (auto it = range.first; it != range.second; ++it) + for (auto it = range.first; it != range.second; ++it) { issuers->push_back(it->second.cert); + } } CertificateTrust TrustStoreInMemory::GetTrust(const ParsedCertificate *cert) {
diff --git a/pki/verify_certificate_chain.cc b/pki/verify_certificate_chain.cc index e1d8d3f..f504ef0 100644 --- a/pki/verify_certificate_chain.cc +++ b/pki/verify_certificate_chain.cc
@@ -25,19 +25,24 @@ bool IsHandledCriticalExtension(const ParsedExtension &extension, const ParsedCertificate &cert) { - if (extension.oid == der::Input(kBasicConstraintsOid)) + if (extension.oid == der::Input(kBasicConstraintsOid)) { return true; + } // Key Usage is NOT processed for end-entity certificates (this is the // responsibility of callers), however it is considered "handled" here in // order to allow being marked as critical. - if (extension.oid == der::Input(kKeyUsageOid)) + if (extension.oid == der::Input(kKeyUsageOid)) { return true; - if (extension.oid == der::Input(kExtKeyUsageOid)) + } + if (extension.oid == der::Input(kExtKeyUsageOid)) { return true; - if (extension.oid == der::Input(kNameConstraintsOid)) + } + if (extension.oid == der::Input(kNameConstraintsOid)) { return true; - if (extension.oid == der::Input(kSubjectAltNameOid)) + } + if (extension.oid == der::Input(kSubjectAltNameOid)) { return true; + } if (extension.oid == der::Input(kCertificatePoliciesOid)) { // Policy qualifiers are skipped during processing, so if the // extension is marked critical need to ensure there weren't any @@ -56,12 +61,15 @@ // TODO(eroman): Give a better error message. } - if (extension.oid == der::Input(kPolicyMappingsOid)) + if (extension.oid == der::Input(kPolicyMappingsOid)) { return true; - if (extension.oid == der::Input(kPolicyConstraintsOid)) + } + if (extension.oid == der::Input(kPolicyConstraintsOid)) { return true; - if (extension.oid == der::Input(kInhibitAnyPolicyOid)) + } + if (extension.oid == der::Input(kInhibitAnyPolicyOid)) { return true; + } if (extension.oid == der::Input(kMSApplicationPoliciesOid)) { // Per https://crbug.com/1439638 and // https://learn.microsoft.com/en-us/windows/win32/seccertenroll/supported-extensions#msapplicationpolicies @@ -111,11 +119,13 @@ // notBefore through notAfter, inclusive. void VerifyTimeValidity(const ParsedCertificate &cert, const der::GeneralizedTime &time, CertErrors *errors) { - if (time < cert.tbs().validity_not_before) + if (time < cert.tbs().validity_not_before) { errors->AddError(cert_errors::kValidityFailedNotBefore); + } - if (cert.tbs().validity_not_after < time) + if (cert.tbs().validity_not_after < time) { errors->AddError(cert_errors::kValidityFailedNotAfter); + } } // Adds errors to |errors| if |cert| has internally inconsistent signature @@ -145,8 +155,9 @@ // Ensure that the two DER-encoded signature algorithms are byte-for-byte // equal. - if (alg1_tlv == alg2_tlv) + if (alg1_tlv == alg2_tlv) { return true; + } // But make a compatibility concession if alternate encodings are used // TODO(eroman): Turn this warning into an error. @@ -874,19 +885,22 @@ // (e) If the certificate policies extension is not present, set the // valid_policy_tree to NULL. - if (!cert.has_policy_oids()) + if (!cert.has_policy_oids()) { valid_policy_graph_.SetNull(); + } // (f) Verify that either explicit_policy is greater than 0 or the // valid_policy_tree is not equal to NULL; - if (!((explicit_policy_ > 0) || !valid_policy_graph_.IsNull())) + if (!((explicit_policy_ > 0) || !valid_policy_graph_.IsNull())) { errors->AddError(cert_errors::kNoValidPolicy); + } } void PathVerifier::VerifyPolicyMappings(const ParsedCertificate &cert, CertErrors *errors) { - if (!cert.has_policy_mappings()) + if (!cert.has_policy_mappings()) { return; + } // From RFC 5280 section 6.1.4: // @@ -1030,8 +1044,9 @@ errors->AddError(cert_errors::kVerifySignedDataFailed); } } - if (*shortcircuit_chain_validation) + if (*shortcircuit_chain_validation) { return; + } // Check the time range for the certificate's validity, ensuring it is valid // at |time|. @@ -1044,8 +1059,9 @@ // 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(cert_errors::kSubjectDoesNotMatchIssuer); + } // 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 @@ -1089,24 +1105,28 @@ // of |working_spki|. // From RFC 5280 section 6.1.4 step g: - if (cert.has_name_constraints()) + if (cert.has_name_constraints()) { name_constraints_list_.push_back(&cert.name_constraints()); + } // (h) If certificate i is not self-issued: if (!IsSelfIssued(cert)) { // (1) If explicit_policy is not 0, decrement explicit_policy by // 1. - if (explicit_policy_ > 0) + if (explicit_policy_ > 0) { explicit_policy_ -= 1; + } // (2) If policy_mapping is not 0, decrement policy_mapping by 1. - if (policy_mapping_ > 0) + if (policy_mapping_ > 0) { policy_mapping_ -= 1; + } // (3) If inhibit_anyPolicy is not 0, decrement inhibit_anyPolicy // by 1. - if (inhibit_any_policy_ > 0) + if (inhibit_any_policy_ > 0) { inhibit_any_policy_ -= 1; + } } // RFC 5280 section 6.1.4 step i-j: @@ -1203,8 +1223,9 @@ CertErrors *errors) { // From RFC 5280 section 6.1.5: // (a) If explicit_policy is not 0, decrement explicit_policy by 1. - if (explicit_policy_ > 0) + if (explicit_policy_ > 0) { explicit_policy_ -= 1; + } // (b) If a policy constraints extension is included in the // certificate and requireExplicitPolicy is present and has a @@ -1299,8 +1320,9 @@ // The following enforcements follow from RFC 5937 (primarily section 3.2): // Initialize name constraints initial-permitted/excluded-subtrees. - if (cert.has_name_constraints()) + if (cert.has_name_constraints()) { name_constraints_list_.push_back(&cert.name_constraints()); + } if (cert.has_basic_constraints()) { // Enforce CA=true if basicConstraints is present. This matches behavior of @@ -1353,8 +1375,9 @@ case CertificateTrustType::TRUSTED_ANCHOR_OR_LEAF: break; } - if (*shortcircuit_chain_validation) + if (*shortcircuit_chain_validation) { return; + } if (trust.enforce_anchor_expiry) { VerifyTimeValidity(cert, time, errors); @@ -1441,8 +1464,9 @@ } // Check if the key is acceptable by the delegate. - if (!delegate_->IsPublicKeyAcceptable(pkey.get(), errors)) + if (!delegate_->IsPublicKeyAcceptable(pkey.get(), errors)) { errors->AddError(cert_errors::kUnacceptablePublicKey); + } return pkey; }
diff --git a/pki/verify_name_match.cc b/pki/verify_name_match.cc index 5952fff..10297b0 100644 --- a/pki/verify_name_match.cc +++ b/pki/verify_name_match.cc
@@ -72,8 +72,9 @@ // multiple whitespace chars to a single space, otherwise ignore trailing // whitespace. std::string::const_iterator next_iter = read_iter + 1; - if (next_iter != output->end() && *next_iter != ' ') + if (next_iter != output->end() && *next_iter != ' ') { *(write_iter++) = ' '; + } } else if (c >= 'A' && c <= 'Z') { // Fold case. *(write_iter++) = c + ('a' - 'A'); @@ -85,12 +86,14 @@ // See NormalizePrintableStringValue comment for the acceptable list // of characters. if (!((c >= 'a' && c <= 'z') || (c >= '\'' && c <= ':') || c == '=' || - c == '?')) + c == '?')) { return false; + } break; case ENFORCE_ASCII: - if (c > 0x7F) + if (c > 0x7F) { return false; + } break; case NO_ENFORCEMENT: break; @@ -98,8 +101,9 @@ *(write_iter++) = c; } } - if (write_iter != output->end()) + if (write_iter != output->end()) { output->erase(write_iter, output->end()); + } return true; } @@ -177,13 +181,15 @@ // TODO(eroman): Plumb this down. CertErrors unused_errors; if (!NormalizeValue(a, &a_normalized, &unused_errors) || - !NormalizeValue(b, &b_normalized, &unused_errors)) + !NormalizeValue(b, &b_normalized, &unused_errors)) { return false; + } return a_normalized == b_normalized; } // Attributes encoded with different types may be assumed to be unequal. - if (a.value_tag != b.value_tag) + if (a.value_tag != b.value_tag) { return false; + } // All other types use binary comparison. return a.value == b.value; } @@ -194,15 +200,17 @@ bool VerifyRdnMatch(der::Parser *a_parser, der::Parser *b_parser) { RelativeDistinguishedName a_type_and_values, b_type_and_values; if (!ReadRdn(a_parser, &a_type_and_values) || - !ReadRdn(b_parser, &b_type_and_values)) + !ReadRdn(b_parser, &b_type_and_values)) { return false; + } // RFC 5280 section 7.1: // Two relative distinguished names RDN1 and RDN2 match if they have the same // number of naming attributes and for each naming attribute in RDN1 there is // a matching naming attribute in RDN2. - if (a_type_and_values.size() != b_type_and_values.size()) + if (a_type_and_values.size() != b_type_and_values.size()) { return false; + } // The ordering of elements may differ due to denormalized values sorting // differently in the DER encoding. Since the number of elements should be @@ -216,8 +224,9 @@ break; } } - if (b_iter == b_type_and_values.end()) + if (b_iter == b_type_and_values.end()) { return false; + } // Remove the matched element from b_type_and_values to ensure duplicate // elements in a_type_and_values can't match the same element in // b_type_and_values multiple times. @@ -273,10 +282,12 @@ // If doing exact match and either of the sequences has more elements than the // other, not a match. If doing a subtree match, the first Name may have more // RDNs than the second. - if (b_rdn_sequence_counter.HasMore()) + if (b_rdn_sequence_counter.HasMore()) { return false; - if (match_type == EXACT_MATCH && a_rdn_sequence_counter.HasMore()) + } + if (match_type == EXACT_MATCH && a_rdn_sequence_counter.HasMore()) { return false; + } // Verify that RDNs in |a| and |b| match. der::Parser a_rdn_sequence(a); @@ -287,8 +298,9 @@ !b_rdn_sequence.ReadConstructed(der::kSet, &b_rdn)) { return false; } - if (!VerifyRdnMatch(&a_rdn, &b_rdn)) + if (!VerifyRdnMatch(&a_rdn, &b_rdn)) { return false; + } } return true; @@ -305,21 +317,25 @@ der::Parser rdn_sequence_parser(name_rdn_sequence); bssl::ScopedCBB cbb; - if (!CBB_init(cbb.get(), 0)) + if (!CBB_init(cbb.get(), 0)) { return false; + } while (rdn_sequence_parser.HasMore()) { // RelativeDistinguishedName ::= SET SIZE (1..MAX) OF AttributeTypeAndValue der::Parser rdn_parser; - if (!rdn_sequence_parser.ReadConstructed(der::kSet, &rdn_parser)) + if (!rdn_sequence_parser.ReadConstructed(der::kSet, &rdn_parser)) { return false; + } RelativeDistinguishedName type_and_values; - if (!ReadRdn(&rdn_parser, &type_and_values)) + if (!ReadRdn(&rdn_parser, &type_and_values)) { return false; + } CBB rdn_cbb; - if (!CBB_add_asn1(cbb.get(), &rdn_cbb, CBS_ASN1_SET)) + if (!CBB_add_asn1(cbb.get(), &rdn_cbb, CBS_ASN1_SET)) { return false; + } for (const auto &type_and_value : type_and_values) { // AttributeTypeAndValue ::= SEQUENCE { @@ -342,30 +358,35 @@ // AttributeValue ::= ANY -- DEFINED BY AttributeType if (IsNormalizableDirectoryString(type_and_value.value_tag)) { std::string normalized_value; - if (!NormalizeValue(type_and_value, &normalized_value, errors)) + if (!NormalizeValue(type_and_value, &normalized_value, errors)) { return false; + } if (!CBB_add_asn1(&attribute_type_and_value_cbb, &value_cbb, CBS_ASN1_UTF8STRING) || !CBB_add_bytes( &value_cbb, reinterpret_cast<const uint8_t *>(normalized_value.data()), - normalized_value.size())) + normalized_value.size())) { return false; + } } else { if (!CBB_add_asn1(&attribute_type_and_value_cbb, &value_cbb, type_and_value.value_tag) || !CBB_add_bytes(&value_cbb, type_and_value.value.UnsafeData(), - type_and_value.value.Length())) + type_and_value.value.Length())) { return false; + } } - if (!CBB_flush(&rdn_cbb)) + if (!CBB_flush(&rdn_cbb)) { return false; + } } // Ensure the encoded AttributeTypeAndValue values in the SET OF are sorted. - if (!CBB_flush_asn1_set_of(&rdn_cbb) || !CBB_flush(cbb.get())) + if (!CBB_flush_asn1_set_of(&rdn_cbb) || !CBB_flush(cbb.get())) { return false; + } } normalized_rdn_sequence->assign(CBB_data(cbb.get()), @@ -392,12 +413,14 @@ der::Parser rdn_sequence_parser(name_rdn_sequence); while (rdn_sequence_parser.HasMore()) { der::Parser rdn_parser; - if (!rdn_sequence_parser.ReadConstructed(der::kSet, &rdn_parser)) + if (!rdn_sequence_parser.ReadConstructed(der::kSet, &rdn_parser)) { return false; + } RelativeDistinguishedName type_and_values; - if (!ReadRdn(&rdn_parser, &type_and_values)) + if (!ReadRdn(&rdn_parser, &type_and_values)) { return false; + } for (const auto &type_and_value : type_and_values) { if (type_and_value.type == der::Input(kTypeEmailAddressOid)) {
diff --git a/pki/verify_name_match_unittest.cc b/pki/verify_name_match_unittest.cc index 29fc844..53cd012 100644 --- a/pki/verify_name_match_unittest.cc +++ b/pki/verify_name_match_unittest.cc
@@ -32,8 +32,9 @@ } bool TypesAreComparable(const std::string &type_1, const std::string &type_2) { - if (type_1 == type_2) + if (type_1 == type_2) { return true; + } if ((type_1 == "PRINTABLESTRING" || type_1 == "UTF8" || type_1 == "BMPSTRING" || type_1 == "UNIVERSALSTRING") && (type_2 == "PRINTABLESTRING" || type_2 == "UTF8" ||
diff --git a/pki/verify_signed_data.cc b/pki/verify_signed_data.cc index f477178..9bf8414 100644 --- a/pki/verify_signed_data.cc +++ b/pki/verify_signed_data.cc
@@ -214,13 +214,15 @@ break; } - if (expected_pkey_id != EVP_PKEY_id(public_key)) + if (expected_pkey_id != EVP_PKEY_id(public_key)) { return false; + } // For the supported algorithms the signature value must be a whole // number of bytes. - if (signature_value.unused_bits() != 0) + if (signature_value.unused_bits() != 0) { return false; + } const der::Input &signature_value_bytes = signature_value.bytes(); std::string cache_key; @@ -244,8 +246,9 @@ bssl::ScopedEVP_MD_CTX ctx; EVP_PKEY_CTX *pctx = nullptr; // Owned by |ctx|. - if (!EVP_DigestVerifyInit(ctx.get(), &pctx, digest, nullptr, public_key)) + if (!EVP_DigestVerifyInit(ctx.get(), &pctx, digest, nullptr, public_key)) { return false; + } if (is_rsa_pss) { // All supported RSASSA-PSS algorithms match signing and MGF-1 digest. They @@ -279,8 +282,9 @@ const der::Input &public_key_spki, SignatureVerifyCache *cache) { bssl::UniquePtr<EVP_PKEY> public_key; - if (!ParsePublicKey(public_key_spki, &public_key)) + if (!ParsePublicKey(public_key_spki, &public_key)) { return false; + } return VerifySignedData(algorithm, signed_data, signature_value, public_key.get(), cache); }