Fixing binary port decoder to bail out quickly when receiving a big mes… #5049
Conversation
zajko
changed the title
zajko
force-pushed
the
binary_port_max_message_size_fix
branch
from
3d5d3a6 to
6ad22ed
Compare
zajko
force-pushed
the
binary_port_max_message_size_fix
branch
from
6ad22ed to
17d2759
Compare
binary_port/src/binary_message.rs
Outdated
| if let [b1, b2, b3, b4, remainder @ ..] = &src[..] { | ||
| let length = LengthEncoding::from_le_bytes([*b1, *b2, *b3, *b4]) as usize; | ||
| let remainder_length = remainder.len(); | ||
| (length, remainder_length, remainder_length >= length) |
There was a problem hiding this comment.
In the case of remainder_length > length, I think enforcing request-response means the buffer should hold at most one message. The total buffer length should match the value foretold by the first 4 bytes, and so remainder_length should exactly equal length once we have the full frame. Any leftover bytes are a sign of misuse, and then, we should clear the buffer and err out.
There was a problem hiding this comment.
any leftover bytes are a sign of misuse, and then, we should clear the buffer and err out.
Can you provide a scenario where any remaining left over bytes could be misused and how the decoder could be exploited?
See my other comment here: #5049 (comment)
binary_port/src/binary_message.rs
Outdated
|
|
||
| if length > self.max_message_size_bytes as usize { | ||
| return Err(Error::RequestTooLarge { | ||
| allowed: self.max_message_size_bytes, | ||
| got: length as u32, | ||
| }); | ||
| } | ||
|
|
||
| if remainder_length > self.max_message_size_bytes as usize { |
There was a problem hiding this comment.
I don't think we need to do this as I mentioned on slack. In my view, the remainder could be either empty, or could contain partial frame, or multiple frames, or frame + another partial frame etc. I think in some extreme cases of very slow senders this may reject legitimate connections. This check above is sufficient (length > self.max_message_size_bytes).
Keep in mind there are backpressure mechanisms at the TCP level that should prevent clients from sending gigabytes of garbage as long as you don't drain the socket
There was a problem hiding this comment.
i agree...i would want proof that this is necessary.
|
bors r+ |
|
Build succeeded: |
…sage. This PR should have no downstream effects.