Skip to content

20260729 Coverity fixes #11006

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
rlm2002 wants to merge 3 commits into
base: master
Choose a base branch
from
+107 −8
Open

20260729 Coverity fixes #11006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c7af8bd
prevent uninitialized scalar variable issue
rlm2002 Jul 27, 2026
44f582a
Lock globalRng before access if HAVE_GLOBAL_RNG and OPENSSL_EXTRA are…
rlm2002 Jul 28, 2026
460f447
cap derLen at largest valid DER size
rlm2002 Jul 28, 2026
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
6 changes: 6 additions & 0 deletions src/pk_rsa.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,12 @@ static int wolfssl_read_der_bio(WOLFSSL_BIO* bio, unsigned char** out)
WOLFSSL_ERROR_MSG("DER SEQUENCE decode failed");
err = 1;
}
/* Cap at 8x the maximum modulus size, leaves headroom for the full

@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 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));

@rlm2002 rlm2002 Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

New test has been added see test_wolfSSL_d2i_RSAPrivateKey_bio_oversized()

* private key encoding. */
if ((!err) && (derLen > RSA_MAX_SIZE)) {
WOLFSSL_ERROR_MSG("DER length too large");
err = 1;
}
/* Allocate a buffer to read DER data into. */
if ((!err) && ((der = (unsigned char*)XMALLOC((size_t)derLen, bio->heap,
DYNAMIC_TYPE_TMP_BUFFER)) == NULL)) {
Expand Down
28 changes: 26 additions & 2 deletions src/ssl_sess.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2165,13 +2165,37 @@ void AddSession(WOLFSSL* ssl)
if (ssl->rng != NULL)
rng = ssl->rng;
#if defined(HAVE_GLOBAL_RNG) && defined(OPENSSL_EXTRA)

@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] 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;
}
}

@rlm2002 rlm2002 Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed thanks.

else if (initGlobalRNG == 1 || wolfSSL_RAND_Init() == WOLFSSL_SUCCESS) {
else if (initGlobalRNG == 1 ||
wolfSSL_RAND_Init() == WOLFSSL_SUCCESS) {
rng = &globalRNG;
}
if (rng == &globalRNG) {
if (wc_LockMutex(&globalRNGMutex) != 0) {
WOLFSSL_MSG("Bad Lock Mutex rng");
return;
}
/* The above access requires initGlobalRNG recheck now
* that we have the lock. */
if (initGlobalRNG == 0) {
wc_UnLockMutex(&globalRNGMutex);
return;
}
}
#endif
if (wc_RNG_GenerateBlock(rng, ssl->session->altSessionID,
ID_LEN) != 0)
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
ssl->session->haveAltSessionID = 1;
id = ssl->session->altSessionID;
idSz = ID_LEN;
Expand Down
65 changes: 65 additions & 0 deletions tests/api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20491,6 +20491,68 @@ static int test_wolfSSL_d2i_PrivateKeys_bio(void)
}
#endif /* OPENSSL_ALL || (WOLFSSL_ASIO && !NO_RSA) */

#if defined(OPENSSL_EXTRA) && !defined(NO_RSA) && defined(WOLFSSL_KEY_GEN) && \
defined(USE_WOLFSSL_MEMORY) && !defined(WOLFSSL_NO_MALLOC) && \
!defined(WOLFSSL_STATIC_MEMORY) && !defined(WOLFSSL_DEBUG_MEMORY)
/* Counts and refuses allocations >= big_alloc_threshold, to observe the
* oversized DER allocation in wolfssl_read_der_bio(). */
static size_t big_alloc_threshold = 0; /* 0 = disabled */
static int big_alloc_attempts = 0;

static void* big_malloc_cb(size_t size)
{
if (big_alloc_threshold != 0 && size >= big_alloc_threshold) {
big_alloc_attempts++;
return NULL; /* refuse; records the attempt */
}
return malloc(size);
}
#endif /* memory-hook guards */

/* Oversized-DER cap in wolfssl_read_der_bio(): a SEQUENCE far over the cap must
* be rejected before allocation. NULL alone doesn't prove it (uncapped returns
* NULL too), so refuse large allocations and assert none was attempted. */
static int test_wolfSSL_d2i_RSAPrivateKey_bio_oversized(void)
{
EXPECT_DECLS;
#if defined(OPENSSL_EXTRA) && !defined(NO_RSA) && defined(WOLFSSL_KEY_GEN) && \
defined(USE_WOLFSSL_MEMORY) && !defined(WOLFSSL_NO_MALLOC) && \
!defined(WOLFSSL_STATIC_MEMORY) && !defined(WOLFSSL_DEBUG_MEMORY)
/* SEQUENCE, canonical 4-byte length 0x01000000 (16 MB), over the cap.
* Must be canonical -- 0x00FFFFFF is rejected by the parser first. */
static const unsigned char hugeSeq[] =
{ 0x30, 0x84, 0x01, 0x00, 0x00, 0x00 };
BIO* bio = NULL;
RSA* rsa = NULL;
wolfSSL_Malloc_cb prev_mc = NULL;
wolfSSL_Free_cb prev_fc = NULL;
wolfSSL_Realloc_cb prev_rc = NULL;
int allocators_set = 0;

ExpectIntEQ(wolfSSL_GetAllocators(&prev_mc, &prev_fc, &prev_rc), 0);
ExpectIntEQ(wolfSSL_SetAllocators(big_malloc_cb, prev_fc, prev_rc), 0);
if (EXPECT_SUCCESS())
allocators_set = 1;

ExpectNotNull(bio = BIO_new(BIO_s_mem()));
ExpectIntGT(BIO_write(bio, hugeSeq, (int)sizeof(hugeSeq)), 0);

big_alloc_attempts = 0;
big_alloc_threshold = 0x10000;
ExpectNull(d2i_RSAPrivateKey_bio(bio, &rsa));
big_alloc_threshold = 0;

ExpectIntEQ(big_alloc_attempts, 0);

BIO_free(bio);
RSA_free(rsa);

if (allocators_set)
(void)wolfSSL_SetAllocators(prev_mc, prev_fc, prev_rc);
#endif
return EXPECT_RESULT();
}

#endif /* !NO_BIO */


Expand Down Expand Up @@ -38233,6 +38295,9 @@ TEST_CASE testCases[] = {
TEST_DECL(test_wolfSSL_d2i_PrivateKeys_bio),
#endif /* !NO_BIO */
#endif
#ifndef NO_BIO
TEST_DECL(test_wolfSSL_d2i_RSAPrivateKey_bio_oversized),
#endif /* !NO_BIO */


#if !defined(NO_WOLFSSL_CLIENT) && !defined(NO_WOLFSSL_SERVER)
Expand Down
16 changes: 10 additions & 6 deletions tests/api/test_coding.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,18 +330,22 @@ int test_wc_Base64_EncodeDecisionCoverage(void)
byte enc[128];
word32 i;
int nlCount = 0;

XMEMSET(enc, 0, sizeof(enc));
for (i = 0; i < (word32)sizeof(in48); i++)
in48[i] = (byte)(i + 1);
outLen = (word32)sizeof(enc);

@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] 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));

@rlm2002 rlm2002 Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thanks.

ExpectIntEQ(Base64_Encode(in48, (word32)sizeof(in48), enc, &outLen),
0);
for (i = 0; i < outLen; i++) {
if (enc[i] == '\n')
nlCount++;
if (EXPECT_SUCCESS()) {
for (i = 0; i < outLen; i++) {
if (enc[i] == '\n')
nlCount++;
}
/* exactly one (trailing) newline -- none inserted mid-stream */
ExpectIntEQ(nlCount, 1);
ExpectIntEQ(enc[outLen - 1], '\n');
}
/* exactly one (trailing) newline -- none inserted mid-stream */
ExpectIntEQ(nlCount, 1);
ExpectIntEQ(enc[outLen - 1], '\n');
}

/* --- force a BUFFER_E from CEscape() inside the *main* while loop
Expand Down
Loading