blockfilter: Avoid out-of-bounds script access. #14073

pull jimpo wants to merge 1 commits into bitcoin:master from jimpo:bip-158-fix changing 3 files +9 −4

jimpo commented at 6:27 PM on August 26, 2018: contributor

Caught during review of #12254 by @TheBlueMatt. #12254 (review)
practicalswift commented at 7:55 PM on August 26, 2018: contributor

Concept ACK

Very nice find @TheBlueMatt
laanwj commented at 8:50 AM on August 27, 2018: member

utACK 40a9c6c38c6aeaac83fe2639c9ac2d2b262296fc
laanwj added the label P2P on Aug 27, 2018

in src/blockfilter.cpp:212 in 40a9c6c38c outdated

 207 | @@ -208,7 +208,7 @@ static GCSFilter::ElementSet BasicFilterElements(const CBlock& block,
 208 |      for (const CTransactionRef& tx : block.vtx) {
 209 |          for (const CTxOut& txout : tx->vout) {
 210 |              const CScript& script = txout.scriptPubKey;
 211 | -            if (script[0] == OP_RETURN) continue;
 212 | +            if (script.size() > 0 && script[0] == OP_RETURN) continue;
 213 |              elements.emplace(script.begin(), script.end());

gmaxwell commented at 3:57 PM on August 27, 2018:

Now it looks like it adds size 0 entries to the filter, which is a violation of BIP158.

I believe this should be "script.size() < 1 ||". No?

promag commented at 7:41 PM on August 27, 2018:

nit to @gmaxwell comment, script.empty() || ....

in src/test/blockfilter_tests.cpp:70 in 40a9c6c38c outdated

  66 | @@ -67,6 +67,7 @@ BOOST_AUTO_TEST_CASE(blockfilter_basic_test)
  67 |      CMutableTransaction tx_2;
  68 |      tx_2.vout.emplace_back(300, included_scripts[2]);
  69 |      tx_2.vout.emplace_back(0, excluded_scripts[0]);
  70 | +    tx_2.vout.emplace_back(600, included_scripts[5]); // Script is empty

promag commented at 7:51 PM on August 27, 2018:

So this should be excluded_scripts[2]?

nit, above in L62 add something along

// This script is left empty.
// excluded_scripts[2];

blockfilter: Omit empty scripts from filter contents.
Code change also avoids out-of-bounds script access bug.
f05599557a
jimpo force-pushed on Aug 28, 2018
promag commented at 11:14 PM on August 28, 2018: member

utACK f055995.
gmaxwell commented at 8:00 AM on August 29, 2018: contributor

utACK
laanwj commented at 11:29 AM on August 31, 2018: member

re-utACK f05599557a8305d16bd5965921583af9d012fc27
laanwj merged this on Aug 31, 2018
laanwj closed this on Aug 31, 2018
laanwj referenced this in commit 3f96908448 on Aug 31, 2018
jimpo deleted the branch on Nov 28, 2018
PastaPastaPasta referenced this in commit 2ee9526dfd on Apr 15, 2020
PastaPastaPasta referenced this in commit 919d9211b5 on Apr 16, 2020
PastaPastaPasta referenced this in commit b85986189d on Apr 16, 2020
PastaPastaPasta referenced this in commit 6d26c14e28 on Apr 16, 2020
PastaPastaPasta referenced this in commit 374eb49568 on Apr 16, 2020
PastaPastaPasta referenced this in commit cbb91e5d08 on Jun 12, 2020
PastaPastaPasta referenced this in commit ea90d1b6c1 on Jun 13, 2020
PastaPastaPasta referenced this in commit 6e873a5b91 on Jun 17, 2020
gades referenced this in commit 302aadff56 on Jul 1, 2021
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#12254 BIP 158: Compact Block Filters for Light Clients