From e2dfbc019958c0e380f74806c0209046c5252ad9 Mon Sep 17 00:00:00 2001 From: Kareem Date: Mon, 17 Aug 2026 18:02:26 -0700 Subject: [PATCH 1/5] Fix multiple issues in XMSS. Thanks to 007bsd for the report. --- wolfcrypt/src/wc_xmss_impl.c | 48 ++++++++++++++++++++++-- wolfcrypt/test/test.c | 71 ++++++++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 4 deletions(-) diff --git a/wolfcrypt/src/wc_xmss_impl.c b/wolfcrypt/src/wc_xmss_impl.c index 7cd27a90e4f..36366ffbc4c 100644 --- a/wolfcrypt/src/wc_xmss_impl.c +++ b/wolfcrypt/src/wc_xmss_impl.c @@ -2702,6 +2702,8 @@ static int wc_xmss_bds_state_load(const XmssState* state, byte* sk, const word8 k = params->bds_k; const word32 retainLen = XMSS_RETAIN_LEN(k, n); int i; + int j; + word16 used; /* Skip past standard SK = idx || wots_sk || SK_PRF || root || SEED; */ sk += params->idx_len + 4 * n; @@ -2730,6 +2732,30 @@ static int wc_xmss_bds_state_load(const XmssState* state, byte* sk, sk += 3; bds[i].offset = sk[0]; sk += 1; + + /* Stack holds at most hs + 1 nodes. */ + if (bds[i].offset > (word8)(hs + 1)) { + return WC_FAILURE; + } + /* Leaf index must be within the subtree. */ + if (bds[i].next > ((word32)1U << hs)) { + return WC_FAILURE; + } + /* An update pops one stack node per node a tree hash uses. Tree hash + * j completes at height j, so it holds at most j nodes, and all of + * them are on the stack. */ + used = 0; + for (j = 0; j < (int)hsk; j++) { + word8 tu = (word8)(bds[i].treeHash[j * 4 + 3] & 0x7f); + + if (tu > j) { + return WC_FAILURE; + } + used = (word16)(used + tu); + } + if (used > bds[i].offset) { + return WC_FAILURE; + } } if (wots_sigs != NULL) { @@ -2992,10 +3018,17 @@ static void wc_xmss_bds_treehash_update(XmssState* state, BdsState* bds, const word8 n = params->n; HashAddress addrLocal; TreeHash treeHash[1]; - byte* sp = bds->stack + bds->offset * n; + byte* sp; byte* node = state->stack + WC_XMSS_MAX_STACK_LEN - n; word8 h; + /* Stack holds at most sub_h + 1 nodes. */ + if (bds->offset > (word8)(params->sub_h + 1)) { + state->ret = WC_FAILURE; + return; + } + sp = bds->stack + bds->offset * n; + /* Get the tree hash data. */ wc_xmss_bds_state_treehash_get(bds, height, treeHash); /* Copy hash address into local as OTS type. */ @@ -3013,7 +3046,8 @@ static void wc_xmss_bds_treehash_update(XmssState* state, BdsState* bds, h = 0; /* Top node on Stack has same height t' as node. */ - while ((treeHash->used > 0) && (h == bds->height[bds->offset - 1])) { + while ((treeHash->used > 0) && (bds->offset > 0) && + (h == bds->height[bds->offset - 1])) { sp -= n; /* Copy from stack to before last calculated node. */ node -= n; @@ -3036,6 +3070,11 @@ static void wc_xmss_bds_treehash_update(XmssState* state, BdsState* bds, XMEMCPY(bds->treeHashNode + height * n, node, n); treeHash->completed = 1; } + else if (bds->offset > params->sub_h) { + /* No slot left on the stack to push onto. */ + state->ret = WC_FAILURE; + return; + } else { /* Push calculated node onto stack. */ XMEMCPY(sp, node, n); @@ -3141,14 +3180,15 @@ static void wc_xmss_bds_update(XmssState* state, BdsState* bds, { if (bds->next < ((word32)1U << state->params->sub_h)) { const XmssParams* params = state->params; - byte* sp = bds->stack + bds->offset * params->n; + byte* sp; HashAddress addrCopy; XMSS_ADDR_OTS_SET_SUBTREE(addrCopy, addr); - if (bds->height == NULL) { + if ((bds->height == NULL) || (bds->offset > params->sub_h)) { state->ret = WC_FAILURE; return; } + sp = bds->stack + bds->offset * params->n; wc_xmss_bds_next_idx(state, bds, sk_seed, pk_seed, addrCopy, bds->next, bds->height, &bds->offset, &sp); bds->offset++; diff --git a/wolfcrypt/test/test.c b/wolfcrypt/test/test.c index 0556fd53513..4b03709c4dd 100644 --- a/wolfcrypt/test/test.c +++ b/wolfcrypt/test/test.c @@ -61443,6 +61443,9 @@ WOLFSSL_TEST_SUBROUTINE wc_test_ret_t xmss_test(void) #endif int ret2 = -1; int ret = WC_TEST_RET_ENC_NC; +#ifndef WOLFSSL_NO_MALLOC + byte * sk_snapshot = NULL; +#endif WOLFSSL_ENTER("xmss_test"); #ifndef HAVE_FIPS @@ -61551,6 +61554,17 @@ WOLFSSL_TEST_SUBROUTINE wc_test_ret_t xmss_test(void) ret = wc_XmssKey_Verify(&verifyKey, sig, sigSz, (byte *) msg, msgSz); if (ret != 0) { ERROR_OUT(WC_TEST_RET_ENC_I(i), out); } +#ifndef WOLFSSL_NO_MALLOC + /* Keep an early state - it still has a tree hash in progress, which + * is what the traversal counters below drive. */ + if ((i == 2) && (sk_snapshot == NULL)) { + sk_snapshot = (byte *)XMALLOC(skSz, HEAP_HINT, + DYNAMIC_TYPE_TMP_BUFFER); + if (sk_snapshot == NULL) { ERROR_OUT(WC_TEST_RET_ENC_NC, out); } + XMEMCPY(sk_snapshot, sk, skSz); + } +#endif + /* Flip bits in a few places throughout the signature, stepping in multiple * of hash size. These should all fail with -1. */ for (j = 0; j < (int) sigSz; j+= 4 * 32) { @@ -61568,10 +61582,67 @@ WOLFSSL_TEST_SUBROUTINE wc_test_ret_t xmss_test(void) } } +#ifndef WOLFSSL_NO_MALLOC + /* The BDS traversal counters sit at the end of the persisted private key + * and are re-parsed on every sign. Corrupt them and the library must come + * back with an error, having stayed inside the key's own buffers. */ + if (sk_snapshot != NULL) { + XmssKey reloadKey; + + /* One extra pass with the state left alone, to show that a good state + * is still accepted. */ + for (j = (int)skSz - 16; j <= (int)skSz; j++) { + XMEMCPY(sk, sk_snapshot, skSz); + if (j < (int)skSz) { + sk[j] = 0xc8; + } + + ret = wc_XmssKey_Init(&reloadKey, NULL, devId); + if (ret == 0) { + ret = wc_XmssKey_SetParamStr(&reloadKey, param); + } + if (ret == 0) { + ret = wc_XmssKey_SetWriteCb(&reloadKey, xmss_write_key_mem); + } + if (ret == 0) { + ret = wc_XmssKey_SetReadCb(&reloadKey, xmss_read_key_mem); + } + if (ret == 0) { + ret = wc_XmssKey_SetContext(&reloadKey, (void *) sk); + } + if (ret == 0) { + ret = wc_XmssKey_Reload(&reloadKey); + } + if (ret == 0) { + sigSz = bufSz; + ret = wc_XmssKey_Sign(&reloadKey, sig, &sigSz, (byte *) msg, + msgSz); + } + if ((ret == 0) && (j == (int)skSz)) { + /* Untouched state - the signature must be the real one. */ + ret = wc_XmssKey_Verify(&verifyKey, sig, sigSz, (byte *) msg, + msgSz); + } + wc_XmssKey_Free(&reloadKey); + + /* A corrupt state may sign or may fail; an untouched one may not + * fail. Under a sanitizer this is also what catches a write that + * left the key's buffers. */ + if ((ret != 0) && (j == (int)skSz)) { + ERROR_OUT(WC_TEST_RET_ENC_I(j), out); + } + } + ret = 0; + } +#endif /* !WOLFSSL_NO_MALLOC */ + out: /* Cleanup everything. */ #ifndef WOLFSSL_NO_MALLOC + XFREE(sk_snapshot, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); + sk_snapshot = NULL; + XFREE(sig, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); sig = NULL; From b069fb3209b6c913395d0968f7ee266a1dc3788f Mon Sep 17 00:00:00 2001 From: Kareem Date: Mon, 17 Aug 2026 18:10:06 -0700 Subject: [PATCH 2/5] Code review feedback --- wolfcrypt/src/wc_xmss_impl.c | 2 +- wolfcrypt/test/test.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/wolfcrypt/src/wc_xmss_impl.c b/wolfcrypt/src/wc_xmss_impl.c index 36366ffbc4c..bb7b5d719d6 100644 --- a/wolfcrypt/src/wc_xmss_impl.c +++ b/wolfcrypt/src/wc_xmss_impl.c @@ -2738,7 +2738,7 @@ static int wc_xmss_bds_state_load(const XmssState* state, byte* sk, return WC_FAILURE; } /* Leaf index must be within the subtree. */ - if (bds[i].next > ((word32)1U << hs)) { + if (bds[i].next >= ((word32)1U << hs)) { return WC_FAILURE; } /* An update pops one stack node per node a tree hash uses. Tree hash diff --git a/wolfcrypt/test/test.c b/wolfcrypt/test/test.c index 4b03709c4dd..d5c8e65b31a 100644 --- a/wolfcrypt/test/test.c +++ b/wolfcrypt/test/test.c @@ -61560,7 +61560,7 @@ WOLFSSL_TEST_SUBROUTINE wc_test_ret_t xmss_test(void) if ((i == 2) && (sk_snapshot == NULL)) { sk_snapshot = (byte *)XMALLOC(skSz, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER); - if (sk_snapshot == NULL) { ERROR_OUT(WC_TEST_RET_ENC_NC, out); } + if (sk_snapshot == NULL) { ERROR_OUT(WC_TEST_RET_ENC_ERRNO, out); } XMEMCPY(sk_snapshot, sk, skSz); } #endif From 3a7f17b2e6684e832631e791e6acefde909ec61d Mon Sep 17 00:00:00 2001 From: Kareem Date: Tue, 18 Aug 2026 16:42:46 -0700 Subject: [PATCH 3/5] Undo code review feedback on next check, add documentation. --- wolfcrypt/src/wc_xmss_impl.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/wolfcrypt/src/wc_xmss_impl.c b/wolfcrypt/src/wc_xmss_impl.c index bb7b5d719d6..0ff9c9ea95b 100644 --- a/wolfcrypt/src/wc_xmss_impl.c +++ b/wolfcrypt/src/wc_xmss_impl.c @@ -2543,7 +2543,8 @@ typedef struct BdsState { byte* treeHashNode; /* Hashes of nodes to retain - based on K parameter. */ byte* retain; - /* Next leaf to calculate - max 20 bits. */ + /* Next leaf to calculate - max 20 bits. Equals 2^sub_h when the subtree + * has been completed. */ word32 next; /* Current offset into stack - 0... */ word8 offset; @@ -2737,8 +2738,10 @@ static int wc_xmss_bds_state_load(const XmssState* state, byte* sk, if (bds[i].offset > (word8)(hs + 1)) { return WC_FAILURE; } - /* Leaf index must be within the subtree. */ - if (bds[i].next >= ((word32)1U << hs)) { + /* next counts leaves done rather than indexing one: it is only used + * as an index while below 2^hs and comes to rest at 2^hs when the + * subtree is complete. */ + if (bds[i].next > ((word32)1U << hs)) { return WC_FAILURE; } /* An update pops one stack node per node a tree hash uses. Tree hash From c5fd245aa338567f905fde02edd3b60c192b32ea Mon Sep 17 00:00:00 2001 From: Kareem Date: Tue, 18 Aug 2026 17:40:35 -0700 Subject: [PATCH 4/5] Add some bounds checks to LMS. --- wolfcrypt/src/wc_lms_impl.c | 59 +++++++++++++++++++++++++++++++++---- 1 file changed, 53 insertions(+), 6 deletions(-) diff --git a/wolfcrypt/src/wc_lms_impl.c b/wolfcrypt/src/wc_lms_impl.c index 15f7ddf8434..3a2133902f8 100644 --- a/wolfcrypt/src/wc_lms_impl.c +++ b/wolfcrypt/src/wc_lms_impl.c @@ -1787,8 +1787,10 @@ static int wc_lmots_sign(LmsState* state, const byte* seed, const byte* msg, * @param [in] params LMS parameters. * @param [out] state Private key state. * @param [in] priv_data Private key data. + * @return 0 on success. + * @return BUFFER_E when a stored index is out of range for the parameters. */ -static void wc_lms_priv_state_load(const LmsParams* params, LmsPrivState* state, +static int wc_lms_priv_state_load(const LmsParams* params, LmsPrivState* state, byte* priv_data) { /* Authentication path data. */ @@ -1812,6 +1814,21 @@ static void wc_lms_priv_state_load(const LmsParams* params, LmsPrivState* state, priv_data += 4; ato32(priv_data, &state->leaf.offset); /* priv_data += 4; */ + + /* Stack offset is a byte count into a stack of height + 1 nodes. + * leaf.idx is deliberately wrapped when the cache is empty - don't + * bound it. */ + if ((state->stack.offset > + LMS_STACK_CACHE_LEN(params->height, params->hash_len)) || + ((state->stack.offset % params->hash_len) != 0)) { + return BUFFER_E; + } + /* Leaf cache is a ring of 2^cacheBits nodes. */ + if (state->leaf.offset >= ((word32)1U << params->cacheBits)) { + return BUFFER_E; + } + + return 0; } /* Store the LMS private state into data. @@ -2321,6 +2338,7 @@ static int wc_lms_treehash_update(LmsState* state, LmsPrivState* privState, byte* temp = left + params->hash_len; WC_DECLARE_VAR(stack, byte, (LMS_MAX_HEIGHT + 1) * LMS_MAX_NODE_LEN, 0); byte* sp; + byte* spEnd; word32 max_cb = (word32)1 << params->cacheBits; word32 i; @@ -2338,6 +2356,7 @@ static int wc_lms_treehash_update(LmsState* state, LmsPrivState* privState, XMEMCPY(stack, stackCache->stack, (word32)params->height * params->hash_len); sp = stack + stackCache->offset; + spEnd = stack + LMS_STACK_CACHE_LEN(params->height, params->hash_len); } /* Compute all nodes requested. */ @@ -2393,6 +2412,11 @@ static int wc_lms_treehash_update(LmsState* state, LmsPrivState* privState, j >>= 1; h++; + /* Node to combine with must be on the stack. */ + if ((size_t)(sp - stack) < params->hash_len) { + ret = BUFFER_E; + break; + } sp -= params->hash_len; if (useRoot && (h > params->height - params->rootLevels) && (h <= params->height)) { @@ -2426,6 +2450,10 @@ static int wc_lms_treehash_update(LmsState* state, LmsPrivState* privState, params->hash_len); } } + if ((ret == 0) && ((size_t)(spEnd - sp) < params->hash_len)) { + /* No room on the stack to push onto. */ + ret = BUFFER_E; + } if (ret == 0) { /* Push temp onto the data stack. */ XMEMCPY(sp, temp, params->hash_len); @@ -3486,10 +3514,13 @@ static int wc_hss_presign(LmsState* state, HssPrivKey* priv_key) * @param [in] params LMS parameters. * @param [in, out] key HSS private key. * @param [in] priv_data Private key data. + * @return 0 on success. + * @return BUFFER_E when a stored index is out of range for the parameters. */ -static void wc_hss_priv_data_load(const LmsParams* params, HssPrivKey* key, +static int wc_hss_priv_data_load(const LmsParams* params, HssPrivKey* key, byte* priv_data) { + int ret = 0; #ifndef WOLFSSL_WC_LMS_SMALL int l; #endif @@ -3500,8 +3531,13 @@ static void wc_hss_priv_data_load(const LmsParams* params, HssPrivKey* key, #ifndef WOLFSSL_WC_LMS_SMALL for (l = 0; l < params->levels; l++) { - /* Caches for subtree. */ - wc_lms_priv_state_load(params, &key->state[l], priv_data); + /* Caches for subtree. Keep mapping the rest of the data even on a bad + * state so every pointer is set; the first error is returned. */ + int rc = wc_lms_priv_state_load(params, &key->state[l], priv_data); + + if (ret == 0) { + ret = rc; + } priv_data += LMS_PRIV_STATE_LEN(params->height, params->rootLevels, params->cacheBits, params->hash_len); } @@ -3512,7 +3548,11 @@ static void wc_hss_priv_data_load(const LmsParams* params, HssPrivKey* key, priv_data += LMS_PRIV_KEY_LEN(params->levels, params->hash_len); for (l = 0; l < params->levels - 1; l++) { /* Next subtree's caches. */ - wc_lms_priv_state_load(params, &key->next_state[l], priv_data); + int rc = wc_lms_priv_state_load(params, &key->next_state[l], priv_data); + + if (ret == 0) { + ret = rc; + } priv_data += LMS_PRIV_STATE_LEN(params->height, params->rootLevels, params->cacheBits, params->hash_len); } @@ -3523,6 +3563,8 @@ static void wc_hss_priv_data_load(const LmsParams* params, HssPrivKey* key, key->y = priv_data; #endif /* WOLFSSL_LMS_NO_SIG_CACHE */ #endif /* WOLFSSL_WC_LMS_SMALL */ + + return ret; } #ifndef WOLFSSL_WC_LMS_SMALL @@ -3571,6 +3613,8 @@ static void wc_hss_priv_data_store(const LmsParams* params, HssPrivKey* key, * @param [out] priv_data Private key data. * @param [out] pub_root Public key root node. * @return 0 on success. + * @return BAD_FUNC_ARG when the parameters would make a shift undefined. + * @return BUFFER_E when the stored state has an index out of range. */ int wc_hss_reload_key(LmsState* state, const byte* priv_raw, HssPrivKey* priv_key, byte* priv_data, byte* pub_root) @@ -3589,7 +3633,10 @@ int wc_hss_reload_key(LmsState* state, const byte* priv_raw, } #endif - wc_hss_priv_data_load(state->params, priv_key, priv_data); + /* Not returned on error here: only the no-root branch below uses the state + * as loaded. The others recompute it, over values that may be + * uninitialized. */ + ret = wc_hss_priv_data_load(state->params, priv_key, priv_data); #ifdef WOLFSSL_WC_LMS_SERIALIZE_STATE if (pub_root != NULL) From fce72fee35883b4e6e65810718fe180c73983d42 Mon Sep 17 00:00:00 2001 From: Kareem Date: Tue, 18 Aug 2026 17:55:53 -0700 Subject: [PATCH 5/5] Always initialize key priv_data. --- wolfcrypt/src/wc_lms.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/wolfcrypt/src/wc_lms.c b/wolfcrypt/src/wc_lms.c index 6ddb9164ec2..97e9ab029b2 100644 --- a/wolfcrypt/src/wc_lms.c +++ b/wolfcrypt/src/wc_lms.c @@ -1210,11 +1210,10 @@ int wc_LmsKey_MakeKey(LmsKey* key, WC_RNG* rng) if (key->priv_data == NULL) { ret = MEMORY_E; } - #ifdef WOLFSSL_WC_LMS_SERIALIZE_STATE else { + /* Loading reads the state before it is computed over. */ XMEMSET(key->priv_data, 0, priv_data_len); } - #endif } if (ret == 0) { WC_DECLARE_VAR(state, LmsState, 1, 0); @@ -1334,6 +1333,10 @@ int wc_LmsKey_Reload(LmsKey* key) if (key->priv_data == NULL) { ret = MEMORY_E; } + else { + /* Loading reads the state before it is computed over. */ + XMEMSET(key->priv_data, 0, priv_data_len); + } } if (ret == 0) { int rv;