Various security fixes and tests by anhu · Pull Request #10088 · wolfSSL/wolfssl

anhu · 2026-03-27T13:47:38Z

Fixes:
zd21412
zd21413
zd21414
zd21415
zd21417
zd21418
zd21422
zd21423
zd21426

Add missing hashLen validation in dilithium_verify_ctx_hash() to match the check already present in the sign path. Without this, a caller could pass a hashLen larger than WC_MAX_DIGEST_SIZE.

Validate that the received key share data length (keLen) is at least as large as the expected ciphertext size (ctSz) before passing it to wc_KyberKey_Decapsulate. A malicious TLS 1.3 server could send a short ML-KEM key share.

Add bounds check for oriOIDSz against MAX_OID_SZ before copying into the oriOID stack buffer in wc_PKCS7_DecryptOri. A crafted CMS EnvelopedData message could set an OID length larger than 32 bytes.

Save key->heap before calling wc_*_free(), which zeros the entire key structure via ForceZero. The saved heap pointer is then passed to XFREE instead of the now-zeroed key->heap.

Add check before word32 addition in dilithium_hash256() that could wrap to zero, bypassing the size check. Also reject absurdly large msgLen (> UINT32_MAX/2) in wc_dilithium_verify_ctx_msg.

Validate ECC key imports when the untrusted flag is set to prevent invalid curve attacks. The TLS key exchange path (internal.c) passes untrusted=1 for peer keys. Trusted imports (cert parsing) pass untrusted=0 and skip the check.

…1422) Replace single last-byte padding check with full PKCS#5/PKCS#7 validation: verify padLen is non-zero and within block size. Both wc_PKCS7_DecodeEnvelopedData and wc_PKCS7_DecodeEncryptedData paths are fixed.

Reinitialize pointer fields in WOLFSSL_SESSION after raw XMEMCPY or XFREAD in wolfSSL_memrestore_session_cache and wolfSSL_restore_session_cache. After restore, ticket is reset to staticTicket, ticketLenAlloc to 0, and peer to NULL.

Increase buff size from 8 to 24 bytes in PrintPubKeyRSA and related EVP PKEY print functions.

douzzer · 2026-03-30T18:33:00Z

retest this please

FAIL: scripts/ocsp-stapling2.test

douzzer

Nit from clang-tidy:

[quantum-safe-wolfssl-all-clang-tidy] [33 of 61] [4f295cfb83]
    configure${config_analyzer_note}...   real 0m13.426s  user 0m8.036s  sys 0m6.468s
    build...83c456b463 (<anthony@wolfssl.com> 2026-03-27 09:15:34 -0400 34762)         msg, 0xFFFFFFC0u, &res, &key), WC_NO_ERR_TRACE(BAD_FUNC_ARG));
/tmp/tmp.4346_28411/wolfssl_test_workdir.15574/wolfssl/tests/api.c:34762:14: warning: integer literal has suffix 'u', which is not uppercase [readability-uppercase-literal-suffix]
34762 |         msg, 0xFFFFFFC0u, &res, &key), WC_NO_ERR_TRACE(BAD_FUNC_ARG));
|              ^         ~
       |                        U

anhu · 2026-04-01T18:32:31Z

jenkins retest this please.

douzzer

Testing configuration:
--enable-ecc --enable-tlsx --enable-supportedcurves --enable-heapmath --enable-ocspstapling --enable-harden --enable-secure-renegotiation --enable-certgen --enable-pkcs7 --enable-indef --enable-smallcache --enable-pkcallbacks --enable-cmac --enable-keygen --enable-poly1305 --enable-chacha --disable-oldtls --disable-sha224 --disable-errorqueue --enable-aesgcm=small --disable-inline --disable-asm --enable-opensslextra=x509small --enable-tls13 --enable-des3 --enable-atomicuser --enable-crl --enable-wpas=small --enable-eccencrypt
Testing DEFAULT: --enable-ecc --enable-tlsx --enable-supportedcurves --enable-heapmath --enable-ocspstapling --enable-harden --enable-secure-renegotiation --enable-certgen --enable-pkcs7 --enable-indef --enable-smallcache --enable-pkcallbacks --enable-cmac --enable-keygen --enable-poly1305 --enable-chacha --disable-oldtls --disable-sha224 --disable-errorqueue --enable-aesgcm=small --disable-inline --disable-asm --enable-opensslextra=x509small --enable-tls13 --enable-des3 --enable-atomicuser --enable-crl --enable-wpas=small --enable-eccencrypt
Configure RESULT = 0
make[2]: warning: -j3 forced in submake: resetting jobserver mode.
In file included from ./wolfssl/wolfcrypt/error-crypt.h:34,
                 from ./wolfssl/error-ssl.h:27,
                 from ./wolfssl/ssl.h:35,
                 from ./tests/unit.h:40,
                 from tests/api.c:32:
tests/api.c: In function ‘test_zd21422_pkcs7_padding_oracle’:
./wolfssl/wolfcrypt/types.h:987:31: error: ‘encodedSz’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  987 |     #define XMEMCPY(d,s,l)    memcpy((d),(s),(l))
      |                               ^~~~~~
tests/api.c:34811:9: note: ‘encodedSz’ was declared here
34811 |     int encodedSz;
      |         ^~~~~~~~~

Testing configuration:
--enable-sp-math-all=small --enable-all --disable-asm --enable-smallstack --enable-32bit CFLAGS="-m32" 
The case with CFLAGS + double-quotes
EXTRACTED_CFLAGS = -m32
Testing CONFIG_REMAINDER = --enable-sp-math-all=small --enable-all --disable-asm --enable-smallstack --enable-32bit  
Configure RESULT = 0
make[2]: warning: -j3 forced in submake: resetting jobserver mode.
In file included from ./wolfssl/wolfcrypt/error-crypt.h:34,
                 from ./wolfssl/error-ssl.h:27,
                 from ./wolfssl/ssl.h:35,
                 from ./tests/unit.h:40,
                 from tests/api.c:32:
tests/api.c: In function ‘test_zd21422_pkcs7_padding_oracle’:
./wolfssl/wolfcrypt/types.h:987:31: error: ‘encodedSz’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
  987 |     #define XMEMCPY(d,s,l)    memcpy((d),(s),(l))
      |                               ^~~~~~
tests/api.c:34811:9: note: ‘encodedSz’ was declared here
34811 |     int encodedSz;
      |         ^~~~~~~~~

Testing CONFIG_REMAINDER = CC=clang   --enable-opensslextra --enable-des3 --enable-dh --enable-ecc --enable-dtls --enable-aesgcm --enable-aesccm --enable-sniffer --enable-psk --enable-camellia --enable-sha512 --enable-crl --enable-ocsp --enable-savesession --enable-savecert --enable-atomicuser --enable-pkcallbacks --enable-scep 
32
Configure RESULT = 0
33
34
make[2]: warning: -j3 forced in submake: resetting jobserver mode.
35
In file included from src/ssl.c:288:
36
./src/ssl_sess.c:530:34: error: unused variable 's' [-Werror,-Wunused-variable]
37
                WOLFSSL_SESSION* s = &SessionCache[i].Sessions[j];
38
                                 ^
39
./src/ssl_sess.c:705:34: error: unused variable 's' [-Werror,-Wunused-variable]
40
                WOLFSSL_SESSION* s = &SessionCache[i].Sessions[j];
41
                                 ^

and others

JacobBarthelmeh · 2026-04-02T22:00:26Z

wolfcrypt/src/ecc.c

+#else
+    /* Validate untrusted ECC imports to prevent invalid curve attacks */
+    if ((err == MP_OKAY) && untrusted)
+        err = wc_ecc_check_key(key);


I'm not sure that this addition is necessary. There was a test for "point on curve" added before here (#9684).

I have confirmed it is required. Steps to confirm:

build with ./configure and then make check and observe all tests pass.
remove this addition and make check and the tests fails. The failure is based on the test that was made for this report.

I think this full wc_ecc_check_key will cause performance issues -- checking more than needs to be checked. Looks like this additional check was a duplicate from what Tobias was working on (#10133).

Yeah this seems like a duplicate of #10133

dgarske · 2026-04-06T15:44:24Z

src/ssl_sess.c

+    #if defined(HAVE_SESSION_TICKET) || \
+       (defined(SESSION_CERTS) && defined(OPENSSL_EXTRA))
+        /* Reset pointers to safe values after raw copy */
+        {


NIT: Prefer not to have empty brace sections to appease the int j.

dgarske · 2026-04-06T15:45:41Z

wolfcrypt/src/evp.c

    int indent, int bitlen, WOLFSSL_ASN1_PCTX* pctx)
 {
-    byte   buff[8] = { 0 };
+    byte   buff[24] = { 0 };


Seems like we need an enum/macro for the 24? I'm surprised there isn't one already....

dgarske · 2026-04-06T15:46:27Z

wolfcrypt/src/pkcs7.c

                break;
            }
+            /* Constant-time check all padding bytes */
+            {


Again, let's not put empty braces, just add the new variables needed to top of function.

dgarske · 2026-04-06T15:46:51Z

wolfcrypt/src/pkcs7.c

+            /* Constant-time check all padding bytes */
+            {
+                byte padCheck = 0;
+                int pi;


Might consider a variable name that won't possibly conflict "pi"

douzzer

AI review -- several of these should be fixed before merge and are easy:

PR Summary

This is a security-fix batch: 9 ZD-tracked vulnerabilities (21412–21426), each with a targeted fix, plus 5 regression tests. The fixes span ML-DSA, TLS 1.3 PQC key exchange, PKCS7/CMS, ECC import, session cache serialization, and EVP PKEY printing.

Findings

1. PKCS7 padding check is not actually constant-time (ZD 21422 — both paths)

The comment says "Constant-time check all padding bytes," but the loop iterates exactly padLen times:

for (padIndex = encryptedContentSz - padLen;
     padIndex < encryptedContentSz; padIndex++) {
    padCheck |= encryptedContent[padIndex] ^ padLen;
}

The iteration count is data-dependent — it leaks padLen (i.e. the last plaintext byte) through timing. A true constant-time check would iterate over the full final block and use a mask to conditionally accumulate. The early-bail checks (padLen == 0 || padLen > expBlockSz) also leak through control flow, though those reject structurally invalid values.

Practical impact is low for PKCS7/CMS, which is typically not a network-facing oracle the way TLS CBC is (Vaudenay/Lucky13). The fix is a substantial improvement over the pre-existing single-last-byte check. But the comment should either be corrected or the implementation made truly constant-time, depending on threat model.

2. Session cache restore: `ticketLen` not sanitized (ZD 21423)

After the raw XMEMCPY/XFREAD, the fix resets ticket = staticTicket and ticketLenAlloc = 0, but does not cap ticketLen. If the serialized session had a dynamically-allocated ticket with ticketLen > SESSION_TICKET_LEN, the restored session now has ticket pointing to staticTicket (size SESSION_TICKET_LEN) with a ticketLen that exceeds it. Any subsequent read of ticket[0..ticketLen-1] would be OOB from staticTicket.

This is arguably pre-existing in the save/restore mechanism (it serializes raw structs including stale pointer values), but this fix makes the inconsistency more dangerous: the pointer is now valid (so no obvious crash), but the length can exceed the buffer (silent OOB). Consider adding:

#ifdef HAVE_SESSION_TICKET
    s->ticket = s->staticTicket;
    s->ticketLenAlloc = 0;
    if (s->ticketLen > SESSION_TICKET_LEN)
        s->ticketLen = SESSION_TICKET_LEN;
#endif

3. Incomplete `wc_*_delete()` coverage (ZD 21415)

The heap-before-free pattern is fixed for curve25519, ed25519, and dilithium. If ed448, curve448, or any other key types have the same wc_*_delete() → wc_*_free() → ForceZero → XFREE(key, key->heap, ...) pattern, they remain vulnerable. Not a regression from this PR, but worth an audit pass.

4. `MP_DECIMAL_MAX_LEN` naming is misleading (ZD 21426)

The define is 24, which is fine for RSA/DSA/DH/EC exponents and small parameters. But the name MP_DECIMAL_MAX_LEN reads as "maximum decimal representation length for any MP integer," which it clearly isn't (a 2048-bit MP integer needs ~617 decimal digits). If any future code uses this define for general mp_int-to-decimal conversion, it will overflow. A name like EVP_EXPONENT_PRINT_MAX or similar would be less error-prone.

5. Test callback `oriDecryptCb_zd21414` has an unchecked OOB read (test-only)

keySz = oriValue[1];
if (*decryptedKeySz < keySz) return -1;
XMEMCPY(decryptedKey, oriValue + 2, keySz);

There's a check if (oriValueSz < 2) return -1; but no check that keySz <= oriValueSz - 2. If the ORI value data is shorter than oriValue[1] claims, this reads OOB from oriValue. This is test-only code and the crafted DER is designed to fail at the OID bounds check before the callback is reached, so it's not a production issue — but a fuzzer hitting this callback with a different input could cause a confusing test crash. A one-liner if (keySz > oriValueSz - 2) return -1; would harden it.

6. No regression test for ZD 21413 (TLS 1.3 PQC key share over-read)

This is the most critical fix — a malicious TLS 1.3 server can trigger it remotely. The others all require local/crafted input. Understandable that it's hard to unit-test without TLS handshake infrastructure, but worth noting as a gap.

Fixes that look clean

ZD 21412 (dilithium hashLen check) — straightforward, mirrors sign-path check.
ZD 21413 (TLS PQC keLen < ctSz check) — correct, positioned before the Decapsulate call, proper error code.
ZD 21414 (ORI OID bounds check) — correct, oriOIDSz > MAX_OID_SZ before XMEMCPY into oriOID[MAX_OID_SZ].
ZD 21415 (heap-before-free) — correct pattern, clean across all three files.
ZD 21417 (integer overflow check) — both the data2Len > (UINT32_MAX - data1Len) and the msgLen > UINT32_MAX / 2 guards are correct.
ZD 21418 (ECC point validation for untrusted imports) — clean. In the #else of WOLFSSL_VALIDATE_ECC_IMPORT, gates on untrusted so trusted imports (cert parsing) are unaffected.

Binary blob check: The malformed[] array in the ZD 21414 test is a verifiable crafted CMS EnvelopedData with visible DER structure — OIDs, the patched 0x40 length byte, 54 ASCII 'X' (0x58) pad bytes, AES-256-CBC OID, IV, and ciphertext. No opaque or suspicious blobs.

Verdict

No security regressions introduced. The fixes are well-targeted and the tests are solid. Findings #1 (constant-time comment) and #2 (ticketLen cap) are the most actionable items before merge.

anhu · 2026-04-06T20:52:34Z

448 variants don't have XFREE() in them.

anhu · 2026-04-06T21:00:15Z

Not fixing oriDecryptCb_zd21414(). Suggested change is confusing and hard to read.

anhu · 2026-04-06T21:03:30Z

I tried making a regression text for zd21413. Couldn't get it done. Maybe later. We need to get pushed.

anhu added 9 commits March 27, 2026 09:44

Fix ML-DSA HashML-DSA verify (ZD 21412)

978c37f

Add missing hashLen validation in dilithium_verify_ctx_hash() to match the check already present in the sign path. Without this, a caller could pass a hashLen larger than WC_MAX_DIGEST_SIZE.

Fix TLS 1.3 PQC key share over heap read (ZD 21413)

d790313

Validate that the received key share data length (keLen) is at least as large as the expected ciphertext size (ctSz) before passing it to wc_KyberKey_Decapsulate. A malicious TLS 1.3 server could send a short ML-KEM key share.

Fix PKCS7 ORI OID (ZD 21414)

8a60289

Add bounds check for oriOIDSz against MAX_OID_SZ before copying into the oriOID stack buffer in wc_PKCS7_DecryptOri. A crafted CMS EnvelopedData message could set an OID length larger than 32 bytes.

Fix wc_*_delete() functions (ZD 21415)

327b926

Save key->heap before calling wc_*_free(), which zeros the entire key structure via ForceZero. The saved heap pointer is then passed to XFREE instead of the now-zeroed key->heap.

Fix Dilithium with USE_INTEL_SPEEDUP (ZD 21417)

392ce13

Add check before word32 addition in dilithium_hash256() that could wrap to zero, bypassing the size check. Also reject absurdly large msgLen (> UINT32_MAX/2) in wc_dilithium_verify_ctx_msg.

Fix missing ECC point validation on import (ZD 21418)

feb8782

Validate ECC key imports when the untrusted flag is set to prevent invalid curve attacks. The TLS key exchange path (internal.c) passes untrusted=1 for peer keys. Trusted imports (cert parsing) pass untrusted=0 and skip the check.

Fix session cache restore dangling pointer (ZD 21423)

32506fc

Reinitialize pointer fields in WOLFSSL_SESSION after raw XMEMCPY or XFREAD in wolfSSL_memrestore_session_cache and wolfSSL_restore_session_cache. After restore, ticket is reset to staticTicket, ticketLenAlloc to 0, and peer to NULL.

Fix RSA exponent printing (ZD 21426)

c4bedf2

Increase buff size from 8 to 24 bytes in PrintPubKeyRSA and related EVP PKEY print functions.

anhu requested a review from wolfSSL-Bot March 27, 2026 13:48

anhu self-assigned this Mar 27, 2026

anhu mentioned this pull request Mar 27, 2026

Various security fixes and tests #10074

Closed

Add regression tests for fixes

83c456b

anhu force-pushed the new_various branch from a0019fb to 83c456b Compare March 27, 2026 18:31

Frauschi added the For This Release Release version 5.9.1 label Mar 30, 2026

anhu assigned wolfSSL-Bot and unassigned anhu Mar 30, 2026

douzzer added the Staged Staged for merge pending final test results and review label Mar 30, 2026

Remove UTF-8 chars

1b6b3ea

douzzer removed the Staged Staged for merge pending final test results and review label Mar 31, 2026

douzzer requested changes Mar 31, 2026

View reviewed changes

douzzer assigned anhu Mar 31, 2026

Get rid of weird character

f2c4fde

douzzer added the Staged Staged for merge pending final test results and review label Apr 1, 2026

douzzer requested changes Apr 2, 2026

View reviewed changes

douzzer removed the Staged Staged for merge pending final test results and review label Apr 2, 2026

Fix warning found by CI

a33731a

anhu force-pushed the new_various branch from 179c337 to a33731a Compare April 2, 2026 14:25

JacobBarthelmeh reviewed Apr 2, 2026

View reviewed changes

anhu requested review from JacobBarthelmeh and douzzer April 4, 2026 01:27

anhu removed their assignment Apr 4, 2026

dgarske requested changes Apr 6, 2026

View reviewed changes

dgarske assigned anhu Apr 6, 2026

Style changes

d9a8a81

anhu requested a review from dgarske April 6, 2026 17:36

anhu assigned wolfSSL-Bot and unassigned anhu and wolfSSL-Bot Apr 6, 2026

douzzer requested changes Apr 6, 2026

View reviewed changes

douzzer assigned anhu Apr 6, 2026

Addressed 1 and 2.

7a9557c

anhu requested a review from douzzer April 6, 2026 21:12

anhu assigned wolfSSL-Bot and unassigned anhu and wolfSSL-Bot Apr 6, 2026

Conversation

anhu commented Mar 27, 2026

Uh oh!

douzzer commented Mar 30, 2026

Uh oh!

douzzer left a comment

Choose a reason for hiding this comment

Uh oh!

anhu commented Apr 1, 2026

Uh oh!

douzzer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

douzzer left a comment

Choose a reason for hiding this comment

PR Summary

Findings

1. PKCS7 padding check is not actually constant-time (ZD 21422 — both paths)

2. Session cache restore: ticketLen not sanitized (ZD 21423)

3. Incomplete wc_*_delete() coverage (ZD 21415)

4. MP_DECIMAL_MAX_LEN naming is misleading (ZD 21426)

5. Test callback oriDecryptCb_zd21414 has an unchecked OOB read (test-only)

6. No regression test for ZD 21413 (TLS 1.3 PQC key share over-read)

Fixes that look clean

Verdict

Uh oh!

anhu commented Apr 6, 2026

Uh oh!

anhu commented Apr 6, 2026

Uh oh!

anhu commented Apr 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

2. Session cache restore: `ticketLen` not sanitized (ZD 21423)

3. Incomplete `wc_*_delete()` coverage (ZD 21415)

4. `MP_DECIMAL_MAX_LEN` naming is misleading (ZD 21426)

5. Test callback `oriDecryptCb_zd21414` has an unchecked OOB read (test-only)