Supply-chain hardening, what is left after #1679: signed setup lists, unpinned Linux deps, list validation in the Python builder #2045

Open

opened 2026-11-11 01:02:53 +00:00 by RasAlGhul96 · 1 comment

RasAlGhul96 commented

2026-11-11 01:02:53 +00:00

(Migrated from github.com)

The idea

Follow-up to #1679, which was closed when #1792 (the Updater.cs digest check) shipped in 2.1.0-rc.1. That fixed one item of the list; this issue keeps the ones that are still open, checked again at f8efe05. Static review only, nothing was run.

Since #1679, three items are done, thank you: Updater.cs verifies the asset digest (#1792), releases are built and uploaded by CI with actions/attest-build-provenance, and the release tooling no longer runs npm install (release-please).

Still open (medium)

Nothing on the player's PC checks who made an update. The SHA-256 of each part comes from hookshot-setup.txt / hookshot-linux.txt, downloaded from the same release as the parts, and the Updater.cs digest comes from GitHub's release metadata. Both protect against a broken download, not against a release changed by someone with the account or a write token. The attestations now exist, but the setup and the updaters do not check them. Suggestion: sign the lists (minisign/ed25519, public key compiled into the setup and the Linux updater), and/or Authenticode-sign HookshotBuilder.exe.
Some Linux dependencies are fetched without a checksum. scripts/linux/deps.sh checks a SHA-256 for almost everything, but not for dav1d (:135) nor 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.
The Python builder does not validate the setup list. builder/py/hookshot_builder/setup_parts.py (read_manifest, :113) takes tag, dest, frm, to and sha as they come, while setup.c passes them through list_name_ok and checks the checksum's form. With a tag that is an absolute path or has .., rmtree(work) / rmtree(final) (:215-216) delete that folder before any checksum is looked at. :179 also matches the checksum as a substring of parts.txt. It needs a tampered list (item 1), so this is about the port following the C setup's rules.

Still open (low)

setup.c:158 (mem_sink): m->p = realloc(m->p, ...) loses the old pointer on failure, and the list has no size limit.
setup.c:652-663 (and :183-192): the BCrypt return values are not checked, so the hash handle is used uninitialised if they fail.
setup.c:328-331: the tag read from current.txt builds a path without going through list_name_ok. A local file, so low risk; for consistency.
linux_update.py:439 reads each part fully into memory. urllib also follows https→http redirects; the hash check covers that, but a redirect handler that refuses them would make it explicit.
deps.sh:102: pip install pins versions but does not use --require-hashes; that Python goes inside the AppImage.
scripts/release_upload.sh:56, :83: gh release upload --clobber lets a published file be replaced with no trace. GitHub's immutable releases would close this for version tags.
Runner.cs:19: when the package's tools\python\python.exe is missing, the builder starts whatever python Windows finds first. Stopping with the "restore it from quarantine" message Check already has would be safer.

Anything else

There is still no SECURITY.md; turning on GitHub's private vulnerability reporting would give future reports a private channel. Not covered by the review: third_party/ and the shader recompiler under gpu/vk/eden.

### The idea Follow-up to #1679, which was closed when #1792 (the `Updater.cs` digest check) shipped in 2.1.0-rc.1. That fixed one item of the list; this issue keeps the ones that are still open, checked again at `f8efe05`. Static review only, nothing was run. Since #1679, three items are done, thank you: `Updater.cs` verifies the asset digest (#1792), releases are built and uploaded by CI with `actions/attest-build-provenance`, and the release tooling no longer runs `npm install` (release-please). ### Still open (medium) 1. **Nothing on the player's PC checks who made an update.** The SHA-256 of each part comes from `hookshot-setup.txt` / `hookshot-linux.txt`, downloaded from the same release as the parts, and the `Updater.cs` digest comes from GitHub's release metadata. Both protect against a broken download, not against a release changed by someone with the account or a write token. The attestations now exist, but the setup and the updaters do not check them. Suggestion: sign the lists (minisign/ed25519, public key compiled into the setup and the Linux updater), and/or Authenticode-sign `HookshotBuilder.exe`. 2. **Some Linux dependencies are fetched without a checksum.** `scripts/linux/deps.sh` checks a SHA-256 for almost everything, but not for dav1d (`:135`) nor 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. 3. **The Python builder does not validate the setup list.** `builder/py/hookshot_builder/setup_parts.py` (`read_manifest`, `:113`) takes `tag`, `dest`, `frm`, `to` and `sha` as they come, while `setup.c` passes them through `list_name_ok` and checks the checksum's form. With a `tag` that is an absolute path or has `..`, `rmtree(work)` / `rmtree(final)` (`:215-216`) delete that folder before any checksum is looked at. `:179` also matches the checksum as a substring of `parts.txt`. It needs a tampered list (item 1), so this is about the port following the C setup's rules. ### Still open (low) 4. `setup.c:158` (`mem_sink`): `m->p = realloc(m->p, ...)` loses the old pointer on failure, and the list has no size limit. 5. `setup.c:652-663` (and `:183-192`): the BCrypt return values are not checked, so the hash handle is used uninitialised if they fail. 6. `setup.c:328-331`: the tag read from `current.txt` builds a path without going through `list_name_ok`. A local file, so low risk; for consistency. 7. `linux_update.py:439` reads each part fully into memory. urllib also follows https→http redirects; the hash check covers that, but a redirect handler that refuses them would make it explicit. 8. `deps.sh:102`: `pip install` pins versions but does not use `--require-hashes`; that Python goes inside the AppImage. 9. `scripts/release_upload.sh:56`, `:83`: `gh release upload --clobber` lets a published file be replaced with no trace. GitHub's immutable releases would close this for version tags. 10. `Runner.cs:19`: when the package's `tools\python\python.exe` is missing, the builder starts whatever `python` Windows finds first. Stopping with the "restore it from quarantine" message `Check` already has would be safer. ### Anything else There is still no `SECURITY.md`; turning on GitHub's private vulnerability reporting would give future reports a private channel. Not covered by the review: `third_party/` and the shader recompiler under `gpu/vk/eden`.

NotRexed commented

2026-11-11 21:22:04 +00:00

(Migrated from github.com)