Remove XMSS private key shared mutable state#5366
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request removes shared mutable state from the XMSS private key implementation to fix thread-safety issues when multiple threads sign with the same key concurrently. Previously, XMSS_PrivateKey_Internal stored an XMSS_Hash member that was reused across signature operations, causing race conditions when the key was used from multiple threads. The fix makes XMSS consistent with other signature algorithms like ECDSA and RSA, which can safely be used concurrently without external locking.
Changes:
- Removed the m_hash member variable and hash() accessor from XMSS_PrivateKey_Internal
- Updated tree_hash and tree_hash_subtree methods to accept an XMSS_Hash parameter instead of using shared state
- Enabled XMSS in concurrent public key operation tests without requiring long test runs
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/lib/pubkey/xmss/xmss_privatekey.cpp | Removed m_hash member from XMSS_PrivateKey_Internal, updated tree_hash methods to accept hash parameter, created local XMSS_Hash instances where needed |
| src/lib/pubkey/xmss/xmss.h | Updated tree_hash signature to include hash parameter, removed overloaded tree_hash_subtree method |
| src/lib/pubkey/xmss/xmss_signature_operation.cpp | Updated tree_hash call to pass m_hash parameter from signature operation |
| src/tests/test_concurrent_pk.cpp | Added XMSS to concurrent signing tests, removed conditional logic that skipped XMSS tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
fe82a93 to
9764f03
Compare
|
FYI: This will conflict with #5358 and the same change will need to be done for XMSS^MT |
Yes. Also after rebasing be sure to add XMSS^MT to the concurrent operations test |
… disabled for now (see randombit#5369)
XMSS^MT: implement pubkey and verify XMSS^MT: implement keygen and sign XMSS^MT: refactor common XMSS and XMSS^MT code adjust for leaf idx >32 bit in the index registry add test vector files to git clang tidy XMSS^MT: add some tests and documentation various small fixes Remove XMSS^MT private key shared mutable state (see randombit#5366), disabled for now (see randombit#5369)
XMSS^MT: implement pubkey and verify XMSS^MT: implement keygen and sign XMSS^MT: refactor common XMSS and XMSS^MT code adjust for leaf idx >32 bit in the index registry add test vector files to git clang tidy XMSS^MT: add some tests and documentation various small fixes Remove XMSS^MT private key shared mutable state (see randombit#5366), disabled for now (see randombit#5369)
XMSS^MT: implement pubkey and verify XMSS^MT: implement keygen and sign XMSS^MT: refactor common XMSS and XMSS^MT code adjust for leaf idx >32 bit in the index registry add test vector files to git clang tidy XMSS^MT: add some tests and documentation various small fixes Remove XMSS^MT private key shared mutable state (see randombit#5366), disabled for now (see randombit#5369)
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.