Internal Security Review Results (of note-spending-v5 branch)

[defuse]

I looked at all of zcash_client_backend, zcash_client_sqlite, and some of zcash_primitives in revision ae63116. I was only focused on stuff related to the mobile wallet. I'm posting here instead of in @str4d's fork for greater visibility.

tl;dr: No major issues, mostly just stuff that hasn't been implemented yet, side-channel attack ideas, and a suggestion to adjust the threat model so that light wallets can recover from briefly connecting to a compromised lightwalletd.

Design Comments

• The light client is written assuming lightwalletd is sending correct data to the light client. In practice this won't be perfectly true. I think the threat model should be adjusted so that a light wallet must be able to recover from having once connected to a malicious lightwalletd. This seems likely to happen, e.g. when a popular lightwalletd server gets compromised temporarily. Right now it's possible for a bad lightwalletd to permanently DoS a light wallet, probably in a couple ways. One is that the highest-numbered block in the cache is taken as authoritative in validate_combined_chain, so a bad lightwalletd can send a very-high-numbered fake block which won't be fixed by syncing with a good lightwalletd. A malicious lightwalletd could also corrupt past transaction data by sending fake transaction whose txid already exists in the DB and then stmt_update_tx updates its block number to the wrong value. Another way to permanently mess up a light wallet is to omit any spending transactions it sent (i.e. don't send it back to the wallet when it gets mined); the wallet will think the transaction expired and mark that balance as spendable again, and any subsequent spends of the same notes will fail.
• It doesn't look like the PoW checking specified in ZIP 307 is implemented yet. Having those checks in place will help defend against some kinds of malicious-lightwalletd attacks.
• How reorgs are implemented isn't really clear to me (i.e. the contract between the Rust stuff and the user of the Rust stuff). Splitting the responsibility for handling reorgs across this interface feels like an area where bugs will turn up. Good discussion topic for Zcon1.
• (Not a problem. Just mentioning so that I remember.) If transactions were malleable, then it would be possible to spend the same notes as a light wallet spent, but in a different transaction, which would make it look like the wallet's spend expired and it would erroneously mark those notes as unspent. (edit: I later found out transactions are malleable... so maybe this is a problem?)

zcash_client_backend

• Comparison of nullifiers is not constant time in scan_block, so an attacker observing this through local side-channels could potentially learn which nullifiers belong to the user.
• scan_block_from_bytes looks unused.
• My paranoia wants unit tests for the functions that encode/decode the keys in encoding.rs (there is already a test for encoding/decoding addresses).

zcash_client_sqlite

• rewind_to_height doesn't seem to "rewind" any notes that were discovered (and put into the received_notes table) in blocks that got rewound, so the user might be left thinking they have balance that they don't.
• Where is validate_combined_chain used? I can't find its use in the Rust crates or the SDK. (I was looking for it because I was concerned about threading/TOCTTOU bugs.)
• In send_to_address, there's a non-constant-time comparison of the viewing keys inside an SQL query. This potentially makes it easier to steal the keys through side-channels.

zcash_primitives

• parse_note_plaintext_minus_memo extracts the values from the unauthenticated plaintext. There are a couple ways this function can return None, and if an adversary can distinguish between them through side-channels they can use it as a decryption oracle. I think you could get both rcm and v by flipping bits in the ciphertext in a binary-search-like manner until you find a value that's one off from the maximums they are compared against (MAX_MONEY and MODULUS respectively). The comparison of the note commitment is also not constant time so it may be possible to learn the computed commitment value based on how long it takes the comparison to fail.
• spending_key and the ZIP32 functions it calls don't enforce ZIP32's requirement that the seed "MUST be at least 32 bytes."

Stuff I didn't look at

• Dependencies.
• ZIP32 implementation (a bug here could lead to use of weak keys, but it looks like it has good tests).
• Most of zcash_primitives.
• Protobuf, I assume the spec and generated code work correctly.

[daira]

ZIP 307 should explicitly call out how to do the decryption in as side-channel-resistant a way as possible.

[str4d]

I've split out the concrete tasks into separate issues. Leaving this issue open until we decide where to have discussions about the threat model and reorg handling.

[nuttycom]

@defuse Is there anything from this ticket that needs to be further broken out (see the individual issues that were created from it) or can it be closed?