Skip to content

Commit

Permalink
Reland: CertVerifyProcConstraintsTest: add policy constraints tests
Browse files Browse the repository at this point in the history 
This relands 18c6dc1. The CL is changed to inline some test expectations for clarity but the expectations are not changed.

Bug: 1370748
Change-Id: Ica1265781e2e40e1ec0380b9bb416d889a881a98
Cq-Include-Trybots: luci.chromium.try:mac_chromium_10.13_rel_ng,mac_chromium_10.14_rel_ng,mac_chromium_10.15_rel_ng,mac_chromium_11.0_rel_ng,ios15-beta-simulator
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4010802
Reviewed-by: David Benjamin <davidben@chromium.org>
Commit-Queue: Matt Mueller <mattm@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1070618}
  • Loading branch information
@matt-mueller
matt-mueller authored and Chromium LUCI CQ committed Nov 12, 2022
1 parent fc9b365 commit 73a8ed4
Show file tree
Hide file tree
Showing 3 changed files with 87 additions and 49 deletions.
127 changes: 79 additions & 48 deletions net/cert/cert_verify_proc_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -885,54 +885,6 @@ TEST_P(CertVerifyProcInternalTest, UnnecessaryInvalidIntermediate) {
}
}

// A regression test for https://crbug.com/31497: If an intermediate has
// requireExplicitPolicy in its policyConstraints extension, verification
// should still succeed as long as some policy is valid for the chain, since
// Chrome does not specify any required policy as an input to certificate
// verification (allows anyPolicy).
TEST_P(CertVerifyProcInternalTest, IntermediateCARequireExplicitPolicy) {
if (verify_proc_type() == CERT_VERIFY_PROC_ANDROID) {
// Disabled on Android, as the Android verification libraries require an
// explicit policy to be specified, even when anyPolicy is permitted.
LOG(INFO) << "Skipping test on Android";
return;
}

for (bool leaf_has_policy : {false, true}) {
SCOPED_TRACE(leaf_has_policy);

std::unique_ptr<CertBuilder> leaf, intermediate, root;
CertBuilder::CreateSimpleChain(&leaf, &intermediate, &root);
ASSERT_TRUE(leaf && intermediate && root);

static const char kPolicy1[] = "1.2.3.4";
static const char kPolicy2[] = "1.2.3.4.5";
static const char kPolicy3[] = "1.2.3.5";
intermediate->SetCertificatePolicies({kPolicy1, kPolicy2, kPolicy3});
intermediate->SetPolicyConstraints(
/*require_explicit_policy=*/0,
/*inhibit_policy_mapping=*/absl::nullopt);

if (leaf_has_policy)
leaf->SetCertificatePolicies({kPolicy1});

scoped_refptr<X509Certificate> cert = leaf->GetX509CertificateChain();
ScopedTestRoot scoped_root(root->GetX509Certificate().get());

int flags = 0;
CertVerifyResult verify_result;
int error = Verify(cert.get(), "www.example.com", flags,
CRLSet::BuiltinCRLSet().get(), CertificateList(),
&verify_result);
if (leaf_has_policy) {
EXPECT_THAT(error, IsOk());
EXPECT_EQ(0u, verify_result.cert_status);
} else {
EXPECT_THAT(error, IsError(ERR_CERT_INVALID));
}
}
}

TEST_P(CertVerifyProcInternalTest, RejectExpiredCert) {
base::FilePath certs_dir = GetTestCertsDirectory();

Expand Down Expand Up @@ -4506,6 +4458,85 @@ TEST_P(CertVerifyProcConstraintsTest, ValidityNotYetValidIntermediate) {
}
}

TEST_P(CertVerifyProcConstraintsTest, PolicyConstraints0Root) {
for (bool leaf_has_policy : {false, true}) {
SCOPED_TRACE(leaf_has_policy);

static const char kPolicy1[] = "1.2.3.4";
static const char kPolicy2[] = "1.2.3.4.5";
static const char kPolicy3[] = "1.2.3.5";
chain_[3]->SetPolicyConstraints(
/*require_explicit_policy=*/0,
/*inhibit_policy_mapping=*/absl::nullopt);
chain_[3]->SetCertificatePolicies({kPolicy1, kPolicy2});
chain_[2]->SetCertificatePolicies({kPolicy3, kPolicy1});
chain_[1]->SetCertificatePolicies({kPolicy1});

if (leaf_has_policy) {
chain_[0]->SetCertificatePolicies({kPolicy1});
EXPECT_THAT(Verify(), IsOk());
} else {
chain_[0]->SetCertificatePolicies({});
if (VerifyProcTypeIsBuiltin() ||
verify_proc_type() == CERT_VERIFY_PROC_MAC ||
verify_proc_type() == CERT_VERIFY_PROC_IOS ||
verify_proc_type() == CERT_VERIFY_PROC_ANDROID) {
EXPECT_THAT(Verify(), IsOk());
} else {
EXPECT_THAT(Verify(), IsError(ERR_CERT_INVALID));
}
}
}
}

TEST_P(CertVerifyProcConstraintsTest, PolicyConstraints4Root) {
// Explicit policy is required after 4 certs. Since the chain is 4 certs
// long, an explicit policy is never required.
chain_[3]->SetPolicyConstraints(
/*require_explicit_policy=*/4,
/*inhibit_policy_mapping=*/absl::nullopt);

EXPECT_THAT(Verify(), IsOk());
}

// This is also a regression test for https://crbug.com/31497: If an
// intermediate has requireExplicitPolicy in its policyConstraints extension,
// verification should still succeed as long as some policy is valid for the
// chain, since Chrome does not specify any required policy as an input to
// certificate verification (allows anyPolicy).
TEST_P(CertVerifyProcConstraintsTest, PolicyConstraints0Intermediate) {
for (bool leaf_has_policy : {false, true}) {
SCOPED_TRACE(leaf_has_policy);

static const char kPolicy1[] = "1.2.3.4";
static const char kPolicy2[] = "1.2.3.4.5";
static const char kPolicy3[] = "1.2.3.5";
chain_[2]->SetPolicyConstraints(
/*require_explicit_policy=*/0,
/*inhibit_policy_mapping=*/absl::nullopt);
chain_[2]->SetCertificatePolicies({kPolicy1, kPolicy2});
chain_[1]->SetCertificatePolicies({kPolicy3, kPolicy1});

if (leaf_has_policy) {
chain_[0]->SetCertificatePolicies({kPolicy1});
EXPECT_THAT(Verify(), IsOk());
} else {
chain_[0]->SetCertificatePolicies({});
EXPECT_THAT(Verify(), IsError(ExpectedIntermediateConstraintError()));
}
}
}

TEST_P(CertVerifyProcConstraintsTest, PolicyConstraints3Intermediate) {
// Explicit policy is required after 3 certs. Since the chain up to
// |chain_[2]| is 3 certs long, an explicit policy is never required.
chain_[2]->SetPolicyConstraints(
/*require_explicit_policy=*/3,
/*inhibit_policy_mapping=*/absl::nullopt);

EXPECT_THAT(Verify(), IsOk());
}

TEST(CertVerifyProcTest, RejectsPublicSHA1Leaves) {
scoped_refptr<X509Certificate> cert(
ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem"));
Expand Down
8 changes: 7 additions & 1 deletion net/test/cert_builder.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -739,6 +739,11 @@ void CertBuilder::SetCertificatePolicies(
// PolicyQualifierInfo OPTIONAL }
//
// CertPolicyId ::= OBJECT IDENTIFIER
if (policy_oids.empty()) {
EraseExtension(der::Input(kCertificatePoliciesOid));
return;
}

bssl::ScopedCBB cbb;
CBB certificate_policies;
ASSERT_TRUE(CBB_init(cbb.get(), 64));
Expand Down Expand Up @@ -788,7 +793,8 @@ void CertBuilder::SetPolicyConstraints(
der::ContextSpecificPrimitive(1)));
}

SetExtension(der::Input(kPolicyConstraintsOid), FinishCBB(cbb.get()));
SetExtension(der::Input(kPolicyConstraintsOid), FinishCBB(cbb.get()),
/*critical=*/true);
}

void CertBuilder::SetValidity(base::Time not_before, base::Time not_after) {
Expand Down
1 change: 1 addition & 0 deletions net/test/cert_builder.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ class CertBuilder {

// Sets the certificatePolicies extension with the specified policyIdentifier
// OIDs, which must be specified in dotted string notation (e.g. "1.2.3.4").
// If |policy_oids| is empty, the extension will be removed.
void SetCertificatePolicies(const std::vector<std::string>& policy_oids);

// Sets the PolicyConstraints extension. If both |require_explicit_policy|
Expand Down

0 comments on commit 73a8ed4

Please sign in to comment.