Fix panic on malformed string length in Decode - #22
Conversation
| return nil, err | ||
| } | ||
| if stringLength < 0 { | ||
| return nil, errors.New("bad string length") |
There was a problem hiding this comment.
Include the stringLength in the error string, so that the error is more informative.
| buf := make([]byte, stringLength) | ||
|
|
||
| _, err = readAtLeast(data, buf, int(stringLength)) | ||
| // Read exactly stringLength bytes. Copy incrementally rather than |
There was a problem hiding this comment.
Avoid wordy comments like this. Is it possible that you are using a LLM to generate your code? If so, instruct it to use the correct go style, and manually fix up your commits to remove comments like this.
LLMs like to generate comments like this, to explain to the user what the code is doing, but it just clutters the code without adding any value.
|
Thanks, added the length to the error and trimmed the noisy comments down to match the file's style. |
| "testing" | ||
| ) | ||
|
|
||
| // A malformed string length (negative, or so large it overflows the slice |
There was a problem hiding this comment.
That's too brief -- please bring back the original commits comments on the tests.
| // declares a huge string. io.CopyN only grows the buffer to the number | ||
| // of bytes actually available, and reports a truncated string as an | ||
| // error. | ||
| // Copy incrementally so an untrusted length can't force a huge |
There was a problem hiding this comment.
Please remove this comment, it doesn't add anything.
|
Thanks! Could you update the pull request to include the latest round of feedback on comments, and also squash the three commits together to have just one commit? |
Decode panicked with makeslice: len out of range on tiny malformed inputs such as "-1:" (negative length) or "900000000000000000:" (length larger than the slice allocator allows). The string branch of the incremental unmarshaler parsed an attacker-controlled length and immediately called make([]byte, stringLength) with no validation, so a crafted length crashed the decoder or exhausted memory. Reject negative lengths and copy the string incrementally with io.CopyN so the buffer only grows to the number of bytes actually available, reporting a truncated string as an error instead of pre-allocating an untrusted size.
0862321 to
2bd19cf
Compare
|
Done, brought the test comments back, removed the one on incswparse.go, and squashed it to a single commit. |
2 participants
Decodepanics withmakeslice: len out of rangeon tiny malformed inputs, because the string branch of the incremental unmarshaler allocates an attacker-controlled length before validating it.Failure mode
The string case in
unmarshal(incswparse.go) parses a length prefix and immediately doesmake([]byte, stringLength)with no bounds check:A negative length or one larger than the slice allocator allows crashes the decoder (
makeslice: len out of range); a large positive length can also exhaust memory since the full size is allocated up front from a stream whose real length is unknown. Theparse.godecoder used byUnmarshalalready rejects negative lengths, but theDecodepath (which uses the incremental unmarshaler) does not.Repro
Fix
Reject negative lengths, and copy the string with
io.CopyNinto abytes.Bufferso the buffer only grows to the number of bytes actually available; a truncated string is reported asio.ErrUnexpectedEOFinstead of pre-allocating an untrusted size. Well-formed strings decode unchanged.Tests
Added
decode_stringlen_test.gocovering negative, oversized, and truncated lengths (all now return an error rather than panicking) plus a round-trip check that valid strings still decode. Fails before the change (panics), passes after; fullgo test ./...is green.