Fix multiple issues in XMSS.

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;

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)

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..<subtree height>. */
     word8     offset;
@@ -2702,6 +2703,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 +2733,32 @@ 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;
+        }
+        /* 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
+         * 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 +3021,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 +3049,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 +3073,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 +3183,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++;

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_ERRNO, 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;