fix(xmp): audit - bound decode recursion and growth, escape encoder output - #5405
Open
lgritz wants to merge 1 commit into
Open
fix(xmp): audit - bound decode recursion and growth, escape encoder output#5405lgritz wants to merge 1 commit into
lgritz wants to merge 1 commit into
Conversation
The shared XMP decoder is fed file-controlled bytes from jpeg, png, tiff, psd, and heif. Three issues fixed: - Recursion depth was tracked but never enforced, so a deeply nested XMP packet (easy to make tiny via a compressed png chunk) could crash the decoder with a stack overflow. Depth is now capped at 64 levels. - Building up a spec's attribute list was quadratic: each new list item re-read and re-wrote the entire accumulated value. A crafted file with a large attribute list could burn seconds of CPU and gigabytes of memory. Added a budget capping attribute count and total attribute payload size per spec. - The XMP encoder wrote attribute values straight into XML with no escaping, so a value read from an untrusted file could break out of its attribute and forge others, or produce malformed XML. Values are now escaped. Added fixtures covering all three cases. Known gap left as follow-up: decode_xmp() always returns true, so these caps truncate silently instead of surfacing an error. Assisted-by: Claude Code / claude-opus-5 Signed-off-by: Larry Gritz <lg@larrygritz.com>
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 shared XMP decoder is fed file-controlled bytes from jpeg, png, tiff, psd, and heif. Doing a Claude-assisted audit of this code found and fixed three issues:
Recursion depth was tracked but never enforced, so a deeply nested XMP packet (easy to make tiny via a compressed png chunk) could crash the decoder with a stack overflow. Depth is now capped at 64.
Building up a spec's attribute list was quadratic: each new list item re-read and re-wrote the entire accumulated value. A crafted file with a large attribute list could burn seconds of CPU and gigabytes of memory. Added a budget capping attribute count and total attribute payload size per spec.
The XMP encoder wrote attribute values straight into XML with no escaping, so a value read from an untrusted file could break out of its attribute and forge others, or produce malformed XML. Values are now escaped.
Added tests covering all three cases (and Python code to generate certain malformed files).
Note: currently, decode_xmp() always returns true, so these caps truncate silently instead of surfacing an error.
Assisted-by: Claude Code / claude-opus-5