Respect size limits in read_seq and read_map.

[canndrew]

It's possible to cause the bincode rustc_serialise decoder to panic with OOM even if a size limit is set. This PR makes the read_seq and read_map decoder methods check that the number of elements about to be read is not greater than the number of bytes remaining before we hit the limit. This is not a perfect solution as it sort-of assumes that all types have size 1. For instance, if the size limit is 1KB it should still be allowable to decode a Vec of a million ()s whereas it shouldn't be allowable to decode a Vec of 500 u32s. With this fix, neither of these situations are handled correctly. However I don't see a solution to this, and having this fix is better than not having it.

This PR also uses checked_add in read_bytes to so that people can't use malicious data to cause an overflow exception.

[canndrew]

Ah, I just noticed there's been an issue open for this since last May: #41

[canndrew]

An alternative way to fix this would be to add a max_container_size argument to decode_from.

[nox]

Please squash the two commits and I'll merge this. Thanks for your contribution!

[nox]

Oh wait sorry never mind. That's why I shouldn't read my mails why I'm leaving home… Misread your commit message. I'll read tonight what's the story about #41 and try to find a better solution.

[TyOverby]

There is a really long history behind #41. I actually have two pull requests out against rustc-serialize right now that would fix the problem "correctly."

rust-lang-deprecated/rustc-serialize#121 is a breaking change to rustc-serialize, but is required to fix the problem and maintain good performance.

rust-lang-deprecated/rustc-serialize#122 is a non-breaking change, but it works by removing the pre-allocation step from deserialization which could cause slowdowns.

@canndrew I'm taking this issue very seriously (it's the one thing holding bincode back from version 1.0.0), but I'd love for this to be fixed upstream in rustc-serialize.

The good news about your PR is that we can remove the check if one of the real fixes makes its way upstream. (I'm wary about adding max_container_size for this reason).

@canndrew @nox I'm going to do some more investigations soon. I want to see if this is reproducable with Serde and I want to get the attention of the rustc-serialize maintainers once again and push for the breaking change.

[TyOverby]

More context: The real bug is rust-lang-deprecated/rustc-serialize#115

More than Bincode is vulnerable to this attack. cbor's fast-decoder also dies in the same way. (Cbor's slow decoder currently sidesteps this issue by buffering the entire message)

[canndrew]

@TyOverby: Would you consider merging this PR for now and reverting it if/when rustc_serialise gets fixed? It's unlikely to break any code, unless there's someone out there serialising big Vec<()>s.

[TyOverby]

Yep. Merged.

[canndrew]

Awesome, thanks for that. Could you bump the version number and publish it so I can depend on it?

[TyOverby]

@canndrew Done. 0.5.1 is good to go!

[TyOverby]

@canndrew Thanks for your contribution! Also, I appreciate your comments on the rustc-serialize issue.