Repository navigation
security: validate uploaded image bytes before decoding (decompression bomb) - #13
Merged
Merged
Conversation
verifyIntegrity checked only the declared size against R2's stored Content-Length plus a client-supplied SHA-256, and validateUpload matched contentType -- a client-supplied string -- against the allowlist. Nothing ever looked at the bytes. The object then went straight into Thumbnails.of(...) (Thumbnailator -> ImageIO), and ImageProcessingProperties bounded only the OUTPUT thumbnail, so a small file advertising an enormous resolution in its header could ask ImageIO to allocate a multi-gigabyte raster on the async pool. New ImageValidationService sniffs the container from magic bytes (JPEG/PNG/GIF/WEBP/HEIC), requires it to be on the allowlist AND to match what the client declared, then reads width and height off the header via ImageIO.getImageReaders and rejects anything above a configurable megapixel ceiling (default 50) -- all before any decode happens. No new dependency; the JDK's readers plus a small signature table are enough. The rejection message names only the declared type: echoing the sniffed one back would tell a prober which signatures the server recognises. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011xQusSt39vP3c82gAUXRCm
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Collaborator
Author
|
This is fine and can be merged safely |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
Nothing ever looked at the bytes of an uploaded file.
UploadWorkflowService.verifyIntegritychecked the declared size against R2's storedContent-Lengthand a client-supplied SHA-256. Both measure length, neither inspects content.UploadLimitsService.validateUploadmatchedcontentType— a client-supplied string — againstuploadProperties.getAllowedFileTypes().Thumbnails.of(new ByteArrayInputStream(originalBytes))(Thumbnailator → ImageIO).ImageProcessingPropertiesbounded only the output thumbnail; nothing bounded the input.Two consequences. A file could declare
image/png, be a ZIP, and reach the decoder. And a PNG whose IHDR advertises an enormous resolution could be a few hundred bytes on the wire while asking ImageIO to allocate a multi-gigabyte raster on the async thumbnail pool — a classic decompression bomb, reachable viapresign→ PUT →completebecause the size and hash checks both pass.The fix
A new
ImageValidationService:ftypbrand), requires the result to be on the allowlist, and requires it to match what the client declared.ImageIO.getImageReaders/getWidth(0)/getHeight(0)and rejects anything above a configurable megapixel ceiling (imageprocessing.max-megapixels, default 50) — before any decode happens.No new dependency: the JDK's own readers plus a small signature table are enough, so no Tika. The rejection message names only the declared type — echoing the sniffed one back would tell a prober exactly which signatures the server recognises.
Verification
The bomb fixture is the real shape, not a mock: a valid PNG header advertising 40000×40000 (1.6 gigapixels) in under 1 KB, asserted to be rejected without its image data ever being decoded. Also covered: a ZIP declared as PNG, a genuine PNG declared as JPEG, empty and truncated input, that the message does not leak the sniffed type, and that the ceiling is actually configurable (4 MP passes at 50 MP, fails at 1 MP).
Verified by test rather than the live
presign→ PUT →completeflow, which needs a reachable R2 bucket this machine does not have.🤖 Generated with Claude Code
https://claude.ai/code/session_011xQusSt39vP3c82gAUXRCm