sigcache: harden range proof cache keys and add -norangeproofcache option - #1600
Conversation
Switch range-proof and surjection-proof cache hashers from raw CSHA256 concatenation to CHashWriter (SER_GETHASH). This serializes each field with a length prefix, so distinct argument tuples with byte-identical raw concatenations no longer collide to the same cache key. A cache entry is a positive verification result; a collision would let an attacker bypass verification. The fix affects two caches: - ComputeEntryRangeProof: proof, commitment, asset_commitment, scriptPubKey - ComputeEntrySurjectionProof: add vTags to key (was missing entirely) Both caches retain the per-process 64-byte salted midstate (nonce || PADDING_RANGE_PROOF / PADDING_SURJECTION_PROOF) for domain separation between the two proof types. Expose thin test-only hooks (TestComputeEntryRangeProof / TestComputeEntrySurjectionProof) so unit tests can reach the anonymous-namespace cache internals.
Test the field-boundary, script-sensitivity, domain-separation, determinism, and vTags-sensitivity properties of the range-proof and surjection-proof cache entry computation.
Add -norangeproofcache startup option. InitRangeproofCache() early-returns (allocating nothing) when the flag is set. VerifyRangeProof reads the flag on each call and bypasses both the Get and Set paths, so cache behaviour is fully skipped without restarting.
|
ARM CI task failure is because Debian 11 is EOL |
|
A fuzz oracle to test the combinations with and without cache could help catching these problems next time. |
| // 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. |
There was a problem hiding this comment.
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.
| 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)); |
There was a problem hiding this comment.
In 9400096:
Just highlighting that this use of the internal tag.data array is fine, although the libsecp256k1-zkp docs say that the internals of secp256k1_generator are implementation-defined and nonportable. We do not need portability here or any particular property of the bytes, only that they represent the generator in question.
| // To be called once in AppInit2/TestingSetup to initialize the rangeproof cache | ||
| void InitRangeproofCache() | ||
| { | ||
| if (!gArgs.GetBoolArg("-rangeproofcache", true)) { |
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.
|
Done reviewing 1bfa1ee. Just two nits. |
|
ACK 1bfa1ee |
ca17280
into
ElementsProject:elements-23.3.x
4 participants
Target branch is elements-23.3.x
Switch the range-proof and surjection-proof cache hashers from raw
CSHA256 concatenation to CHashWriter . This length-prefixes
every field, so distinct argument tuples with byte-identical raw
concatenations no longer collide to the same cache key.
Both caches retain per-process salted midstates with distinct domain
separators ('r' / 's').
Also adds a -norangeproofcache startup option to
disable the range proof cache without recompiling.
Unit tests cover field-boundary collisions, script sensitivity, domain
separation, determinism, and vTags sensitivity.
Bump version to 23.3.4 and updates manpages.