Skip to content

Crypto layer: Add missing input validation #10819

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
lealem47 wants to merge 1 commit into
base: master
Choose a base branch
from
+56 −14
Open

Crypto layer: Add missing input validation #10819

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
3 changes: 3 additions & 0 deletions wolfcrypt/src/dh.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1713,6 +1713,9 @@ int wc_DhCheckPubValue(const byte* prime, word32 primeSz, const byte* pub,
int ret = 0;
word32 i;

if (prime == NULL || pub == NULL)
return BAD_FUNC_ARG;

for (i = 0; i < pubSz && pub[i] == 0; i++) {
}
pubSz -= i;
Expand Down
22 changes: 15 additions & 7 deletions wolfcrypt/src/ecc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10982,16 +10982,24 @@ static int _ecc_import_x963_ex2(const byte* in, word32 inLen, ecc_key* key,
if (err == MP_OKAY) {
#ifdef HAVE_COMP_KEY
/* adjust inLen if compressed */
if (compressed)
inLen = inLen*2 + 1; /* used uncompressed len */
if (compressed) {
/* a compressed coordinate cannot exceed MAX_ECC_BYTES; bound it
* before doubling so inLen*2 + 1 cannot overflow word32. */
if (inLen > MAX_ECC_BYTES)
err = BAD_FUNC_ARG;
Comment thread
lealem47 marked this conversation as resolved.
else
inLen = inLen*2 + 1; /* used uncompressed len */
}
#endif

/* determine key size */
keysize = (int)(inLen>>1);
/* NOTE: FIPS v6.0.0 or greater, no restriction on imported keys, only
* on created keys or signatures */
err = wc_ecc_set_curve(key, keysize, curve_id);
key->type = ECC_PUBLICKEY;
if (err == MP_OKAY) {
keysize = (int)(inLen>>1);
/* NOTE: FIPS v6.0.0 or greater, no restriction on imported keys,
* only on created keys or signatures */
err = wc_ecc_set_curve(key, keysize, curve_id);
key->type = ECC_PUBLICKEY;
}
}

/* read data */
Expand Down
39 changes: 33 additions & 6 deletions wolfcrypt/src/kdf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,10 @@ int wc_PRF(byte* result, word32 resLen, const byte* secret,
Hmac hmac[1];
#endif

if ((result == NULL && resLen != 0) || (secret == NULL && secLen != 0) ||

@Frauschi Frauschi Jul 2, 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.

⚪ [Info] Non-conforming indentation in wc_PRF NULL-argument check
🔧 NIT style

The new NULL-check block uses 3-space continuation indentation and a 2-space indent on the return, unlike the 4-space style used consistently in the sibling checks added to wc_PRF_TLSv1 (lines 241-246) and wc_PRF_TLS (lines 305-310) in the same PR, and unlike the rest of the file. Aligning it keeps the newly added validation blocks uniform.

Suggestion:

Suggested change
if ((result == NULL && resLen != 0) || (secret == NULL && secLen != 0) ||
if ((result == NULL && resLen != 0) || (secret == NULL && secLen != 0) ||
(seed == NULL && seedLen != 0)) {
return BAD_FUNC_ARG;
}

(seed == NULL && seedLen != 0))
return BAD_FUNC_ARG;

switch (hash) {
#ifndef NO_MD5
case md5_mac:
Expand Down Expand Up @@ -234,8 +238,18 @@ int wc_PRF_TLSv1(byte* digest, word32 digLen, const byte* secret,
WC_DECLARE_VAR(sha_result, byte, MAX_PRF_DIG, heap); /* digLen is real size */
WC_DECLARE_VAR(labelSeed, byte, MAX_PRF_LABSEED, heap);

if ((digest == NULL && digLen != 0) ||
(secret == NULL && secLen != 0) ||
(label == NULL && labLen != 0) ||
(seed == NULL && seedLen != 0)) {
return BAD_FUNC_ARG;
}

/* labLen + seedLen is checked with subtraction to avoid word32 wraparound
* (the labLen bound first ensures MAX_PRF_LABSEED - labLen cannot
* underflow). */
if (half > MAX_PRF_HALF ||
labLen + seedLen > MAX_PRF_LABSEED ||
labLen > MAX_PRF_LABSEED || seedLen > (MAX_PRF_LABSEED - labLen) ||
digLen > MAX_PRF_DIG)
{
return BUFFER_E;
Comment thread
lealem47 marked this conversation as resolved.
Expand All @@ -251,8 +265,10 @@ int wc_PRF_TLSv1(byte* digest, word32 digLen, const byte* secret,
sha_half = secret + half - secLen % 2;
md5_result = digest;

XMEMCPY(labelSeed, label, labLen);
XMEMCPY(labelSeed + labLen, seed, seedLen);
if (labLen != 0)
XMEMCPY(labelSeed, label, labLen);
if (seedLen != 0)
XMEMCPY(labelSeed + labLen, seed, seedLen);

if ((ret = wc_PRF(md5_result, digLen, md5_half, half, labelSeed,
labLen + seedLen, md5_mac, heap, devId)) == 0) {
Expand Down Expand Up @@ -286,6 +302,13 @@ int wc_PRF_TLS(byte* digest, word32 digLen, const byte* secret, word32 secLen,
{
int ret = 0;

if ((digest == NULL && digLen != 0) ||
(secret == NULL && secLen != 0) ||
(label == NULL && labLen != 0) ||
(seed == NULL && seedLen != 0)) {
return BAD_FUNC_ARG;
}

#ifdef WOLFSSL_DEBUG_TLS
WOLFSSL_MSG(" secret");
WOLFSSL_BUFFER(secret, secLen);
Expand All @@ -298,15 +321,19 @@ int wc_PRF_TLS(byte* digest, word32 digLen, const byte* secret, word32 secLen,
if (useAtLeastSha256) {
WC_DECLARE_VAR(labelSeed, byte, MAX_PRF_LABSEED, 0);

if (labLen + seedLen > MAX_PRF_LABSEED) {
/* Checked with subtraction to avoid word32 wraparound of
* labLen + seedLen. */
if (labLen > MAX_PRF_LABSEED || seedLen > (MAX_PRF_LABSEED - labLen)) {
return BUFFER_E;
Comment thread
lealem47 marked this conversation as resolved.
}

WC_ALLOC_VAR_EX(labelSeed, byte, MAX_PRF_LABSEED, heap,
DYNAMIC_TYPE_DIGEST, return MEMORY_E);

XMEMCPY(labelSeed, label, labLen);
XMEMCPY(labelSeed + labLen, seed, seedLen);
if (labLen != 0)
XMEMCPY(labelSeed, label, labLen);
if (seedLen != 0)
XMEMCPY(labelSeed + labLen, seed, seedLen);

/* If a cipher suite wants an algorithm better than sha256, it
* should use better. */
Expand Down
6 changes: 5 additions & 1 deletion wolfcrypt/src/rsa.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1825,7 +1825,7 @@ static int RsaUnPad_OAEP(byte *pkcsBlock, unsigned int pkcsBlockLen,
if (ret != 0) {
ForceZero(tmp, hLen);
#ifdef WOLFSSL_SMALL_STACK
XFREE(tmp, NULL, DYNAMIC_TYPE_RSA_BUFFER);
XFREE(tmp, heap, DYNAMIC_TYPE_RSA_BUFFER);
#elif defined(WOLFSSL_CHECK_MEM_ZERO)
wc_MemZero_Check(tmp, hLen);
#endif
Expand Down Expand Up @@ -5412,7 +5412,11 @@ int wc_MakeRsaKey(RsaKey* key, int size, long e, WC_RNG* rng)
goto out;
}

#if defined(HAVE_FIPS)

@Frauschi Frauschi Jul 2, 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] FIPS RSA exponent floor reuses WC_RSA_EXPONENT default macro
💡 SUGGEST question

The new FIPS branch rejects e < WC_RSA_EXPONENT. WC_RSA_EXPONENT is defined in wolfssl/wolfcrypt/rsa.h as the default public exponent (65537L) and is user-overridable via -DWC_RSA_EXPONENT. It defaults to the correct FIPS minimum (2^16+1), so in the common case this is right. But repurposing the "default exponent" macro as a "minimum allowed exponent" for FIPS couples two unrelated concepts: if a user overrides WC_RSA_EXPONENT to a smaller value (e.g. 3), the FIPS floor silently weakens below the FIPS-required 65537, and if they raise it, valid FIPS keys could be rejected. A hardcoded literal (65537L) or a dedicated WC_RSA_MIN_EXPONENT-style macro would express the FIPS constraint unambiguously.

Suggestion:

Suggested change
#if defined(HAVE_FIPS)
#if defined(HAVE_FIPS)
if (e < 65537L || (e & 1) == 0) {
#else
if (e < 3 || (e & 1) == 0) {
#endif

if (e < WC_RSA_EXPONENT || (e & 1) == 0) {
#else
if (e < 3 || (e & 1) == 0) {
#endif
err = BAD_FUNC_ARG;
goto out;
}
Expand Down
Loading