Skip to content

fix: Reject witness version > 16 in validate_segwit#279

Open
alexgrad42 wants to merge 1 commit into
rust-bitcoin:masterfrom
alexgrad42:fix/checked_hrpstring_validate_segwit
Open

fix: Reject witness version > 16 in validate_segwit#279
alexgrad42 wants to merge 1 commit into
rust-bitcoin:masterfrom
alexgrad42:fix/checked_hrpstring_validate_segwit

Conversation

@alexgrad42

Copy link
Copy Markdown
Contributor

BIP-173 and BIP-350 state that the witness version must be between 0 and 16 inclusive. However, CheckedHrpstring::validate_segwit only validated the length of the witness program, allowing invalid versions from 17 to 31 to be incorrectly treated as valid.

Fix this by explicitly checking if the extracted witness_version exceeds 16, returning InvalidWitnessVersion if it does.

Fixes #274

BIP-173 and BIP-350 state that the witness version must be between
0 and 16 inclusive. However, `CheckedHrpstring::validate_segwit`
only validated the length of the witness program, allowing invalid
versions from 17 to 31 to be incorrectly treated as valid.

Fix this by explicitly checking if the extracted `witness_version`
exceeds 16, returning `InvalidWitnessVersion` if it does.

Fixes rust-bitcoin#274
@tcharding

Copy link
Copy Markdown
Member

This and your other PRs smell like LLM. Please make an attempt to convince me otherwise.

@apoelstra

Copy link
Copy Markdown
Member

They are clearly LLM-generated but many of them are correct the first time, and they are helping to burn down the pile of Loupe bug reports. I am reviewing them quite adversarially.

@apoelstra

Copy link
Copy Markdown
Member

This check already exists in multiple places -- notably SegwitHrpstring::new. It does appear that CheckedHrpstring::validate_segwit bypasses this check.

But now the check happens twice, because SegwitHrpstring::new calls CheckedHrpstring::validate_segwit.

I don't think we can let a drive-by LLM fix this. We need to back up and have a human decide at what level the check needs to happen, and maybe better-encapsulate some constructors.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CheckedHrpstring::validate_segwit accepts witness version > 16

3 participants