mirror of
https://github.com/bitcoin/bitcoin.git
synced 2025-11-12 06:58:57 +01:00
Merge #18806: net: remove is{Empty,Full} flags from CBloomFilter, clarify CVE fix
1ad8ea2b73net: remove is{Empty,Full} flags from CBloomFilter, clarify CVE fix (Sebastian Falbesoner) Pull request description: 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 commit37c6389c5aby gmaxwell) was a _covert fix_ of [CVE-2013-5700](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-5700), a vulnerability that allowed a divide-by-zero remote node crash. According to gmaxwell himself (https://github.com/bitcoin/bitcoin/pull/9060#issuecomment-257749165): > 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 https://github.com/bitcoin/bitcoin/pull/18515 which contains a detailled description and a regression test. It has also been discussed on a [recent PR club meeting on fuzzing](https://bitcoincore.reviews/18521.html). 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. ACKs for top commit: meshcollider: utACK1ad8ea2b73laanwj: Concept and code review ACK1ad8ea2b73jkczyz: ACK1ad8ea2b73MarcoFalke: ACK1ad8ea2b73fjahr: Code review ACK1ad8ea2b73Tree-SHA512: 29f7ff9faece0285e11e16c024851f5bcb772dec64118ccc3f9067ec256267ec8e1b1e3105c7de2a72fd122c3b085e8fc840ab8f4e49813f1cc7a444df1867f7
This commit is contained in:
@@ -3216,7 +3216,6 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec
|
||||
{
|
||||
LOCK(pfrom->m_tx_relay->cs_filter);
|
||||
pfrom->m_tx_relay->pfilter.reset(new CBloomFilter(filter));
|
||||
pfrom->m_tx_relay->pfilter->UpdateEmptyFull();
|
||||
pfrom->m_tx_relay->fRelayTxes = true;
|
||||
}
|
||||
return true;
|
||||
|
||||
Reference in New Issue
Block a user