20260729 Coverity fixes by rlm2002 · Pull Request #11006 · wolfSSL/wolfssl

rlm2002 · 2026-07-29T19:46:14Z

Description

CID 561836: Untrusted value as argument - Bound untrusted DER length before allocation in wolfssl_read_der_bio
CID 561978: Uninitialized scalar variable - Initialize enc in test_coding.c to avoid use before set.
CID 562025: Data race condition - Lock globalRNG in AddSession when falling back to the global RNG

Testing

./configure --enable-jni && make check

github-actions · 2026-07-29T19:58:47Z

MemBrowse Memory Report

gcc-arm-cortex-m4-openssl-compat

FLASH: .rodata +24 B, .text +64 B (+0.0%, 771,500 B / 1,048,576 B, total: 74% used)
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-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: REQUEST_CHANGES
Findings: 12 total — 4 posted, 8 skipped

Posted findings

[High] globalRNGMutex leaked when wc_RNG_GenerateBlock() fails in AddSession() — src/ssl_sess.c:2172-2185
[Medium] initGlobalRNG is not re-checked after taking the lock (documented racy pattern elsewhere) — src/ssl_sess.c:2167-2177
[Medium] No test exercises the new oversized-DER rejection path — src/pk_rsa.c:654-659
[Low] test_coding.c change guards the read but does not initialize enc as the PR describes — tests/api/test_coding.c:328-347

Skipped findings

[High] globalRNGMutex leaked on wc_RNG_GenerateBlock failure path in AddSession
[High] globalRNGMutex left locked when wc_RNG_GenerateBlock() fails in AddSession()
[Medium] DER size cap expression (RSA_MAX_SIZE / 8) * 8 reduces to RSA_MAX_SIZE and contradicts its comment
[Medium] AddSession() global-RNG locking path has no regression test
[Medium] initGlobalRNG not re-checked after acquiring globalRNGMutex in AddSession (races wolfSSL_RAND_Cleanup)
[Low] Misindented return block does not match wolfSSL brace/indent style
[Info] DER cap expression (RSA_MAX_SIZE / 8) * 8 is a no-op and its comment contradicts the code
[Info] New error-return block in AddSession() uses non-conforming indentation

Review generated by Skoll via Claude/Codex

Frauschi · 2026-07-31T10:16:21Z

+                 wolfSSL_RAND_Init() == WOLFSSL_SUCCESS) {
            rng = &globalRNG;
        }
+        if (rng == &globalRNG && wc_LockMutex(&globalRNGMutex) != 0) {


🟠 [High] globalRNGMutex leaked when wc_RNG_GenerateBlock() fails in AddSession()
🚫 BLOCK bug

The new locking added by this PR takes globalRNGMutex before wc_RNG_GenerateBlock() but only releases it on the success path. If wc_RNG_GenerateBlock() returns non-zero (e.g. DRBG reseed failure, entropy source failure, WC_NO_ERR_TRACE(RNG_FAILURE_E)), the function does a bare return; while still holding the mutex. globalRNGMutex is a plain (non-recursive, non-owner-tracked) wolfSSL_Mutex, so every subsequent user of the global RNG in the process — wolfSSL_RAND_bytes(), wolfSSL_RAND_seed(), wolfSSL_RAND_Cleanup(), wolfSSL_d2i_PKCS12_bio paths in src/ssl_p7p12.c:1563, and AddSession() itself — blocks forever on wc_LockMutex(). wolfSSL_Cleanup() (src/ssl.c:3726) will also fail to wc_FreeMutex() a still-held mutex. Before this PR the same error path simply returned with no lock held, so this deadlock is introduced by the change. Note the pre-existing early return on the lock-failure branch is fine (nothing held), but the RNG-failure return is not.

Suggestion:

Suggested change

if (rng == &globalRNG && wc_LockMutex(&globalRNGMutex) != 0) {

if (wc_RNG_GenerateBlock(rng, ssl->session->altSessionID,

ID_LEN) != 0) {

#if defined(HAVE_GLOBAL_RNG) && defined(OPENSSL_EXTRA)

if (rng == &globalRNG) {

wc_UnLockMutex(&globalRNGMutex);

}

#endif

return;

}

#if defined(HAVE_GLOBAL_RNG) && defined(OPENSSL_EXTRA)

if (rng == &globalRNG) {

wc_UnLockMutex(&globalRNGMutex);

}

#endif

Fixed thanks.

Frauschi · 2026-07-31T10:16:21Z

@@ -2165,13 +2165,24 @@ void AddSession(WOLFSSL* ssl)
        if (ssl->rng != NULL)
            rng = ssl->rng;
 #if defined(HAVE_GLOBAL_RNG) && defined(OPENSSL_EXTRA)


🟡 [Medium] initGlobalRNG is not re-checked after taking the lock (documented racy pattern elsewhere)
💡 SUGGEST bug

The decision rng = &globalRNG is made from an unlocked read of initGlobalRNG, and the mutex is only taken afterwards. wolfSSL_RAND_Cleanup() (src/ssl_crypto.c:3980-3985) calls wc_FreeRng(&globalRNG) and clears initGlobalRNG under that same mutex, so a concurrent cleanup that lands between the unlocked read and wc_LockMutex() leaves this code generating from a freed RNG. The existing in-tree pattern explicitly handles this — see src/ssl_crypto.c:4071-4106, which comments "the above access to initGlobalRNG is racey -- recheck it now that we have the lock" and re-tests initGlobalRNG while holding the mutex. Since this PR is specifically a data-race fix for this call site, it is worth closing the same window rather than leaving a narrower version of it.

Suggestion:

Suggested change

#if defined(HAVE_GLOBAL_RNG) && defined(OPENSSL_EXTRA)

if (rng == &globalRNG) {

if (wc_LockMutex(&globalRNGMutex) != 0) {

WOLFSSL_MSG("Bad Lock Mutex rng");

return;

}

/* the above access to initGlobalRNG is racey -- recheck it now

* that we have the lock. */

if (initGlobalRNG == 0) {

wc_UnLockMutex(&globalRNGMutex);

return;

}

}

Fixed thanks.

Frauschi · 2026-07-31T10:16:21Z

        WOLFSSL_ERROR_MSG("DER SEQUENCE decode failed");
        err = 1;
    }
+    /* Cap at 8x the maximum modulus size, leaves headroom for the full


🟡 [Medium] No test exercises the new oversized-DER rejection path
💡 SUGGEST test

The only existing coverage for this code path is tests/api.c:20400-20416 (d2i_RSAPrivateKey_bio with NULL args, an empty BIO, and a valid key). Nothing feeds a BIO whose outer SEQUENCE header declares a length above the new cap, so the added branch is never executed and a future refactor of the bound would go unnoticed. This is cheap to cover because only the SEQUENCE header needs to be well-formed — the body never gets read once the length check trips.

Suggestion:

Suggested change

/* Cap at 8x the maximum modulus size, leaves headroom for the full

/* SEQUENCE, 4-byte length = 0x00FFFFFF, followed by nothing: the header

* parses, the declared length exceeds the cap, so no allocation happens. */

static const byte hugeSeq[] = { 0x30, 0x84, 0x00, 0xFF, 0xFF, 0xFF,

0x00, 0x00, 0x00, 0x00 };

ExpectNotNull(bio = BIO_new_mem_buf(hugeSeq, (int)sizeof(hugeSeq)));

ExpectNull(d2i_RSAPrivateKey_bio(bio, &rsa));

New test has been added see test_wolfSSL_d2i_RSAPrivateKey_bio_oversized()

Frauschi · 2026-07-31T10:16:21Z

@@ -335,13 +335,15 @@ int test_wc_Base64_EncodeDecisionCoverage(void)
        outLen = (word32)sizeof(enc);


🔵 [Low] test_coding.c change guards the read but does not initialize enc as the PR describes
🔧 NIT question

The PR description states "Initialize enc in test_coding.c to avoid use before set" (CID 561978), but the change actually wraps the reads in if (EXPECT_SUCCESS()). The guard is functionally correct — EXPECT_SUCCESS() (tests/unit.h:157) is false once ExpectIntEQ on Base64_Encode fails, so enc and outLen are only read after a successful encode — but enc remains uninitialized, and Coverity's uninitialized-read checker generally does not model the _ret state machine, so the CID may well survive. Adding an explicit initializer is one line and makes the intent unambiguous for both the reader and the checker.

Suggestion:

Suggested change

outLen = (word32)sizeof(enc);

byte enc[128];

XMEMSET(enc, 0, sizeof(enc));

Fixed, thanks.

… defined prevent potential deadlock

Test oversized DER is rejected before allocation

rlm2002 self-assigned this Jul 29, 2026

rlm2002 assigned wolfSSL-Bot and unassigned rlm2002 Jul 29, 2026

Frauschi requested changes Jul 31, 2026

View reviewed changes

Frauschi assigned rlm2002 and unassigned wolfSSL-Bot Jul 31, 2026

rlm2002 added 3 commits July 31, 2026 17:40

prevent uninitialized scalar variable issue

c7af8bd

Lock globalRng before access if HAVE_GLOBAL_RNG and OPENSSL_EXTRA are…

44f582a

… defined prevent potential deadlock

cap derLen at largest valid DER size

460f447

Test oversized DER is rejected before allocation

rlm2002 force-pushed the coverity branch from 085948e to 460f447 Compare July 31, 2026 23:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

20260729 Coverity fixes - #11006

20260729 Coverity fixes#11006
rlm2002 wants to merge 3 commits into
wolfSSL:masterfrom
rlm2002:coverity

rlm2002 commented Jul 29, 2026

Uh oh!

github-actions Bot commented Jul 29, 2026 •

edited

Loading

Uh oh!

Frauschi left a comment

Uh oh!

Frauschi Jul 31, 2026

Uh oh!

rlm2002 Jul 31, 2026

Uh oh!

Frauschi Jul 31, 2026

Uh oh!

rlm2002 Jul 31, 2026

Uh oh!

Frauschi Jul 31, 2026

Uh oh!

rlm2002 Jul 31, 2026

Uh oh!

Frauschi Jul 31, 2026

Uh oh!

rlm2002 Jul 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-        if (rng == &globalRNG && wc_LockMutex(&globalRNGMutex) != 0) {
+if (wc_RNG_GenerateBlock(rng, ssl->session->altSessionID,
+                ID_LEN) != 0) {
+        #if defined(HAVE_GLOBAL_RNG) && defined(OPENSSL_EXTRA)
+            if (rng == &globalRNG) {
+                wc_UnLockMutex(&globalRNGMutex);
+            }
+        #endif
+            return;
+        }
+#if defined(HAVE_GLOBAL_RNG) && defined(OPENSSL_EXTRA)
+        if (rng == &globalRNG) {
+            wc_UnLockMutex(&globalRNGMutex);
+        }
+#endif

-#if defined(HAVE_GLOBAL_RNG) && defined(OPENSSL_EXTRA)
+if (rng == &globalRNG) {
+            if (wc_LockMutex(&globalRNGMutex) != 0) {
+                WOLFSSL_MSG("Bad Lock Mutex rng");
+                return;
+            }
+            /* the above access to initGlobalRNG is racey -- recheck it now
+             * that we have the lock. */
+            if (initGlobalRNG == 0) {
+                wc_UnLockMutex(&globalRNGMutex);
+                return;
+            }
+        }

-    /* Cap at 8x the maximum modulus size, leaves headroom for the full
+/* SEQUENCE, 4-byte length = 0x00FFFFFF, followed by nothing: the header
+ * parses, the declared length exceeds the cap, so no allocation happens. */
+static const byte hugeSeq[] = { 0x30, 0x84, 0x00, 0xFF, 0xFF, 0xFF,
+x00, 0x00, 0x00, 0x00 };
+ExpectNotNull(bio = BIO_new_mem_buf(hugeSeq, (int)sizeof(hugeSeq)));
+ExpectNull(d2i_RSAPrivateKey_bio(bio, &rsa));

		@@ -335,13 +335,15 @@ int test_wc_Base64_EncodeDecisionCoverage(void)
		outLen = (word32)sizeof(enc);

-        outLen = (word32)sizeof(enc);
+byte enc[128];
+        XMEMSET(enc, 0, sizeof(enc));

Uh oh!

Conversation

rlm2002 commented Jul 29, 2026

Description

Testing

Uh oh!

github-actions Bot commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

MemBrowse Memory Report

Uh oh!

Frauschi left a comment

Choose a reason for hiding this comment

🐺 Skoll Code Review

Posted findings

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Jul 29, 2026 •

edited

Loading