Skip to content

Fix multiple issues with WOLFSSL_CHECK_SIG_FAULTS + HAVE_PK_CALLBACKS. #11000

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kareem-wolfssl wants to merge 1 commit into
base: master
Choose a base branch
from
+38 −18
Open

Fix multiple issues with WOLFSSL_CHECK_SIG_FAULTS + HAVE_PK_CALLBACKS. #11000

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/os-check.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,8 @@ jobs:
"configure": ["--enable-all", "CPPFLAGS=-DWOLFSSL_DEBUG_CERTS"]},
{"name": "all-hash-keep", "minutes": 7.8,
"configure": ["--enable-all", "CPPFLAGS=-DWOLFSSL_HASH_KEEP"]},
{"name": "all-faultharden", "minutes": 7.8,

@Frauschi Frauschi Jul 31, 2026

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.

🟡 [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"]},

"configure": ["--enable-all", "--enable-faultharden"]},
{"name": "all-no-pkcs7-stream", "minutes": 7.0,
"comment": "Non-streaming PKCS#7 exercises the multi-certificate decode bound in pkcs7_signed_multi_cert_test(); the streaming default cannot reach that off-by-idx path.",
"configure": ["--enable-all", "CPPFLAGS=-DNO_PKCS7_STREAM"]},
Expand Down
6 changes: 5 additions & 1 deletion .github/workflows/wolfsm.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,11 @@ jobs:
{"name": "sm4-all-modes", "minutes": 1.5,
"configure": ["--enable-sm2", "--enable-sm3", "--enable-sm4-ecb",
"--enable-sm4-cbc", "--enable-sm4-ctr", "--enable-sm4-gcm",
"--enable-sm4-ccm", "--enable-sha3"]}
"--enable-sm4-ccm", "--enable-sha3"]},
{"name": "sm-faultharden", "minutes": 1.5,
"configure": ["--enable-sm2", "--enable-sm3", "--enable-sm4-ecb",
"--enable-sm4-cbc", "--enable-sm4-ctr", "--enable-sm4-gcm",
"--enable-sm4-ccm", "--enable-sha3", "--enable-faultharden"]}

@Frauschi Frauschi Jul 31, 2026

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.

🟡 [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"]}

]
EOF
.github/scripts/parallel-make-check.py \
Expand Down
41 changes: 26 additions & 15 deletions src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36519,15 +36519,20 @@ int SendCertificateVerify(WOLFSSL* ssl)
#ifdef HAVE_PK_CALLBACKS
buffer tmp;

tmp.length = ssl->buffers.key->length;
tmp.buffer = ssl->buffers.key->buffer;
/* Private key may be held by the PK callback. */
tmp.length = ssl->buffers.key ?
ssl->buffers.key->length : 0;
tmp.buffer = ssl->buffers.key ?
ssl->buffers.key->buffer : NULL;
#endif

ret = Sm3wSm2Verify(ssl,
/* Sm2wSm3Sign() was given the handshake messages and
* not the digest - verify over the same data. */
ret = Sm2wSm3Verify(ssl,
TLS12_SM2_SIG_ID, TLS12_SM2_SIG_ID_SZ,
ssl->buffers.sig.buffer, ssl->buffers.sig.length,
ssl->buffers.digest.buffer,
ssl->buffers.digest.length, key,
ssl->hsHashes->messages,
ssl->hsHashes->length, key,
#ifdef HAVE_PK_CALLBACKS
&tmp
#else
Expand All @@ -36541,8 +36546,11 @@ int SendCertificateVerify(WOLFSSL* ssl)
#ifdef HAVE_PK_CALLBACKS
buffer tmp;

tmp.length = ssl->buffers.key->length;
tmp.buffer = ssl->buffers.key->buffer;
/* Private key may be held by the PK callback. */
tmp.length = ssl->buffers.key ?
ssl->buffers.key->length : 0;
tmp.buffer = ssl->buffers.key ?
ssl->buffers.key->buffer : NULL;
#endif

ret = EccVerify(ssl,
Expand Down Expand Up @@ -38584,6 +38592,16 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl)
#ifdef WOLFSSL_CHECK_SIG_FAULTS
{
ecc_key* key = (ecc_key*)ssl->hsKey;
#ifdef HAVE_PK_CALLBACKS
buffer tmp;

/* Private key may be held by the PK
* callback. */
tmp.length = ssl->buffers.key ?

@Frauschi Frauschi Jul 31, 2026

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.

🟡 [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.

ssl->buffers.key->length : 0;
tmp.buffer = ssl->buffers.key ?
ssl->buffers.key->buffer : NULL;
#endif

#if defined(WOLFSSL_SM2) && defined(WOLFSSL_SM3)
if (ssl->options.sigAlgo == sm2_sa_algo) {
Expand All @@ -38595,7 +38613,7 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl)
ssl->buffers.sig.length,
key,
#ifdef HAVE_PK_CALLBACKS
ssl->buffers.key
&tmp
#else
NULL
#endif
Expand All @@ -38604,13 +38622,6 @@ static int AddPSKtoPreMasterSecret(WOLFSSL* ssl)
else
#endif /* WOLFSSL_SM2 */
{
#ifdef HAVE_PK_CALLBACKS
buffer tmp;

tmp.length = ssl->buffers.key->length;
tmp.buffer = ssl->buffers.key->buffer;
#endif

ret = EccVerify(ssl,
args->output + LENGTH_SZ + args->idx,
args->sigSz,
Expand Down
7 changes: 5 additions & 2 deletions src/tls13.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10720,8 +10720,11 @@ static int SendTls13CertificateVerify(WOLFSSL* ssl)
#ifdef HAVE_PK_CALLBACKS
buffer tmp;

tmp.length = ssl->buffers.key->length;
tmp.buffer = ssl->buffers.key->buffer;
/* Private key may be held by the PK callback. */
tmp.length = ssl->buffers.key ?
ssl->buffers.key->length : 0;
tmp.buffer = ssl->buffers.key ?
ssl->buffers.key->buffer : NULL;

@Frauschi Frauschi Jul 31, 2026

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.

🔵 [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

#endif
ret = EccVerify(ssl, sigOut, args->sigLen,
args->sigData, args->sigDataSz,
Expand Down
Loading