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

merkle_block: add resource limit check during deserialization #2607

Merged
merged 1 commit into from Mar 19, 2024

Conversation

apoelstra
Copy link
Member

Fixes #2606

@github-actions github-actions bot added the C-bitcoin PRs modifying the bitcoin crate label Mar 17, 2024
@apoelstra
Copy link
Member Author

I don't think this method is used except by applications implementing the p2p protocol (and I am not aware of any, at least not major ones) so I don't believe we need to bend over backward to contact or patch anybody.

But we should backport to all version back to 0.28 which is the oldest version I think is in wide use.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 8318744357

Details

  • 19 of 19 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 83.834%

Totals Coverage Status
Change from base Build 8318043190: 0.01%
Covered Lines: 19769
Relevant Lines: 23581

💛 - Coveralls

@apoelstra
Copy link
Member Author

Fuzzed deserialization of MerkleBlock for 30 minutes so for with this PR. Will let it run overnight.

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK f1dcfab

Super cool to see this fixed.

@apoelstra
Copy link
Member Author

Fuzzing has passed for 14.5 hours now (on a 32-core machine).

@tcharding
Copy link
Member

Does that make it well and truly fu**ed?

@apoelstra
Copy link
Member Author

Does that make it well and truly fu**ed?

It means that the fix works. (Though we may want to tighten it; I think MAX_VEC_SIZE is orders of magnitude larger than we need....but I also want something that's obviously sufficient so we can merge this quickly. We can tighten it later.)

@tcharding
Copy link
Member

Ugh, sorry man - that was a joke (I literally had "(joke)" after it at first and removed it. fuzzed == fu**ed and the other interpretation. Too smart for my own good :)

@apoelstra
Copy link
Member Author

Oh, lol :) I missed the pun.

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACK f1dcfab

@apoelstra apoelstra merged commit 002590b into rust-bitcoin:master Mar 19, 2024
187 checks passed
@apoelstra apoelstra deleted the 2024-03--merkle-deser branch March 19, 2024 15:31
apoelstra added a commit that referenced this pull request Apr 2, 2024
…ation

866781d merkle_block: add resource limit check during deserialization (Andrew Poelstra)

Pull request description:

  Backport of #2607 to 0.31.x.

ACKs for top commit:
  tcharding:
    ACK 866781d
  sanket1729:
    ACK 866781d

Tree-SHA512: 19cd2ecf75e4cfedb8abded43571cf1fe14512807a0e5fd9925ebbc932d5208a27f72a4c450a864f4ab9d6aca89bb4062678e1516f523cb5298e4ee44cd792f4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bitcoin PRs modifying the bitcoin crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deserializing network message can request arbitrary sized allocations
4 participants