Validate the BIP-39 checksum when decoding a Standard SeedQR - #967
Open
ruipereira1 wants to merge 1 commit into
Open
Validate the BIP-39 checksum when decoding a Standard SeedQR#967ruipereira1 wants to merge 1 commit into
ruipereira1 wants to merge 1 commit into
Conversation
The numeric SeedQR path mapped 4-digit indices to words and returned COMPLETE
without ever validating the mnemonic, unlike the plaintext-mnemonic path which
constructs a Seed and therefore checks it. Two consequences:
* A SeedQR whose checksum is wrong (a hand-transcribed QR drawn incorrectly)
was accepted here and only rejected later in ScanView, where the resulting
InvalidSeedException is uncaught and routes the user to the generic
"System Error" screen.
* `num_words = int(len(segment) / 4)` decoded a prefix and silently discarded
any leftover digits, so a 50-digit payload loaded a 12-word seed.
Also anchor the detection regex. `re.search(r'\d{48,96}')` claimed any QR that
merely contained a long run of digits; note that this check runs before the
SettingsQR check in detect_segment_type, so a SettingsQR containing 48+ digits
was classified as a SeedQR.
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
SeedQrDecoder.add()decodes a Standard (numeric) SeedQR by mapping 4-digit indices towords and returns
COMPLETEwithout ever validating the mnemonic. The plaintext-mnemonicpath directly below it does validate, because it constructs a
Seed.Two consequences:
a) A SeedQR with an invalid checksum is accepted by the decoder. A 48-digit payload
encoding
abandon × 12(valid words, invalid checksum) decodes toCOMPLETE.ScanView.run()then does:with no
try, so the resultingInvalidSeedExceptionpropagates toController.handle_exceptionand the user lands onSystem Error / InvalidSeedException.Hand-transcribing SeedQRs from the project's own printable templates is exactly the
workflow where a wrong checksum is expected to happen, and the checksum is what exists to
catch it. Today it surfaces as a crash rather than an explanation.
b) Extra digits are silently discarded.
num_words = int(len(segment) / 4)decodes aprefix and ignores the remainder, so a 50-digit payload loads a 12-word seed from the
first 48 digits.
c) Detection over-matches.
detect_segment_type()usedre.search(r'\d{48,96}', s),a loose search that claims any QR merely containing a long digit run. That check runs
before the
settings::check, so"settings::v1 " + "1"*48was classified as a SeedQR.Solution
bip39.mnemonic_to_bytes()— checksum only, no PBKDF2, so no meaningfulcost in the scan loop.
seed_phraseon the failure path.re.fullmatch(r'\d{48}|\d{96}', s.strip()).An invalid SeedQR now routes to the existing
ScanInvalidQRTypeView("Unknown QR Type")instead of crashing.
Additional Information
Open question for maintainers: would you prefer a dedicated error screen ("Invalid
SeedQR — checksum failed") over the generic "Unknown QR Type"? That's a UX decision and
I kept it out of this PR.
Screenshots
No new or modified screens — an invalid SeedQR now reaches the existing
ScanInvalidQRTypeViewinstead ofUnhandledExceptionView. Both screens are unchanged.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: