Skip to content

consensus: Add CompactSize range check to deserialization#5697

Merged
apoelstra merged 1 commit intorust-bitcoin:masterfrom
erickcestari:add-max-size-compactsize
Mar 14, 2026
Merged

consensus: Add CompactSize range check to deserialization#5697
apoelstra merged 1 commit intorust-bitcoin:masterfrom
erickcestari:add-max-size-compactsize

Conversation

@erickcestari
Copy link
Copy Markdown
Contributor

@erickcestari erickcestari commented Feb 18, 2026

Reject CompactSize values exceeding MAX_COMPACT_SIZE (0x02000000) during deserialization, matching Bitcoin Core's serialize.h limit.

Extract read_compact_size logic into a private free function to allow tests to bypass the range check when validating non-minimal encoding.

Add test for the new OversizedCompactSize error at the boundary.

Update bip152 error test to expect failure at deserialization rather than at index computation, since the range check now triggers earlier.

Found by differential fuzzing using bitcoinfuzz.

@github-actions github-actions bot added C-bitcoin PRs modifying the bitcoin crate C-p2p labels Feb 18, 2026
@erickcestari
consensus: Add CompactSize range check to deserialization
1dd1d92 
Reject CompactSize values exceeding MAX_COMPACT_SIZE (0x02000000) during
deserialization, matching Bitcoin Core's serialize.h limit.

Extract read_compact_size logic into a private free function to allow
tests to bypass the range check when validating non-minimal encoding.

Add test for the new SizeTooLargeCompactSize error at the boundary.

Update bip152 error test to expect failure at deserialization rather
than at index computation, since the range check now triggers earlier.
@erickcestari erickcestari force-pushed the add-max-size-compactsize branch from 3f3f5ec to 1dd1d92 Compare February 18, 2026 17:16
@tcharding
Copy link
Copy Markdown
Member

tcharding commented Feb 18, 2026

Thanks man! Good work on the fuzzing.

A couple of comments/questions for the project more than just for you.

  • The patched code is more or less on its way out so if we need this fix we likely need it consensus_enocding as well.
  • We have done a fair bit of work with compact sizes and we don't have this check, that makes me think we discussed it and I can't remember? If not how did we miss this?

@erickcestari
Copy link
Copy Markdown
Contributor Author

erickcestari commented Feb 19, 2026

Thanks man! Good work on the fuzzing.

A couple of comments/questions for the project more than just for you.

  • The patched code is more or less on its way out so if we need this fix we likely need it consensus_enocding as well.
  • We have done a fair bit of work with compact sizes and we don't have this check, that makes me think we discussed it and I can't remember? If not how did we miss this?

More context:

I found this while doing differential fuzzing in the PSBT parser. Bitcoin Core always checks if the compact_size value is greater than 33,554,432 to throw an exception. While debugging, I found that the PSBT key has a type value that is the compact size encoded, which Bitcoin Core checks against the upper bound limit, but rust-bitcoin does not. Making rust-bitcoin accepting the PSBT and Bitcoin Core rejects it.

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Feb 19, 2026

Ha! Interesting timing then. I only recently raised this issue: #5622 which is around the key type and its usage of a CompactSize encoding. Note in the associated PR the code comment 'the protocol abuses ...' (which I suggested). Amusing that its not the only bug we have on this data field and I"m now not surprised we missed it.

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Feb 19, 2026 •
edited
Loading

BTW we are trying to move PSBT dev over to rust-psbt. Would be mad if you fuzzed against that too if its not too much additional work. Would give us more confidence as we migrate dev that we don't make any mistakes. ref: #5700

@jrakibi
Copy link
Copy Markdown
Contributor

jrakibi commented Feb 19, 2026

We have done a fair bit of work with compact sizes and we don't have this check, that makes me think we discussed it and I can't remember? If not how did we miss this?

we previously had MAX_ENCODABLE_VALUE constant for this, but it was removed in 460976939 because it wasn’t enforced anywhere.

this was discussed here: #5057 (comment)

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Feb 20, 2026

Ouch so I removed it. Thanks @jrakibi!

@erickcestari
Copy link
Copy Markdown
Contributor Author

erickcestari commented Feb 20, 2026

BTW we are trying to move PSBT dev over to rust-psbt. Would be mad if you fuzzed against that too if its not too much additional work. Would give us more confidence as we migrate dev that we don't make any mistakes. ref: #5700

Yes, adding the rust-psbt is already on the plan.

bitcoinfuzz/bitcoinfuzz#419

I'll check to see if it gets done this week and report any issues I find.

@tcharding
Copy link
Copy Markdown
Member

tcharding commented Feb 20, 2026

Boss!

@erickcestari erickcestari mentioned this pull request Feb 21, 2026
Merged
@apoelstra
Copy link
Copy Markdown
Member

apoelstra commented Feb 27, 2026

I'm gonna go ahead and ACK/merge this. But

  • Does this need to be backported to 0.32.x to be picked up by your fuzzer?
  • Do we want to move any of this PR to rust-psbt?

apoelstra
apoelstra approved these changes Feb 28, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 1dd1d92; successfully ran local tests

@tcharding tcharding added the bug label Mar 2, 2026
@tcharding tcharding mentioned this pull request Mar 2, 2026
Closed
@tcharding
Copy link
Copy Markdown
Member

tcharding commented Mar 2, 2026 •
edited
Loading

I created rust-bitcoin/rust-psbt#74 to track it over in rust-psbt and added the backport label (after checking we can backport the changes). And #5752 to track fixing it in consensus_encoding.

@tcharding tcharding mentioned this pull request Mar 2, 2026
Open
@erickcestari
Copy link
Copy Markdown
Contributor Author

erickcestari commented Mar 2, 2026

I'm gonna go ahead and ACK/merge this. But

  • Does this need to be backported to 0.32.x to be picked up by your fuzzer?
  • Do we want to move any of this PR to rust-psbt?
  • Does this need to be backported to 0.32.x to be picked up by your fuzzer?

No, We can do fuzzing in any commit/branch.

  • Do we want to move any of this PR to rust-psbt?

Yes, I've tested with rust-psbt and it has the same issue as rust-bitcoin. Since rust-psbt v0 uses the Key from rust-bitcoin it also will accept a key type that core doesn't accept.

@apoelstra apoelstra merged commit 9129379 into rust-bitcoin:master Mar 14, 2026
28 checks passed
tcharding added a commit to tcharding/rust-bitcoin that referenced this pull request Mar 29, 2026
@tcharding
consensus: Add CompactSize range check to deserialization
88722b0 
Manually backport rust-bitcoin#5697. On `master` the `VarInt` type is gone but the
deserialization bug fixed by 5697 exists here none the less.

Add the same range check to `Decodable for VarInt`.
@tcharding tcharding mentioned this pull request Mar 29, 2026
Open
@tcharding
Copy link
Copy Markdown
Member

tcharding commented Mar 29, 2026

Backported in #5921

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@apoelstra apoelstra apoelstra approved these changes

Assignees

No one assigned

Labels

bug C-bitcoin PRs modifying the bitcoin crate C-p2p Needs Backport

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@erickcestari @tcharding @jrakibi @apoelstra