Bugfix- wolf ssl add0 chain cert not incrementing certchain and causing TLS1.3 to fail - #10517
Conversation
|
@julek-wolfssl as we discussed on the ticket this is the PR for the bug in add0 function + added a test for it |
|
Can one of the admins verify this patch? |
56b78bc to
416d317
Compare
|
@dgarske I fixed the whitespace issue + rebased to master for the second test . can we re-run workflow here? |
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
Skoll Code Review
Scan type: reviewOverall recommendation: COMMENT
Findings: 2 total — 2 posted, 0 skipped
2 finding(s) posted as inline comments (see file-level comments below)
Posted findings
- [Low] Test verifies the counter but not the end-to-end chain-send behavior it fixes —
tests/api.c:3696-3739 - [Info] Increment is unconditional while CTX equivalent guards it with WOLFSSL_TLS13 —
src/ssl_load.c:5225
Review generated by Skoll
| return EXPECT_RESULT(); | ||
| } | ||
|
|
||
| static int test_wolfSSL_add0_chain_cert_increments_count(void) |
There was a problem hiding this comment.
🔵 [Low] Test verifies the counter but not the end-to-end chain-send behavior it fixes
The new test directly asserts ssl->buffers.certChainCnt increments per chain cert, which is a good, targeted regression guard for this specific fix. It does not, however, exercise the actual symptom (a TLS 1.3 handshake emitting a full chain rather than a leaf-only Certificate message), nor the wolfSSL_add1_chain_cert path which funnels through wolfSSL_add0_chain_cert and is therefore also affected by the fix. The unit test is acceptable as-is given how much harder a full handshake assertion would be, but coverage of the add1 wrapper would be cheap and worthwhile. Also note: if SSL_add0_chain_cert ever returned 0 inside the loop, the just-loaded x509 would leak since it is unconditionally set to NULL afterward without being freed on the failure path -- only a concern under a future regression, not current behavior.
Fix: Consider adding a parallel assertion via SSL_add1_chain_cert (verifying both the count increments and the X509 ref count is bumped), and/or a comment noting the unit test stands in for the harder-to-author TLS 1.3 full-chain handshake check.
| ssl->buffers.certChainCnt++; | ||
| /* We now own cert chain. */ | ||
| ssl->buffers.weOwnCertChain = 1; | ||
| /* Create a stack to put certificate into. */ |
There was a problem hiding this comment.
⚪ [Info] Increment is unconditional while CTX equivalent guards it with WOLFSSL_TLS13
The new ssl->buffers.certChainCnt++; is unconditional, whereas the analogous CTX helper wolfssl_ctx_add_to_chain (src/ssl_load.c:4968-4971) wraps ctx->certChainCnt++; in #ifdef WOLFSSL_TLS13. This is a minor stylistic inconsistency, not a bug: the certChainCnt member is always defined (under #ifndef NO_CERTS, wolfssl/internal.h:5029), so the unconditional increment always compiles, and the count is consumed both by TLS 1.3 (SendTls13Certificate) and by the non-TLS1.3 OCSP-multi path (MIN(cnt, certChainCnt + 1) at src/internal.c:26269). The unconditional form here is arguably more correct than the CTX version because it keeps the count accurate even in non-TLS1.3 builds. No change required; flagging only for awareness of the divergence between the two helpers.
Fix: Leave as-is (unconditional is fine and slightly more correct). Optionally note the divergence, or align the CTX helper to also increment unconditionally in a follow-up for consistency.
dgarske
left a comment
dgarske
left a comment
There was a problem hiding this comment.
See my peer review comment from skoll. Thanks
…n SendTls13Certificate not updating the chain count made TLS 1.3 handshake fail and leave with leaft-only certficiate and then peer rejected the chain
Cover wolfSSL_add1_chain_cert, which funnels through add0. Assert both the chain count and the X509 reference bump so the caller's own free is proven safe. Free the X509 when add0 fails instead of leaking it. Place certChainCnt++ exactly as the missing_openssl_functionality branch does. That branch carries the same fix, and with the two spellings apart git merges both insertions without a conflict, incrementing twice.
416d317 to
07f9b11
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a TLS 1.3 handshake failure caused by SSL_add0_chain_cert() not updating the certificate-chain count (certChainCnt) used by SendTls13Certificate, leading to only the leaf certificate being sent.
Changes:
- Update
wolfSSL_add0_chain_cert()to incrementssl->buffers.certChainCntwhen a certificate is appended to the chain. - Add an API test to validate
SSL_add0_chain_cert()/SSL_add1_chain_cert()correctly update the chain count and reference ownership semantics.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/ssl_load.c |
Increments certChainCnt when adding a cert to the per-SSL chain to support correct TLS 1.3 Certificate message construction. |
tests/api.c |
Adds a regression test ensuring add0/add1 chain APIs increment the chain count as expected (and validate add1 refcount behavior). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
in TLS 1.3 handshake we're dependent on the cert chain cnt in SendTls13Certificate . not updating the chain count upon using
SSL_add0_chain_certmade TLS 1.3 to leave with leaf-only certificate and then peer rejected the chain and handshake failed.Testing
Tested it on my platform after encountering the handshake bug as part of migrating from openssl -> wolfssl + made a simple unitest for it
Checklist