Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Aug 31, 2018

Conversation

Projects
None yet
5 participants
@jimpo
Copy link
Contributor

commented Aug 26, 2018

Caught during review of #12254 by @TheBlueMatt. #12254 (comment)

@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 26, 2018

Concept ACK

Very nice find @TheBlueMatt

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 27, 2018

utACK 40a9c6c38c6aeaac83fe2639c9ac2d2b262296fc

@laanwj laanwj added the P2P label Aug 27, 2018

src/blockfilter.cpp Outdated
@@ -208,7 +208,7 @@ static GCSFilter::ElementSet BasicFilterElements(const CBlock& block,
for (const CTransactionRef& tx : block.vtx) {
for (const CTxOut& txout : tx->vout) {
const CScript& script = txout.scriptPubKey;
if (script[0] == OP_RETURN) continue;
if (script.size() > 0 && script[0] == OP_RETURN) continue;
elements.emplace(script.begin(), script.end());

This comment has been minimized.

Copy link
@gmaxwell

gmaxwell Aug 27, 2018

Member

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?

This comment has been minimized.

Copy link
@promag

promag Aug 27, 2018

Member

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

src/blockfilter.cpp Outdated
@@ -208,7 +208,7 @@ static GCSFilter::ElementSet BasicFilterElements(const CBlock& block,
for (const CTransactionRef& tx : block.vtx) {
for (const CTxOut& txout : tx->vout) {
const CScript& script = txout.scriptPubKey;
if (script[0] == OP_RETURN) continue;
if (script.size() > 0 && script[0] == OP_RETURN) continue;
elements.emplace(script.begin(), script.end());

This comment has been minimized.

Copy link
@promag

promag Aug 27, 2018

Member

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

src/test/blockfilter_tests.cpp Outdated
@@ -67,6 +67,7 @@ BOOST_AUTO_TEST_CASE(blockfilter_basic_test)
CMutableTransaction tx_2;
tx_2.vout.emplace_back(300, included_scripts[2]);
tx_2.vout.emplace_back(0, excluded_scripts[0]);
tx_2.vout.emplace_back(600, included_scripts[5]); // Script is empty

This comment has been minimized.

Copy link
@promag

promag Aug 27, 2018

Member

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.

@jimpo jimpo force-pushed the jimpo:bip-158-fix branch to f055995 Aug 28, 2018

@promag

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

utACK f055995.

@gmaxwell

This comment has been minimized.

Copy link
Member

commented Aug 29, 2018

utACK

@laanwj

This comment has been minimized.

Copy link
Member

commented Aug 31, 2018

re-utACK f055995

@laanwj laanwj merged commit f055995 into bitcoin:master Aug 31, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Aug 31, 2018

Merge #14073: blockfilter: Avoid out-of-bounds script access.
f055995 blockfilter: Omit empty scripts from filter contents. (Jim Posen)

Pull request description:

  Caught during review of #12254 by @TheBlueMatt. #12254 (comment)

Tree-SHA512: cfc9e3eeaba12a14fd3d2e1ccce1a1f89e8cf44cc340ceec05d2d5fa61d27ff64e355603f4ad2184ff73c0ed23dfdab6e2103bddc48f3b76cb13b88d428770ac

@jimpo jimpo deleted the jimpo:bip-158-fix branch Nov 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.