Bound seq_len when decoding UR2 fountain parts - #970
Open
ruipereira1 wants to merge 1 commit into
Open
Conversation
A part's seq_len comes straight off a scanned QR. URDecoder.parse_sequence_component only rejects values below 1 and Part.from_cbor only rejects above 2**64, so a single frame can declare an arbitrary part count. Two things then scale with it: * FountainDecoder.validate_part() builds a set holding seq_len ints. Measured: seq_len=1,000,000 costs 67 MB and 0.37s on an x86 desktop. * choose_fragments() runs a Fisher-Yates shuffle over seq_len items using list.pop(index), which is quadratic. It is reached whenever seq_num > seq_len. Measured on x86: 16,000 -> 0.15s, 50,000 -> 1.29s, scaling ~3.5x per doubling. A Pi Zero has 512 MB and a single ~1 GHz ARMv6 core, so a frame declaring a seq_len in the millions hangs the device until the user pulls power. The shuffle cannot be made linear: its exact permutation is part of the UR fountain code and changing it would break interoperability with other wallets. Bounding seq_len is the fix. MAX_SEQ_LEN is set to 10,000, which at the 10-byte minimum fragment length still covers a 100 KB message - far beyond any real PSBT. Maintainers may want to pick a different number; the value is a suggestion, the bound is the point.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Problem or Issue being addressed
A UR2 part's
seq_lencomes straight off a scanned QR and is effectively unbounded:URDecoder.parse_sequence_component()only rejects values below 1, andPart.from_cbor()only rejects values above 2**64. Two things then scale with it.
1. Set allocation in
FountainDecoder.validate_part():Measured on an x86 desktop:
seq_len = 1,000,000costs 67 MB and 0.37s for one frame.2. Quadratic shuffle in
choose_fragments(), reached wheneverseq_num > seq_len.Its Fisher-Yates shuffle uses
list.pop(index), which is O(n) per step:Scaling measured at ~3.5x per doubling. Extrapolating, 500,000 is roughly two minutes and
5,000,000 is hours — on x86. A Pi Zero has 512 MB and a single ~1 GHz ARMv6 core, so it is
considerably worse there: the device appears frozen and the only recourse is to pull
power, which discards any seeds held in memory.
Both are reachable from one scanned frame, with
seq_numandseq_lenfully chosen bywhoever produced the QR. This is denial of service, not key compromise.
Solution
Bound
seq_leninFountainDecoder.validate_part(), which runs before both the setallocation and
choose_fragments().The shuffle itself cannot be made linear: its exact permutation is part of the UR fountain
code, so changing it would break interoperability with other wallets. The bound is the fix.
MAX_SEQ_LENis set to 10,000, which at the 10-byte minimum fragment length still coversa 100 KB message — far beyond any real PSBT. That number is a suggestion; please pick
whatever ceiling you're comfortable with, the bound itself is the point.
Additional Information
Reproduction:
Use
seq_num = seq_len + 1for the quadratic path.helpers/ur2/is vendored from Foundation Devices' ur-py. I couldn't locate the currentupstream source to check whether it shares this, so it may be worth reporting there too.
Screenshots
No new or modified screens.
This pull request is categorized as a:
Checklist
I ran
pytestlocallyI included screenshots of any new or modified screens
I added or updated tests
I tested this PR hands-on on the following platform(s):
I have reviewed these notes: