Skip to content

Fixes from Fenrir review #420

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

Merged
ColtonWilley merged 1 commit into from
Jun 30, 2026
Merged

Fixes from Fenrir review #420

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
67 changes: 49 additions & 18 deletions src/wp_drbg.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,9 @@ static void wp_drbg_free(wp_DrbgCtx* ctx)
}

static int wp_drbg_uninstantiate(wp_DrbgCtx* ctx);
static int wp_drbg_reseed(wp_DrbgCtx* ctx, int predResist,
const unsigned char* entropy, size_t entropyLen,
const unsigned char* addIn, size_t addInLen);

/**
* Instantiate a new DRBG.
Expand Down Expand Up @@ -295,12 +298,20 @@ static int wp_drbg_uninstantiate(wp_DrbgCtx* ctx)
* Generate random data.
*
* @param [in, out] ctx DRBG context object.
* @param [out] out Buffer to hold generated random data.
* @param [in] outLen Number of random bytes to generate.
* @param [in] strength Strength in bits required.
* @param [in] predResist Prediction resistance required.
* @param [in] addIn Additional input data to seed with.
* @param [in] predResist Prediction resistance required. When set, the
* DRBG is reseeded with fresh entropy before
* generating (SP 800-90A 9.3.1). Honored only when
* built with WP_HAVE_DRBG_RESEED; otherwise
* ignored.
* @param [in] addIn Additional input data. Ignored: wolfCrypt has no
* public generate API that accepts additional
* input on a live WC_RNG.
* @param [in] addInLen Length of additional input data in bytes.
* @return 1 on success.
* @return 0 on success.
* @return 0 on failure.
*/
static int wp_drbg_generate(wp_DrbgCtx* ctx, unsigned char* out,
size_t outLen, unsigned int strength, int predResist,
Expand All @@ -311,8 +322,6 @@ static int wp_drbg_generate(wp_DrbgCtx* ctx, unsigned char* out,

WOLFPROV_ENTER(WP_LOG_COMP_RNG, "wp_drbg_generate");

(void)predResist;

if (strength > WP_DRBG_STRENGTH) {
ok = 0;
}
Expand All @@ -326,29 +335,50 @@ static int wp_drbg_generate(wp_DrbgCtx* ctx, unsigned char* out,
ok = 0;
}
#endif
#if 0

if (ok && (outLen > 0xFFFFFFFFU)) {
WOLFPROV_MSG_DEBUG(WP_LOG_COMP_RNG, "Request length is too big");
ok = 0;
}

/* wolfCrypt exposes no public generate API that accepts additional input on
* a live WC_RNG (wc_RNG_GenerateBlock passes NULL to Hash_DRBG_Generate),
* so addIn must be ignored. Per SP 800-90A additional_input on Generate is
* optional, so dropping it is standards-compliant. */
if (ok && (addInLen > 0)) {
rc = wc_RNG_DRBG_Reseed(ctx->rng, addIn, addInLen);
if (rc != 0) {
WOLFPROV_MSG_DEBUG(WP_LOG_COMP_RNG, "Additional data ignored");
(void)addIn;
}

/* SP 800-90A 9.3.1: prediction resistance requires reseeding with fresh
* entropy immediately before generating. wolfCrypt's public generate API
* has no PR flag, so emulate it via the in-place reseed path (fresh OS
* entropy, no caller-supplied material). Only available where a true
* reseed exists; without it the fallback re-instantiates the whole DRBG,
* so predResist is ignored. */
#ifdef WP_HAVE_DRBG_RESEED
if (ok && predResist) {
if (!wp_drbg_reseed(ctx, 1, NULL, 0, NULL, 0)) {
WOLFPROV_MSG_DEBUG(WP_LOG_COMP_RNG,
"Prediction resistance reseed failed");
ok = 0;
}
}
#else
(void)addIn;
(void)addInLen;
(void)predResist;
#endif
if (ok && (outLen > 0xFFFFFFFFU)) {
ok = 0;
}

if (ok) {
rc = wc_RNG_GenerateBlock(ctx->rng, out, (word32)outLen);
if (rc != 0) {
WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_COMP_RNG, "wc_RNG_GenerateBlock", rc);
WOLFPROV_MSG_DEBUG_RETCODE(WP_LOG_COMP_RNG, "wc_RNG_GenerateBlock",
rc);
ok = 0;
}
}

WOLFPROV_LEAVE(WP_LOG_COMP_RNG, __FILE__ ":" WOLFPROV_STRINGIZE(__LINE__), ok);
WOLFPROV_LEAVE(WP_LOG_COMP_RNG, __FILE__ ":" WOLFPROV_STRINGIZE(__LINE__),
ok);
return ok;
}

Expand All @@ -366,7 +396,7 @@ static int wp_drbg_generate(wp_DrbgCtx* ctx, unsigned char* out,
* @param [in] addInLen Length of additional input data in bytes.
* @param [in] params Other parameters.
* @return 1 on success.
* @return 0 on success.
* @return 0 on failure.
*/
static int wp_drbg_reseed(wp_DrbgCtx* ctx, int predResist,
const unsigned char* entropy, size_t entropyLen,
Expand Down Expand Up @@ -504,7 +534,7 @@ static int wp_drbg_reseed(wp_DrbgCtx* ctx, int predResist,
*
* @param [in, out] ctx DRBG context object.
* @return 1 on success.
* @return 0 on success.
* @return 0 on failure.
*/
static int wp_drbg_enable_locking(wp_DrbgCtx* ctx)
{
Expand Down Expand Up @@ -539,7 +569,7 @@ static int wp_drbg_enable_locking(wp_DrbgCtx* ctx)
*
* @param [in, out] ctx DRBG context object.
* @return 1 on success.
* @return 0 on success.
* @return 0 on failure.
*/
static int wp_drbg_lock(wp_DrbgCtx* ctx)
{
Expand Down Expand Up @@ -690,6 +720,7 @@ static int wp_drbg_set_ctx_params(wp_DrbgCtx* ctx, const OSSL_PARAM params[])
*
* @param [in] ctx DRBG context object. Unused.
* @return 1 on success.
* @return 0 on failure.
*/
static int wp_drbg_verify_zeroization(wp_DrbgCtx* ctx)
{
Expand Down
58 changes: 54 additions & 4 deletions src/wp_pbkdf2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,17 @@
OSSL_PARAM_uint64(OSSL_KDF_PARAM_ITER, NULL)


/* SP 800-132 lower bounds applied for the PKCS5=0 opt-in, matching OpenSSL's
* pbkdf2 KDF (providers/implementations/kdfs/pbkdf2.c). Only key length, salt
* length and iteration count are enforced. OpenSSL's handling of a minimum
* password length varies across versions (e.g. 3.5 accepts an empty password
* even with the checks enabled), so that bound is intentionally not applied
* here. FIPS enforcement is left to wolfCrypt's wc_PBKDF2_ex. */
#define WP_PBKDF2_MIN_KEY_LEN_BITS 112
#define WP_PBKDF2_MIN_ITERATIONS 1000
#define WP_PBKDF2_MIN_SALT_LEN (128 / 8)


/**
* The PBKDF2 context structure.
* Includes everything for PBKDF2.
Expand All @@ -64,7 +75,7 @@ typedef struct wp_Pbkdf2Ctx {
size_t saltSz;
/** Number of iterations. */
uint64_t iterations;
/** Used for PKCS#5. */
/** SP 800-132 lower-bound checks enabled (set when PKCS5 parameter is 0). */
int pkcs5;
/** PKCS12 key usage byte used in derivation. */
int keyUse;
Expand Down Expand Up @@ -263,6 +274,29 @@ static int wp_kdf_pbkdf2_derive(wp_Pbkdf2Ctx* ctx, unsigned char* key,
ok = 0;
}

/* PKCS5 == 0 opts into the SP 800-132 lower bounds, matching OpenSSL's
* pbkdf2 KDF: minimum key length, salt length and iteration count. The
* minimum-password-length check is omitted as OpenSSL's behaviour there is
* version-dependent. Applied here for the non-FIPS opt-in case; in a FIPS
* build the equivalent SP 800-132 checks are performed inside
* wc_PBKDF2_ex, so they are not duplicated here. */
if (ok && ctx->pkcs5) {
if ((keyLen * 8) < WP_PBKDF2_MIN_KEY_LEN_BITS) {
WOLFPROV_MSG_DEBUG(WP_LOG_COMP_PBKDF2, "PBKDF2 key size too small");
ok = 0;
}
if (ok && (ctx->saltSz < WP_PBKDF2_MIN_SALT_LEN)) {
WOLFPROV_MSG_DEBUG(WP_LOG_COMP_PBKDF2, "PBKDF2 salt too small");
ok = 0;
}
if (ok && (ctx->iterations < WP_PBKDF2_MIN_ITERATIONS)) {
WOLFPROV_MSG_DEBUG(WP_LOG_COMP_PBKDF2,
"PBKDF2 iteration count too low");
ok = 0;
}
}
/* wc_PBKDF2_ex fails when ctx->iterations is 0. */

if (ok) {
int rc;

Expand Down Expand Up @@ -297,8 +331,20 @@ static int wp_kdf_pbkdf2_set_ctx_params(wp_Pbkdf2Ctx* ctx,
WOLFPROV_ENTER(WP_LOG_COMP_PBKDF2, "wp_kdf_pbkdf2_set_ctx_params");

ok = wp_pbkdf2_base_set_ctx_params(ctx, params);
if (ok && !wp_params_get_int(params, OSSL_KDF_PARAM_PKCS5, &ctx->pkcs5)) {
ok = 0;
if (ok && (params != NULL)) {
const OSSL_PARAM* p = OSSL_PARAM_locate_const(params,
OSSL_KDF_PARAM_PKCS5);
if (p != NULL) {
int pkcs5;
if (!OSSL_PARAM_get_int(p, &pkcs5)) {
ok = 0;
}
else {
/* PKCS5 == 0 opts into the SP 800-132 lower-bound checks; a
* non-zero (legacy PKCS#5) value disables them. */
ctx->pkcs5 = (pkcs5 == 0);
}
}
}

WOLFPROV_LEAVE(WP_LOG_COMP_PBKDF2, __FILE__ ":" WOLFPROV_STRINGIZE(__LINE__), ok);
Expand Down Expand Up @@ -382,10 +428,14 @@ static int wp_kdf_pkcs12_derive(wp_Pbkdf2Ctx* ctx, unsigned char* key,

if (ok) {
int rc;
/* OpenSSL's PKCS12KDF derive loop always runs at least once, so an
* iteration count of 0 produces the same output as 1. wolfCrypt
* rejects iterations <= 0, so map 0 to 1 to match OpenSSL. */
int iterations = (ctx->iterations == 0) ? 1 : (int)ctx->iterations;

PRIVATE_KEY_UNLOCK();
rc = wc_PKCS12_PBKDF_ex(key, ctx->password, (int)ctx->passwordSz,
ctx->salt, (int)ctx->saltSz, (int)ctx->iterations, (int)keyLen,
ctx->salt, (int)ctx->saltSz, iterations, (int)keyLen,
ctx->mdType, ctx->keyUse, NULL);
PRIVATE_KEY_LOCK();
if (rc != 0) {
Expand Down
1 change: 1 addition & 0 deletions test/include.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ test_unit_test_SOURCES = \
test/test_kbkdf.c \
test/test_logging.c \
test/test_pbe.c \
test/test_pbkdf2.c \
test/test_pkey.c \
test/test_pkcs7_x509.c \
test/test_mlkem.c \
Expand Down
Loading
Loading