diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 21ae71da72b..f65fbe3c49f 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -2171,6 +2171,18 @@ static bssl::UniquePtr MakeCRLDPLeaf( return leaf; } +static bssl::UniquePtr ReencodeCRL(X509_CRL *crl) { + uint8_t *der = nullptr; + int len = i2d_X509_CRL(crl, &der); + bssl::UniquePtr free_der(der); + if (len <= 0) { + return nullptr; + } + + const uint8_t *inp = der; + return bssl::UniquePtr(d2i_X509_CRL(nullptr, &inp, len)); +} + // Helper to create a CRL, optionally with an IDP extension and revoked serials. static bssl::UniquePtr MakeTestCRL( X509 *issuer_cert, EVP_PKEY *key, const char *idp_uri, @@ -2242,15 +2254,62 @@ static bssl::UniquePtr MakeTestCRL( // Re-encode and re-parse so that internal fields like crl->idp and // crl->idp_flags are populated from the IDP extension. These are only // set during parsing (ASN1_OP_D2I_POST), not programmatic construction. - uint8_t *der = nullptr; - int der_len = i2d_X509_CRL(crl.get(), &der); - if (der_len <= 0) { + return ReencodeCRL(crl.get()); +} + +// Kinds of single defect |MakeInvalidTestCRL| can introduce. Each makes the +// CRL score as a "near match" (usable but not fully valid) rather than valid. +enum class CRLDefect { + kExpired, // nextUpdate before kReferenceTime; drops CRL_SCORE_TIME. + kUnknownCritical, // unhandled critical extension; drops CRL_SCORE_NOCRITICAL. +}; + +// Like |MakeTestCRL|, but introduces a single |defect| so the CRL is correctly +// signed yet fails one validity check, exercising the near-match fallback in +// |check_all_crls|. +static bssl::UniquePtr MakeInvalidTestCRL( + X509 *issuer_cert, EVP_PKEY *key, const char *idp_uri, + const std::vector &revoked_serials, CRLDefect defect, + int crl_age = 0) { + bssl::UniquePtr crl = + MakeTestCRL(issuer_cert, key, idp_uri, revoked_serials, crl_age); + if (!crl) { return nullptr; } - const uint8_t *inp = der; - crl.reset(d2i_X509_CRL(nullptr, &inp, der_len)); - OPENSSL_free(der); - return crl; + switch (defect) { + case CRLDefect::kExpired: { + bssl::UniquePtr expired(ASN1_TIME_new()); + if (!expired || !ASN1_TIME_adj(expired.get(), kReferenceTime, -1, 0) || + !X509_CRL_set1_nextUpdate(crl.get(), expired.get())) { + return nullptr; + } + break; + } + case CRLDefect::kUnknownCritical: { + static const uint8_t kUnknownOID[] = {0x2b, 0x06, 0x01, 0x04, 0x01, + 0x82, 0x37, 0x15, 0x24}; + bssl::UniquePtr oid( + OBJ_txt2obj("1.3.6.1.4.1.311.21.36", /*dont_search_names=*/1)); + bssl::UniquePtr ext_val(ASN1_OCTET_STRING_new()); + if (!oid || !ext_val || + !ASN1_OCTET_STRING_set(ext_val.get(), kUnknownOID, + sizeof(kUnknownOID))) { + return nullptr; + } + bssl::UniquePtr ext(X509_EXTENSION_create_by_OBJ( + nullptr, oid.get(), /*crit=*/1, ext_val.get())); + if (!ext || !X509_CRL_add_ext(crl.get(), ext.get(), -1)) { + return nullptr; + } + break; + } + } + // Re-sign to cover the mutation, then re-encode and re-parse so cached + // extension flags (EXFLAG_CRITICAL, times, ...) reflect it. + if (!X509_CRL_sign(crl.get(), key, EVP_sha256())) { + return nullptr; + } + return ReencodeCRL(crl.get()); } // Test that CRL distribution point scope checking (crl_crldp_check) correctly @@ -2337,10 +2396,13 @@ TEST(X509Test, CRLDistributionPointScope) { } } -// A CRL whose IDP specifically matches the certificate's CRLDP must be -// preferred over a broad CRL (no IDP or empty IDP), regardless of freshness -// or load order. -TEST(X509Test, CRLSpecificIDPPreferredOverBroadCRL) { +// When no fully valid CRL is available, |check_all_crls| falls back to the +// best-scoring "near match" CRL so the specific CRL-validity error (expired, +// unhandled critical extension, ...) is surfaced through the verify callback +// instead of the generic X509_V_ERR_UNABLE_TO_GET_CRL. Among several unusable +// CRLs, the highest-scoring one is selected deterministically, independent of +// load order. +TEST(X509Test, CRLNearMatchFallback) { bssl::UniquePtr root(CertFromPEM(kCRLTestRoot)); bssl::UniquePtr key(PrivateKeyFromPEM(kCRLTestRootKey)); ASSERT_TRUE(root); @@ -2361,188 +2423,294 @@ TEST(X509Test, CRLSpecificIDPPreferredOverBroadCRL) { auto leaf = MakeCRLDPLeaf(root.get(), key.get(), kLeafSerial, crldp.get()); ASSERT_TRUE(leaf); - // broad_newer_vs_specific_older: clean no-IDP CRL (lastUpdate=-1d) vs - // revoking specific CRL (lastUpdate=-2d). The broad CRL is newer. + // single_expired_reports_expired: the only CRL is expired. The near-match + // fallback must report X509_V_ERR_CRL_HAS_EXPIRED, not the generic + // X509_V_ERR_UNABLE_TO_GET_CRL. { - SCOPED_TRACE("broad_newer_vs_specific_older"); - auto broad_new = MakeTestCRL(root.get(), key.get(), - nullptr, {}, /*crl_age=*/-1); - auto specific_old = MakeTestCRL(root.get(), key.get(), - kCRLURI, {kLeafSerial}, - /*crl_age=*/-2); - ASSERT_TRUE(broad_new); - ASSERT_TRUE(specific_old); + SCOPED_TRACE("single_expired_reports_expired"); + auto expired = MakeInvalidTestCRL(root.get(), key.get(), nullptr, {}, + CRLDefect::kExpired, /*crl_age=*/-1); + ASSERT_TRUE(expired); - EXPECT_EQ(X509_V_ERR_CERT_REVOKED, - Verify(leaf.get(), {root.get()}, {root.get()}, - {broad_new.get(), specific_old.get()}, + EXPECT_EQ(X509_V_ERR_CRL_HAS_EXPIRED, + Verify(leaf.get(), {root.get()}, {root.get()}, {expired.get()}, X509_V_FLAG_CRL_CHECK)); - EXPECT_EQ(X509_V_ERR_CERT_REVOKED, - Verify(leaf.get(), {root.get()}, {root.get()}, - {specific_old.get(), broad_new.get()}, + } + + // single_unknown_critical_reports_critical: the only CRL carries an unhandled + // critical extension. The near-match fallback reports + // X509_V_ERR_UNHANDLED_CRITICAL_CRL_EXTENSION. + { + SCOPED_TRACE("single_unknown_critical_reports_critical"); + auto crit = MakeInvalidTestCRL(root.get(), key.get(), nullptr, {}, + CRLDefect::kUnknownCritical, /*crl_age=*/-1); + ASSERT_TRUE(crit); + + EXPECT_EQ(X509_V_ERR_UNHANDLED_CRITICAL_CRL_EXTENSION, + Verify(leaf.get(), {root.get()}, {root.get()}, {crit.get()}, X509_V_FLAG_CRL_CHECK)); } - // empty_idp_newer_vs_specific_older: clean empty-IDP CRL (lastUpdate=-1d) vs - // revoking specific CRL (lastUpdate=-2d). The empty-IDP CRL is newer. + // higher_scoring_near_match_wins: no valid CRL. An expired-but-otherwise-clean + // CRL (missing only CRL_SCORE_TIME) outscores a time-valid CRL with an + // unhandled critical extension (missing CRL_SCORE_NOCRITICAL, a higher-value + // bit). The higher-scoring near match is selected regardless of load order, so + // the expired CRL's error is what surfaces. { - SCOPED_TRACE("empty_idp_newer_vs_specific_older"); - auto empty_new = MakeTestCRL(root.get(), key.get(), - "", {}, /*crl_age=*/-1); - auto specific_old = MakeTestCRL(root.get(), key.get(), - kCRLURI, {kLeafSerial}, - /*crl_age=*/-2); - ASSERT_TRUE(empty_new); - ASSERT_TRUE(specific_old); + SCOPED_TRACE("higher_scoring_near_match_wins"); + auto expired = MakeInvalidTestCRL(root.get(), key.get(), nullptr, {}, + CRLDefect::kExpired, /*crl_age=*/-1); + auto crit = MakeInvalidTestCRL(root.get(), key.get(), nullptr, {}, + CRLDefect::kUnknownCritical, /*crl_age=*/-1); + ASSERT_TRUE(expired); + ASSERT_TRUE(crit); - EXPECT_EQ(X509_V_ERR_CERT_REVOKED, + EXPECT_EQ(X509_V_ERR_CRL_HAS_EXPIRED, Verify(leaf.get(), {root.get()}, {root.get()}, - {empty_new.get(), specific_old.get()}, - X509_V_FLAG_CRL_CHECK)); - EXPECT_EQ(X509_V_ERR_CERT_REVOKED, + {expired.get(), crit.get()}, X509_V_FLAG_CRL_CHECK)); + EXPECT_EQ(X509_V_ERR_CRL_HAS_EXPIRED, Verify(leaf.get(), {root.get()}, {root.get()}, - {specific_old.get(), empty_new.get()}, - X509_V_FLAG_CRL_CHECK)); + {crit.get(), expired.get()}, X509_V_FLAG_CRL_CHECK)); } +} - // broad_same_age_vs_specific_same_age: both lastUpdate=-1d. - // Before the fix, load order determined the result. +// Per RFC 5280 section 6.3.3, a certificate must be checked against every +// applicable CRL, not just a single "best" one selected by score/freshness. A +// certificate found revoked in any valid CRL is revoked, even if some other, +// higher-scoring or fresher valid CRL does not list it. These cases would have +// passed (X509_V_OK) under single-CRL selection. +TEST(X509Test, CheckAllValidCRLsForRevocation) { + bssl::UniquePtr root(CertFromPEM(kCRLTestRoot)); + bssl::UniquePtr key(PrivateKeyFromPEM(kCRLTestRootKey)); + ASSERT_TRUE(root); + ASSERT_TRUE(key); + + const int kLeafSerial = 0x3000; + const char *kCRLURI = "http://example.com/crl.pem"; + + bssl::UniquePtr crldp(sk_DIST_POINT_new_null()); + ASSERT_TRUE(crldp); + bssl::UniquePtr dp(DIST_POINT_new()); + ASSERT_TRUE(dp); + dp->distpoint = MakeDistPointName(kCRLURI); + ASSERT_TRUE(dp->distpoint); + ASSERT_TRUE(bssl::PushToStack(crldp.get(), std::move(dp))); + + auto leaf = MakeCRLDPLeaf(root.get(), key.get(), kLeafSerial, crldp.get()); + ASSERT_TRUE(leaf); + + // same_scope_fresher_clean_older_revoking: two broad (no-IDP) CRLs of equal + // scope class. The fresher one is clean; the older one revokes the leaf. + // Single-CRL selection would pick the fresher (clean) CRL by the freshness + // tie-break and miss the revocation. { - SCOPED_TRACE("broad_same_age_vs_specific_same_age"); - auto broad_same = MakeTestCRL(root.get(), key.get(), - nullptr, {}, /*crl_age=*/-1); - auto specific_same = MakeTestCRL(root.get(), key.get(), - kCRLURI, {kLeafSerial}, - /*crl_age=*/-1); - ASSERT_TRUE(broad_same); - ASSERT_TRUE(specific_same); + SCOPED_TRACE("same_scope_fresher_clean_older_revoking"); + auto clean_fresh = + MakeTestCRL(root.get(), key.get(), nullptr, {}, /*crl_age=*/-1); + auto revoking_old = MakeTestCRL(root.get(), key.get(), nullptr, + {kLeafSerial}, /*crl_age=*/-2); + ASSERT_TRUE(clean_fresh); + ASSERT_TRUE(revoking_old); EXPECT_EQ(X509_V_ERR_CERT_REVOKED, Verify(leaf.get(), {root.get()}, {root.get()}, - {broad_same.get(), specific_same.get()}, + {clean_fresh.get(), revoking_old.get()}, X509_V_FLAG_CRL_CHECK)); EXPECT_EQ(X509_V_ERR_CERT_REVOKED, Verify(leaf.get(), {root.get()}, {root.get()}, - {specific_same.get(), broad_same.get()}, + {revoking_old.get(), clean_fresh.get()}, X509_V_FLAG_CRL_CHECK)); } - // empty_idp_same_age_vs_specific_same_age: both lastUpdate=-1d. + // higher_scoring_clean_lower_scoring_revoking: a specific-IDP clean CRL (which + // scores higher via CRL_SCORE_IDP_MATCH) and a broad CRL that revokes the + // leaf. Single-CRL selection would pick the higher-scoring specific CRL and + // miss the revocation carried by the broad CRL. { - SCOPED_TRACE("empty_idp_same_age_vs_specific_same_age"); - auto empty_same = MakeTestCRL(root.get(), key.get(), - "", {}, /*crl_age=*/-1); - auto specific_same = MakeTestCRL(root.get(), key.get(), - kCRLURI, {kLeafSerial}, - /*crl_age=*/-1); - ASSERT_TRUE(empty_same); - ASSERT_TRUE(specific_same); + SCOPED_TRACE("higher_scoring_clean_lower_scoring_revoking"); + auto specific_clean = + MakeTestCRL(root.get(), key.get(), kCRLURI, {}, /*crl_age=*/-1); + auto broad_revoking = MakeTestCRL(root.get(), key.get(), nullptr, + {kLeafSerial}, /*crl_age=*/-1); + ASSERT_TRUE(specific_clean); + ASSERT_TRUE(broad_revoking); EXPECT_EQ(X509_V_ERR_CERT_REVOKED, Verify(leaf.get(), {root.get()}, {root.get()}, - {empty_same.get(), specific_same.get()}, + {specific_clean.get(), broad_revoking.get()}, X509_V_FLAG_CRL_CHECK)); EXPECT_EQ(X509_V_ERR_CERT_REVOKED, Verify(leaf.get(), {root.get()}, {root.get()}, - {specific_same.get(), empty_same.get()}, + {broad_revoking.get(), specific_clean.get()}, X509_V_FLAG_CRL_CHECK)); } - // specific_expired_vs_broad_in_window: a specific-IDP CRL that revokes the - // leaf but has expired (nextUpdate before verification time) should lose to - // a time-valid broad CRL. SCOPE_MATCH is intentionally not part of - // CRL_SCORE_VALID, so an expired specific CRL cannot outrank a valid broad - // CRL via the SCOPE_MATCH bit alone. + // not_revoked_when_absent_from_all: several valid CRLs, none listing the + // leaf. The loop must examine all of them and conclude the cert is not + // revoked (no false positive). { - SCOPED_TRACE("specific_expired_vs_broad_in_window"); - auto broad = MakeTestCRL(root.get(), key.get(), nullptr, {}, - /*crl_age=*/-1); - auto specific = MakeTestCRL(root.get(), key.get(), kCRLURI, - {kLeafSerial}, /*crl_age=*/-1); - ASSERT_TRUE(broad); - ASSERT_TRUE(specific); - - // Set the specific CRL's nextUpdate to before kReferenceTime so it - // appears expired at verification time, then re-sign and re-parse. - bssl::UniquePtr expired(ASN1_TIME_new()); - ASSERT_TRUE(expired); - ASSERT_TRUE(ASN1_TIME_adj(expired.get(), kReferenceTime, -1, 0)); - ASSERT_TRUE(X509_CRL_set1_nextUpdate(specific.get(), expired.get())); - ASSERT_TRUE(X509_CRL_sign(specific.get(), key.get(), EVP_sha256())); - uint8_t *der = nullptr; - int der_len = i2d_X509_CRL(specific.get(), &der); - ASSERT_GT(der_len, 0); - const uint8_t *inp = der; - specific.reset(d2i_X509_CRL(nullptr, &inp, der_len)); - OPENSSL_free(der); - ASSERT_TRUE(specific); - - // The broad in-window CRL should be preferred. Since it doesn't list - // the revocation, the cert verifies as OK. - EXPECT_EQ(X509_V_OK, - Verify(leaf.get(), {root.get()}, {root.get()}, - {broad.get(), specific.get()}, - X509_V_FLAG_CRL_CHECK)); - EXPECT_EQ(X509_V_OK, - Verify(leaf.get(), {root.get()}, {root.get()}, - {specific.get(), broad.get()}, - X509_V_FLAG_CRL_CHECK)); + SCOPED_TRACE("not_revoked_when_absent_from_all"); + auto broad_clean = + MakeTestCRL(root.get(), key.get(), nullptr, {}, /*crl_age=*/-1); + auto specific_clean = + MakeTestCRL(root.get(), key.get(), kCRLURI, {}, /*crl_age=*/-2); + ASSERT_TRUE(broad_clean); + ASSERT_TRUE(specific_clean); + + EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, {root.get()}, + {broad_clean.get(), specific_clean.get()}, + X509_V_FLAG_CRL_CHECK)); } - // specific_with_unknown_critical_ext: a specific-IDP CRL that revokes the - // leaf but has an unhandled critical extension should lose to a processable - // broad CRL. The broad CRL doesn't list the revocation, so the cert should - // NOT be reported as revoked — we can't trust the specific CRL we can't - // fully process. + // invalid_revoking_ignored_valid_clean_wins: a revoking CRL that is NOT valid + // (expired) must be excluded from the check-all set, so a valid clean CRL + // yields X509_V_OK. We must never honor a revocation from an unusable CRL. { - SCOPED_TRACE("specific_with_unknown_critical_ext"); - auto broad = MakeTestCRL(root.get(), key.get(), nullptr, {}, - /*crl_age=*/-1); - auto specific = MakeTestCRL(root.get(), key.get(), kCRLURI, - {kLeafSerial}, /*crl_age=*/-1); - ASSERT_TRUE(broad); - ASSERT_TRUE(specific); - - // Add an unknown critical extension to the specific CRL, then re-sign - // and re-parse. - static const uint8_t kUnknownOID[] = {0x2b, 0x06, 0x01, 0x04, 0x01, - 0x82, 0x37, 0x15, 0x24}; - bssl::UniquePtr oid( - OBJ_txt2obj("1.3.6.1.4.1.311.21.36", /*dont_search_names=*/1)); - ASSERT_TRUE(oid); - bssl::UniquePtr ext_val(ASN1_OCTET_STRING_new()); - ASSERT_TRUE(ext_val); - ASSERT_TRUE(ASN1_OCTET_STRING_set(ext_val.get(), kUnknownOID, sizeof(kUnknownOID))); - bssl::UniquePtr ext( - X509_EXTENSION_create_by_OBJ(nullptr, oid.get(), /*crit=*/1, - ext_val.get())); - ASSERT_TRUE(ext); - ASSERT_TRUE(X509_CRL_add_ext(specific.get(), ext.get(), -1)); - ASSERT_TRUE(X509_CRL_sign(specific.get(), key.get(), EVP_sha256())); + SCOPED_TRACE("invalid_revoking_ignored_valid_clean_wins"); + auto valid_clean = + MakeTestCRL(root.get(), key.get(), nullptr, {}, /*crl_age=*/-1); + auto revoking_expired = + MakeInvalidTestCRL(root.get(), key.get(), nullptr, {kLeafSerial}, + CRLDefect::kExpired, /*crl_age=*/-1); + ASSERT_TRUE(valid_clean); + ASSERT_TRUE(revoking_expired); + + EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, {root.get()}, + {valid_clean.get(), revoking_expired.get()}, + X509_V_FLAG_CRL_CHECK)); + } +} - // Re-encode and re-parse to populate internal flags. - uint8_t *der = nullptr; - int der_len = i2d_X509_CRL(specific.get(), &der); - ASSERT_GT(der_len, 0); - const uint8_t *inp = der; - specific.reset(d2i_X509_CRL(nullptr, &inp, der_len)); - OPENSSL_free(der); - ASSERT_TRUE(specific); - - // The broad CRL should be preferred since the specific one has an - // unprocessable critical extension. The broad CRL doesn't revoke the - // leaf, so verification should succeed (cert not revoked). - EXPECT_EQ(X509_V_OK, +// A candidate CRL that scores as valid but fails |check_crl| (bad signature, +// unresolved issuer, missing cRLSign) is unusable, not fatal. It must be +// skipped so another usable same-issuer CRL can still determine the +// certificate's status — the situation that arises during CA key rollover or +// when a store merges CRLs from multiple sources. Only when no candidate is +// usable should verification surface the CRL error. +TEST(X509Test, CheckAllValidCRLsSkipsUnusable) { + bssl::UniquePtr root(CertFromPEM(kCRLTestRoot)); + bssl::UniquePtr key(PrivateKeyFromPEM(kCRLTestRootKey)); + // |wrong_key| is unrelated to |root|, so a CRL signed with it names |root| as + // issuer yet fails signature verification — it scores valid (scoring does not + // check signatures) but is rejected by |check_crl|. + bssl::UniquePtr wrong_key(PrivateKeyFromPEM(kP256Key)); + ASSERT_TRUE(root); + ASSERT_TRUE(key); + ASSERT_TRUE(wrong_key); + + const int kLeafSerial = 0x3200; + const char *kCRLURI = "http://example.com/crl.pem"; + + bssl::UniquePtr crldp(sk_DIST_POINT_new_null()); + ASSERT_TRUE(crldp); + bssl::UniquePtr dp(DIST_POINT_new()); + ASSERT_TRUE(dp); + dp->distpoint = MakeDistPointName(kCRLURI); + ASSERT_TRUE(dp->distpoint); + ASSERT_TRUE(bssl::PushToStack(crldp.get(), std::move(dp))); + + auto leaf = MakeCRLDPLeaf(root.get(), key.get(), kLeafSerial, crldp.get()); + ASSERT_TRUE(leaf); + + // bad_revoking_skipped_clean_wins: a bad-signature CRL that revokes the leaf + // is skipped, and the valid clean CRL yields X509_V_OK. Before the fix, the + // bad CRL aborted the whole verification with a spurious signature error. + { + SCOPED_TRACE("bad_revoking_skipped_clean_wins"); + auto clean = + MakeTestCRL(root.get(), key.get(), nullptr, {}, /*crl_age=*/-1); + auto bad_revoking = MakeTestCRL(root.get(), wrong_key.get(), nullptr, + {kLeafSerial}, /*crl_age=*/-1); + ASSERT_TRUE(clean); + ASSERT_TRUE(bad_revoking); + + EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, {root.get()}, + {clean.get(), bad_revoking.get()}, + X509_V_FLAG_CRL_CHECK)); + EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, {root.get()}, + {bad_revoking.get(), clean.get()}, + X509_V_FLAG_CRL_CHECK)); + } + + // bad_clean_skipped_valid_revoking_wins: a bad-signature clean CRL is skipped, + // and a valid CRL that revokes the leaf still reports CERT_REVOKED. + { + SCOPED_TRACE("bad_clean_skipped_valid_revoking_wins"); + auto bad_clean = + MakeTestCRL(root.get(), wrong_key.get(), nullptr, {}, /*crl_age=*/-1); + auto revoking = + MakeTestCRL(root.get(), key.get(), nullptr, {kLeafSerial}, + /*crl_age=*/-1); + ASSERT_TRUE(bad_clean); + ASSERT_TRUE(revoking); + + EXPECT_EQ(X509_V_ERR_CERT_REVOKED, Verify(leaf.get(), {root.get()}, {root.get()}, - {broad.get(), specific.get()}, - X509_V_FLAG_CRL_CHECK)); - EXPECT_EQ(X509_V_OK, + {bad_clean.get(), revoking.get()}, X509_V_FLAG_CRL_CHECK)); + EXPECT_EQ(X509_V_ERR_CERT_REVOKED, Verify(leaf.get(), {root.get()}, {root.get()}, - {specific.get(), broad.get()}, + {revoking.get(), bad_clean.get()}, X509_V_FLAG_CRL_CHECK)); + } + + // all_unusable_reports_error: when every candidate fails check_crl, the CRL + // error is still surfaced rather than silently passing. + { + SCOPED_TRACE("all_unusable_reports_error"); + auto bad = + MakeTestCRL(root.get(), wrong_key.get(), nullptr, {}, /*crl_age=*/-1); + ASSERT_TRUE(bad); + + EXPECT_EQ(X509_V_ERR_CRL_SIGNATURE_FAILURE, + Verify(leaf.get(), {root.get()}, {root.get()}, {bad.get()}, X509_V_FLAG_CRL_CHECK)); } } +// The check-all logic must gather candidate CRLs from the store lookup source +// (ctx->lookup_crls), not only from the context-supplied CRLs. +TEST(X509Test, CheckAllValidCRLsFromStore) { + bssl::UniquePtr root(CertFromPEM(kCRLTestRoot)); + bssl::UniquePtr key(PrivateKeyFromPEM(kCRLTestRootKey)); + ASSERT_TRUE(root); + ASSERT_TRUE(key); + + const int kLeafSerial = 0x3100; + const char *kCRLURI = "http://example.com/crl.pem"; + + bssl::UniquePtr crldp(sk_DIST_POINT_new_null()); + ASSERT_TRUE(crldp); + bssl::UniquePtr dp(DIST_POINT_new()); + ASSERT_TRUE(dp); + dp->distpoint = MakeDistPointName(kCRLURI); + ASSERT_TRUE(dp->distpoint); + ASSERT_TRUE(bssl::PushToStack(crldp.get(), std::move(dp))); + + auto leaf = MakeCRLDPLeaf(root.get(), key.get(), kLeafSerial, crldp.get()); + ASSERT_TRUE(leaf); + auto crl = + MakeTestCRL(root.get(), key.get(), kCRLURI, {kLeafSerial}, /*crl_age=*/-1); + ASSERT_TRUE(crl); + + // Put the trusted root and the revoking CRL in the store, so the CRL is only + // reachable via the store lookup (ctx->crls is left empty). + bssl::UniquePtr store(X509_STORE_new()); + ASSERT_TRUE(store); + ASSERT_TRUE(X509_STORE_add_cert(store.get(), root.get())); + ASSERT_TRUE(X509_STORE_add_crl(store.get(), crl.get())); + + bssl::UniquePtr ctx(X509_STORE_CTX_new()); + ASSERT_TRUE(ctx); + ASSERT_TRUE(X509_STORE_CTX_init(ctx.get(), store.get(), leaf.get(), + /*chain=*/nullptr)); + + X509_VERIFY_PARAM *param = X509_STORE_CTX_get0_param(ctx.get()); + X509_VERIFY_PARAM_set_time_posix(param, kReferenceTime); + X509_VERIFY_PARAM_set_flags(param, X509_V_FLAG_CRL_CHECK); + + EXPECT_EQ(0, X509_verify_cert(ctx.get())); + EXPECT_EQ(X509_V_ERR_CERT_REVOKED, X509_STORE_CTX_get_error(ctx.get())); +} + TEST(X509Test, TestX509GettersSetters) { bssl::UniquePtr obj(X509_OBJECT_new()); bssl::UniquePtr x509(CertFromPEM(kCRLTestRoot)); @@ -3243,18 +3411,6 @@ static bssl::UniquePtr ReencodeCertificate(X509 *cert) { return bssl::UniquePtr(d2i_X509(nullptr, &inp, len)); } -static bssl::UniquePtr ReencodeCRL(X509_CRL *crl) { - uint8_t *der = nullptr; - int len = i2d_X509_CRL(crl, &der); - bssl::UniquePtr free_der(der); - if (len <= 0) { - return nullptr; - } - - const uint8_t *inp = der; - return bssl::UniquePtr(d2i_X509_CRL(nullptr, &inp, len)); -} - static bssl::UniquePtr ReencodeCSR(X509_REQ *req) { uint8_t *der = nullptr; int len = i2d_X509_REQ(req, &der); diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index 0de9b82be9f..0952f308c21 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -82,11 +82,19 @@ static int check_policy(X509_STORE_CTX *ctx); static int get_issuer(X509 **issuer, X509_STORE_CTX *ctx, X509 *x); static int get_crl_score(X509_STORE_CTX *ctx, X509 **pissuer, X509_CRL *crl, X509 *x); -static int get_crl(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509 *x); static int crl_akid_check(X509_STORE_CTX *ctx, X509_CRL *crl, X509 **pissuer, int *pcrl_score); static int crl_crldp_check(X509 *x, X509_CRL *crl, int crl_score, int *idp_match); -static int cert_crl(X509_STORE_CTX *ctx, X509_CRL *crl, X509 *x); +static int cert_revoked(X509_STORE_CTX *ctx, X509_CRL *crl, X509 *x, + int *out_revoked); +static int check_all_crls(X509_STORE_CTX *ctx, X509 *x); +static int check_crl(X509_STORE_CTX *ctx, X509_CRL *crl); +static int check_crl_time(X509_STORE_CTX *ctx, X509_CRL *crl, int notify); +static int crl_check_validity(X509_STORE_CTX *ctx, X509_CRL *crl, X509 *issuer, + int score, int notify); +static STACK_OF(X509_CRL) *collect_usable_crls( + X509_STORE_CTX *ctx, X509 *x, X509_CRL **fallback_crl, + X509 **fallback_issuer, int *fallback_score); static int internal_verify(X509_STORE_CTX *ctx); @@ -895,35 +903,203 @@ static int check_revocation(X509_STORE_CTX *ctx) { } static int check_cert(X509_STORE_CTX *ctx) { - X509_CRL *crl = NULL; - int ok = 0, cnum = ctx->error_depth; + int cnum = ctx->error_depth; X509 *x = sk_X509_value(ctx->chain, cnum); ctx->current_cert = x; ctx->current_issuer = NULL; ctx->current_crl_score = 0; - // Try to retrieve relevant CRL - ok = ctx->get_crl(ctx, &crl, x); - // If error looking up CRL, nothing we can do except notify callback - if (!ok) { - ctx->error = X509_V_ERR_UNABLE_TO_GET_CRL; - ok = call_verify_cb(0, ctx); - goto err; + // Check against every applicable CRL (RFC 5280 section 6.3.3). + return check_all_crls(ctx, x); +} + +// crl_check_validity checks that |crl| is usable: |issuer| may sign CRLs, |x| +// is in scope, the IDP is valid, the CRL is time-valid, and the signature +// verifies. |score| is |crl|'s |get_crl_score| result. When |notify| is set, +// failures are reported through the verify callback (overridable, matching the +// historical |check_crl|); otherwise it is a silent predicate returning 0 on +// the first failure. +static int crl_check_validity(X509_STORE_CTX *ctx, X509_CRL *crl, X509 *issuer, + int score, int notify) { + if (issuer == NULL) { + return 0; } - ctx->current_crl = crl; - ok = ctx->check_crl(ctx, crl); - if (!ok) { - goto err; + // Issuer must be permitted to sign CRLs. + if ((issuer->ex_flags & EXFLAG_KUSAGE) && + !(issuer->ex_kusage & X509v3_KU_CRL_SIGN)) { + if (!notify) { + return 0; + } + ctx->error = X509_V_ERR_KEYUSAGE_NO_CRL_SIGN; + if (!call_verify_cb(0, ctx)) { + return 0; + } } + // Certificate must be within the CRL's scope. + if (!(score & CRL_SCORE_SCOPE)) { + if (!notify) { + return 0; + } + ctx->error = X509_V_ERR_DIFFERENT_CRL_SCOPE; + if (!call_verify_cb(0, ctx)) { + return 0; + } + } + if (crl->idp_flags & IDP_INVALID) { + if (!notify) { + return 0; + } + ctx->error = X509_V_ERR_INVALID_EXTENSION; + if (!call_verify_cb(0, ctx)) { + return 0; + } + } + // CRL must be time-valid, unless scoring already established that. + if (!(score & CRL_SCORE_TIME) && !check_crl_time(ctx, crl, notify)) { + return 0; + } + // CRL signature must verify against the issuer's public key. + EVP_PKEY *ikey = X509_get0_pubkey(issuer); + if (ikey == NULL) { + if (!notify) { + return 0; + } + ctx->error = X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY; + if (!call_verify_cb(0, ctx)) { + return 0; + } + } else if (X509_CRL_verify(crl, ikey) <= 0) { + if (!notify) { + return 0; + } + ctx->error = X509_V_ERR_CRL_SIGNATURE_FAILURE; + if (!call_verify_cb(0, ctx)) { + return 0; + } + } + return 1; +} - ok = cert_crl(ctx, crl, x); - if (!ok) { - goto err; +// crl_is_newer returns one if |b|'s lastUpdate is strictly later than |a|'s. +static int crl_is_newer(const X509_CRL *a, const X509_CRL *b) { + int day, sec; + if (!ASN1_TIME_diff(&day, &sec, X509_CRL_get0_lastUpdate(a), + X509_CRL_get0_lastUpdate(b))) { + return 0; + } + return day > 0 || sec > 0; +} + +// push_candidate_crls scores every CRL in |in| for |x|. Usable ones (in scope +// and passing |crl_check_validity|) are appended to |usable| with a new +// reference. The highest-scoring relevant-but-unusable candidate (near match or +// valid-scored-but-invalid, ties broken by newer lastUpdate) is tracked in +// |*fallback_crl| (new ref) / |*fallback_issuer| (borrowed) / |*fallback_score| +// for use when no usable CRL exists. +static void push_candidate_crls(X509_STORE_CTX *ctx, X509 *x, + STACK_OF(X509_CRL) *usable, + STACK_OF(X509_CRL) *in, X509_CRL **fallback_crl, + X509 **fallback_issuer, int *fallback_score) { + for (size_t i = 0; i < sk_X509_CRL_num(in); i++) { + X509_CRL *crl = sk_X509_CRL_value(in, i); + X509 *issuer = NULL; + int score = get_crl_score(ctx, &issuer, crl, x); + if (score >= CRL_SCORE_VALID && + crl_check_validity(ctx, crl, issuer, score, /*notify=*/0)) { + if (sk_X509_CRL_push(usable, crl)) { + X509_CRL_up_ref(crl); + } + } else if (score > 0 && + (*fallback_crl == NULL || score > *fallback_score || + (score == *fallback_score && + crl_is_newer(*fallback_crl, crl)))) { + X509_CRL_up_ref(crl); + X509_CRL_free(*fallback_crl); + *fallback_crl = crl; + *fallback_issuer = issuer; + *fallback_score = score; + } + } +} + +// collect_usable_crls returns the usable CRLs for |x| from the context and the +// store lookup (see |push_candidate_crls| for the fallback out-params). The +// caller owns the returned stack (free with |sk_X509_CRL_pop_free|) and +// |*fallback_crl|. Returns NULL on allocation failure. +static STACK_OF(X509_CRL) *collect_usable_crls( + X509_STORE_CTX *ctx, X509 *x, X509_CRL **fallback_crl, + X509 **fallback_issuer, int *fallback_score) { + *fallback_crl = NULL; + *fallback_issuer = NULL; + *fallback_score = 0; + STACK_OF(X509_CRL) *usable = sk_X509_CRL_new_null(); + if (usable == NULL) { + return NULL; + } + // Source 1: CRLs attached directly to the context. + push_candidate_crls(ctx, x, usable, ctx->crls, fallback_crl, fallback_issuer, + fallback_score); + // Source 2: CRLs from the store, looked up by the certificate's issuer name. + STACK_OF(X509_CRL) *skcrl = ctx->lookup_crls(ctx, X509_get_issuer_name(x)); + if (skcrl != NULL) { + push_candidate_crls(ctx, x, usable, skcrl, fallback_crl, fallback_issuer, + fallback_score); + sk_X509_CRL_pop_free(skcrl, X509_CRL_free); + } + return usable; +} + +// check_all_crls checks |x| against every usable CRL (RFC 5280 section 6.3.3): +// revoked if it appears in any, stopping at the first match. Pre-filtering the +// candidates means one bad CRL cannot mask a usable one. When none are usable, +// it reports the highest-scoring fallback candidate's error via |check_crl| +// (once, overridable), or X509_V_ERR_UNABLE_TO_GET_CRL if no CRL is relevant. +static int check_all_crls(X509_STORE_CTX *ctx, X509 *x) { + X509_CRL *fallback_crl = NULL; + X509 *fallback_issuer = NULL; + int fallback_score = 0; + STACK_OF(X509_CRL) *usable = collect_usable_crls( + ctx, x, &fallback_crl, &fallback_issuer, &fallback_score); + + int ok = 1; + if (usable != NULL && sk_X509_CRL_num(usable) > 0) { + for (size_t i = 0; i < sk_X509_CRL_num(usable); i++) { + X509_CRL *crl = sk_X509_CRL_value(usable, i); + // These CRLs are already validated; just set the state cert_revoked and + // the callback rely on. + X509 *issuer = NULL; + ctx->current_crl = crl; + ctx->current_crl_score = get_crl_score(ctx, &issuer, crl, x); + ctx->current_issuer = issuer; + + int revoked = 0; + ok = cert_revoked(ctx, crl, x, &revoked); + if (!ok || revoked) { + // Callback aborted, or the cert was found revoked; no need to continue. + break; + } + } + ctx->current_crl = NULL; + } else if (fallback_crl != NULL) { + // No usable CRL: report the fallback candidate's error; if overridden, + // still check revocation against it. + ctx->current_issuer = fallback_issuer; + ctx->current_crl_score = fallback_score; + ctx->current_crl = fallback_crl; + ok = check_crl(ctx, fallback_crl); + if (ok) { + int revoked = 0; + ok = cert_revoked(ctx, fallback_crl, x, &revoked); + } + ctx->current_crl = NULL; + } else { + // No relevant CRL at all. + ctx->error = X509_V_ERR_UNABLE_TO_GET_CRL; + ok = call_verify_cb(0, ctx); } -err: - X509_CRL_free(crl); - ctx->current_crl = NULL; + sk_X509_CRL_pop_free(usable, X509_CRL_free); + X509_CRL_free(fallback_crl); return ok; } @@ -994,54 +1170,6 @@ static int check_crl_time(X509_STORE_CTX *ctx, X509_CRL *crl, int notify) { return 1; } -static int get_crl_sk(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509 **pissuer, - int *pscore, STACK_OF(X509_CRL) *crls) { - int crl_score, best_score = *pscore; - X509 *x = ctx->current_cert; - X509_CRL *best_crl = NULL; - X509 *crl_issuer = NULL, *best_crl_issuer = NULL; - - for (size_t i = 0; i < sk_X509_CRL_num(crls); i++) { - X509_CRL *crl = sk_X509_CRL_value(crls, i); - crl_score = get_crl_score(ctx, &crl_issuer, crl, x); - if (crl_score < best_score || crl_score == 0) { - continue; - } - // If current CRL is equivalent use it if it is newer - if (crl_score == best_score && best_crl != NULL) { - int day, sec; - if (ASN1_TIME_diff(&day, &sec, X509_CRL_get0_lastUpdate(best_crl), - X509_CRL_get0_lastUpdate(crl)) == 0) { - continue; - } - // ASN1_TIME_diff never returns inconsistent signs for |day| - // and |sec|. - if (day <= 0 && sec <= 0) { - continue; - } - } - best_crl = crl; - best_crl_issuer = crl_issuer; - best_score = crl_score; - } - - if (best_crl) { - if (*pcrl) { - X509_CRL_free(*pcrl); - } - *pcrl = best_crl; - *pissuer = best_crl_issuer; - *pscore = best_score; - X509_CRL_up_ref(best_crl); - } - - if (best_score >= CRL_SCORE_VALID) { - return 1; - } - - return 0; -} - // For a given CRL return how suitable it is for the supplied certificate // 'x'. The return value is a mask of several criteria. If the issuer is not // the certificate issuer this is returned in *pissuer. @@ -1240,49 +1368,11 @@ static int crl_crldp_check(X509 *x, X509_CRL *crl, int crl_score, int *idp_match // CRL could still be a good candidate CRL to check against although we // cannot check if it matches the DP in the certificate. A CRL with a // specific IDP match receives (CRL_SCORE_SCOPE | CRL_SCORE_IDP_MATCH), so it will be preferred - // over a broad match. Among CRLs with the same scope class, get_crl_sk() + // over a broad match. Among CRLs with the same scope class, check_all_crls() // will pick the freshest one. return !crl->idp || !crl->idp->distpoint; } -// Retrieve CRL corresponding to current certificate. -static int get_crl(X509_STORE_CTX *ctx, X509_CRL **pcrl, X509 *x) { - int ok; - X509 *issuer = NULL; - int crl_score = 0; - X509_CRL *crl = NULL; - STACK_OF(X509_CRL) *skcrl; - X509_NAME *nm = X509_get_issuer_name(x); - ok = get_crl_sk(ctx, &crl, &issuer, &crl_score, ctx->crls); - if (ok) { - goto done; - } - - // Lookup CRLs from store - skcrl = ctx->lookup_crls(ctx, nm); - - // If no CRLs found and a near match from get_crl_sk use that - if (!skcrl && crl) { - goto done; - } - - get_crl_sk(ctx, &crl, &issuer, &crl_score, skcrl); - - sk_X509_CRL_pop_free(skcrl, X509_CRL_free); - -done: - - // If we got any kind of CRL use it and return success - if (crl) { - ctx->current_issuer = issuer; - ctx->current_crl_score = crl_score; - *pcrl = crl; - return 1; - } - - return 0; -} - // Check CRL validity static int check_crl(X509_STORE_CTX *ctx, X509_CRL *crl) { X509 *issuer = NULL; @@ -1308,61 +1398,24 @@ static int check_crl(X509_STORE_CTX *ctx, X509_CRL *crl) { } } - if (issuer) { - // Check for cRLSign bit if keyUsage present - if ((issuer->ex_flags & EXFLAG_KUSAGE) && - !(issuer->ex_kusage & X509v3_KU_CRL_SIGN)) { - ctx->error = X509_V_ERR_KEYUSAGE_NO_CRL_SIGN; - if (!call_verify_cb(0, ctx)) { - return 0; - } - } - - if (!(ctx->current_crl_score & CRL_SCORE_SCOPE)) { - ctx->error = X509_V_ERR_DIFFERENT_CRL_SCOPE; - if (!call_verify_cb(0, ctx)) { - return 0; - } - } - - if (crl->idp_flags & IDP_INVALID) { - ctx->error = X509_V_ERR_INVALID_EXTENSION; - if (!call_verify_cb(0, ctx)) { - return 0; - } - } - - if (!(ctx->current_crl_score & CRL_SCORE_TIME)) { - if (!check_crl_time(ctx, crl, 1)) { - return 0; - } - } - - // Attempt to get issuer certificate public key - EVP_PKEY *ikey = X509_get0_pubkey(issuer); - if (!ikey) { - ctx->error = X509_V_ERR_UNABLE_TO_DECODE_ISSUER_PUBLIC_KEY; - if (!call_verify_cb(0, ctx)) { - return 0; - } - } else { - // Verify CRL signature - if (X509_CRL_verify(crl, ikey) <= 0) { - ctx->error = X509_V_ERR_CRL_SIGNATURE_FAILURE; - if (!call_verify_cb(0, ctx)) { - return 0; - } - } - } + if (issuer == NULL) { + // No issuer resolved; historically this skipped the validity checks. + return 1; } - - return 1; + // Report validity failures (bad key usage, scope, IDP, time, signature) + // through the verify callback. + return crl_check_validity(ctx, crl, issuer, ctx->current_crl_score, + /*notify=*/1); } -// Check certificate against CRL -static int cert_crl(X509_STORE_CTX *ctx, X509_CRL *crl, X509 *x) { +// Check certificate against CRL. On return, |*out_revoked| is set to one if the +// certificate's serial number was found in |crl| (regardless of whether the +// verify callback chose to continue), and zero otherwise. +static int cert_revoked(X509_STORE_CTX *ctx, X509_CRL *crl, X509 *x, + int *out_revoked) { int ok; X509_REVOKED *rev; + *out_revoked = 0; // The rules changed for this... previously if a CRL contained unhandled // critical extensions it could still be used to indicate a certificate // was revoked. This has since been changed since critical extension can @@ -1377,6 +1430,7 @@ static int cert_crl(X509_STORE_CTX *ctx, X509_CRL *crl, X509 *x) { } // Look for serial number of certificate in CRL. if (X509_CRL_get0_by_cert(crl, &rev, x)) { + *out_revoked = 1; ctx->error = X509_V_ERR_CERT_REVOKED; ok = call_verify_cb(0, ctx); if (!ok) { @@ -1771,18 +1825,6 @@ int X509_STORE_CTX_init(X509_STORE_CTX *ctx, X509_STORE *store, X509 *x509, ctx->verify_cb = null_callback; } - if (store->get_crl) { - ctx->get_crl = store->get_crl; - } else { - ctx->get_crl = get_crl; - } - - if (store->check_crl) { - ctx->check_crl = store->check_crl; - } else { - ctx->check_crl = check_crl; - } - if (store->lookup_crls) { ctx->lookup_crls = store->lookup_crls; } else {