search

zcash / zcash

Zcash - Internet Money
https://z.cash/
Other
4.94k stars 2.04k forks source link

Add "sanity check magic numbers" to libsnark serializations #418

Closed defuse closed 5 years ago

defuse commented 9 years ago

I just spent a lot of time debugging for #378. In the end, the problem was I was trying to deserialize a r1cs_ppzksnark_proving_key when what was serialized was in fact a zerocash_pour_proving_key. The difference between the two is small (the former is a suffix of the latter), so crazy things happened.

This bug would have been caught instantly if every class that does serialization and deserialization first writes (or verifies) a magic string unique to that class. This would serve as a sanity check that what you're about to deserialize is actually what you think it is.

ebfull commented 9 years ago

We'd probably want to hide that behind an ifdef to avoid wasted space in serialization.

nathan-at-least commented 9 years ago

See also: #416

defuse commented 9 years ago

Another problem is that you can generate the parameters with point compression off, and try loading it with them on, and it will just infinite loop.

defuse commented 8 years ago

There's also other problems, like having to pass tree_depth around even though the parameters highly depend on it and include it in the serialization. Same for the number of input and output coins.

daira commented 8 years ago

@ebfull wrote:

We'd probably want to hide that behind an ifdef to avoid wasted space in serialization.

Hmm? I would have thought the extra space for that would be pretty negligable.

daira commented 8 years ago

(I agree with not encoding redundant information, but for security and complexity rather than space reasons. A tag for each type/class that can be serialized isn't redundant, since the case where the tag is wrong can actually occur.)

daira commented 8 years ago

Note that the libsnark serializations are not consensus-critical, so we could still do this. It's low priority though.