Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 31, 2018
Merged

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

merged 1 commit into from
Aug 31, 2018

Conversation

jimpo
Copy link
Contributor

@jimpo jimpo commented Aug 26, 2018

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

@practicalswift
Copy link
Contributor

Concept ACK

Very nice find @TheBlueMatt

@laanwj
Copy link
Member

laanwj commented Aug 27, 2018

utACK 40a9c6c38c6aeaac83fe2639c9ac2d2b262296fc

@laanwj laanwj added the P2P label Aug 27, 2018
gmaxwell
gmaxwell reviewed Aug 27, 2018
View reviewed changes
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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

promag
promag reviewed Aug 27, 2018
View reviewed changes
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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this should be excluded_scripts[2]?

nit, above in L62 add something along

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

@jimpo
blockfilter: Omit empty scripts from filter contents.
f055995 
Code change also avoids out-of-bounds script access bug.
@promag
Copy link
Contributor

promag commented Aug 28, 2018

utACK f055995.

@gmaxwell
Copy link
Contributor

utACK

@laanwj
Copy link
Member

laanwj commented Aug 31, 2018

re-utACK f055995

@laanwj laanwj merged commit f055995 into bitcoin:master Aug 31, 2018
laanwj added a commit that referenced this pull request Aug 31, 2018
@laanwj
Merge #14073: blockfilter: Avoid out-of-bounds script access.
3f96908 
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
@Bushstar Bushstar mentioned this pull request Sep 4, 2018
Merged
@jimpo jimpo deleted the bip-158-fix branch November 28, 2018 00:55
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 15, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#14073: blockfilter: Avoid out-of-bounds script access.
2ee9526 
f055995 blockfilter: Omit empty scripts from filter contents. (Jim Posen)

Pull request description:

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

Tree-SHA512: cfc9e3eeaba12a14fd3d2e1ccce1a1f89e8cf44cc340ceec05d2d5fa61d27ff64e355603f4ad2184ff73c0ed23dfdab6e2103bddc48f3b76cb13b88d428770ac
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 16, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#14073: blockfilter: Avoid out-of-bounds script access.
919d921 
f055995 blockfilter: Omit empty scripts from filter contents. (Jim Posen)

Pull request description:

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

Tree-SHA512: cfc9e3eeaba12a14fd3d2e1ccce1a1f89e8cf44cc340ceec05d2d5fa61d27ff64e355603f4ad2184ff73c0ed23dfdab6e2103bddc48f3b76cb13b88d428770ac
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 16, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#14073: blockfilter: Avoid out-of-bounds script access.
b859861 
f055995 blockfilter: Omit empty scripts from filter contents. (Jim Posen)

Pull request description:

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

Tree-SHA512: cfc9e3eeaba12a14fd3d2e1ccce1a1f89e8cf44cc340ceec05d2d5fa61d27ff64e355603f4ad2184ff73c0ed23dfdab6e2103bddc48f3b76cb13b88d428770ac
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 16, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#14073: blockfilter: Avoid out-of-bounds script access.
6d26c14 
f055995 blockfilter: Omit empty scripts from filter contents. (Jim Posen)

Pull request description:

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

Tree-SHA512: cfc9e3eeaba12a14fd3d2e1ccce1a1f89e8cf44cc340ceec05d2d5fa61d27ff64e355603f4ad2184ff73c0ed23dfdab6e2103bddc48f3b76cb13b88d428770ac
@PastaPastaPasta PastaPastaPasta mentioned this pull request Apr 16, 2020
Merged
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 16, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#14073: blockfilter: Avoid out-of-bounds script access.
374eb49 
f055995 blockfilter: Omit empty scripts from filter contents. (Jim Posen)

Pull request description:

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

Tree-SHA512: cfc9e3eeaba12a14fd3d2e1ccce1a1f89e8cf44cc340ceec05d2d5fa61d27ff64e355603f4ad2184ff73c0ed23dfdab6e2103bddc48f3b76cb13b88d428770ac
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 12, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#14073: blockfilter: Avoid out-of-bounds script access.
cbb91e5 
f055995 blockfilter: Omit empty scripts from filter contents. (Jim Posen)

Pull request description:

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

Tree-SHA512: cfc9e3eeaba12a14fd3d2e1ccce1a1f89e8cf44cc340ceec05d2d5fa61d27ff64e355603f4ad2184ff73c0ed23dfdab6e2103bddc48f3b76cb13b88d428770ac
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 13, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#14073: blockfilter: Avoid out-of-bounds script access.
ea90d1b 
f055995 blockfilter: Omit empty scripts from filter contents. (Jim Posen)

Pull request description:

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

Tree-SHA512: cfc9e3eeaba12a14fd3d2e1ccce1a1f89e8cf44cc340ceec05d2d5fa61d27ff64e355603f4ad2184ff73c0ed23dfdab6e2103bddc48f3b76cb13b88d428770ac
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 17, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#14073: blockfilter: Avoid out-of-bounds script access.
6e873a5 
f055995 blockfilter: Omit empty scripts from filter contents. (Jim Posen)

Pull request description:

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

Tree-SHA512: cfc9e3eeaba12a14fd3d2e1ccce1a1f89e8cf44cc340ceec05d2d5fa61d27ff64e355603f4ad2184ff73c0ed23dfdab6e2103bddc48f3b76cb13b88d428770ac
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Jul 1, 2021
@laanwj @gades
Merge bitcoin#14073: blockfilter: Avoid out-of-bounds script access.
302aadf 
f055995 blockfilter: Omit empty scripts from filter contents. (Jim Posen)

Pull request description:

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

Tree-SHA512: cfc9e3eeaba12a14fd3d2e1ccce1a1f89e8cf44cc340ceec05d2d5fa61d27ff64e355603f4ad2184ff73c0ed23dfdab6e2103bddc48f3b76cb13b88d428770ac
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
2 more reviewers

@gmaxwell gmaxwell gmaxwell left review comments

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
P2P
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jimpo @practicalswift @laanwj @promag @gmaxwell