Supply-chain hardening: sign the setup lists/binaries, verify Updater.cs downloads, lockfile for release tooling #1679

Closed

opened 2026-11-10 13:17:13 +00:00 by RasAlGhul96 · 1 comment

RasAlGhul96 commented

2026-11-10 13:17:13 +00:00

(Migrated from github.com)

The idea

Harden the update / release supply chain. I did a static review of the installer, the updaters, the CI workflows and the host code that touches the network or starts processes (commit 1a429f6). The code is careful and I found nothing malicious, but every player's PC currently trusts whatever is attached to the latest GitHub release, with nothing that ties a download to the maintainers. This would also help with the antivirus reports in #1003.

Findings (medium)

Updates are hashed but not signed. The SHA-256 of each part comes from hookshot-setup.txt / hookshot-linux.txt, which is downloaded from the same release as the parts. That protects against a broken download, not against a tampered release: anyone who gets the account or RELEASE_TOKEN can push code that runs on every PC that presses Update.
- builder/setup/setup.c:574-663, builder/linux/linux_update.py:440
- Suggestion: sign the list (minisign/ed25519 with the public key compiled into the setup and the Linux updater) and/or Authenticode-sign HookshotBuilder.exe.
Updater.cs runs downloads with no hash check at all. RunLatestSetup and UpdateFolder download HookshotBuilder.exe and start it, checking only the size, and only when there is a Content-Length. The HookshotBuilder.zip fallback is unzipped and its .exe started the same way.
- builder/HookshotBuilder/Updater.cs:97-104, :172-180, :186-214
- Suggestion: check against a signed list, as in item 1.
Release binaries are built and uploaded by hand. windows-builder.yml:197 keeps the CI release job off. Users cannot check that the published .exe matches the source. Suggestion: publish from CI and add actions/attest-build-provenance, so gh attestation verify works.
release.yml runs npm install without a lockfile while holding a write token. The direct devDependencies are pinned, but the transitive ones are not, and semantic-release runs with RELEASE_TOKEN (contents: write). --ignore-scripts only covers install scripts. Suggestion: commit package-lock.json and use npm ci.
- .github/workflows/release.yml:41, :58-63

Findings (low)

setup.c:145 (mem_sink): m->p = realloc(m->p, ...) loses the old pointer on failure, and there is no size limit on the manifest.
setup.c:639-640: the BCrypt return values are not checked, so fs.h is used uninitialised if they fail.
setup.c:316-318 and :756-760: the tag read from current.txt builds paths without going through list_name_ok. It is a local file, so the risk is low; it is about consistency.
linux_update.py:417 reads each part fully into memory (up to the 4 GiB limit). urllib also follows https→http redirects; the hash check covers that, but a redirect handler that refuses them would make it explicit.
HOOKSHOT_SETUP_LIST, HOOKSHOT_SETUP_BASE and HOOKSHOT_LINUX_LIST can point the setup at another manifest, including over http. This only matters to someone who already controls the environment. Requiring https, or a dev build, would close it.

Anything else

Already done well: actions pinned by SHA, pull_request (not _target), minimal permissions, persist-credentials: false, Dependabot cooldown, name validation in the setup lists, zip-slip and symlink protection in the Linux updater, SetDefaultDllDirectories, and safe_tag before tags reach a command line. I found no committed secrets. There is no SECURITY.md; a private reporting channel (GitHub private vulnerability reporting) would help with future reports.

### The idea Harden the update / release supply chain. I did a static review of the installer, the updaters, the CI workflows and the host code that touches the network or starts processes (commit `1a429f6`). The code is careful and I found nothing malicious, but every player's PC currently trusts whatever is attached to the latest GitHub release, with nothing that ties a download to the maintainers. This would also help with the antivirus reports in #1003. ### Findings (medium) 1. **Updates are hashed but not signed.** The SHA-256 of each part comes from `hookshot-setup.txt` / `hookshot-linux.txt`, which is downloaded from the same release as the parts. That protects against a broken download, not against a tampered release: anyone who gets the account or `RELEASE_TOKEN` can push code that runs on every PC that presses *Update*. - `builder/setup/setup.c:574-663`, `builder/linux/linux_update.py:440` - Suggestion: sign the list (minisign/ed25519 with the public key compiled into the setup and the Linux updater) and/or Authenticode-sign `HookshotBuilder.exe`. 2. **`Updater.cs` runs downloads with no hash check at all.** `RunLatestSetup` and `UpdateFolder` download `HookshotBuilder.exe` and start it, checking only the size, and only when there is a Content-Length. The `HookshotBuilder.zip` fallback is unzipped and its `.exe` started the same way. - `builder/HookshotBuilder/Updater.cs:97-104`, `:172-180`, `:186-214` - Suggestion: check against a signed list, as in item 1. 3. **Release binaries are built and uploaded by hand.** `windows-builder.yml:197` keeps the CI release job off. Users cannot check that the published `.exe` matches the source. Suggestion: publish from CI and add `actions/attest-build-provenance`, so `gh attestation verify` works. 4. **`release.yml` runs `npm install` without a lockfile while holding a write token.** The direct devDependencies are pinned, but the transitive ones are not, and `semantic-release` runs with `RELEASE_TOKEN` (contents: write). `--ignore-scripts` only covers install scripts. Suggestion: commit `package-lock.json` and use `npm ci`. - `.github/workflows/release.yml:41`, `:58-63` ### Findings (low) 5. `setup.c:145` (`mem_sink`): `m->p = realloc(m->p, ...)` loses the old pointer on failure, and there is no size limit on the manifest. 6. `setup.c:639-640`: the BCrypt return values are not checked, so `fs.h` is used uninitialised if they fail. 7. `setup.c:316-318` and `:756-760`: the tag read from `current.txt` builds paths without going through `list_name_ok`. It is a local file, so the risk is low; it is about consistency. 8. `linux_update.py:417` reads each part fully into memory (up to the 4 GiB limit). urllib also follows https→http redirects; the hash check covers that, but a redirect handler that refuses them would make it explicit. 9. `HOOKSHOT_SETUP_LIST`, `HOOKSHOT_SETUP_BASE` and `HOOKSHOT_LINUX_LIST` can point the setup at another manifest, including over http. This only matters to someone who already controls the environment. Requiring https, or a dev build, would close it. ### Anything else Already done well: actions pinned by SHA, `pull_request` (not `_target`), minimal `permissions`, `persist-credentials: false`, Dependabot cooldown, name validation in the setup lists, zip-slip and symlink protection in the Linux updater, `SetDefaultDllDirectories`, and `safe_tag` before tags reach a command line. I found no committed secrets. There is no `SECURITY.md`; a private reporting channel (GitHub private vulnerability reporting) would help with future reports.

👍 1

RasAlGhul96 commented

2026-11-10 18:26:33 +00:00

(Migrated from github.com)

Follow-up: I went over the parts I had not read the first time, at b2ed731 (mod loader, nn_fs.c, hookshot_env.txt, DLL loading, nup.py, the build scripts and the Python builder in builder/py). Again static only, nothing was run. Five more items, none serious:

Some Linux dependencies are fetched without a checksum (medium-low). scripts/linux/deps.sh checks a SHA-256 for almost everything, but fetch is called without one for dav1d (:135) and for the Vulkan-Headers, SPIRV-Headers and VulkanMemoryAllocator tag archives (:171). dav1d is compiled and linked into the game executable the AppImage ships, and a tag archive can change at its source. Suggestion: pin a SHA-256 for these four like the rest.
The Python builder does not validate the setup list (medium-low). builder/py/hookshot_builder/setup_parts.py:147-152 takes tag, dest, frm, to and sha as they come, while setup.c:592-597 passes them through list_name_ok. With a tag that is an absolute path or has .., :215-216 (rmtree(work), rmtree(final)) delete that folder before any part's checksum is looked at. :179 also matches the checksum as a substring of parts.txt. It needs a tampered list, so it is the same trust root as item 1; this is about the port following the same rules as the C setup.
pip install without hashes (low). deps.sh:102 pins versions (requirements.txt) but does not use --require-hashes; that Python goes inside the AppImage.
Release assets can be replaced (low). scripts/release_upload.sh:21 uploads with --clobber, so a published file can be swapped with no trace. GitHub's immutable releases would close this, together with the attestation in item 3.
Fallback to python on the PATH (low). When the package's tools\python\python.exe is missing, Runner.cs:19 starts whatever python Windows finds first. Stopping with the "restore it from quarantine" message that Check already has would be safer.

Also done well, from this pass: the mod loader does not follow folder links and has a depth limit, guest_path_ok in nn_fs.c, the hookshot_env.txt filter (env_line_allowed), system DLLs loaded with LOAD_LIBRARY_SEARCH_SYSTEM32, and extraction_path plus the size checks in nup.py. The symlink checks added to linux_update.py since my first pass are good too.

Not covered: third_party/ and the shader recompiler under gpu/vk/eden.

Follow-up: I went over the parts I had not read the first time, at `b2ed731` (mod loader, `nn_fs.c`, `hookshot_env.txt`, DLL loading, `nup.py`, the build scripts and the Python builder in `builder/py`). Again static only, nothing was run. Five more items, none serious: 10. **Some Linux dependencies are fetched without a checksum (medium-low).** `scripts/linux/deps.sh` checks a SHA-256 for almost everything, but `fetch` is called without one for dav1d (`:135`) and for the Vulkan-Headers, SPIRV-Headers and VulkanMemoryAllocator tag archives (`:171`). dav1d is compiled and linked into the game executable the AppImage ships, and a tag archive can change at its source. Suggestion: pin a SHA-256 for these four like the rest. 11. **The Python builder does not validate the setup list (medium-low).** `builder/py/hookshot_builder/setup_parts.py:147-152` takes `tag`, `dest`, `frm`, `to` and `sha` as they come, while `setup.c:592-597` passes them through `list_name_ok`. With a `tag` that is an absolute path or has `..`, `:215-216` (`rmtree(work)`, `rmtree(final)`) delete that folder before any part's checksum is looked at. `:179` also matches the checksum as a substring of `parts.txt`. It needs a tampered list, so it is the same trust root as item 1; this is about the port following the same rules as the C setup. 12. **`pip install` without hashes (low).** `deps.sh:102` pins versions (`requirements.txt`) but does not use `--require-hashes`; that Python goes inside the AppImage. 13. **Release assets can be replaced (low).** `scripts/release_upload.sh:21` uploads with `--clobber`, so a published file can be swapped with no trace. GitHub's immutable releases would close this, together with the attestation in item 3. 14. **Fallback to `python` on the PATH (low).** When the package's `tools\python\python.exe` is missing, `Runner.cs:19` starts whatever `python` Windows finds first. Stopping with the "restore it from quarantine" message that `Check` already has would be safer. Also done well, from this pass: the mod loader does not follow folder links and has a depth limit, `guest_path_ok` in `nn_fs.c`, the `hookshot_env.txt` filter (`env_line_allowed`), system DLLs loaded with `LOAD_LIBRARY_SEARCH_SYSTEM32`, and `extraction_path` plus the size checks in `nup.py`. The symlink checks added to `linux_update.py` since my first pass are good too. Not covered: `third_party/` and the shader recompiler under `gpu/vk/eden`.