zeroshade commented on code in PR #955:
URL: https://github.com/apache/arrow-go/pull/955#discussion_r3589006600
##########
arrow/ipc/message.go:
##########
@@ -227,6 +247,13 @@ func (r *messageReader) Message() (*Message, error) {
meta := flatbuf.GetRootAsMessage(buf, 0)
bodyLen := meta.BodyLength()
+ maxInt := int64(^uint(0) >> 1)
+ if bodyLen < 0 || bodyLen > maxInt {
+ return nil, fmt.Errorf("arrow/ipc: invalid message body length
%d", bodyLen)
+ }
+ if r.maxBodySize > 0 && bodyLen > r.maxBodySize {
Review Comment:
By default `maxBodySize` is `math.MaxInt` (set in ipc.go), so on 64-bit a
malformed stream advertising a huge-but-addressable `BodyLength` (e.g. `1<<40`)
passes this check and reaches `body.Resize(int(bodyLen))` at L260 — a ~1 TiB
allocation attempt — *before* `io.ReadFull` can discover the stream is
truncated. That's an allocation-based DoS from a single malformed message on
the default reader.
Suggest a sane default cap (cf. #953, which defaults to 64 MiB compressed /
256 MiB uncompressed) with an explicit opt-out for `0`, and/or bounding the
allocation to the bytes actually available. Also worth adding tests for
negative `BodyLength`, `BodyLength > MaxInt`, and a huge-but-addressable body
over the default limit — the current tests cover metadata-length prefixes and
file blocks but not stream body rejection.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
