Secure by design deserialize_from()

[alin-at-dfinity]

We just realized after blissfully having used deserialize_from() for quite a while that it is potentially very dangerous when not used in conjunction with with_limit(). And unless you carefully read through the Bincode readme, you won't be aware of the existence of this issue.

I've spent some time looking into why this is as unsafe as it is and realized that it's due to the fact that the std::io::Read trait provides zero information about the remaining number of bytes. So all Bincode can do is blindly allocate however many bytes the u64 value it just decoded tells it to. Enforcing a default limit is similarly problematic, as it may both break some clients that are not aware of it and still result in unsafely large allocations for other clients.

But then it dawned on me that one could modify the Bincode API so that it requires an explicit limit whenever using deserialize_from(). E.g. by replacing deserialize_from(reader) with deserialize_from(reader, limit). This would immediately make it obvious to Bincode users that they need to make a trade-off between convenience and safety.

Those that only ever deal with trusted inputs can always go with usize::MAX. And those that (like us) are actually dealing with a reader on top of a slice, can simply get rid of the reader (and the need to pick an explicit limit) and use deserialize() instead.

Just an idea, but it feels to me like a net improvement over the status quo.

[VictorKoenders]

For bincode 1.3.3 we're not going to be making a breaking change for this.

For bincode 2 we have made the config mandatory, meaning that all methods have 2 (or 3) arguments already. I think adding an extra argument to that would add a lot of visual overhead for developers.

I do think this is an important feature. We could add a paragraph about this on the first page of the documentation.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[dtolnay]

The paragraph of documentation mentioned above has not been added.

[alin-at-dfinity]

I fully understand the reasoning against adding yet another argument to some methods. And it's obviously your decision to make as maintainers to prioritize simplicity over security.

But I still believe that keeping this in the documentation (even on the front page) is objectively less secure than making it explicit in the API. One way this could be done technically in bincode 2 (not sure about about whether this is necessarily good API design) would be for bincode::config::standard() to require an explicit limit argument, instead of defaulting to NoLimit.

[alin-at-dfinity]

FWIW, I "fixed" this on our end by adding bincode::deserialize_from to the list of disallowed-methods in clippy.toml. So, combined with running cargo clippy as part of presubmit tests, this will prevent its use in that particular repository. Others may still have to deal with the fallout of not thoroughly reading the documentation before using bincode.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.