fix: validate FileSize in NewDataBuilder to prevent OOM DoS#25710
Conversation
…vent OOM DoS NewDataBuilder allocated make([]byte, 0, req.FileSize) using the client-supplied int64 with no upper bound. A crafted DRPC message with FileSize=1<<40 forces a 1 TiB allocation, triggering an unrecoverable runtime.throw that kills coderd. Add MaxFileSize (100 MiB) constant and reject negative or oversized FileSize and negative Chunks before the allocation.
|
/coder-agents-review |
|
Review posted | Chat Review history
deep-review v0.5.0 | Round 1 | Last posted: Round 1, 6 findings (2 P3, 2 P4, 2 Nit), COMMENT. Review Finding inventoryFindings
Contested and acknowledged(none) Round logRound 1Panel. 2 P3, 2 P4, 2 Nit new. 3 Notes dropped. Reviewed against 20b50dd..cc2d2a8. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Well-placed fix. The vulnerability diagnosis is accurate: an untrusted int64 reaches make([]byte, 0, req.FileSize) and the DRPC wire cap does not bound the integer value. The constant matches HTTPFileMaxBytes by expression, the validation is in the single constructor that gates both callers, and the sibling make audit is confirmed. Pariston ran four alternative framings and couldn't build a case against the approach. Commit message and PR description are strong.
Severity count: 2 P3, 2 P4, 2 Nit.
The two P3s are the boundary test gap at exactly MaxFileSize (off-by-one regression risk) and the broken round-trip contract between BytesToDataUpload and NewDataBuilder for inputs exceeding the new limit. The P4s are the missing Chunks upper bound (5 reviewers flagged this, practical impact bounded by DRPC transport) and a pre-existing reversed subtraction in the Add() error message.
"The contract is broken: 'encode then decode' is no longer a guaranteed identity for all valid Go []byte inputs." (Mafuuu)
provisionersdk/proto/dataupload.go:74
P4 [CRF-4] Pre-existing: when expectedSize > b.Size, the computation b.Size - int64(expectedSize) yields a negative number. The error message reports "data is now 110 bytes, -10 bytes over the limit of 100" instead of "10 bytes over the limit." Should be int64(expectedSize) - b.Size.
Not introduced by this PR, but same file and same error handling path.
Chopper: "An operator receiving 'data is now 110 bytes, -10 bytes over the limit of 100' has to mentally invert the sign."
(Chopper P4)
🤖
🤖 This review was automatically generated with Coder Agents.
CRF-1 (P3): Add ExactMaxFileSize boundary test at exactly MaxFileSize. CRF-2 (P3): Add error return to BytesToDataUpload so the encode/decode round-trip contract holds; data exceeding MaxFileSize is rejected at the sender. Update all 5 callers. CRF-3 (P4): Add Chunks upper-bound validation; chunk count must be consistent with FileSize and ChunkSize. CRF-4 (P4): Fix pre-existing reversed subtraction in Add() error message (b.Size-int64(expectedSize) to int64(expectedSize)-b.Size).
NewDataBuilderallocatedmake([]byte, 0, req.FileSize)using the client-suppliedint64with no upper-bound check. The DRPC 4 MiB wire cap limits message size but not the integer value, so a crafted message withFileSize = 1<<40forces a 1 TiB allocation, triggering an unrecoverableruntime.throwthat kills the entirecoderdprocess.Add a
MaxFileSizeconstant (100 MiB, matchingHTTPFileMaxBytesincoderd/files.go) and reject negative or oversizedFileSize, plus negative or excessiveChunks, before the allocation.BytesToDataUploadalso returns an error for oversized data to preserve the encode/decode round-trip contract. Fix a pre-existing reversed subtraction in theAdd()overflow error message.Closes https://linear.app/codercom/issue/PLAT-231
Implementation details
provisionersdk/proto/dataupload.go: New exportedMaxFileSizeconstant; validation inNewDataBuilderandBytesToDataUpload. Fixed reversed subtraction inAdd()error.provisionersdk/proto/dataupload_test.go: NewTestNewDataBuilderValidationwith 7 subtests.BytesToDataUploadfor new error return.make([]byte, ...)in provisioner paths; no other client-supplied sizes.