Skip to content

Fix multiple issues with WOLFSSL_CHECK_SIG_FAULTS + HAVE_PK_CALLBACKS. - #11000

Open
kareem-wolfssl wants to merge 4 commits into
wolfSSL:masterfrom
kareem-wolfssl:zd22213
Open

Fix multiple issues with WOLFSSL_CHECK_SIG_FAULTS + HAVE_PK_CALLBACKS.#11000
kareem-wolfssl wants to merge 4 commits into
wolfSSL:masterfrom
kareem-wolfssl:zd22213

Conversation

@kareem-wolfssl

Copy link
Copy Markdown
Contributor

Description

Fixes zd#22213
Fixes NULL deref since ssl->buffers.key is expected to be NULL when using WOLFSSL_CHECK_SIG_FAULTS + HAVE_PK_CALLBACKS.
Fixes Sm2wSm3Verify call to match the earlier Sm2wSm3Sign call.

Testing

Built in tests

Checklist

  • added tests
  • updated/added doxygen
  • updated appropriate READMEs
  • Updated manual and documentation

@kareem-wolfssl kareem-wolfssl self-assigned this Jul 29, 2026
Copilot AI review requested due to automatic review settings July 29, 2026 01:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes fault-hardening signature verification paths when WOLFSSL_CHECK_SIG_FAULTS is enabled alongside HAVE_PK_CALLBACKS, avoiding NULL dereferences when ssl->buffers.key is intentionally absent, and corrects the SM2/SM3 verify call to match the signing input.

Changes:

  • Guard PK-callback key-buffer access (ssl->buffers.key) in TLS 1.3 and TLS 1.2 fault-check verification paths.
  • Align TLS 1.2 SM2/SM3 verification with the SM2/SM3 signing input (verify over handshake messages, not the digest).
  • Expand CI coverage by adding faultharden-enabled configurations to existing GitHub Actions workflows.

Reviewed changes

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

File Description
src/tls13.c Prevents NULL dereference when building PK-callback key buffer info for fault-check ECC verify in TLS 1.3.
src/internal.c Fixes NULL deref for PK-callback key buffer info in multiple TLS 1.2 fault-check verify sites and corrects SM2/SM3 verify input to match signing.
.github/workflows/wolfsm.yml Adds an SM-focused faultharden CI configuration.
.github/workflows/os-check.yml Adds an all-config faultharden CI configuration.

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

@Frauschi Frauschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🐺 Skoll Code Review

Overall recommendation: APPROVE
Findings: 4 total — 4 posted, 0 skipped

Posted findings

  • [Medium] New sm-faultharden CI config omits PK callbacks, so the SM2 + HAVE_PK_CALLBACKS code this PR fixes is still never compiled.github/workflows/wolfsm.yml:101
  • [Medium] Fault check still runs without usable key material when the private key is held by a PK callback, so the handshake failssrc/internal.c:38600
  • [Medium] No test exercises the NULL private-key scenario the PR fixes; CI additions give compile coverage only.github/workflows/os-check.yml:168
  • [Low] Dual-algorithm alt-key site left with the unguarded dereference patternsrc/tls13.c:10727

Review generated by Skoll via Claude/Codex

Comment thread .github/workflows/wolfsm.yml Outdated
Comment thread src/internal.c
Comment thread .github/workflows/os-check.yml Outdated
Comment thread src/tls13.c
@Frauschi

Frauschi commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Three CI notes, from having built these combinations while chasing a related async NULL deref in EccVerify().

1. The all-faultharden entry needs to move on rebase. It is currently added to the heredoc in .github/workflows/os-check.yml, but #11036 moved that config list out to .github/configs/os-check-linux.json, so the workflow no longer reads it from the YAML. The entry itself is good, it just lands in a file that no longer holds the list. Same shape as before, one entry per line in the JSON file.

2. Offering a second entry, if you want it in the same PR. TEST_PK_PRIVKEY is what makes examples/client.c and examples/server.c leave the private key to the PK callback, so ssl->buffers.key stays NULL, which is the state this PR fixes the handling of. Nothing under .github/ defines it today, so scripts/pkcallbacks.test always runs with the key loaded and never reaches those branches.

{"name": "pkcallbacks-testprivkey", "minutes": 1.5,
 "comment": "TEST_PK_PRIVKEY makes the examples leave the private key to the PK callback, so ssl->buffers.key stays NULL. Without it scripts/pkcallbacks.test always runs with the key loaded and never reaches those branches",
 "configure": ["--enable-pkcallbacks", "CPPFLAGS=-DTEST_PK_PRIVKEY"]}

Verified locally with the CFLAGS the workflow applies at make time: it configures, builds without warnings, and scripts/pkcallbacks.test passes rather than skipping.

It has to stay separate from all-faultharden rather than being folded into it. The two together do not currently complete a handshake: the fault check hands the verify callback a zero key size and myEccVerify() in wolfssl/test.h rejects it with -173, so a combined entry would fail. That looks like a real product gap rather than anything this PR introduces.

3. sm-faultharden in wolfsm.yml configures Public Key Callbacks: no. Building master with exactly those flags catches Sm3wSm2Verify being undeclared, but the sibling DerBuffer* to buffer* break in internal.c sits inside #ifdef HAVE_PK_CALLBACKS and slips through. Adding --enable-pkcallbacks to that entry covers both.

@kareem-wolfssl

Copy link
Copy Markdown
Contributor Author

Retest this please

Comment thread .github/configs/os-check-linux.json Outdated
Fix passing in NULL key to PK callbacks.
Fix --enable-all + TEST_PK_PRIVKEY build (DTLS and benchmark issues).
…h --enable-all --enable-pkcallbacks are now resolved.
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.

5 participants