axelarnetwork / tofn

A threshold cryptography library in Rust
Apache License 2.0
107 stars 22 forks source link

Guard against length attacks from incoming binary messages #169

Closed ggutoski closed 2 years ago

ggutoski commented 2 years ago

Spawned from https://github.com/axelarnetwork/tofn/issues/158#issuecomment-925338726

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.

ggutoski commented 2 years ago

In offline discussion we decided to compute a reasonable global maximum length and enforce that at the SDK level via bincode bounded buffer config. I had too much trouble trying to use bincode to do this so we'll just do it the old-fashioned way---with an if statement before calling bincode::deserialize.

The original plan was to put this length check in wire_bytes::decode but I encountered a problem: wire_bytes::decode is also used by SecretKeyShare::recover, which decodes key recovery info for all parties in a single blob, the size of which is proportional to the number of shares and is much larger than any ordinary GG20 message. My work-around is to instead put the length check in Round::msg_in, which applies the check only to GG20 protocol messages, which is what we want anyway.