Skip to content

fix(picky): honor CTL NotBefore EKU restriction - #574

Merged
Benoît Cortier (CBenoit) merged 2 commits into
masterfrom
cbenoit-fix-ctl-notbefore-eku-lookup
Oct 26, 2026
Merged

Benoît Cortier (CBenoit) merged 2 commits into
masterfrom
cbenoit-fix-ctl-notbefore-eku-lookup

Conversation

@CBenoit

@CBenoit Benoît Cortier (CBenoit) commented Oct 15, 2026 •

Copy link
Copy Markdown
Member

A CTL entry can carry a NotBefore date (property 126) together with the set of EKUs it restricts (property 127). The Authenticode CTL check searched for property 126 when looking up the EKU set, so every future NotBefore rejected the root regardless of its listed EKUs. Look up property 127 so the restriction applies when its EKU set includes code signing or is absent, and return a malformed-data error for an empty property-127 SET instead of panicking.

A CTL entry can carry a NotBefore date (property 126) together with the
set of EKUs it restricts (property 127). The Authenticode CTL check
searched for property 126 when looking up the EKU set, so the lookup
never matched and every future NotBefore rejected the root as not yet
valid, whatever EKUs it listed.

Look up property 127 instead, so a future NotBefore rejects the root
only when its EKU set includes code signing or is absent.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@CBenoit

Copy link
Copy Markdown
Member Author

Notes for reviewers:

  • Regression test. ctl_not_before_applies_only_to_listed_ekus appends a crafted entry (future NotBefore, with and without an EKU set) to the test CTL and calls h_verify_ca_certificate_against_ctl directly. Without the fix, the server-auth-only case fails with CaCertificateNotYetValid. This also covers the kp_oid == &eku_code_signing_oid comparison that cargo-mutants reported as missed.
  • Overlap with fix(picky): anchor Authenticode CTL check on the root certificate #533. fix(picky): anchor Authenticode CTL check on the root certificate #533 moves this block into h_check_root_against_ctl_entries. The test-only CertificateTrustList::from_der added here is identical to the one in fix(picky): anchor Authenticode CTL check on the root certificate #533, so that hunk should merge cleanly. Whichever PR lands second needs a one-token rebase of the find matcher and an adjustment of the new test to the call signature (ctl, leaf, certificates, and a SHA-1 thumbprint as cert_fingerprint instead of the subject-name MD5).
  • Out of scope: NotBefore semantics. The check rejects the root when not_before > now, which reads as "the root is not yet trusted". Microsoft's root deprecation policy documents NotBefore differently: certificates issued after that date by the root are distrusted for the listed EKUs, while earlier ones remain trusted. A faithful check would compare the date against the leaf's issuance (its notBefore or the signing time), not the current time. This PR does not change that; it may deserve a follow-up issue.

Note

LLM-assisted content (no human feedback).

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

An empty property-127 SET now reaches an unchecked expectation and panics during verification.

1 open finding
What changed in this PR

Corrects Authenticode CTL validation so NotBefore restrictions apply to the intended extended key usages (EKUs).

Changes:

  • Looks up property 127 for NotBefore EKU restrictions.
  • Adds regression coverage for absent, unrelated, and code-signing EKUs.
File Description
picky/​src/​x509/​pkcs7/​ctl.rs Adds a test-only DER constructor.
picky/​src/​x509/​pkcs7/​authenticode.rs Corrects the property lookup and adds regression tests.

🧠 Review effort: Balanced


💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread picky/src/x509/pkcs7/authenticode.rs

Benoît Cortier (CBenoit) commented Oct 26, 2026 •

Copy link
Copy Markdown
Member Author

Updated head: e8075f6dfbd9015f09a81d49a45554d8cdbd847c.

The branch now includes current master. CTL verification looks up property 127 for NotBefore EKUs and returns Asn1DerError::InvalidData for an empty outer SET. The regression distinguishes an empty outer SET from a present empty EKU sequence. Existing NotBefore date semantics are unchanged.

All 33 Authenticode tests passed. The empty-SET regression reproduced the reported panic before the fix and passes afterward. Independent review of this head: “No material findings.” The fixed automated thread is resolved; CI is running.

Diff against master: production code +2/-5; tests +68/-0; other files 0 lines, all in picky/src/x509/pkcs7/authenticode.rs. No new UNTESTED: properties.

Note

LLM-assisted content (no human feedback).

Copy link
Copy Markdown
Member Author

Final verification for e8075f6dfbd9015f09a81d49a45554d8cdbd847c: all CI checks passed, the automated finding is fixed and resolved, and independent review found no material issues. The branch is conflict-free. A required reviewer approval remains outstanding.

Note

LLM-assisted content (no human feedback).

@CBenoit
Benoît Cortier (CBenoit) merged commit aa834a9 into master Oct 26, 2026
20 checks passed
@CBenoit
Benoît Cortier (CBenoit) deleted the cbenoit-fix-ctl-notbefore-eku-lookup branch October 26, 2026 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants