Reject malformed element counts in message decoders - #451
Conversation
Several decoders read a repeated-element count as a signed i16/i32 and pass it to Vec::with_capacity before reading any element. A negative count (0xffff read as i16 = -1) sign-extends to usize::MAX and panics with "capacity overflow", so a short crafted message crashes the peer decoding it (the shipped client/proxy on backend messages, the server on Bind). sunng87#192 fixed this for Parse, Bind and ParameterDescription by widening the count to u16 but left RowDescription, the three Copy*Response messages, NegotiateProtocolVersion, and Bind's result_column_format_codes. Read those counts as unsigned and route them through a shared codec::read_count that rejects a count exceeding the bytes remaining, turning a malformed message into a decode error instead of a panic.
| /// collection. Counts are unsigned; a value larger than the bytes left in the | ||
| /// buffer cannot describe a real message (each element takes at least one byte), | ||
| /// so it is rejected instead of driving an oversized `Vec::with_capacity`. | ||
| pub(crate) fn read_count(count: usize, buf: &BytesMut) -> PgWireResult<usize> { |
There was a problem hiding this comment.
this count is element count, but here we are comparing it with byte count, which need to be corrected. For reading bytes this is ok, but can be wrong for numbers.
let's rename this function to ensure_count and return PgWireResult<()> so we won't need to get count as u16, then convert back to i16 again.
There was a problem hiding this comment.
Done in 074ca57 — renamed to ensure_count, returns PgWireResult<()>, and callers keep the wire count as-is (single conversion). 150 tests pass.
| /// so it is rejected instead of driving an oversized `Vec::with_capacity`. | ||
| pub(crate) fn ensure_count(count: usize, buf: &BytesMut) -> PgWireResult<()> { | ||
| let remaining = buf.remaining(); | ||
| if count > remaining { |
There was a problem hiding this comment.
as I said, we cannot simply compare count with remaining. this count is about how many elements, for example, in Bind, it's the number of Int16s, so to compare with remaining we will need to multiply 2
There was a problem hiding this comment.
Fixed in e6dac70 — ensure_count now takes the per-element size and compares count × elem_size against the remaining bytes (2 for Int16 format codes, 1 for startup option names, 19 for row-description fields). Added a Bind case with 5 codes over 6 bytes to the rejection test. 150 tests pass.
Counts on the wire are element counts, not byte counts: Bind's result format codes are Int16s, so a count that fits the remaining bytes can still overrun the buffer when read as 2-byte elements. ensure_count now takes the per-element size and rejects count * size > remaining.
2 participants
Several message decoders read a repeated-element count as a signed
i16/i32and hand it toVec::with_capacitybefore reading any element. A negative count (0xffffasi16= -1) sign-extends tousize::MAXand panics withcapacity overflow, so a ~7-byte crafted message crashes whoever decodes it — the shipped client/proxy on a backend message, or the server onBind.Repro (debug and release):
PgWireBackendMessage::decode(&mut BytesMut::from(&b"T\0\0\0\x06\xff\xff"[..]), &ctx)→capacity overflow.#192 fixed this for
Parse,BindandParameterDescriptionby widening the count tou16but stopped short of the siblings that share the shape. This completes it for the six missed:RowDescription,CopyInResponse/CopyOutResponse/CopyBothResponse,NegotiateProtocolVersion(re-introduced in #281), andBind'sresult_column_format_codes(#192 widened the other two counts in that same function, not this third).The counts are read unsigned and routed through one shared
codec::read_countthat rejects a count larger than the remaining bytes, turning a malformed message into a decodeErr. A single helper also keeps the pattern from silently reappearing as it did in #281; and reading unsigned alone isn't enough for thei32option count, whereu32::MAXstill forces a huge allocation, so the bound is applied uniformly. The four counts #192 already widened are left untouched.Tests exercise all six sites through the public
decodeAPI, plus zero-count and well-formed positive-count controls.