HomePhabricator

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

Description

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

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>

Reviewers: #bitcoin_abc, PiRK

Reviewed By: #bitcoin_abc, PiRK

Differential Revision: https://reviews.bitcoinabc.org/D9051

Details

Provenance
Sebastian Falbesoner <sebastian.falbesoner@gmail.com>Authored on Apr 28 2020, 17:19
FabienCommitted on Jan 26 2021, 07:25
FabienPushed on Jan 26 2021, 07:28
Reviewer
Restricted Project
Differential Revision
D9051: net: remove is{Empty,Full} flags from CBloomFilter, clarify CVE fix
Parents
rABC245041dc2324: [net processing] Move Misbehaving() to PeerManager
Branches
Unknown
Tags
Unknown