Guard against bignum length attacks

[milapsheth]

tofn doesn't quite restrict how big numbers can be that are received from other peers. In practice since we target a specific security level, all crypto parameters will be within a known bound. An attacker could exploit the fact that tofn accepts arbitrary-sized inputs to impart an asymmetric computational cost on the peer. Since various routines run in quadratic/cubic time, an attacker only has to pay linear space cost to send a really long message/bigint (while still using a valid value and not be caught) that causes a peer to perform a lot more computational effort that could cause the peer to timeout and removed as a participant, reducing the pool of honest peers.

Note that tofn in various places avoids this issue due to implicit modular reduction of the values. But we should make these checks explicit to be more careful. Of particular concern is the size of the peer keys, zk setup, and zk proofs containing values that are used as exponents. It would be useful to spend some time thinking about other parts of tofn/tofnd that have this asymmetric cost an attacker can exploit.

[milapsheth]

Also, as a side note, BigNumber can be negative, so we should check for that too.

[tarcieri]

It may not provide all the functionality you need yet, but crypto-bigint provides fixed-size stack allocated unsigned big integers:

https://github.com/rustcrypto/crypto-bigint

[milapsheth]

It may not provide all the functionality you need yet, but crypto-bigint provides fixed-size stack allocated unsigned big integers:

https://github.com/rustcrypto/crypto-bigint

Yep, we would certainly like to switch to using that for fixed-sized numbers in the future.

[ggutoski]

More generally (but arguably still within scope for this issue), we should guard against length attacks from incoming binary messages. Milap already has a TODO in the code:

https://github.com/axelarnetwork/tofn/blob/cbd9eb7e820d75a6e89cdad9488bc8fce4c912db/src/sdk/wire_bytes.rs#L43

You'd think that bincode::deserialize for type T would be smart enough to short-cut to failure if the byte slice is too long for any possible instance of T. If so then we could rely on bincode to guard against these attacks for us.

Looking way ahead, we might want to consider eventually ditching serde altogether in favour of custom encodings to a fully-specified binary format. (See RustCrypto zulip discussion. ) But we're nowhere near that yet.

P.S. +1 for crypto-bigint. Looking forward to it!

[ggutoski]

More generally (but arguably still within scope for this issue), we should guard against length attacks from incoming binary messages.

This is a separate issue #169