Add OneFuzz harness for ZipRangeExtractor and fix a bounds gap it found

[Alexandre Zollinger Chohfi (azchohfi)]

Why

ZipRangeExtractor is the only hand-written parser in winapp that consumes bytes originating off the network. WinDbgJsProviderAcquirer range-downloads the WinDbg .msixbundle and parses its ZIP64 central directory — then a nested inner .msix — to locate JsProvider.dll. That parsing necessarily happens before the extracted DLL's Authenticode signature can be checked, so the archive bytes are untrusted at parse time.

The SDL fuzzing requirement applies to memory-safe languages that use unsafe code or P/Invoke, which winapp does extensively (pointer dereferencing in SlugGenerator/CrashDumpService, P/Invoke into wintrust.dll, d3d11.dll, catalog crypto APIs). This is the one parser sitting on an untrusted-input boundary.

The bug this found

ParseCentralDirectory read three attacker-controlled 16-bit lengths — name, extra, comment — and sliced on all of them while only guarding the 46-byte fixed header:

ArgumentOutOfRangeException x608  (of 3000 seeded mutations)
"Index and count must refer to a location within the buffer."

It now rejects malformed directories with InvalidDataException. To be precise about severity: .NET's bounds checks caught this, so it is not memory corruption, and TryAcquireCoreAsync already swallowed it and degraded gracefully. The value is a correct rejection contract, plus a clean baseline for the fuzzer.

What's here

• WinApp.Cli.Fuzz — two libFuzzer targets: the pure central-directory parser (high iteration rate) and the full outer → inner → extract descent mirroring ExtractJsProviderAsync.
• OneFuzzConfig.json — fuzzer.$type, FuzzingTargetBinaries: winapp.dll so the compliance claim attributes to the shipping binary, and SdlWorkItemId linking claims and bugs to the SDL task.
• .pipelines/fuzz.yml — trigger: none / pr: none. Fuzzing runs on demand, not per-build; jobs run for hours and file bugs.
• FuzzHarnessTests — guards that the target signature and OneFuzzConfig.json stay in sync with the code, generates a seed corpus, and asserts every seed is structurally valid.

Verification

Both targets ran on OneFuzz against a seeded corpus:

Target	Job	Iterations	Crashes
ziprangeextractor-centraldirectory	f1646fe1-be66-48ed-b9d5-3de43e98dcd0	~155,000,000	0
ziprangeextractor-archive	2c2fc7a1-4c80-409c-81ac-8660a4934659	~127,000,000	0

Submission dc437f41-cbf4-49eb-af73-bfc24b224536, 2/2 job configs succeeded, no task errors.

The seed corpus matters more than it looks. libFuzzer mutates existing inputs and will not synthesise a valid ZIP64 central directory from random bytes — an unseeded run looks busy and covers nothing. GenerateSeedCorpus round-trips every seed through the parser and fails if any produces no entries, because a corpus the parser rejects at byte zero is worse than none.

Local: 25/25 tests pass, build-cli.ps1 clean.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Note

This error may be related to your runner configuration. You can now configure runners for Copilot code review separately from Copilot cloud agent by creating a copilot-code-review.yml file with your setup steps. Read the docs for details.

[github-actions]

Build Metrics Report

Binary Sizes

Artifact	Baseline	Current	Delta
CLI (ARM64)	38.62 MB	38.63 MB	📈 +7.5 KB (+0.02%)
CLI (x64)	38.74 MB	38.74 MB	📈 +7.0 KB (+0.02%)
MSIX (ARM64)	16.02 MB	16.02 MB	📈 +0.7 KB (+0.00%)
MSIX (x64)	17.02 MB	17.02 MB	📈 +5.8 KB (+0.03%)
NPM Package	33.42 MB	33.43 MB	📈 +5.1 KB (+0.01%)
NuGet Package	33.46 MB	33.46 MB	📈 +5.6 KB (+0.02%)

Test Results

✅ 4596 passed, 5 skipped out of 4601 tests in 612.8s (+20 tests, -1.0s vs. baseline)

Test Coverage

✅ 89.1% line coverage, 82.4% branch coverage · ✅ no change vs. baseline

CLI Startup Time

49ms median (x64, winapp --version) · ✅ no change vs. baseline

Try This Build

Installs the MSIX for your architecture, replacing any previously installed build. Needs the GitHub CLI — the command offers to install it and sign you in if it is missing.

& ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) 765

Switching between builds often?

Put the tool on your PATH once:

& ([scriptblock]::Create((irm https://raw.githubusercontent.com/microsoft/winappCli/main/scripts/winapp-pr.ps1))) -AddToPath

Then this build is just:

winapp-pr 765

Run winapp-pr with no arguments to pick from a list of open PRs.

---

Updated 2026-08-18 08:45:30 UTC · commit 1a6df98 · workflow run

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/winapp-CLI/WinApp.Cli.Fuzz/FuzzableCode.cs:118

• Suppressing every ArgumentOutOfRangeException masks the same class of parser bounds gaps this harness is intended to find. For example, an input ending with only the EOCD signature reaches tail.AsSpan(eocd + 10) in FindCentralDirectoryAsync and throws this exception, but OneFuzz reports it as a normal rejection. Translate out-of-range reads to a dedicated expected rejection at the reader boundary, then let parser-originated ArgumentOutOfRangeException escape.

        or ArgumentOutOfRangeException;  // range read outside the archive, which both range readers reject

src/winapp-CLI/WinApp.Cli.Fuzz/FuzzableCode.cs:81

• BuildNestedBundle puts AppxMetadata/AppxBundleManifest.xml before the inner MSIX. This first iteration treats the XML as an archive, FindCentralDirectoryAsync rejects it, and the method-level catch exits the entire loop, so the seeded target never reaches the nested MSIX or its entries. Catch expected rejection per candidate and continue to the next outer entry.

This issue also appears on line 118 of the same file.

                VisitAsInnerArchive(reader, inner);

[Alexandre Zollinger Chohfi (azchohfi)]

Follow-up on the Copilot review

Both of the suppressed comments were correct, and they were the two most valuable findings on this PR. Fixed in the latest push.

1. The archive target was swallowing the bug class it exists to find.

IsExpectedArchiveRejection listed ArgumentOutOfRangeException, on the reasoning that a range read past the end of the archive is a normal rejection. It is - but it was collapsing two very different things: "the parser asked the reader for bytes past EOF" (fine, malformed input) and "the parser indexed past a buffer it already holds" (a bounds bug). The fix is the one suggested: translate the reader-level case, then let parser-originated ones escape. BoundedRangeReader now converts an out-of-range read into InvalidDataException, and ArgumentOutOfRangeException is no longer in the expected set.

2. There was a second, real bounds gap behind it - the exact one called out.

FindCentralDirectoryAsync locates the EOCD signature and then reads tail.AsSpan(eocd + 10 / +12 / +16) without ever checking that the full 22-byte record fits. Since the signature can legitimately appear in the final 21 bytes, any archive whose tail ends in PK\x05\x06 walks off the buffer. It now rejects with InvalidDataException, matching the ParseCentralDirectory fix already in this PR.

This is not theoretical. Disabling only the new guard and re-running the mutation campaign:

Assert.AreEqual failed. Expected:<0>. Actual:<29>.
FuzzArchive let 29 unexpected exception(s) escape:
ArgumentOutOfRangeException x29 (e.g. ... (Parameter 'length'))

29 of 3000 mutations reach it. With the guard, clean. That is also the proof that the harness is no longer blind to this class - before this change all 29 were silently absorbed as "expected".

Production is not crashing on this today: WinDbgJsProviderAcquirer.TryAcquireCoreAsync has a catch-all and degrades gracefully. Which is really the point - because production tolerates everything, "what production tolerates" was the wrong thing to model the filter on. The harness has to enforce a tighter contract than the caller does, or it validates nothing.

3. The archive target was also much shallower than intended.

BuildNestedBundle puts AppxMetadata/AppxBundleManifest.xml first, and the try wrapped the whole foreach. So the first entry (XML, not an archive) threw, the catch outside the loop swallowed it, and the walk ended before ever reaching the nested .msix or its entries. The catch is now per entry, so a non-archive entry no longer ends the walk.

This one has a consequence beyond the code: the OneFuzz run I recorded on ziprangeextractor-archive (126,910,485 iterations, 0 crashes) was exercising a shallower path than intended, so that clean result is weaker evidence than it looks. The central-directory target is unaffected. I would re-run the archive job against the merged commit before treating its result as evidence.

The remaining inline comment about Describe being evaluated eagerly is answered in-thread - it was not correct for MSTest v4, but I made the helper total anyway.

[Copilot]

Pull request overview

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

Suppressed comments (3)

src/winapp-CLI/WinApp.Cli.Tests/FuzzHarnessTests.cs:172

• These output directories are reused across test runs but never cleared. If a seed is renamed or removed, its old file remains under bin, and the final file-count assertions fail until the developer manually cleans the build output. Delete the corpus root before recreating the two directories so generation is deterministic.

        var root = Path.Join(AppContext.BaseDirectory, "fuzz-corpus");
        var archiveDir = Path.Join(root, "ziprangeextractor-archive");
        var directoryDir = Path.Join(root, "ziprangeextractor-centraldirectory");
        Directory.CreateDirectory(archiveDir);
        Directory.CreateDirectory(directoryDir);

src/winapp-CLI/WinApp.Cli/Helpers/ZipRangeExtractor.cs:69

• The last raw EOCD signature is not necessarily the EOCD record: an archive comment may legally contain PK\x05\x06. If the comment ends with that sequence, eocd points at the final four bytes and this guard rejects an otherwise valid archive instead of continuing backward to the real record. Scan backward for a candidate with a complete fixed record whose declared comment length reaches the archive end, and add a regression test with the signature in a valid comment.

        // The signature can appear in the last 21 bytes, leaving the fixed fields below truncated.
        if (eocd + MinEocdSize > tail.Length)
        {
            throw new InvalidDataException(
                $"End-of-central-directory record at offset {eocd} is truncated: {tail.Length - eocd} of {MinEocdSize} bytes present.");

src/winapp-CLI/WinApp.Cli.Tests/FuzzHarnessTests.cs:107

• This only verifies that the array is nonempty, so changing it to a non-shipping binary such as WinApp.Cli.Fuzz.dll would still pass while OneFuzz no longer attributes the compliance claim to winapp.dll. Assert that winapp.dll is present, which is the invariant described by the message and PR.

This issue also appears on line 168 of the same file.

            Assert.IsTrue(target.GetProperty("FuzzingTargetBinaries").GetArrayLength() > 0,
                "FuzzingTargetBinaries must name the shipping binary, or OneFuzz generates no claim.");

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/winapp-CLI/WinApp.Cli/Helpers/ZipRangeExtractor.cs:238

• This still accepts a complete EOCD-shaped sequence inside a legal ZIP comment. For example, append a 22-byte zero-field EOCD as the comment and set the real EOCD's comment length to 22: the reverse scan accepts the fake record at the start of the comment and returns offset/size zero, so a valid non-empty archive parses as empty. The added test covers only a four-byte truncated signature. Candidate selection needs to validate the referenced central directory (and ZIP64 chain where applicable) and continue scanning when that validation fails.

            var commentLen = BinaryPrimitives.ReadUInt16LittleEndian(tail.Slice(i + 20));
            if (i + MinEocdSize + commentLen == tail.Length)
            {
                return i;

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/winapp-CLI/WinApp.Cli.Tests/FuzzHarnessTests.cs:188

• Every generated seed here is a small ZipArchive, so it uses ZIP32 EOCD and 32-bit entry fields. The corpus therefore does not seed either the ZIP64 locator/record path or ZIP64 extra-field parsing, despite the harness targeting this ZIP64 parser and the remarks noting libFuzzer will not synthesize that structure. Add at least one valid ZIP64 archive seed (and its carved directory seed), ideally including the nested path.

        var seeds = new Dictionary<string, byte[]>
        {
            ["minimal-stored"] = BuildZip([("a.bin", Encoding.UTF8.GetBytes("stored"), CompressionLevel.NoCompression)]),
            ["deflate"] = BuildZip([("a.bin", FakePe(), CompressionLevel.Optimal)]),
            ["multi-entry"] = BuildZip([("a.bin", Encoding.UTF8.GetBytes("one"), CompressionLevel.NoCompression),

src/winapp-CLI/WinApp.Cli.Fuzz/FuzzableCode.cs:161

• The wrapped reader checks offset + length, which can overflow for a positive ZIP64-derived offset near long.MaxValue; that bypasses its guard and Array.Copy can then throw ArgumentException. This catch misses that range failure, so the harness reports a false parser crash even though its contract says out-of-archive reads become InvalidDataException. Catch the base argument exception from this byte-array reader, or validate the range with an overflow-safe comparison first.

        catch (ArgumentOutOfRangeException ex)

[Copilot]

Pull request overview

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

Suppressed comments (3)

src/winapp-CLI/WinApp.Cli/Helpers/ZipRangeExtractor.cs:74

• A legal ZIP comment can contain a fake EOCD with a ZIP64 marker. If that candidate has no valid locator/record, ReadZip64DirectoryAsync throws and exits the loop, so the scanner never reaches the real earlier EOCD and rejects an otherwise valid archive. Treat an invalid ZIP64 record as a rejected candidate and continue scanning; only return after validating the resolved directory.

            {
                // The ZIP64 record carries its own signature, so that path rejects a fake for us.
                (cdOffset, cdSize) = await ReadZip64DirectoryAsync(
                    reader, archiveBase, tail, tailStart, eocd, cancellationToken);
                return (archiveBase + cdOffset, cdSize);

src/winapp-CLI/WinApp.Cli/Helpers/ZipRangeExtractor.cs:107

• Checking only the first four bytes does not confirm the candidate's declared directory. A fake EOCD in the comment can set count = 1, cdSize = 4, and cdOffset to the real directory; this check succeeds, the caller parses only four bytes as zero entries, and the real EOCD is never considered. Validate the complete declared size/count (or another structural relationship to the candidate EOCD) before accepting it.

            return false;
        }

        var head = await reader.ReadAsync(archiveBase + cdOffset, 4, cancellationToken);
        return BinaryPrimitives.ReadUInt32LittleEndian(head) == CentralHeaderSignature;

src/winapp-CLI/WinApp.Cli.Tests/FuzzHarnessTests.cs:244

• These ToZip64 seeds exercise only the ZIP64 EOCD path. The helper leaves every central-directory size/offset as a normal 32-bit value and adds no 0x0001 extra field, so ApplyZip64Extra is never reached despite the stated corpus goal. Add a seed whose central header uses 0xFFFFFFFF markers and carries a valid ZIP64 extra field.

    /// <see cref="ZipArchive"/> only emits ZIP64 for archives too large to build here, so without this
    /// the corpus never reaches the ZIP64 locator/record path or the ZIP64 extra field — the parts a
    /// mutator cannot synthesise on its own.

[Copilot]

Pull request overview

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

Suppressed comments (4)

src/winapp-CLI/WinApp.Cli/Helpers/ZipRangeExtractor.cs:86

• This ZIP64 path returns immediately without the confirmation applied to ZIP32 candidates. Because ZIP comments may contain arbitrary bytes, a comment can legally contain a forged ZIP64 EOCD record, locator, and marker-bearing EOCD; the signature checks all pass and the attacker-controlled directory offset/size wins over the real EOCD. Validate that the resolved directory is in the archive, begins with a central header, and ends at the referenced ZIP64 record before accepting this candidate.

                return (archiveBase + cdOffset, cdSize);

src/winapp-CLI/WinApp.Cli/Helpers/ZipRangeExtractor.cs:286

• This leaves two consecutive <summary>/<remarks> blocks on NextEocdCandidate; the first is stale documentation from the removed finder and causes generated API documentation to expose duplicate summaries. Keep only the block that documents the current method.

    /// <summary>
    /// Finds the end-of-central-directory record in the archive tail.
    /// </summary>

src/winapp-CLI/WinApp.Cli/Helpers/ZipRangeExtractor.cs:83

• A fake marker-bearing candidate can fail with more than InvalidDataException: its locator's 64-bit record offset is used for ReadAsync without bounds validation, so an invalid/overflowed offset produces ArgumentOutOfRangeException for the memory reader and ultimately IOException for the HTTP reader. Those exceptions abort the scan instead of continuing to the genuine EOCD earlier in an otherwise valid archive comment. Validate the locator/record range and convert candidate-format failures to InvalidDataException before reading, while preserving genuine transport failures.

This issue also appears on line 86 of the same file.

                catch (InvalidDataException)
                {
                    // A marker-bearing record planted in a comment has no usable locator or record;
                    // keep scanning rather than failing the whole archive.
                    continue;

src/winapp-CLI/WinApp.Cli/Helpers/ZipRangeExtractor.cs:337

• Although the new guard avoids overflowing offset + length, the exception message still performs that same addition. For the newly tested near-long.MaxValue case it reports a wrapped negative end offset, making the diagnostic misleading. Format the start and length separately instead.

        if (offset < 0 || length < 0 || offset > data.Length || length > data.Length - offset)

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/winapp-CLI/WinApp.Cli/Helpers/ZipRangeExtractor.cs:101

• The empty fallback accepts an unvalidated offset and preserves the newest fake candidate because of ??=. For example, a zero-entry EOCD with cdSize == 0 and cdOffset == uint.MaxValue is returned as a successful location even though it is outside the archive; in an empty archive with a fake EOCD in its comment, the fake also wins over the earlier real record. Require the empty directory to end at this EOCD and let the earlier candidate replace a later comment candidate.

            if (count == 0 && cdSize == 0)
            {
                emptyFallback ??= (archiveBase + cdOffset, 0);
                continue;

src/winapp-CLI/WinApp.Cli/Helpers/ZipRangeExtractor.cs:104

• Every syntactically valid candidate that reaches this branch performs another IRangeReader read before scanning continues. A 64 KiB tail can contain roughly 3,000 consecutive 22-byte EOCD candidates whose comment lengths all run to EOF and whose directory heads are invalid; with the production HttpRangeReader, one malformed archive therefore triggers thousands of sequential HTTP range requests. Reuse the already-downloaded tail for heads that fall inside it and place a bound or batching strategy around any external candidate probes.

            // In a ZIP32 archive the directory runs right up to the record describing it.
            if (await IsCentralDirectoryAsync(reader, archiveBase, archiveSize, cdOffset, cdSize, tailStart + eocd, cancellationToken))

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/winapp-CLI/WinApp.Cli.Tests/ZipRangeExtractorTests.cs:330

• These candidates do not exercise the ranged-read cap: pos remains within the last 20,000 bytes, so pos - 46 is also inside the 65,557-byte tail and IsCentralDirectoryAsync returns with Probed == false. The test therefore passes with only the initial tail read even if the probe limiter is removed. Make the directory start outside the tail while preserving cdOffset + cdSize == pos.

            WriteU32(data, pos + 12, 46);
            WriteU32(data, pos + 16, (uint)(pos - 46));

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/winapp-CLI/WinApp.Cli/Helpers/ZipRangeExtractor.cs:85

• A marker-bearing candidate is charged even when ReadZip64DirectoryAsync fails because no locator exists, which performs no range read. A valid ZIP comment containing eight ZIP64-shaped EOCD records therefore exhausts this budget and breaks before the real EOCD is examined. Track whether resolution actually read outside the tail (or handle the no-locator case without charging) so the network-read cap does not reject valid archives based only on comment bytes.

                    if (++probes >= MaxDirectoryProbes)

[Copilot]

Pull request overview

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