cryptoadvance / specter-diy

DIY airgapped hardware wallet that uses QR codes for communication with the host
MIT License
440 stars 73 forks source link

Compact SeedQR sometimes fails #211

Closed stepansnigirev closed 1 year ago

stepansnigirev commented 1 year ago

Maybe some magic bytes at start or at the end are breaking it.

stepansnigirev commented 1 year ago

Add more debug info: image

Check this: image

Failing on this QR: image

kravens commented 1 year ago

25x25 version of the same SeedQR has no issues importing, seems indeed only Compact QR related. image Here is the same 12-word seed as 25x25, which imports just fine.

https://github.com/SeedSigner/seedsigner/blob/dev/docs/seed_qr/README.md#recoverability Since Compact SeedQR's use binary format instead of a human readable digit-stream, it is likely some strange unintended character that causes the bug.

kdmukai commented 1 year ago

Interesting. zxing.org was fine decoding it. Same for SeedSigner itself. Any clue which bytes were tripping up the DIY?

For testing on SeedSigner I wrote a script to randomly generate new seeds and their resulting Compact SeedQRs and fed those back into our decoder. That's how I uncovered the issue with \x00 chars which required some tweaking of an updated version of zbar to fix (if I remember right). Once that char was handled, I ran the script out to maybe 100k random seeds and didn't have any further decoding problems so I figured the SeedSigner implementation was pretty safe at that point.

Screen Shot 2022-07-25 at 3 01 25 PM
stepansnigirev commented 1 year ago

It was \r (0x0d) - it is used as EOL to separate multiple QR codes. Fixed in #213 by adding a "raw" mode when we don't expect animated QR codes.