script: fix signature cache false positives (security)
Two stacked bugs in CSignatureCache: 1. Set() keyed on vchSig (with trailing hashtype byte) while Get() keyed on vchSigCopy (without), so the cache never hit: a silent no-op. (Found in prior audit session.) 2. Once (1) was fixed, the cache produced FALSE POSITIVES: the 64-bit XOR-mixed key included the pubkey LENGTH but never the pubkey BYTES. All compressed pubkeys are 33 bytes, so a signature validated once hit the cache when re-checked against ANY other pubkey for the same sighash — CheckSig returned true without verifying. A 2-of-3 CHECKMULTISIG could be satisfied by one valid signature duplicated. This also masqueraded as first-match-wins multisig reordering in multisig_tests/script_tests; those tests now pass with their original strict assertions. Cache entries are now the full SHA256 over (sighash || sig || pubkey), matching upstream Bitcoin Core; false positives are cryptographically infeasible.
This commit is contained in:
+37
-25
@@ -1215,33 +1215,37 @@ uint256 SignatureHash(CScript scriptCode, const CTransaction& txTo, unsigned int
|
|||||||
class CSignatureCache
|
class CSignatureCache
|
||||||
{
|
{
|
||||||
private:
|
private:
|
||||||
// Cache key: hash of (sighash + signature + pubkey) for O(1) lookups.
|
// Entry: SHA256 over (sighash || signature || pubkey).
|
||||||
// Using a single uint256 key with unordered_set is much faster than
|
//
|
||||||
// the old std::set<tuple<uint256, vector, vector>> approach which had
|
// SECURITY FIX (2026-07-04): the previous implementation reduced the
|
||||||
// O(log n) lookups and expensive random eviction.
|
// entry to a 64-bit XOR-mix that included the pubkey LENGTH but never
|
||||||
std::unordered_set<uint64_t> setValid;
|
// the pubkey BYTES. Since virtually all pubkeys are the same length
|
||||||
|
// (33 bytes compressed), any signature that had been validated once
|
||||||
|
// would hit the cache when re-checked against a DIFFERENT pubkey for
|
||||||
|
// the same sighash, making CheckSig() return true without verifying.
|
||||||
|
// In a 2-of-3 CHECKMULTISIG this allowed one valid signature,
|
||||||
|
// duplicated, to satisfy the script. Storing the full 256-bit hash of
|
||||||
|
// all three components makes false positives cryptographically
|
||||||
|
// infeasible (matches upstream Bitcoin Core, which also keys its
|
||||||
|
// signature cache on the full (sighash, sig, pubkey) triple).
|
||||||
|
struct EntryHasher
|
||||||
|
{
|
||||||
|
size_t operator()(const uint256& entry) const
|
||||||
|
{
|
||||||
|
size_t ret;
|
||||||
|
memcpy(&ret, entry.begin(), sizeof(ret));
|
||||||
|
return ret; // entry is already a uniform SHA256 output
|
||||||
|
}
|
||||||
|
};
|
||||||
|
std::unordered_set<uint256, EntryHasher> setValid;
|
||||||
CCriticalSection cs_sigcache;
|
CCriticalSection cs_sigcache;
|
||||||
|
|
||||||
// Compute a compact 64-bit cache key from the signature components.
|
uint256 ComputeKey(const uint256& hash, const std::vector<unsigned char>& vchSig,
|
||||||
// Collision probability is negligible (~1 in 2^64 per lookup) and a
|
const std::vector<unsigned char>& vchPubKey) const
|
||||||
// false positive only means we skip one redundant verification.
|
|
||||||
uint64_t ComputeKey(const uint256& hash, const std::vector<unsigned char>& vchSig,
|
|
||||||
const std::vector<unsigned char>& vchPubKey) const
|
|
||||||
{
|
{
|
||||||
// Mix sighash with first 8 bytes of sig and pubkey for a fast key
|
return Hash(hash.begin(), hash.end(),
|
||||||
uint64_t k = hash.Get64();
|
vchSig.begin(), vchSig.end(),
|
||||||
if (vchSig.size() >= 8)
|
vchPubKey.begin(), vchPubKey.end());
|
||||||
k = (k & 0xffffffff00000000ULL) | (k & 0x00000000ffffffffULL);
|
|
||||||
k ^= std::hash<size_t>()(vchSig.size()) * 0x9e3779b97f4a7c15ULL;
|
|
||||||
k ^= std::hash<size_t>()(vchPubKey.size()) * 0x517cc1b727220a95ULL;
|
|
||||||
// Mix in actual signature bytes for uniqueness
|
|
||||||
for (size_t i = 0; i < vchSig.size() && i < 32; i += 8)
|
|
||||||
{
|
|
||||||
uint64_t chunk = 0;
|
|
||||||
memcpy(&chunk, &vchSig[i], std::min((size_t)8, vchSig.size() - i));
|
|
||||||
k ^= chunk * (0x9e3779b97f4a7c15ULL + i);
|
|
||||||
}
|
|
||||||
return k;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
public:
|
public:
|
||||||
@@ -1303,7 +1307,15 @@ bool CheckSig(const vector<unsigned char>& vchSig, const vector<unsigned char>&
|
|||||||
if (!key.Verify(sighash, vchSigCopy))
|
if (!key.Verify(sighash, vchSigCopy))
|
||||||
return false;
|
return false;
|
||||||
|
|
||||||
signatureCache.Set(sighash, vchSig, vchPubKey);
|
// CRITICAL FIX (2026-07-04): Cache Set must use vchSigCopy (the actual bytes
|
||||||
|
// we just verified), NOT vchSig (which has the trailing hashtype byte still
|
||||||
|
// attached). The hashtype byte is already folded into the cache key via
|
||||||
|
// sighash = SignatureHash(..., nHashType), and mixing it into the key bytes
|
||||||
|
// too would (a) make Set write a key that Get would never query for, leaving
|
||||||
|
// the cache as a silent no-op, and (b) risk collisions if the hashtype byte
|
||||||
|
// were the only difference between two signatures. vchSigCopy is the
|
||||||
|
// canonical operand on both sides (matches upstream Bitcoin Core fix).
|
||||||
|
signatureCache.Set(sighash, vchSigCopy, vchPubKey);
|
||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user