Skip to content

Adjustment of CA pathlen check and glitch hardening#10296

Open
JacobBarthelmeh wants to merge 2 commits intowolfSSL:masterfrom
JacobBarthelmeh:hostap
Open

Adjustment of CA pathlen check and glitch hardening#10296
JacobBarthelmeh wants to merge 2 commits intowolfSSL:masterfrom
JacobBarthelmeh:hostap

Conversation

@JacobBarthelmeh
Copy link
Copy Markdown
Contributor

@JacobBarthelmeh JacobBarthelmeh commented Apr 23, 2026

Re-introducing some hostap CI tests (#10058) exposed an edge case in current master branch with CA pathlen checking. This resolves handling when a trust anchor is being included as part of the certificate chain and adds a regression test.

@JacobBarthelmeh JacobBarthelmeh self-assigned this Apr 23, 2026
Copilot AI review requested due to automatic review settings April 23, 2026 22:23
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates certificate path length handling to exclude the trust anchor certificate itself from RFC 5280 path validation when a peer includes the root in the transmitted chain.

Changes:

  • Detect when the “current cert” is the trust anchor and bypass the maxPathLen == 0 self-issued intermediate rejection.
  • Apply issuer constraint handling for the trust anchor case without triggering the pathLen violation.
  • Add a regression test that parses the trust anchor as a chain cert and expects success.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
wolfcrypt/src/asn.c Adds special-case handling to exclude the trust anchor from path length enforcement per RFC 5280.
tests/api.c Adds a test ensuring a root included in the peer chain doesn’t fail due to its own pathLen=0.
Comments suppressed due to low confidence (1)

wolfcrypt/src/asn.c:1

  • Using public key equality alone to identify that cert is the trust anchor is not equivalent to 'this certificate is the trust anchor'. Distinct certificates can legitimately share the same public key (key reuse/cross-signing scenarios), which would incorrectly skip the maxPathLen==0 rejection and weaken pathLen enforcement. Prefer an identity check tied to trust-anchor selection (e.g., a dedicated 'is trust anchor' flag / explicit trust-anchor match in the cert manager), or at least strengthen the condition by requiring the cert to be self-signed (subject == issuer and signature validates with its own key) and/or match SKID/AKID/issuer+subject linkage rather than just key bytes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 23, 2026

MemBrowse Memory Report

gcc-arm-cortex-m4

  • FLASH: .text +64 B (+0.0%, 195,799 B / 262,144 B, total: 75% used)

gcc-arm-cortex-m4-min-ecc

@JacobBarthelmeh
Copy link
Copy Markdown
Contributor Author

JacobBarthelmeh commented Apr 23, 2026

Retest this please Jenkins. sanitize report that I could not reproduce locally.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes an edge case in CA pathLen enforcement when a peer includes the trust anchor (root) certificate in the transmitted chain, and adds regression coverage for it. Also tightens cipher-text sanity-check logic and hardens an existing TLS 1.3 test against cascading failures.

Changes:

  • Update ParseCertRelative() pathLen handling to treat a self-signed trust anchor cert as excluded from the prospective certification path (RFC 5280 6.1), avoiding a self-triggered pathLen=0 failure.
  • Add a regression test ensuring parsing the trust anchor as a chain cert succeeds even when pathLen=0.
  • Adjust cipher-text check logic to avoid short-length comparisons and make TLS 1.3 empty-record-limit test more robust to earlier expectation failures.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
wolfcrypt/src/asn.c Excludes the trust anchor itself from pathLen step (l) when included as a self-signed chain cert.
tests/api.c Adds regression coverage for the trust-anchor-in-chain pathLen=0 case.
src/tls13.c Tightens cipher-text sanity check to only run with sufficient plaintext length and compares a fixed-size window.
src/internal.c Mirrors the cipher-text sanity check tightening for non-TLS13 encryption path.
tests/api/test_tls13.c Prevents uninitialized/invalid use of recSz when earlier expectations fail in the test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/tls13.c Outdated
Comment thread src/tls13.c Outdated
Comment thread src/internal.c
Comment thread src/internal.c
Copy link
Copy Markdown

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #10296

Scan targets checked: wolfcrypt-bugs, wolfcrypt-src, wolfssl-bugs, wolfssl-src

No new issues found in the changed files. ✅

@JacobBarthelmeh JacobBarthelmeh requested a review from Copilot April 24, 2026 07:08
@JacobBarthelmeh JacobBarthelmeh changed the title exclude the trust anchor from prospective certification path with pat… Adjustment of CA pathlen check and glitch hardening Apr 24, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes an RFC 5280 pathLen corner case where a peer includes the trust anchor (root) in the provided certificate chain, and adds/adjusts tests and internal “cipher text check” logic to avoid flaky false positives.

Changes:

  • Update RFC 5280 6.1.4(l) pathLen handling to exclude the trust anchor itself from prospective path processing when it appears in the chain.
  • Add a regression test that parses the trust anchor as a CHAIN_CERT_TYPE certificate to ensure pathLen=0 doesn’t fail against itself.
  • Tighten WOLFSSL_CIPHER_TEXT_CHECK sampling in TLS 1.3 and TLS 1.2 record encryption to only compare when at least 8 bytes are available; adjust a TLS 1.3 empty-record-limit test to better respect EXPECT_SUCCESS().

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
wolfcrypt/src/asn.c Excludes the trust anchor from step (l) pathLen enforcement when the root is included in the chain.
tests/api.c Adds a regression step verifying the trust anchor can be parsed as a chain cert without triggering ASN_PATHLEN_INV_E.
tests/api/test_tls13.c Improves test robustness by gating post-handshake consumption / record sizing on EXPECT_SUCCESS().
src/tls13.c Avoids short-length false positives in WOLFSSL_CIPHER_TEXT_CHECK by requiring an 8-byte sample.
src/internal.c Same WOLFSSL_CIPHER_TEXT_CHECK tightening for non–TLS 1.3 encryption path.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/api/test_tls13.c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants