From 9764f03004f4433ff12077d14400debd082a60b8 Mon Sep 17 00:00:00 2001 From: Jack Lloyd Date: Thu, 19 Feb 2026 03:03:02 -0500 Subject: [PATCH] Remove XMSS private key shared mutable state The private key had an XMSS_Hash that was reused for signatures, which meant if multiple threads tried to sign with the same key without some form of external locking, they would produce bad results. In general Botan's position is that using an object in multiple threads without locking is not supported. However this case is a bit subtle since the key is being passed as a const reference and there is no obvious reason it shouldn't work. And indeed it does (and likely has for many years) been ok to do this with ECDSA, RSA, and most other algorithms - for XMSS to be a special case here is certainly violating the principle of least astonishment. --- src/lib/pubkey/xmss/xmss.h | 14 +++----- src/lib/pubkey/xmss/xmss_privatekey.cpp | 34 +++++++------------ .../pubkey/xmss/xmss_signature_operation.cpp | 2 +- src/tests/test_concurrent_pk.cpp | 5 +-- 4 files changed, 19 insertions(+), 36 deletions(-) diff --git a/src/lib/pubkey/xmss/xmss.h b/src/lib/pubkey/xmss/xmss.h index e09db4b63c2..55576166d97 100644 --- a/src/lib/pubkey/xmss/xmss.h +++ b/src/lib/pubkey/xmss/xmss.h @@ -265,21 +265,17 @@ class BOTAN_PUBLIC_API(2, 0) XMSS_PrivateKey final : public virtual XMSS_PublicK * @param start_idx The start index. * @param target_node_height Height of the target node. * @param adrs Address of the tree containing the target node. + * @param hash The hash function to use * * @return The root node of a tree of height target_node height with the * leftmost leaf being the hash of the WOTS+ pk with index * start_idx. **/ - secure_vector tree_hash(size_t start_idx, size_t target_node_height, XMSS_Address& adrs); + secure_vector tree_hash(size_t start_idx, + size_t target_node_height, + XMSS_Address& adrs, + XMSS_Hash& hash); - void tree_hash_subtree(secure_vector& result, - size_t start_idx, - size_t target_node_height, - XMSS_Address& adrs); - - /** - * Helper for multithreaded tree hashing. - */ void tree_hash_subtree(secure_vector& result, size_t start_idx, size_t target_node_height, diff --git a/src/lib/pubkey/xmss/xmss_privatekey.cpp b/src/lib/pubkey/xmss/xmss_privatekey.cpp index dd688d022b0..cef0e0315c4 100644 --- a/src/lib/pubkey/xmss/xmss_privatekey.cpp +++ b/src/lib/pubkey/xmss/xmss_privatekey.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include #include @@ -63,7 +64,6 @@ class XMSS_PrivateKey_Internal { m_xmss_params(xmss_params), m_wots_params(wots_params), m_wots_derivation_method(wots_derivation_method), - m_hash(xmss_params), m_prf(rng.random_vec(xmss_params.element_size())), m_private_seed(rng.random_vec(xmss_params.element_size())), m_index_reg(XMSS_Index_Registry::get_instance()) {} @@ -76,7 +76,6 @@ class XMSS_PrivateKey_Internal { m_xmss_params(xmss_params), m_wots_params(wots_params), m_wots_derivation_method(wots_derivation_method), - m_hash(m_xmss_params), m_prf(std::move(prf)), m_private_seed(std::move(private_seed)), m_index_reg(XMSS_Index_Registry::get_instance()) {} @@ -84,10 +83,7 @@ class XMSS_PrivateKey_Internal { XMSS_PrivateKey_Internal(const XMSS_Parameters& xmss_params, const XMSS_WOTS_Parameters& wots_params, std::span key_bits) : - m_xmss_params(xmss_params), - m_wots_params(wots_params), - m_hash(m_xmss_params), - m_index_reg(XMSS_Index_Registry::get_instance()) { + m_xmss_params(xmss_params), m_wots_params(wots_params), m_index_reg(XMSS_Index_Registry::get_instance()) { /* The code requires sizeof(size_t) >= ceil(tree_height / 8) @@ -139,8 +135,6 @@ class XMSS_PrivateKey_Internal { raw_public_key, unused_index, m_prf, m_private_seed, wots_derivation_method); } - XMSS_Hash& hash() { return m_hash; } - const secure_vector& prf_value() const { return m_prf; } const secure_vector& private_seed() { return m_private_seed; } @@ -194,7 +188,6 @@ class XMSS_PrivateKey_Internal { XMSS_WOTS_Parameters m_wots_params; WOTS_Derivation_Method m_wots_derivation_method; - XMSS_Hash m_hash; secure_vector m_prf; secure_vector m_private_seed; XMSS_Index_Registry& m_index_reg; @@ -210,7 +203,8 @@ XMSS_PrivateKey::XMSS_PrivateKey(XMSS_Parameters::xmss_algorithm_t xmss_algo_id, XMSS_PublicKey(xmss_algo_id, rng), m_private(std::make_shared(m_xmss_params, m_wots_params, wots_derivation_method, rng)) { XMSS_Address adrs; - m_root = tree_hash(0, XMSS_PublicKey::m_xmss_params.tree_height(), adrs); + XMSS_Hash hash(m_xmss_params); + m_root = tree_hash(0, XMSS_PublicKey::m_xmss_params.tree_height(), adrs, hash); } XMSS_PrivateKey::XMSS_PrivateKey(XMSS_Parameters::xmss_algorithm_t xmss_algo_id, @@ -230,7 +224,10 @@ XMSS_PrivateKey::XMSS_PrivateKey(XMSS_Parameters::xmss_algorithm_t xmss_algo_id, "XMSS: unexpected byte length of private seed"); } -secure_vector XMSS_PrivateKey::tree_hash(size_t start_idx, size_t target_node_height, XMSS_Address& adrs) { +secure_vector XMSS_PrivateKey::tree_hash(size_t start_idx, + size_t target_node_height, + XMSS_Address& adrs, + XMSS_Hash& hash) { BOTAN_ASSERT_NOMSG(target_node_height <= 30); BOTAN_ASSERT((start_idx % (static_cast(1) << target_node_height)) == 0, "Start index must be divisible by 2^{target node height}."); @@ -245,7 +242,7 @@ secure_vector XMSS_PrivateKey::tree_hash(size_t start_idx, size_t targe // skip parallelization overhead for leaf nodes. if(split_level == 0) { secure_vector result; - tree_hash_subtree(result, start_idx, target_node_height, adrs); + tree_hash_subtree(result, start_idx, target_node_height, adrs, hash); return result; } @@ -262,7 +259,7 @@ secure_vector XMSS_PrivateKey::tree_hash(size_t start_idx, size_t targe std::vector> nodes(subtrees, secure_vector(XMSS_PublicKey::m_xmss_params.element_size())); std::vector node_addresses(subtrees, adrs); - std::vector xmss_hash(subtrees, m_private->hash()); + std::vector xmss_hash(subtrees, hash); std::vector> work; // Calculate multiple subtrees in parallel. @@ -317,22 +314,15 @@ secure_vector XMSS_PrivateKey::tree_hash(size_t start_idx, size_t targe node_addresses[0].set_tree_height(static_cast(target_node_height - 1)); node_addresses[0].set_tree_index((node_addresses[1].get_tree_index() - 1) >> 1); XMSS_Common_Ops::randomize_tree_hash( - nodes[0], nodes[0], nodes[1], node_addresses[0], this->public_seed(), m_private->hash(), m_xmss_params); + nodes[0], nodes[0], nodes[1], node_addresses[0], this->public_seed(), hash, m_xmss_params); return nodes[0]; #else secure_vector result; - tree_hash_subtree(result, start_idx, target_node_height, adrs, m_private->hash()); + tree_hash_subtree(result, start_idx, target_node_height, adrs, hash); return result; #endif } -void XMSS_PrivateKey::tree_hash_subtree(secure_vector& result, - size_t start_idx, - size_t target_node_height, - XMSS_Address& adrs) { - return tree_hash_subtree(result, start_idx, target_node_height, adrs, m_private->hash()); -} - void XMSS_PrivateKey::tree_hash_subtree( secure_vector& result, size_t start_idx, size_t target_node_height, XMSS_Address& adrs, XMSS_Hash& hash) { const secure_vector& seed = this->public_seed(); diff --git a/src/lib/pubkey/xmss/xmss_signature_operation.cpp b/src/lib/pubkey/xmss/xmss_signature_operation.cpp index 577f39f483c..0e6e8240915 100644 --- a/src/lib/pubkey/xmss/xmss_signature_operation.cpp +++ b/src/lib/pubkey/xmss/xmss_signature_operation.cpp @@ -60,7 +60,7 @@ wots_keysig_t XMSS_Signature_Operation::build_auth_path(XMSS_PrivateKey& priv_ke for(size_t j = 0; j < params.tree_height(); j++) { const size_t k = (m_leaf_idx / (static_cast(1) << j)) ^ 0x01; - auth_path[j] = priv_key.tree_hash(k * (static_cast(1) << j), j, adrs); + auth_path[j] = priv_key.tree_hash(k * (static_cast(1) << j), j, adrs, m_hash); } return auth_path; diff --git a/src/tests/test_concurrent_pk.cpp b/src/tests/test_concurrent_pk.cpp index 752e9229338..b7b795472ac 100644 --- a/src/tests/test_concurrent_pk.cpp +++ b/src/tests/test_concurrent_pk.cpp @@ -386,13 +386,10 @@ class Concurrent_Public_Key_Operations_Test : public Test { ConcurrentPkTestCase("Ed448", "", "Pure"), ConcurrentPkTestCase("SLH-DSA", "SLH-DSA-SHA2-128f"), ConcurrentPkTestCase("HSS-LMS", "SHA-256,HW(5,8)"), + ConcurrentPkTestCase("XMSS", "XMSS-SHA2_10_256"), }; for(const auto& tc : test_cases) { - if(tc.algo_name() == "XMSS" && !Test::run_long_tests()) { - continue; - } - auto rng = Test::new_rng(tc.algo_name()); if(auto privkey = tc.try_create_key(*rng)) {