Guard against OOB read when bundle signature is at file start

[christophwille]

Summary

Fixes an out-of-bounds read in SingleFileBundle.IsBundle that can crash the
process when a crafted file is opened, plus a related unbounded allocation in
SingleFileBundle.ReadManifest.

The bug

ICSharpCode.Decompiler/SingleFileBundle.cs scans an opened file for the 32-byte
single-file-bundle signature and then reads the 8-byte header offset stored
immediately before it:

byte* end = data + (size - bundleSignature.Length);
for (byte* ptr = data; ptr < end; ptr++)
{
    if (*ptr == 0x8b && bundleSignature.SequenceEqual(new ReadOnlySpan<byte>(ptr, bundleSignature.Length)))
    {
        bundleHeaderOffset = Unsafe.ReadUnaligned<long>(ptr - sizeof(long)); // reads 8 bytes BEFORE ptr
        ...
    }
}

That header offset only exists in a genuine bundle, where the signature lives near
the end of the file. The scan, however, starts at ptr = data. Because the
file contents are fully attacker-controlled, placing the signature at file
offset 0-7 makes ptr - sizeof(long) point before the start of the buffer.

On the production path the buffer is a page-aligned memory-mapped view
(LoadedPackage.FromBundle -> BundleFileLoader, which runs on every opened
file), so data - 8 lands in the preceding, unmapped page and the read faults
with an AccessViolationException. That is a corrupted-state exception:
on .NET 5+ it is not delivered to managed catch (Exception) blocks, so the
loader pipeline's defensive catch (LoadedAssembly.LoadAsync) does not catch it
and the whole process terminates just from opening a ~30-byte crafted file.

Real-world impact

• Class: CWE-125 out-of-bounds read -> denial of service. It is a read, not
  a write: no memory corruption, no RCE. The read value is only used as a file
  offset that is immediately range-checked and is never returned to the attacker,
  so there is no practical information disclosure either.
• Where it matters: headless / server consumers of ICSharpCode.Decompiler
  and ilspycmd that decompile untrusted, user-submitted files (malware-analysis
  backends, "decompile every upload" services, CI scanners). One crafted file is a
  reliable crash-the-worker primitive, and because the crash is an uncatchable
  AccessViolationException it defeats the try/catch such services typically
  wrap around untrusted parsing. Interactive desktop ILSpy is low-stakes (the user
  just reopens the app).
• Reliability caveat: the crash requires the page before the mapping to be
  unmapped -- reliable on Windows (64 KB-aligned mappings), layout-dependent on
  Linux/macOS (sometimes a silent benign OOB read instead). Reproduced as a real
  fatal AccessViolationException (process exit 134) on the production
  memory-mapped path.

The fix

1. OOB read: skip the backward read unless the match is at least sizeof(long)
   bytes into the buffer (ptr - data >= sizeof(long)). This changes behavior only
   for malformed input that would otherwise fault; well-formed bundles (signature
   near end of file) are unaffected, and the public API is unchanged.

2. Unbounded FileCount (same file, same crafted-bundle chain):
   ReadManifest fed header.FileCount = reader.ReadInt32() straight into
   ImmutableArray.CreateBuilder<Entry>(FileCount) with no bound, so a crafted
   manifest (FileCount = 0x7FFFFFFF) requested ~80 GB before reading any entry.
   It is now bounded against the bytes that can possibly remain (each entry
   occupies >= 1 byte) and throws InvalidDataException instead.

Tests

New ICSharpCode.Decompiler.Tests/SingleFileBundleTests.cs (NUnit), written
test-first (red -> green):

• IsBundle_SignatureAtStart_DoesNotReadBeforeBuffer -- places a sentinel where
  the out-of-bounds read would land; pre-fix the scanner returned it as a valid
  header offset, post-fix the match is rejected without the read. (Uses the public
  unsafe IsBundle(byte*, long, out long) overload with a managed buffer, so the
  out-of-bounds read stays on the GC heap and the test runner never faults --
  deterministic red/green on all platforms.)
• IsBundle_ValidLayout_DetectsBundle -- a well-formed signature preceded by its
  header offset is still detected (guards against over-correction).
• ReadManifest_HugeFileCount_Throws -- a manifest with FileCount = int.MaxValue
  now throws InvalidDataException rather than attempting a huge allocation.

[Copilot]

Pull request overview

This pull request hardens the single-file bundle detection/manifest parsing logic in ICSharpCode.Decompiler against crafted inputs that could otherwise crash the process or trigger excessive memory allocation when opening untrusted files.

Changes:

• Added a guard in SingleFileBundle.IsBundle to prevent reading the 8-byte header offset from before the start of the buffer when a signature match occurs at file offset 0–7.
• Added validation in SingleFileBundle.ReadManifest(Stream) to reject negative or implausibly large FileCount values before pre-sizing the entry array.
• Introduced NUnit tests covering the OOB-read scenario, a valid bundle layout, and the oversized FileCount case.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
ICSharpCode.Decompiler/SingleFileBundle.cs	Adds bounds checks to prevent OOB reads during signature scanning and caps FileCount to avoid pathological allocations.
ICSharpCode.Decompiler.Tests/SingleFileBundleTests.cs	Adds regression tests to ensure the crash/large-allocation cases are rejected and valid layouts still work.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.