search

erickt / rust-zmq

Rust zeromq bindings.
Apache License 2.0
887 stars 189 forks source link

Problem: denial-of-service in Message::gets #288

Closed cite-reader closed 4 years ago

cite-reader commented 4 years ago

Solution: return None on invalid UTF-8 instead of panicking.

The doc for zmq_msg_gets says that "both the property argument and the value shall be NULL-terminated UTF8-strings", but in practice libzmq neither confirms nor enforces this. A buggy or malicious peer can send arbitrary binary garbage, which will be faithfully returned verbatim from zmq_msg_gets. Converting the Result from str::from_utf8 to an Option via .ok() instead of unwrapping saves us from a crash without a client-breaking type change.

birkenfeld commented 4 years ago

This is one way, the other is to actually return a Result here. Currently zmq_msg_gets is documented to return only EINVAL, i.e. "no such property", but this may be extended later and our API can't represent it.

If Option is kept, the non-UTF8 behavior should at least be mentioned in the docstring.

cite-reader commented 4 years ago

If an API change is on the table, I'd actually suggest using byte slices instead of strings. The ZMTP/3.0 and /3.1 specifications as written don't say anything about the encoding of metadata (except for a frustratingly underspecified comment that "metadata names SHALL be case-insensitive"). I suspect that may be an oversight though and intend to follow up with the libzmq folks about it soonish.

For right now though this is a security issue and there should be a mitigation that doesn't require anything more than updating dependencies and rebuilding.

birkenfeld commented 4 years ago

Agreed.

rotty commented 4 years ago

Thanks for identifying this issue! I've included your fix as commit 3606a3ef, adding a code comment pointing out the reason for the .ok() call, and the suggestion of using a byte slice.

I'll add an API comment, as suggested by @birkenfeld.

As a minor remark: I've also taken the liberty to adjusting the commit message, as I'm not particularly fond of 0MQ's "problem/solution" style, and prefer (or attempt) to follow the "imperative mood" guideline used by the git project itself[1].

[1] https://git.kernel.org/pub/scm/git/git.git/tree/Documentation/SubmittingPatches?id=HEAD#n133

rotty commented 4 years ago

I've now released v0.9.2 containing this fix, as well as the changes on master so far.