fix(security): verify Windows updater asset digests #1792

Merged

monsterlag merged 9 commits from fix/verify-updater-assets into main

2026-11-10 21:03:33 +00:00

monsterlag commented

2026-11-10 16:51:24 +00:00

(Migrated from github.com)

The updater rejects missing, malformed and unsupported digests before requesting the payload, hashes streamed bytes on both the browser and API routes, and promotes a unique owned partial file only after the length and digest checks pass. Digest failures are terminal; cleanup preserves the previous cached file. Only the chosen asset requires a valid digest, so an unused legacy ZIP does not block a valid setup.

Related to #1679; signing, setup-list authentication and release-tooling lockfiles remain separate work. GitHub's digest checks bytes against GitHub's release metadata and does not authenticate a compromised publishing account.

Validation:

The entire actual Updater.cs is compiled in a game-free fixture with authored HTTP responses, process/UI spies and temporary directories. No executable is launched or downloaded.
The identical 20-case fixture against the original updater produces 11 expected integrity failures and passes 9 controls. The patched updater passes all 20 cases, covering setup and ZIP routes, both transports, invalid digests, chunked responses, preserved cache, partial cleanup, HTTP errors and cancellation.
Strict selected source checks pass with no failures or skips, including the existing Builder update-decision test after merging current main.
The digest check declares RUN_ALL_ARGS in its own file to match upstream's runner format. The runner is unchanged from main; the strict selection runs all 20 cases and passes without skips.
The actual Windows WPF Builder compiles offline in Release with .NET SDK 8.0.427: zero warnings and errors. This is framework-dependent compilation (SelfContained=false, UseAppHost=false), not a publish/package test.

Local gaps: the full source suite, Linux execution, self-contained publishing, a real self-update or the complete delayed-retry schedule. The full suite was not run locally because an existing owner-imposed stop excludes a shader fixture; no assertions or skip policy were changed. Hosted Windows/Linux source suites and Windows Builder packaging now pass. No installed player, game files, saves or runtime settings are changed by this PR.

Current head 848175ce normally merges main at 117eae79, including #1858's presentation definitions and #1793's shared source-check repairs. The updater, its regression fixtures and the source-check runner are unchanged by those merges. All eight hosted checks pass on this head: both OS source suites, Windows Builder packaging, Windows Vulkan, Linux AppImage, both native lowering checks and workflow lint. Release-upload jobs are correctly skipped for a pull request. No real self-update was executed.

The Windows updater currently checks download length but can launch a same-length altered setup or extract a same-length altered ZIP. This verifies the selected release asset against GitHub's SHA-256 digest before either handoff. The updater rejects missing, malformed and unsupported digests before requesting the payload, hashes streamed bytes on both the browser and API routes, and promotes a unique owned partial file only after the length and digest checks pass. Digest failures are terminal; cleanup preserves the previous cached file. Only the chosen asset requires a valid digest, so an unused legacy ZIP does not block a valid setup. Related to #1679; signing, setup-list authentication and release-tooling lockfiles remain separate work. GitHub's digest checks bytes against GitHub's release metadata and does not authenticate a compromised publishing account. Validation: - The entire actual `Updater.cs` is compiled in a game-free fixture with authored HTTP responses, process/UI spies and temporary directories. No executable is launched or downloaded. - The identical 20-case fixture against the original updater produces 11 expected integrity failures and passes 9 controls. The patched updater passes all 20 cases, covering setup and ZIP routes, both transports, invalid digests, chunked responses, preserved cache, partial cleanup, HTTP errors and cancellation. - Strict selected source checks pass with no failures or skips, including the existing Builder update-decision test after merging current `main`. - The digest check declares `RUN_ALL_ARGS` in its own file to match upstream's runner format. The runner is unchanged from `main`; the strict selection runs all 20 cases and passes without skips. - The actual Windows WPF Builder compiles offline in Release with .NET SDK 8.0.427: zero warnings and errors. This is framework-dependent compilation (`SelfContained=false`, `UseAppHost=false`), not a publish/package test. Local gaps: the full source suite, Linux execution, self-contained publishing, a real self-update or the complete delayed-retry schedule. The full suite was not run locally because an existing owner-imposed stop excludes a shader fixture; no assertions or skip policy were changed. Hosted Windows/Linux source suites and Windows Builder packaging now pass. No installed player, game files, saves or runtime settings are changed by this PR. Current head `848175ce` normally merges `main` at `117eae79`, including #1858's presentation definitions and #1793's shared source-check repairs. The updater, its regression fixtures and the source-check runner are unchanged by those merges. All eight hosted checks pass on this head: both OS source suites, Windows Builder packaging, Windows Vulkan, Linux AppImage, both native lowering checks and workflow lint. Release-upload jobs are correctly skipped for a pull request. No real self-update was executed.

hckrman101 commented

2026-11-10 17:50:45 +00:00

(Migrated from github.com)

Heads-up: this conflicts with main now (a lot landed today). Could you merge main into the branch (or rebase) and resolve the conflicts? Thanks!

hckrman101 (Migrated from github.com) approved these changes

2026-11-10 20:52:52 +00:00

hckrman101 (Migrated from github.com) left a comment

Looks good, and the conflict I flagged earlier is gone (merge state is clean, all hosted checks pass).

Checked the fail-closed paths in Updater.cs:

ParseDigest throws on a missing, non-string, non-sha256:, wrong-length or non-hex digest, and it runs at the top of DownloadWithRetries, before any payload request. InvalidDataException is not HttpRequestException, so IsTransient is false and neither the retry loop nor the API-URL fallback swallows it. Both callers end up in DownloadFailed with nothing launched or extracted.
A mismatch is a FixedTimeEquals over the streamed bytes, checked after the length check and before File.Move, so the final path only ever holds a verified file. The hash is over exactly the bytes written, on both the browser and API routes.
The partial is CreateNew with a GUID name, owned is set only once the stream is open, and the finally deletes only that file, so a previously cached good file is never clobbered or deleted on failure.
Only the selected asset is checked (setup when the setup path is taken, otherwise the ZIP), so a legacy ZIP with no digest doesn't block a valid setup.

On where the expected digest comes from: it's the digest field of the same releases/latest API response over HTTPS, so it stops a same-length swapped or corrupted asset but not a compromised publisher account or API response. The PR description says exactly that and leaves signing to separate work, which seems the right scope.

Nit, no change needed: a hard kill mid-download leaves a .part-<guid> file in the update cache, since nothing sweeps old ones. It's small and the cache is per-user, so fine to leave.

Looks good, and the conflict I flagged earlier is gone (merge state is clean, all hosted checks pass). Checked the fail-closed paths in `Updater.cs`: - `ParseDigest` throws on a missing, non-string, non-`sha256:`, wrong-length or non-hex digest, and it runs at the top of `DownloadWithRetries`, before any payload request. `InvalidDataException` is not `HttpRequestException`, so `IsTransient` is false and neither the retry loop nor the API-URL fallback swallows it. Both callers end up in `DownloadFailed` with nothing launched or extracted. - A mismatch is a `FixedTimeEquals` over the streamed bytes, checked after the length check and before `File.Move`, so the final path only ever holds a verified file. The hash is over exactly the bytes written, on both the browser and API routes. - The partial is `CreateNew` with a GUID name, `owned` is set only once the stream is open, and the `finally` deletes only that file, so a previously cached good file is never clobbered or deleted on failure. - Only the selected asset is checked (setup when the setup path is taken, otherwise the ZIP), so a legacy ZIP with no digest doesn't block a valid setup. On where the expected digest comes from: it's the `digest` field of the same `releases/latest` API response over HTTPS, so it stops a same-length swapped or corrupted asset but not a compromised publisher account or API response. The PR description says exactly that and leaves signing to separate work, which seems the right scope. Nit, no change needed: a hard kill mid-download leaves a `.part-<guid>` file in the update cache, since nothing sweeps old ones. It's small and the cache is per-user, so fine to leave.