Reject malformed signmessage QR data instead of raising - #968
Open
ruipereira1 wants to merge 1 commit into
Open
Conversation
SignMessageQrDecoder.add() indexed parts[1] and parts[2] unconditionally and
then split on the format separator without checking that it exists. Any of
these payloads raises IndexError:
signmessage
signmessage m/84h/0h/0h/0/0
signmessage m/84h/0h/0h/0/0 nocolon
detect_segment_type() classifies anything starting with "signmessage" as
SIGN_MESSAGE, so a QR containing just that word is enough to trigger it.
ScanScreen._run() calls decoder.add_image() with no exception handling, so the
error propagates to Controller.handle_exception and the user lands on the
generic "System Error" screen rather than the "Unknown QR Type" warning. The
camera's video stream is also left running, since ScanScreen only stops it on
its normal exit paths.
The scanner is the device's only untrusted input; no QR payload should be able
to raise out of the decoder.
Also splits with maxsplit=2 and partitions on the first colon, so a message
that itself contains "ascii:" is no longer silently truncated.
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
SignMessageQrDecoder.add()indexes into the split payload with no bounds checks:All three of these raise
IndexError:detect_segment_type()routes anything starting withsignmessagehere, so a QRcontaining just that one word is enough to trigger it.
ScanScreen._run()(gui/screens/scan_screens.py:239) callsself.decoder.add_image(frame)with no exception handling, so the error unwinds toController.handle_exception→UnhandledExceptionView. The user seesSystem Error / IndexErrorinstead of the intended "Unknown QR Type" warning, and thecamera's video stream is left running —
ScanScreenonly callscamera.stop_video_stream_mode()on its normal exit paths, andBaseScreen.display()'sfinallystops threads but not the camera. It self-heals on the next scan, sincestart_video_stream_mode()stops an existing stream first.Separately,
segment.split(f"{fmt}:")[1]splits on every occurrence and takes index 1,so a message that itself contains
ascii:is silently truncated at the second occurrence.Solution
split(maxsplit=2)pluspartition(":"), with explicitDecodeQRStatus.INVALIDreturnsand no assignment to
selfbefore validation succeeds. The truncation is fixed as a sideeffect, since only the first colon now separates format from payload.
Additional Information
Two follow-ups I deliberately kept out of scope, happy to submit either if you agree with
the approach:
add_image()call inScanScreen._run()so that no decoder can take downthe scan flow.
camera.stop_video_stream_mode()into afinally.Note also that this does not fix every crash in the sign-message flow: the decoder
validates the number of fields, not the shape of the derivation path, and
embit_utils.parse_derivation_path()raisesIndexErroron paths with fewer than threecomponents. I've reported that separately.
Screenshots
No new or modified screens — a malformed payload now reaches the existing
ScanInvalidQRTypeViewinstead ofUnhandledExceptionView. Both 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: