Fix multiple issues with WOLFSSL_CHECK_SIG_FAULTS + HAVE_PK_CALLBACKS.

[kareem-wolfssl]

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

[Copilot]

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.

[github-actions]

MemBrowse Memory Report

No memory changes detected for:

• gcc-arm-cortex-m0plus
• gcc-arm-cortex-m3
• gcc-arm-cortex-m4
• gcc-arm-cortex-m4-baremetal
• gcc-arm-cortex-m4-crypto-only
• gcc-arm-cortex-m4-dtls13
• gcc-arm-cortex-m4-min-ecc
• gcc-arm-cortex-m4-openssl-compat
• gcc-arm-cortex-m4-pkcs7
• gcc-arm-cortex-m4-pq
• gcc-arm-cortex-m4-rsa-only
• gcc-arm-cortex-m4-sp-math
• gcc-arm-cortex-m4-tls12
• gcc-arm-cortex-m4-tls13
• gcc-arm-cortex-m7
• gcc-arm-cortex-m7-pq
• gcc-arm-cortex-m7-tls13
• linuxkm-pie
• linuxkm-standard
• stm32-sim-stm32h753

[Frauschi]

🐺 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 fails — src/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 pattern — src/tls13.c:10727

Review generated by Skoll via Claude/Codex

[Frauschi]

🟡 [Medium] New sm-faultharden CI config omits PK callbacks, so the SM2 + HAVE_PK_CALLBACKS code this PR fixes is still never compiled
💡 SUGGEST test

The core compile-level defect this PR fixes is at src/internal.c:38613 (old code), where ssl->buffers.key (a DerBuffer*) was passed to Sm2wSm3Verify()'s buffer* keyBufInfo parameter — an incompatible-pointer-type constraint violation that is fatal under the project's -Werror builds. Reaching that code requires WOLFSSL_SM2 && WOLFSSL_SM3 && WOLFSSL_CHECK_SIG_FAULTS && HAVE_PK_CALLBACKS. Neither new CI entry covers that intersection: the new sm-faultharden entry enables SM2/SM3/SM4 + --enable-faultharden but does NOT enable PK callbacks (it does not use --enable-all, and --enable-pkcallbacks is absent), while all-faultharden in os-check.yml uses --enable-all, which force-enables HAVE_PK_CALLBACKS (configure.ac:9467-9471) but does NOT enable SM2 (SM2 is opt-in only). The all-sm entry (wolfsm.yml:87-90) has --enable-all --enable-sm2 ... — i.e. SM2 + PK callbacks — but no --enable-faultharden. Net effect: the exact combination whose compile error motivated this PR remains unbuilt in CI, so an identical regression could land again unnoticed. The new configs do cover the Sm3wSm2Verify → Sm2wSm3Verify typo fix (that block compiles without PK callbacks), so they are useful — they are just one flag short of covering everything the PR touches.

---

Verified locally (macOS arm64, PR head d4ea9dd08, wolfsm sources installed):

Building master with this PR's exact sm-faultharden flags:

$ ./configure --enable-sm2 --enable-sm3 --enable-sm4-ecb --enable-sm4-cbc \
      --enable-sm4-ctr --enable-sm4-gcm --enable-sm4-ccm --enable-sha3 \
      --enable-faultharden
   * Public Key Callbacks:       no
$ make
src/internal.c:36670:31: error: use of undeclared identifier 'Sm3wSm2Verify'

Only the Sm3wSm2Verify typo is caught. The DerBuffer* -> buffer* constraint violation is
not, because it sits inside #ifdef HAVE_PK_CALLBACKS. Adding --enable-pkcallbacks to the same
config on master surfaces both:

src/internal.c:36670:31: error: use of undeclared identifier 'Sm3wSm2Verify'
src/internal.c:38742:41: error: incompatible pointer types passing 'DerBuffer *'
                         to parameter of type 'buffer *' [-Werror,-Wincompatible-pointer-types]

And the other half of the gap is confirmed too - --enable-all --enable-faultharden reports:

   * SM3:                        no
   * SM2:                        no
   * Public Key Callbacks:       yes

So neither new entry compiles the SM2 + PK-callbacks intersection, and the type error this PR
repairs could return unnoticed.

Suggestion:

Suggested change

	"--enable-sm4-ccm", "--enable-sha3", "--enable-faultharden"]}
	{"name": "sm-faultharden", "minutes": 1.5,
	"comment": "SM2 + PK callbacks + fault hardening: the only config that compiles the SM2 signature-fault check with HAVE_PK_CALLBACKS.",
	"configure": ["--enable-sm2", "--enable-sm3", "--enable-sm4-ecb",
	"--enable-sm4-cbc", "--enable-sm4-ctr", "--enable-sm4-gcm",
	"--enable-sm4-ccm", "--enable-sha3", "--enable-faultharden",
	"--enable-pkcallbacks"]}

[Frauschi]

🟡 [Medium] Fault check still runs without usable key material when the private key is held by a PK callback, so the handshake fails
💡 SUGGEST

The PR guards ssl->buffers.key at all the WOLFSSL_CHECK_SIG_FAULTS sites, but the companion pointer ssl->hsKey is NULL in exactly the same configuration, and the check itself still cannot be performed.

When the private key is supplied only through a PK callback, ssl->buffers.key == NULL, so the send paths take the wolfSSL_CTX_IsPrivatePkSet() branch and call GetPrivateKeySigSize() (src/internal.c:36261, src/internal.c:38125, src/tls13.c:10191). That function sets ssl->hsType = DYNAMIC_TYPE_ECC but never allocates or decodes ssl->hsKey, because DecodePrivateKey() is skipped. The fault check therefore reaches
EccVerify(ssl, ..., key = (ecc_key*)ssl->hsKey /* NULL */, &tmp /* {NULL, 0} */).

Three outcomes, depending on build and application:

1. An EccVerifyCb is registered: it receives keyBuf = NULL, keySz = 0 and must derive the key itself. The in-tree myEccVerify() (wolfssl/test.h) cannot, and rejects it.
2. No EccVerifyCb (note wolfSSL_CTX_IsPrivatePkSet() only tests the sign callbacks): wc_ecc_verify_hash(..., key = NULL) returns ECC_BAD_ARG_E and the handshake aborts.
3. With --enable-asynccrypt: EccVerify() calls wolfSSL_AsyncInit(ssl, &key->asyncDev, ...) with no NULL guard, while EccSign() wraps the identical call in if (key). That asymmetry leaves a NULL dereference reachable.

So the crash is genuinely fixed, but the feature combination still does not produce a working handshake.

---

Verified locally (macOS arm64, PR head d4ea9dd08, wolfsm sources installed):

Server side, ECC client-auth handshake, built --enable-pkcallbacks --enable-faultharden --enable-ecc CPPFLAGS=-DTEST_PK_PRIVKEY (so the private key lives only in the callback):

# master (b844cdcce)
PK ECC KeyGen: ret 0
<Segmentation fault: 11>

  EXC_BAD_ACCESS (SIGSEGV), KERN_INVALID_ADDRESS at 0x0000000000000010
    libwolfssl.45.dylib  SendServerKeyExchange
    libwolfssl.45.dylib  wolfSSL_accept

Address 0x10 is length's offset in DerBuffer, i.e. ssl->buffers.key->length at the site this PR fixes. That is zd#22213 reproduced, and the PR does eliminate it:

# this PR (d4ea9dd08)
PK ECC Sign:   inSz 32, keySz 0   -> ret 0, outSz 72
PK ECC Verify: sigSz 72, hashSz 32, keySz 0
PK ECC Verify: ret -173, result 0
SSL_accept error -173, Bad function argument

No crash, but the handshake still fails: the self-check hands the verify callback a zero-length key. Worth deciding whether the check should simply be skipped when there is no local key to re-verify with.

Recommendation: Skip the sig-fault self-check when there is no local key material, rather than calling EccVerify with a NULL key - e.g. gate the block on ssl->hsKey != NULL && ssl->buffers.key != NULL at each site, with a comment that the private key lives behind a PK callback and the check is not applicable. Independently, mirror EccSign's if (key) guard around EccVerify's wolfSSL_AsyncInit(ssl, &key->asyncDev, ...) so a NULL key cannot crash the async path.

If instead the intended contract is that EccVerifyCb may be called with keyBuf == NULL, keySz == 0, please document that in the PK-callback doxygen and teach myEccVerify() in wolfssl/test.h to handle it.

[Frauschi]

🟡 [Medium] No test exercises the NULL private-key scenario the PR fixes; CI additions give compile coverage only
💡 SUGGEST test

The crash being fixed only happens when ssl->buffers.key == NULL, i.e. when the private key lives entirely in the PK callback (SendCertificateVerify() src/internal.c:36258-36265 and SendTls13CertificateVerify() src/tls13.c:10188-10195 take the wolfSSL_CTX_IsPrivatePkSet() path and never call DecodePrivateKey()). In-tree, that state is only produced when TEST_PK_PRIVKEY is defined: examples/client/client.c:3671-3675 and examples/server/server.c:3023 skip loading the private key only under #if defined(HAVE_PK_CALLBACKS) && defined(TEST_PK_PRIVKEY), and tests/suites.c:687,815 only append -P under the same macro. grep -rn 'TEST_PK_PRIVKEY' .github/ returns nothing, so no CI config defines it. Consequently all-faultharden runs scripts/pkcallbacks.test with the key loaded (ssl->buffers.key != NULL) and never reaches the NULL branch the PR adds — the new configs prove only that the code compiles. The PR checklist claims "added tests", but the diff adds build configurations, not a test of the fixed behaviour. Note that adding such a config will also require myEccVerify() in wolfssl/test.h:3369-3394 to be taught to handle key == NULL, keySz == 0 (it currently calls wc_EccPublicKeyDecode(key, &idx, &myKey, keySz) unconditionally), which is itself evidence that this path has never been run.

---

Verified locally (macOS arm64, PR head d4ea9dd08, wolfsm sources installed):

Confirmed both directions. make check on this PR with --enable-sm2 --enable-sm3 --enable-faultharden --enable-pkcallbacks passes 6/6 including scripts/pkcallbacks.test - but only because TEST_PK_PRIVKEY is undefined, so the key is loaded and ssl->buffers.key != NULL. Rebuilding with CPPFLAGS=-DTEST_PK_PRIVKEY and running an ECC handshake reaches the new branch and fails:

PK ECC Verify: sigSz 72, hashSz 32, keySz 0
PK ECC Verify: ret -173, result 0
SSL_accept error -173, Bad function argument

This is exactly the myEccVerify() gap predicted above, and it confirms a runtime config would have caught something the compile-only configs cannot.

Suggestion:

Suggested change

	{"name": "all-faultharden", "minutes": 7.8,
	{"name": "all-faultharden", "minutes": 7.8,
	"configure": ["--enable-all", "--enable-faultharden"]},
	{"name": "all-faultharden-pk-privkey", "minutes": 7.8,
	"comment": "TEST_PK_PRIVKEY makes the examples/unit suite hold the private key in the PK callback (ssl->buffers.key == NULL), which is the only way to reach the signature-fault check's key-held-by-callback path.",
	"configure": ["--enable-all", "--enable-faultharden",
	"CPPFLAGS=-DTEST_PK_PRIVKEY"]},

[Frauschi]

🔵 [Low] Dual-algorithm alt-key site left with the unguarded dereference pattern
🔧 NIT convention

Three of the four tmp initialisations were hardened against a NULL key; the WOLFSSL_DUAL_ALG_CERTS alt-key copy immediately below the changed hunk still does tmp.length = ssl->buffers.altKey->length; without a NULL check. This is currently safe — SendTls13CertificateVerify() bails out with NO_PRIVATE_KEY at src/tls13.c:10290-10292 whenever sigSpec == BOTH and ssl->buffers.altKey == NULL, and ssl->hsAltType is only set to DYNAMIC_TYPE_ECC by DecodeAltPrivateKey(), which runs only after that check — so this is not a live bug. It is flagged only because it now reads as an oversight next to the three hardened sites, and because the guard it depends on is several hundred lines away.

(Anchored on the changed hunk; the alt-key copy itself is roughly 20 lines below, at src/tls13.c:10747.)

Suggestion:

Suggested change

	ssl->buffers.key->buffer : NULL;
	#ifdef HAVE_PK_CALLBACKS
	buffer tmp;
	/* altKey is non-NULL here: checked when the alt key was decoded. */
	tmp.length = ssl->buffers.altKey ? ssl->buffers.altKey->length : 0;
	tmp.buffer = ssl->buffers.altKey ? ssl->buffers.altKey->buffer : NULL;
	#endif