Skip to content

Harden update installation against malformed archives - #1

Closed
aceofS2 wants to merge 1 commit into
salvcy:mainfrom
aceofS2:security/verify-update-archive
Closed

aceofS2 wants to merge 1 commit into
salvcy:mainfrom
aceofS2:security/verify-update-archive

Conversation

@aceofS2

@aceofS2 aceofS2 commented Oct 7, 2026

Copy link
Copy Markdown

Why this PR

The update path already gets the hard parts right: a download has to match its published SHA-256 and pass a strict code-signature check (same team, same bundle ID, Developer ID, notarized) before anything runs. This PR closes the gaps in what happens before those checks — the unpacking itself — and turns one silently skippable check into a hard requirement. Everything here is defense-in-depth; none of it is reachable without the release signing identity.

Changes

The archive is validated before anything touches it

Updater.prepare previously passed whatever GitHub served straight to ditto -x -k, with no limit on size, no check that entries stayed inside the unpack directory, and only a narrow check afterwards (that Meowse.app is a directory and not a symlink). Three checks now run in order:

  1. Size cap on the archive (100 MB). A release zip is a few megabytes. Anything larger is rejected before it's checksummed or unpacked, so a malformed archive can't balloon disk usage while it's being processed.
  2. Entry check before extraction. The archive's entry list (unzip -Z1) is inspected first. Any entry that is absolute or contains a .. component is rejected before ditto runs, so nothing can write outside the private unpack directory.
  3. Unpacked-tree validation. After extraction the whole tree is walked: any symlink anywhere rejects the update (not just a symlinked Meowse.app — a link inside the bundle could point somewhere that changes after verification), and the total uncompressed size is capped at 300 MB.

Every rejection uses the existing "invalid archive" error, so failures surface exactly like any other bad download.

A missing checksum now fails closed

The SHA-256 check only ran when the release asset carried a digest; a missing one silently skipped the check. It now refuses to install (checksumMissing). Every asset in this repo's release history has a digest — GitHub computes them — so published releases are unaffected, but an asset that ever ships without one now produces a clear error instead of falling back to signature verification alone.

Notes for review

  • The size limits (100 MB archive / 300 MB unpacked) are intentionally loose; the current release zip is a few MB. Tighten or move them wherever they belong.
  • The symlink rejection assumes the bundle stays self-contained, which scripts/build.sh guarantees today. If a future release legitimately needs symlinks (e.g. framework version directories), relax that check to allow links whose targets resolve inside the bundle.
  • I confirmed the digest behavior against the live API (every asset in every release returns digest), but haven't run an end-to-end update against a signed build. The updater isn't unit-tested, so CI won't exercise these paths; the easy manual check is to point MeowseUpdateRepository at a throwaway repo whose release asset is a zip containing ../escape.txt or a symlink, and confirm the update fails with "isn't a valid Meowse release".

Reject an update whose archive is oversized, contains entries that would escape the unpack directory, or unpacks to anything but a plain app bundle of bounded size. Require the published checksum instead of silently skipping it when an asset has no digest.
@salvcy

salvcy commented Oct 8, 2026

Copy link
Copy Markdown
Owner

Thanks for looking at the updater so closely, and for the clear write-up. I tested each case, and I'm going to close this one:

  • Path escapes: in my testing, ditto -x -k already handles these. Absolute and .. entries extract inside the target directory, and it won't write through a symlink. Nothing landed outside in any case I tried, so the unzip -Z1 pass adds a second parser without closing a gap.
  • Symlinks in the bundle: anything not in the signed bundle breaks the code seal, and kSecCSStrictValidate already rejects it before install.
  • Size caps: these run after the bytes are already on disk. A real bound would check the asset size before downloading.
  • Required digest: GitHub serves both the asset and its digest, so the digest guards integrity, not authenticity. The signature check covers authenticity. Making the digest required would break auto-update for every installed copy if that field ever changed.

If you find a way past the signature check, I'd like to hear about it. Thanks again!

@salvcy salvcy closed this Oct 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants