Bound XamarinCompressedFileLoader against crafted XALZ headers#3843
Open
christophwille wants to merge 2 commits into
Open
Bound XamarinCompressedFileLoader against crafted XALZ headers#3843christophwille wants to merge 2 commits into
christophwille wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the Xamarin XALZ compressed-module loader to avoid attacker-controlled allocations, partial-read issues, and parsing beyond the actual decompressed bytes when opening crafted files.
Changes:
- Adds header/payload validation and uses
ReadExactlyAsyncto avoid partial reads of the compressed payload. - Uses the
LZ4Codec.Decodereturned length to bound theMemoryStreamexposed toPEFile. - Adds NUnit tests covering malformed headers/payloads and a valid round-trip XALZ load.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| ICSharpCode.ILSpyX/FileLoaders/XamarinCompressedFileLoader.cs | Adds validation, exact reads, and bounds the decompressed stream to mitigate crafted XALZ inputs. |
| ICSharpCode.Decompiler.Tests/XamarinCompressedFileLoaderTests.cs | Introduces regression tests for crafted XALZ headers/payloads plus a happy-path load test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The Xamarin XALZ loader sized its buffer allocations from an attacker-controlled header field and ignored the partial-read length, so merely opening a crafted file (the loader is registered first and runs on any XALZ-magic input) could crash or over-allocate. The declared uncompressed length, a raw header uint cast to int, had no sanity bound: a tiny file claiming ~2 GB forced a giant ArrayPool.Rent (decompression-bomb amplification), and a high-bit value became negative and made Rent throw ArgumentOutOfRangeException. The compressed length was taken as the whole file (header included) and ReadAsync's return value was discarded, leaving stale pooled bytes in the tail fed to the decoder; the output MemoryStream then exposed the entire rented buffer, so PEFile parsed past the real decompressed data into leftover pool contents. Bound the declared length before renting (reject zero, > int.MaxValue, or more than an LZ4 block could expand from this payload at its 255x maximum ratio), read the payload that actually follows the header with ReadExactlyAsync, and slice the output to the length LZ4Codec.Decode reports. Malformed input now fails as a catchable InvalidDataException, consistent with the bundle and .rsrc hardening; well-formed Xamarin modules load exactly as before. Assisted-by: Claude:claude-opus-4-8:Claude Code
christophwille
force-pushed
the
christophwille/99
branch
from
de49f8e to
3dee49a
Compare
dgrunwald
reviewed
Jun 29, 2026
| using var fileReader = new BinaryReader(stream, Encoding.UTF8, leaveOpen: true); | ||
| // Read compressed file header | ||
| if (stream.Length < sizeof(uint)) | ||
| return null; |
Member
There was a problem hiding this comment.
Useless check (TOCTOU).
Catching the exception is the only proper way of handling IO errors.
Also, some kinds of streams might not know the length ahead-of-time.
Member
There was a problem hiding this comment.
Fixed in fc76fb5: the stream.Length pre-checks are gone; the loader now catches EndOfStreamException from each read instead (magic read returns null, truncated header or payload after a confirmed magic throws InvalidDataException). stream.Length is still used, but only as a sizing hint for the payload buffer; the exact read is guarded by the same exception handling.
(Written by an AI agent on behalf of @siegfriedpammer.)
Pre-checking stream.Length before reading is racy (the stream can shrink between check and read) and not every stream knows its length ahead of time. Instead, catch EndOfStreamException at each read: a stream too short for the magic is passed through as not-XALZ, and a truncated header or payload after a confirmed magic is rejected as InvalidDataException. stream.Length remains only a sizing hint for the payload buffer. Assisted-by: Claude:claude-fable-5:Claude Code
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
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 Xamarin "XALZ" compressed-module loader (
XamarinCompressedFileLoader) sized its buffer allocations from an attacker-controlled header field and ignored the partial-read return value. Because the loader is registered first and runs on any file beginning with theXALZmagic, merely opening a crafted file could crash the process or force a huge allocation.Three concrete defects: the declared uncompressed length is a raw header
uintcast tointwith no sanity bound, so a tiny file claiming ~2 GB forced a giantArrayPool.Rent(a decompression-bomb amplification), while a value with the high bit set became negative after the cast and madeRentthrowArgumentOutOfRangeException. The compressed length was taken as the whole file length (header included) andReadAsync's return value was discarded, so a short read left the tail of the rented buffer as stale pooled bytes fed to the LZ4 decoder. Finally, the outputMemoryStreamexposed the entire rented buffer, whichArrayPool.Rentmay size larger than the data, soPEFileparsed past the real decompressed bytes into leftover pool contents.The fix bounds the declared length before any allocation (rejecting zero, values above
int.MaxValue, or more than an LZ4 block could possibly expand from the payload at its 255x maximum ratio), reads exactly the payload that follows the 12-byte header withReadExactlyAsync, and slices the outputMemoryStreamto the length thatLZ4Codec.Decodeactually reports. Malformed input now surfaces as a catchableInvalidDataException, consistent with the recent bundle-signature and.rsrcresource-tree hardening, instead of crashing or over-allocating. Well-formed Xamarin modules continue to load unchanged.New tests in
ICSharpCode.Decompiler.Tests/XamarinCompressedFileLoaderTests.cscover the crafted cases (high-bit/negative declared length, an implausibly huge declared length rejected without allocating, and a corrupt LZ4 payload) plus a happy-path round trip that LZ4-compresses a real assembly, wraps it in a valid XALZ header, and asserts it still loads.🤖 Generated with Claude Code