Musitdev/prevent untrusted memory allocation - #373
Conversation
…om untrusted length field - add GuardedReadStream wrapper that fails if any single read request exceeds a configurable bound - verify that a PROXY v2 header declaring address_size=u16::MAX never triggers a read larger than the bound
…ize reads and unified drain - determine bytes to parse upfront from the protocol family (compile-time constant, never the wire value) - read only that fixed portion into a stack buffer; parse the result after the stream is fully consumed - drain all remaining declared bytes in a single bounded loop, covering every branch uniformly
seanyoung
left a comment
seanyoung
left a comment
There was a problem hiding this comment.
Some suggestions but looks good to merge
| // Drain whatever the sender declared beyond what we parsed, in bounded chunks. | ||
| // This single drain covers every branch: LOCAL/UDP/UNIX (all bytes), IPv4/IPv6 | ||
| // with exact size (zero bytes), IPv4/IPv6 with extra padding, and error cases. | ||
| let remaining = address_size.saturating_sub(fixed_parse_size); | ||
| let mut scratch = [0u8; 256]; | ||
| let mut left = remaining as usize; | ||
| while left > 0 { | ||
| let take = left.min(scratch.len()); | ||
| stream.read_exact(&mut scratch[..take]).await?; | ||
| left = left.saturating_sub(take); | ||
| } |
There was a problem hiding this comment.
if someone is sending more data, they are likely trying to do something bad. Can't we just error out in this case? It will make the code much simpler.
There was a problem hiding this comment.
Probably but it can be a bug in the address formatting and the rest of the stream is correct. In the protocol they allow more data in the address section. In Aptos they just throw it away I keep the same logic.
|
I have a question. The most an attacker can allocate is 64KiB. That's not very much at all and I don't understand how that could be abused. Is this change worthwhile? |
|
Why is the allocation limited to 64KB ? The size is defined in the message, and the old code does the allocation with the defined size. If an attacker put any big value in the message, with the old version, the code does the allocation. |
|
because let address_size = u16::from_be_bytes(address_size);
let mut address_bytes: Vec<u8> = vec![0; address_size as usize]; |
|
Ah yes, so the risk is more sort of dos attacks where several messages with 64k allocation are sent. |
2 participants
Description
Reimplementation of this PR fix
Correct an unbounded heap allocation from untrusted wire field in PROXY protocol parser.
How Has This Been Tested?
Key Areas to Review
Type of Change
Which Components or Systems Does This Change Impact?
Checklist