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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.