Page MenuHomePhabricator

net: remove is{Empty,Full} flags from CBloomFilter, clarify CVE fix
ClosedPublic

Authored by Fabien on Jan 25 2021, 11:25.

Details

Summary
The BIP37 bloom filter class CBloomFilter contains two flags
isEmpty/isFull together with an update method with the purpose to,
according to the comments, "avoid wasting cpu", i.e. the mechanism
should serve as an optimization for the trivial cases of empty (all bits
zero) or full (all bits one) filters.
However, the real reason of adding those flags (introduced with commit
37c6389 by gmaxwell) was a covert fix of CVE-2013-5700, a vulnerability
that allowed a divide-by-zero remote node crash.
According to gmaxwell himself (#9060 (comment)):

    the IsEmpty/IsFull optimizations were largely a pretextual
optimization intended to make unexploitable a remote crash vulnerability
(integer division by zero) that existed in the original bloom filtering
code without disclosing it. I'm doubtful that they are all that useful.
:)

For more information on how to trigger this crash, see PR #18515 which
contains a detailled description and a regression test. It has also been
discussed on a recent PR club meeting on fuzzing.

The covert fix code already led to issues and PR based on the wrong
assumption that the flags are there for optimization reasons (see #16886
and #16922). This PR gets rid of the flags and the update method and
just focuses on the CVE fix itself, i.e. it can be seen as a revert of
the covert fix commit modulo the actual fix.

Backport of core PR18806.

Test Plan
ninja all check-all
ninja bitcoin-fuzzers
./src/test/fuzz/bloom_filter <path_to_corpus>

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien requested review of this revision.Jan 25 2021, 11:25
This revision is now accepted and ready to land.Jan 25 2021, 13:37

Tail of the build log:

[378/437] bitcoin: testing sighashtype_tests
[379/437] Running utility command for check-bitcoin-merkleblock_tests
[380/437] bitcoin: testing sync_tests
[381/437] Running utility command for check-bitcoin-script_commitment_tests
[382/437] bitcoin: testing torcontrol_tests
[383/437] Running utility command for check-bitcoin-sighashtype_tests
[384/437] bitcoin: testing bip32_tests
[385/437] Running utility command for check-bitcoin-sync_tests
[386/437] Running utility command for check-bitcoin-torcontrol_tests
[387/437] Running utility command for check-bitcoin-bip32_tests
[388/437] bitcoin: testing settings_tests
[389/437] bitcoin: testing blockcheck_tests
[390/437] Running utility command for check-bitcoin-settings_tests
[391/437] Running utility command for check-bitcoin-blockcheck_tests
[392/437] bitcoin: testing streams_tests
[393/437] Running utility command for check-bitcoin-streams_tests
[394/437] bitcoin: testing uint256_tests
[395/437] bitcoin: testing undo_tests
[396/437] Running utility command for check-bitcoin-uint256_tests
[397/437] bitcoin: testing walletdb_tests
[398/437] Running utility command for check-bitcoin-undo_tests
[399/437] Running utility command for check-bitcoin-walletdb_tests
[400/437] bitcoin: testing cuckoocache_tests
[401/437] bitcoin: testing serialize_tests
[402/437] Running utility command for check-bitcoin-serialize_tests
[403/437] Running utility command for check-bitcoin-cuckoocache_tests
[404/437] bitcoin: testing util_threadnames_tests
[405/437] bitcoin: testing validation_flush_tests
[406/437] Running utility command for check-bitcoin-util_threadnames_tests
[407/437] bitcoin: testing radix_tests
[408/437] bitcoin: testing compilerbug_tests
[409/437] bitcoin: testing schnorr_tests
[410/437] Running utility command for check-bitcoin-validation_flush_tests
[411/437] Running utility command for check-bitcoin-radix_tests
[412/437] Running utility command for check-bitcoin-compilerbug_tests
[413/437] Running utility command for check-bitcoin-schnorr_tests
[414/437] bitcoin: testing crypto_tests
[415/437] bitcoin: testing getarg_tests
[416/437] Running utility command for check-bitcoin-getarg_tests
[417/437] Running utility command for check-bitcoin-crypto_tests
[418/437] bitcoin: testing ref_tests
[419/437] Running utility command for check-bitcoin-ref_tests
[420/437] bitcoin: testing monolith_opcodes_tests
[421/437] Running utility command for check-bitcoin-monolith_opcodes_tests
[422/437] bitcoin: testing validation_tests
[423/437] Running utility command for check-bitcoin-validation_tests
[424/437] bitcoin: testing script_tests
[425/437] Running utility command for check-bitcoin-script_tests
[426/437] bitcoin: testing skiplist_tests
[427/437] Running utility command for check-bitcoin-skiplist_tests
[428/437] bitcoin: testing coinselector_tests
[429/437] Running utility command for check-bitcoin-coinselector_tests
[430/437] bitcoin: testing op_reversebytes_tests
[431/437] Running utility command for check-bitcoin-op_reversebytes_tests
[432/437] bitcoin: testing transaction_tests
[433/437] Running utility command for check-bitcoin-transaction_tests
[434/437] bitcoin: testing coins_tests
[435/437] Running utility command for check-bitcoin-coins_tests
ninja: build stopped: cannot make progress due to previous errors.
Build build-clang failed with exit code 1