Skip to content

Reject a hostile Content-Length instead of panicking - #24

Open
objevovat wants to merge 1 commit into
omarroth:mainfrom
objevovat:reject-bad-content-length
Open

Reject a hostile Content-Length instead of panicking#24
objevovat wants to merge 1 commit into
omarroth:mainfrom
objevovat:reject-bad-content-length

Conversation

@objevovat

@objevovat objevovat commented Jul 31, 2026

Copy link
Copy Markdown

The bug

A receiver answering with Content-Length: -1 crashes the sender.

parseHTTPHeader reads the value with Sscanf into an int and doesn't check it. In readPlaintextHTTPResponse the zero case is handled, but a negative value falls through to:

body := make([]byte, contentLength)   // panics: makeslice: len out of range

Nothing recovers it. I reproduced this end to end over a net.Pipe standing in for the receiver — the panic happens inside readPlaintextHTTPResponse, not in a helper.

Also bounded the positive case

Content-Length is receiver-controlled and was used unbounded, so Content-Length: 2147483647 asks the sender for a 2 GB allocation before a single body byte arrives. Control-channel bodies here are small plists, so the 8 MB limit is far above anything legitimate and far below anything that hurts.

Scope

Only these two sites. I checked the other four make([]byte, n) in the package and left them alone:

  • readEncryptedFrame already bounds its uint16 length and rejects anything over 16384
  • hkdfSHA512 and padTo take caller-supplied sizes, not network values
  • audio.go's n comes from a Read, so it can't be negative

Tests

Negative, large-negative and oversized headers, plus a well-formed response to show the normal path still reads its body. Removing the guard makes the first two fail with the original panic, so the tests genuinely cover it.

go test ./... is green.

How I found it

I was fuzzing the receiver-facing parsers — parseHTTPHeader, parseTXT, parseFeatures, parseCaptureFrames, parseXrandrGeometry — for about 11.8 million executions total. All clean; none of them crash. This one isn't reachable by fuzzing those functions in isolation, because the panic is at the use of the parsed value rather than in the parse. It turned up reading the allocation sites afterwards.

Happy to contribute the fuzz targets separately if you'd want them in the tree.

A receiver answering with "Content-Length: -1" crashes the sender. The
value is parsed with Sscanf into an int, is not checked, and reaches
make([]byte, contentLength) in readPlaintextHTTPResponse, which panics
with "makeslice: len out of range". Nothing recovers it.

Reproduced end to end over a net.Pipe standing in for the receiver: the
panic happens inside readPlaintextHTTPResponse, not in a helper.

Also bounds the positive case. Content-Length is attacker- or
bug-controlled and was used unbounded, so "Content-Length: 2147483647"
asks the sender for a 2 GB allocation before a single body byte arrives.
Control-channel bodies here are small plists; the 8 MB limit is far above
anything legitimate.

The other four make([]byte, n) sites in this package are fine and are
left alone -- readEncryptedFrame already bounds its uint16 length,
hkdfSHA512 and padTo take caller-supplied sizes, and audio.go's n comes
from a Read.

Tests cover negative, large-negative and oversized headers, plus a
well-formed response to show the normal path still works. Removing the
guard makes the first two fail with the original panic.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant