Fix race in XMSS_Index_Registry

[randombit]

No description provided.

[Copilot]

Pull request overview

This PR addresses a concurrency issue in the XMSS leaf index tracking by making XMSS_Index_Registry lookups/insertions properly synchronized, and re-enables the concurrent public-key test coverage for XMSS to validate the fix.

Changes:

• Make XMSS_Index_Registry::get() fully mutex-protected and create registry entries on-demand for previously unseen keys.
• Eagerly touch the index registry during XMSS_PrivateKey_Internal construction to ensure registration before concurrent signing.
• Re-enable the XMSS concurrent signing test case.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
src/tests/test_concurrent_pk.cpp	Re-enables XMSS in concurrent signing/verifying test matrix.
src/lib/pubkey/xmss/xmss_privatekey.cpp	Eagerly registers XMSS key material in the global index registry during construction.
src/lib/pubkey/xmss/xmss_index_registry.h	Updates registry API (span-based), moves singleton accessor/destructor out-of-line, clarifies registration behavior.
src/lib/pubkey/xmss/xmss_index_registry.cpp	Implements thread-safe get_instance() and get() with mutex coverage; refactors key-id computation helper.

Comments suppressed due to low confidence (1)

src/lib/pubkey/xmss/xmss_index_registry.cpp:37

• make_xmss_index_key_id() switched from final() (secure_vector) to final_stdvec() (std::vector). Since this hash is computed over private_seed and prf, it unnecessarily places derived data in non-secure heap memory. Consider switching back to hash->final() (or otherwise keeping the digest in a secure container / only materializing the required 8 bytes) to avoid leaving sensitive intermediates in regular memory.

   const std::string_view index_hash_function = "SHA-256";
   std::unique_ptr<HashFunction> hash = HashFunction::create_or_throw(index_hash_function);
   hash->update(private_seed);
   hash->update(prf);
   const auto result = hash->final_stdvec();
   uint64_t key_id = 0;
   for(size_t i = 0; i < sizeof(key_id); i++) {
      key_id = ((key_id << 8) | result[i]);
   }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coveralls]

coverage: 90.34% (+0.007%) from 90.333%
when pulling 8f9c4d6 on jack/fix-xmss-index-race
into 5166623 on master.

[randombit]

5 CI runs plus 2 nightly runs with no crashes, so 🤞 I think we're good

[randombit]

Weirdly this race never showed up with helgrind, GCC or Clang's ThreadSanitizer, etc. I'm not sure why.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/lib/pubkey/xmss/xmss_privatekey.cpp:131

• For consistency and clarity, consider adding a comment or explicit recover_global_leaf_index() call in this constructor similar to the other two constructors. While set_unused_leaf_index() at line 123 does internally call recover_global_leaf_index() which registers the key, having an explicit call with the comment "Eagerly register so the entry exists before any concurrent signing" would make the intent clearer and maintain consistency across all constructors.

      XMSS_PrivateKey_Internal(const XMSS_Parameters& xmss_params,
                               const XMSS_WOTS_Parameters& wots_params,
                               std::span<const uint8_t> key_bits) :
            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)

         Maximum supported tree height is 20, ceil(20/8) == 3, so 4 byte
         size_t is sufficient for all defined parameters, or even a
         (hypothetical) tree height 32, which would be extremely slow to
         compute.
         */
         static_assert(sizeof(size_t) >= 4, "size_t is big enough to support leaf index");

         const secure_vector<uint8_t> raw_key = extract_raw_private_key(key_bits, xmss_params);

         if(raw_key.size() != m_xmss_params.raw_private_key_size() &&
            raw_key.size() != m_xmss_params.raw_legacy_private_key_size()) {
            throw Decoding_Error("Invalid XMSS private key size");
         }

         BufferSlicer s(raw_key);

         // We're not interested in the public key here
         s.skip(m_xmss_params.raw_public_key_size());

         auto unused_leaf_bytes = s.take(sizeof(uint32_t));
         const size_t unused_leaf = load_be<uint32_t>(unused_leaf_bytes.data(), 0);
         if(unused_leaf >= (1ULL << m_xmss_params.tree_height())) {
            throw Decoding_Error("XMSS private key leaf index out of bounds");
         }

         m_prf = s.copy_as_secure_vector(m_xmss_params.element_size());
         m_private_seed = s.copy_as_secure_vector(m_xmss_params.element_size());
         set_unused_leaf_index(unused_leaf);

         // Legacy keys generated prior to Botan 3.x don't feature a
         // WOTS+ key derivation method encoded in their private key.
         m_wots_derivation_method =
            (s.empty()) ? WOTS_Derivation_Method::Botan2x : static_cast<WOTS_Derivation_Method>(s.take(1).front());

         BOTAN_ASSERT_NOMSG(s.empty());
      }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[reneme]

Looks good to me. Comments below are mostly nits and suggestions. The interface of the index registry is much more sensible now and thus easier to reason about with concurrency in mind. Thanks.

[reneme]

Side note: It would be nice if this were possible:

return hash->final_array<32>();

... we have similar API in the RandomNumberGenerator and also proposed for BigInt: #5153

[reneme]

The uint32_t is actually a membre of the xmss_algorithm_t enum. I think is's worth using that type explicitly here.

[reneme]

... ah, nevermind. In #5382 this won't work anyway.

[reneme]

Nit: For the moment XMSS explicitly depends on sha2_32 in its info.txt so this hard-coded hash algorithm is just fine. But if that were to ever change, we'd need to opportunistically select a supported hash algorithm from "SHA-256", "SHA-512", or "SHAKE-256". One of them has to be available, because otherwise XMSS doesn't make sense in the first place.

[randombit]

Alternately we could require that the caller pass a hash reference. This requires that the caller itself is careful to never hash the same key with two different hash functions since otherwise it would result in two distinct key IDs for the same key, breaking the invariants. But in the XMSS/LMS case there is typically already a hash function at hand so that is maybe ok.

IIRC the sha_32 dependency in XMSS is precisely because of this usage in the index.

[reneme]

With #5382 this dependency can move into the generic registry module making it more obvious that the hash is only needed for the registry. And then we might as well just leave it in for future work. I'm not really a fan of giving the responsibility of selecting the hash function to the XMSS/LMS modules for the brittleness it introduces.