Electric-Coin-Company / zashi-android

The Zashi Zcash Wallet, Android Edition
MIT License
26 stars 15 forks source link

Scanning QR code with certain characters in memo fails #1678

Closed Tomas-M closed 4 days ago

Tomas-M commented 1 week ago

When I use zashi wallet for Android and I scan a QR code while sending money, it works most of the time, but fails when the memo includes certain characters in certain order.

For example, using memo as A<B or B>A works fine, but using memo as A<>B fails with message Error parsing query parameters.

So, if the following text is in QR code, it works: zcash:u19spl3y4zu73twemxrzm33tm3eefepecv4zdssn0hfd4tjaqpgmlcm9nhyjqlvaytwpknqjqctvdscjmg47ex20j03cu4gx3zmy26y2hunpenvw083dmtlq4y7re5rwsygpteq57wwllr3zhs4rw43j5puxgrcqdq4f9dd38qksl4f9p2hc7x3kj582zdjxsnj8urmnc3msfjw72kej0?amount=0.01&memo=QT5C

If the following text is in QR code, it works still: zcash:u19spl3y4zu73twemxrzm33tm3eefepecv4zdssn0hfd4tjaqpgmlcm9nhyjqlvaytwpknqjqctvdscjmg47ex20j03cu4gx3zmy26y2hunpenvw083dmtlq4y7re5rwsygpteq57wwllr3zhs4rw43j5puxgrcqdq4f9dd38qksl4f9p2hc7x3kj582zdjxsnj8urmnc3msfjw72kej0?amount=0.01&memo=QTxC

But If the following text is in QR code, it fails zcash:u19spl3y4zu73twemxrzm33tm3eefepecv4zdssn0hfd4tjaqpgmlcm9nhyjqlvaytwpknqjqctvdscjmg47ex20j03cu4gx3zmy26y2hunpenvw083dmtlq4y7re5rwsygpteq57wwllr3zhs4rw43j5puxgrcqdq4f9dd38qksl4f9p2hc7x3kj582zdjxsnj8urmnc3msfjw72kej0?amount=0.01&memo=QTw+Qg

To test, you can scan this QR code, which works:

works

And try this one which fails:

fails

I would expect that both QR codes would be readable, so when scanning the failed QR code I would expect it fills in the memo A<B>C

It is not a problem of < and > only, there are also problems with other characters, for example question mark followed by a letter seems to have problems, $ and % may have also some problems, etc. I assume there are more cases which will fail. I would expect that all ascii characters will be parsed correctly in all times.

HonzaR commented 1 week ago

Hi @Tomas-M, Thank you for providing all the details about the issue! We'll check it and get back to you soon.

HonzaR commented 1 week ago

We can reproduce the described issue. Zashi prompts users with this error handling in such a case:

Tomas-M commented 1 week ago

I just noticed this ZIP https://zips.z.cash/zip-0321

It looks like there is something different in base64 encoding, and my payment URL with + sign in memo (base64 encoded) is invalid.

The payment URL should be zcash:u19spl3y4zu73twemxrzm33tm3eefepecv4zdssn0hfd4tjaqpgmlcm9nhyjqlvaytwpknqjqctvdscjmg47ex20j03cu4gx3zmy26y2hunpenvw083dmtlq4y7re5rwsygpteq57wwllr3zhs4rw43j5puxgrcqdq4f9dd38qksl4f9p2hc7x3kj582zdjxsnj8urmnc3msfjw72kej0?amount=0.01&memo=QTw-Qg

However the iOS zashi app accepts it even with +. Not sure why. Anyway, this issue should probably be closed, sorry, it was my mistake.

HonzaR commented 5 days ago

@Tomas-M Thanks for the update. We'll check the related Zip321 URI logic.

daira commented 4 days ago

The spec is clear:

"base64url" is defined as in [4] with padding omitted. (Note that this uses a different alphabet to the usual base64; the values 62 and 63 in the alphabet are encoded as - and _ respectively. Implementations MUST NOT accept the characters +, /, and = that occur only in the usual base64.)

[4] RFC 4648 section 5: Base64 Encoding with URL and Filename Safe Alphabet

If Zashi iOS accepts +, that's a bug relative to ZIP 321 (filed as https://github.com/Electric-Coin-Company/zashi-ios/issues/1420).

In general we do not follow the Postel-era RFC "Be generous in what you accept." dictum when writing or implementing Zcash specs —or at least we don't intend to— because it demonstrably results in significant security and interoperability problems over the long term. For discussion of this see RFC 9413: Maintaining Robust Protocols, originally drafted with less weasel-wording and under the better title "The Harmful Consequences of the Robustness Principle" (which ruffled a few political feathers with the IETF old-timers, sigh).

Among other benefits, such as easier security analysis, that practice was intended to prevent situations such as that encountered in this issue: someone testing on one implementation (Zashi iOS) and then getting inconsistent behaviour on another (Zashi Android). In this case interoperability problems could also occur if filename-unsafe characters are mangled by some transmission channels, or inconsistently converted to/from a QR code. If we accepted the wider set of characters then we'd have to test all those cases (or whatever subset of them we were able to think of), and would be imposing a similar testing burden on other implementors.

Tomas-M commented 4 days ago

@daira Thank you for clarification regarding your view on the dictum. Considering this, the iOS Zashi wallet does not follow this, since it perfectly accepts memo with pure base64 encoding (buggy relative to ZIP 321) even without the replaced + / = You can test by scanning the QR code above. Furthermore it has a bug complaining that valid address is invalid while scanning QR in iOS Zashi wallet, I've already opened an issue for that, but that is another problem.

daira commented 4 days ago

@daira Thank you for clarification regarding your view on the dictum. Considering this, the iOS Zashi wallet does not follow this, since it perfectly accepts memo with pure base64 encoding (buggy relative to ZIP 321) even without the replaced + / = You can test by scanning the QR code above.

Filed as zashi-ios#1420, thanks.

Furthermore it has a bug complaining that valid address is invalid while scanning QR in iOS Zashi wallet, I've already opened an issue for that, but that is another problem.

For reference this is zashi-ios#1410.