search

Electric-Coin-Company / zashi-ios

The Zashi Zcash Wallet, iOS Edition
MIT License
39 stars 13 forks source link

A ZIP 321 URI with filename-unsafe characters (e.g. `+`) is reportedly accepted when it MUST NOT be according to the spec #1420

Open daira opened 6 days ago

daira commented 6 days ago

ZIP 321 says, of the base64url memo field:

"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

According to https://github.com/Electric-Coin-Company/zashi-android/issues/1678#issuecomment-2477494431 , a URI with a + character is rejected on Zashi Android but accepted on Zashi iOS. According to the spec, it MUST be rejected on both. (I'm intentionally using the more value-neutral terms "accepted"/"rejected" rather than "succeeds"/"fails".)

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 zashi-android#1678: 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.

daira commented 6 days ago

Example of a QR code that is reportedly accepted when it must not be (second image in https://github.com/Electric-Coin-Company/zashi-android/issues/1678#issue-2657985253 ):

image

pacu commented 6 days ago

@daira thanks for reporting this I'll check it out.

If someone can chip in and do it before myself I'd much appreciate it.

daira commented 5 days ago

The problem is as described here: https://github.com/zecdev/zcash-swift-payment-uri/issues/69#issuecomment-2490440246