Repository navigation
sigcache: harden range proof cache keys and add -norangeproofcache option #1600
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
| #include <random.h> | ||
| #include <uint256.h> | ||
| #include <util/system.h> | ||
| #include <hash.h> | ||
|
|
||
| #include <cuckoocache.h> | ||
|
|
||
|
|
@@ -26,23 +27,23 @@ namespace { | |
| class CSignatureCache | ||
| { | ||
| private: | ||
| //! Salted SHA256 midstates, domain-separated by signature or proof type. | ||
| CSHA256 m_salted_hasher_ecdsa; | ||
| CSHA256 m_salted_hasher_schnorr; | ||
| CHashWriter m_salted_hasher_range_proof; | ||
| CHashWriter m_salted_hasher_surjection_proof; | ||
| typedef CuckooCache::cache<uint256, SignatureCacheHasher> map_type; | ||
| map_type setValid; | ||
| std::shared_mutex cs_sigcache; | ||
|
|
||
| public: | ||
| CSignatureCache(): | ||
| m_salted_hasher_range_proof(SER_GETHASH,0), | ||
| m_salted_hasher_surjection_proof(SER_GETHASH,0) | ||
| { | ||
| uint256 nonce = GetRandHash(); | ||
| // Use 64-byte, type-specific salted midstates so later hash computations | ||
| // can start after the first SHA256 chunk. | ||
| static constexpr unsigned char PADDING_ECDSA[32] = {'E'}; | ||
| static constexpr unsigned char PADDING_SCHNORR[32] = {'S'}; | ||
| static constexpr unsigned char PADDING_RANGE_PROOF[32] = {'r'}; | ||
|
|
@@ -51,10 +52,8 @@ class CSignatureCache | |
| m_salted_hasher_ecdsa.Write(PADDING_ECDSA, 32); | ||
| m_salted_hasher_schnorr.Write(nonce.begin(), 32); | ||
| m_salted_hasher_schnorr.Write(PADDING_SCHNORR, 32); | ||
| m_salted_hasher_range_proof << nonce << PADDING_RANGE_PROOF; | ||
| m_salted_hasher_surjection_proof << nonce << PADDING_SURJECTION_PROOF; | ||
| } | ||
|
|
||
| void | ||
|
|
@@ -72,13 +71,39 @@ class CSignatureCache | |
| } | ||
|
|
||
| // ELEMENTS: | ||
| void ComputeEntryRangeProof(uint256& entry, | ||
| const std::vector<unsigned char>& proof, | ||
| const std::vector<unsigned char>& commitment, | ||
| const std::vector<unsigned char>& asset_commitment, | ||
| const CScript& script_pub_key) const | ||
| { | ||
| CHashWriter hasher = m_salted_hasher_range_proof; | ||
| // We commit to both commitments and the scriptPubKey because these are | ||
| // committed to by the rangeproof itself; a change in any of them would | ||
| // invalidate the proof. Since these are exactly the arguments to | ||
| // CachingRangeProofChecker::VerifyRangeProof (below), there is no | ||
| // additional data that could affect the rangeproof's validity. | ||
| // Serialization length-prefixes every field, including the variable-length | ||
| // proof and script, so distinct argument tuples cannot share an encoding. | ||
| hasher << proof << commitment << asset_commitment << script_pub_key; | ||
| entry = hasher.GetSHA256(); | ||
| } | ||
| void ComputeEntrySurjectionProof(uint256& entry, const uint256 &hash, const std::vector<unsigned char>& proof, const std::vector<unsigned char>& commitment, const std::vector<secp256k1_generator>& vTags) const { | ||
| CHashWriter hasher = m_salted_hasher_surjection_proof; | ||
| // We hash all arguments passed to CachingSurjectionProofChecker::VerifySurjectionProof, | ||
| // to ensure that any change in the way that the verification function is called will | ||
| // trigger a cache miss and explicit verification. However, we note that the `wtxid` | ||
| // (hash) commits to all the other data such that we could technically hash only it. | ||
| // We retain the other data as a defense against future refactorings. | ||
| // | ||
| // Serialize vTags as a flat byte vector (each secp256k1_generator is 64 bytes). | ||
| std::vector<unsigned char> vTagsBytes; | ||
| vTagsBytes.reserve(vTags.size() * 64); | ||
| for (const auto& tag : vTags) { | ||
| vTagsBytes.insert(vTagsBytes.end(), std::begin(tag.data), std::end(tag.data)); | ||
|
Member
There was a problem hiding this comment. In 9400096: Just highlighting that this use of the internal |
||
| } | ||
| hasher << hash << proof << commitment << vTagsBytes; | ||
| entry = hasher.GetSHA256(); | ||
| } | ||
|
|
||
| bool | ||
|
|
@@ -154,6 +179,10 @@ bool CachingTransactionSignatureChecker::VerifySchnorrSignature(Span<const unsig | |
| // To be called once in AppInit2/TestingSetup to initialize the rangeproof cache | ||
| void InitRangeproofCache() | ||
| { | ||
| if (!gArgs.GetBoolArg("-rangeproofcache", true)) { | ||
|
Member
There was a problem hiding this comment. In 19d7042: I think we should also disable the surjectionproof cache, throughout. Fine to leave the option name as-is. |
||
| LogPrintf("Range proof cache disabled via -norangeproofcache\n"); | ||
| return; | ||
| } | ||
| // nMaxCacheSize is unsigned. If -maxsigcachesize is set to zero, | ||
| // setup_bytes creates the minimum possible cache (2 elements). | ||
| size_t nMaxCacheSize = std::min(std::max((int64_t)0, gArgs.GetIntArg("-maxsigcachesize", DEFAULT_MAX_SIG_CACHE_SIZE) / 4), MAX_MAX_SIG_CACHE_SIZE) * ((size_t) 1 << 20); | ||
|
|
@@ -175,11 +204,18 @@ void InitSurjectionproofCache() | |
|
|
||
| bool CachingRangeProofChecker::VerifyRangeProof(const std::vector<unsigned char>& vchRangeProof, const std::vector<unsigned char>& vchValueCommitment, const std::vector<unsigned char>& vchAssetCommitment, const CScript& scriptPubKey, const secp256k1_context* secp256k1_ctx_verify_amounts) const | ||
| { | ||
| // ELEMENTS: NOTE FOR FUTURE EDITORS: every argument to this function that | ||
| // carries data (i.e. everything except the secp256k1 context, which is | ||
| // stateless) MUST be included in ComputeEntryRangeProof. Omitting any | ||
| // argument risks returning a cached positive result for a proof that was | ||
| // verified with different inputs. | ||
| uint256 entry; | ||
| const bool useCache = gArgs.GetBoolArg("-rangeproofcache", true); | ||
| if (useCache) { | ||
| rangeProofCache.ComputeEntryRangeProof(entry, vchRangeProof, vchValueCommitment, vchAssetCommitment, scriptPubKey); | ||
| if (rangeProofCache.Get(entry, !store)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| if (vchRangeProof.size() == 0) { | ||
|
|
@@ -208,7 +244,7 @@ bool CachingRangeProofChecker::VerifyRangeProof(const std::vector<unsigned char> | |
| return false; | ||
| } | ||
|
|
||
| if (useCache && store) { | ||
| rangeProofCache.Set(entry); | ||
| } | ||
|
|
||
|
|
@@ -227,7 +263,7 @@ bool CachingSurjectionProofChecker::VerifySurjectionProof(secp256k1_surjectionpr | |
| // wtxid commits to all data including surj targets | ||
| // we need to specify the proof and output asset point to be unique | ||
| uint256 entry; | ||
| surjectionProofCache.ComputeEntrySurjectionProof(entry, wtxid, vchproof, std::vector<unsigned char>(std::begin(gen.data), std::end(gen.data)), vTags); | ||
|
|
||
| if (surjectionProofCache.Get(entry, !store)) { | ||
| return true; | ||
|
|
@@ -244,5 +280,24 @@ bool CachingSurjectionProofChecker::VerifySurjectionProof(secp256k1_surjectionpr | |
| return true; | ||
| } | ||
|
|
||
| // Test-only hooks (see sigcache.h). Forward to the anonymous-namespace caches. | ||
| void TestComputeEntryRangeProof(uint256& entry, | ||
| const std::vector<unsigned char>& proof, | ||
| const std::vector<unsigned char>& commitment, | ||
| const std::vector<unsigned char>& asset_commitment, | ||
| const CScript& script_pub_key) | ||
| { | ||
| rangeProofCache.ComputeEntryRangeProof(entry, proof, commitment, asset_commitment, script_pub_key); | ||
| } | ||
|
|
||
| void TestComputeEntrySurjectionProof(uint256& entry, | ||
| const uint256& hash, | ||
| const std::vector<unsigned char>& proof, | ||
| const std::vector<unsigned char>& commitment, | ||
| const std::vector<secp256k1_generator>& vTags) | ||
| { | ||
| surjectionProofCache.ComputeEntrySurjectionProof(entry, hash, proof, commitment, vTags); | ||
| } | ||
|
|
||
| // END ELEMENTS | ||
| // | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,164 @@ | ||
| // Copyright (c) 2026 The Elements developers | ||
| // Distributed under the MIT software license, see the accompanying | ||
| // file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
| // | ||
| // Tests for the Elements proof-cache entry computation (script/sigcache.cpp). | ||
| // | ||
| // These tests guard the collision-resistance and domain-separation properties | ||
| // of the cache keys used for the range-proof and surjection-proof caches. | ||
| // A cache entry is a *positive* verification result, so a key collision means | ||
| // accepting a proof without ever verifying it. The keys are computed with | ||
| // CHashWriter serialization, which length-prefixes every field, so two | ||
| // distinct argument tuples must never produce the same cache entry, and the | ||
| // two proof types must live in disjoint key spaces (domain separation). | ||
|
|
||
| #include <script/sigcache.h> | ||
| #include <test/util/setup_common.h> | ||
| #include <uint256.h> | ||
|
|
||
| #include <secp256k1_generator.h> | ||
|
|
||
| #include <boost/test/unit_test.hpp> | ||
|
|
||
| #include <vector> | ||
|
|
||
| BOOST_FIXTURE_TEST_SUITE(sigcache_tests, BasicTestingSetup) | ||
|
|
||
| // Two (proof, commitment) tuples whose concatenations would be byte-identical | ||
| // under the OLD raw-CSHA256 scheme (proof=AB commitment=CD vs proof=ABC | ||
| // commitment=D) must produce DIFFERENT cache entries under the new | ||
| // length-prefixing scheme. This is the core collision-resistance property. | ||
| BOOST_AUTO_TEST_CASE(rangeproof_entry_field_boundary) | ||
| { | ||
| std::vector<unsigned char> proof_a = {0xAA, 0xBB}; | ||
| std::vector<unsigned char> commit_a = {0xCC, 0xDD}; | ||
| std::vector<unsigned char> asset_a = {0x11}; | ||
|
|
||
| std::vector<unsigned char> proof_b = {0xAA, 0xBB, 0xCC}; | ||
| std::vector<unsigned char> commit_b = {0xDD}; | ||
| std::vector<unsigned char> asset_b = {0x11}; | ||
|
|
||
| // Concatenations are identical: AA BB CC DD 11 == AA BB CC DD 11. | ||
| // Under the old raw-Write scheme these collided; they must not now. | ||
| CScript script; | ||
|
|
||
| uint256 entry_a, entry_b; | ||
| TestComputeEntryRangeProof(entry_a, proof_a, commit_a, asset_a, script); | ||
| TestComputeEntryRangeProof(entry_b, proof_b, commit_b, asset_b, script); | ||
|
|
||
| BOOST_CHECK(entry_a != entry_b); | ||
| } | ||
|
|
||
| // Same field-boundary property for the surjection-proof entry, varying the | ||
| // proof vs commitment split with a fixed hash. | ||
| BOOST_AUTO_TEST_CASE(surjectionproof_entry_field_boundary) | ||
| { | ||
| uint256 hash = uint256S("0x1234"); | ||
|
|
||
| std::vector<unsigned char> proof_a = {0x01, 0x02}; | ||
| std::vector<unsigned char> commit_a = {0x03, 0x04}; | ||
|
|
||
| std::vector<unsigned char> proof_b = {0x01, 0x02, 0x03}; | ||
| std::vector<unsigned char> commit_b = {0x04}; | ||
|
|
||
| std::vector<secp256k1_generator> vTags; // empty tags — same in both cases | ||
|
|
||
| uint256 entry_a, entry_b; | ||
| TestComputeEntrySurjectionProof(entry_a, hash, proof_a, commit_a, vTags); | ||
| TestComputeEntrySurjectionProof(entry_b, hash, proof_b, commit_b, vTags); | ||
|
|
||
| BOOST_CHECK(entry_a != entry_b); | ||
| } | ||
|
|
||
| // The scriptPubKey is variable-length and part of the range-proof key. Two | ||
| // calls differing only in the script must produce different entries (an | ||
| // attacker must not be able to replay a cached proof against a different | ||
| // output script). | ||
| BOOST_AUTO_TEST_CASE(rangeproof_entry_script_sensitivity) | ||
| { | ||
| std::vector<unsigned char> proof = {0x01, 0x02, 0x03}; | ||
| std::vector<unsigned char> commit = {0x04, 0x05}; | ||
| std::vector<unsigned char> asset = {0x06}; | ||
|
|
||
| CScript script_a; | ||
| script_a << OP_TRUE; | ||
| CScript script_b; | ||
| script_b << OP_FALSE; | ||
|
|
||
| uint256 entry_a, entry_b; | ||
| TestComputeEntryRangeProof(entry_a, proof, commit, asset, script_a); | ||
| TestComputeEntryRangeProof(entry_b, proof, commit, asset, script_b); | ||
|
|
||
| BOOST_CHECK(entry_a != entry_b); | ||
| } | ||
|
|
||
| // Domain separation: a range-proof tuple and a surjection-proof tuple must | ||
| // never share a cache entry even when their byte content is arranged to look | ||
| // similar. The two caches use distinct salted midstates ('r' vs 's'), so the | ||
| // same logical content must hash differently across the two domains. | ||
| BOOST_AUTO_TEST_CASE(proof_caches_domain_separation) | ||
| { | ||
| uint256 hash = uint256S("0xabcd"); | ||
| std::vector<unsigned char> proof = {0x01, 0x02, 0x03}; | ||
| std::vector<unsigned char> commit = {0x04, 0x05, 0x06}; | ||
| std::vector<unsigned char> asset = {0x07, 0x08}; | ||
| CScript script; | ||
| std::vector<secp256k1_generator> vTags; | ||
|
|
||
| uint256 range_entry, surj_entry; | ||
| TestComputeEntryRangeProof(range_entry, proof, commit, asset, script); | ||
| TestComputeEntrySurjectionProof(surj_entry, hash, proof, commit, vTags); | ||
|
|
||
| // Different domains must not collide with each other's entries. | ||
| BOOST_CHECK(range_entry != surj_entry); | ||
| } | ||
|
|
||
| // Determinism: the same tuple must always produce the same entry within a | ||
| // process (the salt is fixed per-process), which is what makes the cache | ||
| // usable at all. | ||
| BOOST_AUTO_TEST_CASE(entries_are_deterministic) | ||
| { | ||
| std::vector<unsigned char> proof = {0xde, 0xad}; | ||
| std::vector<unsigned char> commit = {0xbe, 0xef}; | ||
| std::vector<unsigned char> asset = {0x00}; | ||
| CScript script; | ||
| script << OP_RETURN; | ||
|
|
||
| uint256 e1, e2; | ||
| TestComputeEntryRangeProof(e1, proof, commit, asset, script); | ||
| TestComputeEntryRangeProof(e2, proof, commit, asset, script); | ||
| BOOST_CHECK(e1 == e2); | ||
|
|
||
| uint256 hash = uint256S("0x99"); | ||
| std::vector<secp256k1_generator> vTags; | ||
| uint256 s1, s2; | ||
| TestComputeEntrySurjectionProof(s1, hash, proof, commit, vTags); | ||
| TestComputeEntrySurjectionProof(s2, hash, proof, commit, vTags); | ||
| BOOST_CHECK(s1 == s2); | ||
| } | ||
|
|
||
| // vTags sensitivity: two surjection-proof calls that differ only in vTags | ||
| // must produce different cache entries (otherwise an attacker could cause | ||
| // a cache hit for a proof verified with different input tags). | ||
| BOOST_AUTO_TEST_CASE(surjectionproof_entry_vtags_sensitivity) | ||
| { | ||
| uint256 hash = uint256S("0xdeadbeef"); | ||
| std::vector<unsigned char> proof = {0x01, 0x02}; | ||
| std::vector<unsigned char> commit = {0x03, 0x04}; | ||
|
|
||
| secp256k1_generator gen_a, gen_b; | ||
| // Fill with distinct byte patterns. | ||
| memset(gen_a.data, 0xAA, sizeof(gen_a.data)); | ||
| memset(gen_b.data, 0xBB, sizeof(gen_b.data)); | ||
|
|
||
| std::vector<secp256k1_generator> vTags_a = {gen_a}; | ||
| std::vector<secp256k1_generator> vTags_b = {gen_b}; | ||
|
|
||
| uint256 entry_a, entry_b; | ||
| TestComputeEntrySurjectionProof(entry_a, hash, proof, commit, vTags_a); | ||
| TestComputeEntrySurjectionProof(entry_b, hash, proof, commit, vTags_b); | ||
|
|
||
| BOOST_CHECK(entry_a != entry_b); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_SUITE_END() |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 9400096:
I realize it isn't true that just the wtxid and proof are sufficient -- you can imagine the same proof appearing multiple times in the same transaction, such that it's only valid in one place.
Our code correctly avoids this vulnerability by hashing the commitment and tags, it's just the comment saying "y'know..." that's wrong.