From fd2ccf38a951b9dad42238a22f0952c58402ae14 Mon Sep 17 00:00:00 2001 From: MotherSphere Date: Sat, 25 Jul 2026 11:44:05 +0200 Subject: [PATCH 1/2] fix(security): close audit findings on install paths, URI opening and update trust A full security audit of the launcher (six parallel review axes, each finding double-checked by adversarial verifiers) surfaced seven real issues. All of them are variants of one weakness: remote strings from a manifest or a README were trusted at the point of use rather than at the boundary. The serious one: the raw-binary branch of `extract_binary_from_archive` joined the manifest's `binary` field straight into the install dir, while the zip and tar branches guarded it. A hostile manifest could therefore write an executable anywhere under $HOME - the installer then chmods it 0755 and writes a desktop entry pointing at it - including over Colony's own binary, bypassing the whole ed25519 self-update chain without attacking any crypto. - Hoist the traversal guard above the branch dispatch so all three paths are covered, and validate `repo_name` as a path component in the seven places it is joined (install dir, per-repo caches, desktop entry, remove_dir_all). - Gate everything reaching the desktop URI opener behind an http(s) allowlist. `open::that` is not a browser call: it dispatches file://, UNC paths and any registered scheme handler, so a link in a fetched README was a one-click execution primitive. iced 0.14 made `markdown::Uri` a plain String, dropping the URL parsing 0.13 did, and `fetch_readme` never truncated as documented, so the whole document is rendered with live links. Return the reparsed URL and open that: the WHATWG parser strips control characters anywhere in the input, so checking one string and opening another judged a different value than it executed. - Bind launcher updates to a version and an artefact with a signed `.meta` sidecar. A signature over raw bytes proves only that they came from the release key, so an older, genuinely signed build could be replayed as an update. Enforced fail-closed at download AND at install, like the signature. - Pin the app signature requirement client-side: `manifest.signed` lives in the repo it protects, so a compromised repo could flip it to false and drop the .sig. Once an install is signature-verified, later updates must stay signed. Matched case-insensitively, since a case-only rename made a new directory. - Escape desktop-entry values per spec, and reject control characters: glib keeps the first occurrence of a key, so a newline injected a shadowing Exec=. - Launch installed apps directly instead of through `cmd /C start`, which re-parsed `&` in a manifest-derived path as a command separator; quote the scanned-app path explicitly with raw_arg, where `start` is still needed for .lnk targets. - Never follow a symlink planted at a predictable staging name. Two limits are deliberate and documented in the code: app signatures are still not bound to a version or asset name (that needs the sidecar rolled out to the ecosystem release workflows first), and install-time re-verification enforces "strictly newer than the running build" rather than the exact requested tag. CI declares the asset list once instead of per step, checks each sidecar binds the right tag, and re-verifies against the published release. --- .github/workflows/release-please.yml | 61 +++- docs/colony-spec.md | 29 +- docs/release-signing.md | 54 +++- scripts/sign-release.sh | 63 ++-- src/download.rs | 431 ++++++++++++++++++++++++++- src/oauth.rs | 11 +- src/persistence.rs | 168 +++++++++-- src/signing.rs | 78 +++++ src/update.rs | 105 +++++-- 9 files changed, 901 insertions(+), 99 deletions(-) diff --git a/.github/workflows/release-please.yml b/.github/workflows/release-please.yml index 367330a..4434b53 100644 --- a/.github/workflows/release-please.yml +++ b/.github/workflows/release-please.yml @@ -91,20 +91,25 @@ jobs: needs: [release-please, build] if: ${{ needs.release-please.outputs.release_created }} runs-on: ubuntu-latest + env: + # Single source of truth for the four launcher assets. Declared once so the + # download, sign, verify and post-upload steps cannot drift apart: a list + # repeated per step means adding a platform silently leaves it unsigned, + # which is exactly how v0.7.0 shipped. + ASSETS: colony-linux colony-windows.exe colony-macos colony-macos-x86 + GH_TOKEN: ${{ github.token }} + TAG: ${{ needs.release-please.outputs.tag_name }} steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Download release binaries - env: - GH_TOKEN: ${{ github.token }} - TAG: ${{ needs.release-please.outputs.tag_name }} run: | mkdir dist - gh release download "$TAG" --dir dist \ - --pattern colony-linux \ - --pattern colony-windows.exe \ - --pattern colony-macos \ - --pattern colony-macos-x86 + for a in $ASSETS; do PATTERNS="$PATTERNS --pattern $a"; done + gh release download "$TAG" --dir dist $PATTERNS + for a in $ASSETS; do + [ -s "dist/$a" ] || { echo "::error::release asset $a is missing or empty"; exit 1; } + done - name: Sign env: @@ -116,15 +121,43 @@ jobs: fi printf '%s' "$KEY" > /tmp/colony-release.pem chmod 600 /tmp/colony-release.pem + for a in $ASSETS; do PATHS="$PATHS dist/$a"; done COLONY_SIGNING_KEY=/tmp/colony-release.pem \ - ./scripts/sign-release.sh dist/colony-linux dist/colony-windows.exe dist/colony-macos dist/colony-macos-x86 + COLONY_RELEASE_VERSION="$TAG" \ + ./scripts/sign-release.sh $PATHS rm -f /tmp/colony-release.pem - - name: Upload signatures - env: - GH_TOKEN: ${{ github.token }} - TAG: ${{ needs.release-please.outputs.tag_name }} - run: gh release upload "$TAG" dist/*.sig --clobber + # Self-update is fail-closed on BOTH the signature and the signed metadata + # sidecar that binds it to a version, so a release missing either one is + # un-updatable. Check locally before uploading... + - name: Verify every asset got a signature and a signed sidecar + run: | + for a in $ASSETS; do + for required in "dist/$a.sig" "dist/$a.meta" "dist/$a.meta.sig"; do + [ -s "$required" ] || { echo "::error::missing or empty $required"; exit 1; } + done + grep -qx "version=$TAG" "dist/$a.meta" \ + || { echo "::error::dist/$a.meta does not bind version=$TAG"; exit 1; } + grep -qx "asset=$a" "dist/$a.meta" \ + || { echo "::error::dist/$a.meta does not bind asset=$a"; exit 1; } + done + echo "all assets carry .sig + .meta + .meta.sig, correctly bound" + + - name: Upload signatures and metadata + run: gh release upload "$TAG" dist/*.sig dist/*.meta --clobber + + # ...and again against the PUBLISHED release, because a local file proves + # nothing about what users can actually download. + - name: Verify the published release carries every asset + run: | + gh release view "$TAG" --json assets --jq '.assets[].name' | sort > /tmp/published + for a in $ASSETS; do + for required in "$a" "$a.sig" "$a.meta" "$a.meta.sig"; do + grep -qx "$required" /tmp/published \ + || { echo "::error::$required is not published on $TAG - self-update would refuse this release"; exit 1; } + done + done + echo "published release verified" # Chained here (not on `release: published`) because release-please creates # the release with the default GITHUB_TOKEN, whose events do not trigger diff --git a/docs/colony-spec.md b/docs/colony-spec.md index 8d82e27..fa4e5e3 100644 --- a/docs/colony-spec.md +++ b/docs/colony-spec.md @@ -220,17 +220,28 @@ signing is adopted per-app, no flag day - **unless** the manifest declares: { "signed": true } ``` -With `"signed": true`, a missing signature ABORTS the install: this closes the -remaining hole where a compromised repository could simply omit signatures. -Declare it once every release of the app ships `.sig` assets (all Project- -Colony apps have signed releases as of 2026-07-20). Sign with: +With `"signed": true`, a missing signature ABORTS the install. Declare it once +every release of the app ships `.sig` assets (all Project-Colony apps have signed +releases as of 2026-07-20). Sign with: ```sh -COLONY_SIGNING_KEY=/path/to/colony-release.pem ./scripts/sign-release.sh +COLONY_SIGNING_KEY=/path/to/colony-release.pem \ +COLONY_RELEASE_VERSION= ./scripts/sign-release.sh ``` -Note the trust boundary: signatures protect against tampered release assets +`COLONY_RELEASE_VERSION` is required because the script also emits a signed +`.meta` sidecar binding the bytes to a version and a filename. Only the +launcher's own self-update consumes that sidecar today; for app assets the +`.sig` is what matters, and the extra files are harmless if published. + +Note the trust boundary. Signatures protect against tampered release assets (e.g. an asset swapped after publication), because forging one requires the -org's private signing key, which never lives in any repository. They do not -yet protect against a fully compromised repository *omitting* signatures; -enforcement ("this app must be signed") is a planned follow-up. +org's private signing key, which never lives in any repository. Enforcement is +implemented: `"signed": true` refuses an install with no signature, and the +launcher **pins** the requirement client-side once an app has been installed with +a verified signature, so flipping `signed` back to false in a compromised +repository no longer downgrades that app to unsigned. What remains open is that +an app signature is not bound to a version or an asset name, so an attacker with +push access to a catalogue repo could still replay a *different* org-signed +artefact; binding it needs the `.meta` sidecar rolled out to the ecosystem app +release workflows first. diff --git a/docs/release-signing.md b/docs/release-signing.md index 7c82159..8737963 100644 --- a/docs/release-signing.md +++ b/docs/release-signing.md @@ -12,26 +12,54 @@ key, the self-update is refused and the running binary is left untouched. exactly what `openssl pkeyutl -sign -rawin` emits (base64 text is also accepted). Published as `.sig` next to each release asset. - Enforced only for the **launcher** (`colony-[.exe]`). Third-party - app installs continue to use the optional `sha256` field in `colony.json`. + app installs continue to use the optional `sha256` field in `colony.json`, + plus `"signed": true` to require a signature (pinned client-side once an app + has been installed with one, so a repo cannot silently stop signing). -## Every release MUST ship signatures +### Why a signature alone is not enough -Because verification is fail-closed, a release published **without** the -`colony-.sig` assets will make self-update fail for users on that -channel. Sign every launcher asset and upload the `.sig` files alongside them. +A signature over raw bytes proves only *these bytes came from the release key* — +not **which** artefact or **which** version they are. Anything able to control +what the release host serves could therefore replay an older, genuinely signed +build as an "update" (a downgrade), or serve the macOS asset where the Linux one +was requested. So every asset also gets a **signed metadata sidecar**: + +``` +.meta version=v1.2.3 + asset=colony-linux + sha256= +.meta.sig ed25519 signature over the .meta bytes +``` + +The launcher verifies the sidecar's signature, then requires that it names the +asset it asked for, that its digest matches the bytes actually downloaded, that +its version equals the tag the update check resolved, and that this version is +**strictly newer** than the running build. That last check is the anti-rollback. +Both the signature and the sidecar are re-verified at install time, not only at +download time, so the staging file cannot be swapped in between. + +## Every release MUST ship signatures and sidecars + +Because verification is fail-closed, a release published without the +`colony-.sig`, `.meta` and `.meta.sig` assets will make self-update +fail for users on that channel. The CI `sign` job checks all three exist for all +four platforms and fails the release otherwise. ## Signing (CI or local) The private key never lives in the repo. Point `COLONY_SIGNING_KEY` at the -ed25519 private key (PEM) and run: +ed25519 private key (PEM), set `COLONY_RELEASE_VERSION` to the release tag (it is +bound into each sidecar), and run: ```sh COLONY_SIGNING_KEY=/path/to/colony-release.pem \ +COLONY_RELEASE_VERSION=v1.2.3 \ ./scripts/sign-release.sh colony-linux colony-windows.exe colony-macos colony-macos-x86 ``` -This writes `colony-linux.sig`, `colony-windows.exe.sig`, … each self-verified -before it is written. Upload every `.sig` as a release asset. +For each asset this writes `.sig`, `.meta` and `.meta.sig`, +every signature self-verified before it is kept. Upload all of them as release +assets. ### In CI (the normal path) @@ -39,10 +67,12 @@ Since the v0.7.0 incident (a release shipped unsigned because signing was a manual step, bricking self-update for every existing install), signing is a mandatory job in the release workflow: `.github/workflows/release-please.yml` (`sign` job) downloads the four built binaries, signs them with the -`COLONY_SIGNING_KEY_PEM` secret (the PEM contents), and uploads the `.sig` -assets. The job **fails the release** if the secret is missing, so an unsigned -release can no longer ship silently. The manual procedure above remains for -re-signing an old release by hand. +`COLONY_SIGNING_KEY_PEM` secret (the PEM contents), verifies that every asset +came out with a `.sig`, a `.meta` and a `.meta.sig`, and uploads them. The job +**fails the release** if the secret is missing or if any of those files is +missing or empty, so a release the launcher cannot verify can no longer ship +silently. The manual procedure above remains for re-signing an old release by +hand. ## Key custody diff --git a/scripts/sign-release.sh b/scripts/sign-release.sh index 72cb6d6..b0279fa 100755 --- a/scripts/sign-release.sh +++ b/scripts/sign-release.sh @@ -7,15 +7,28 @@ # produced by `openssl pkeyutl -sign -rawin` — the same format src/signing.rs # verifies against the embedded public key. openssl is the only dependency. # +# A signature over raw bytes proves only "these bytes came from the org" — not +# WHICH artefact or WHICH version they are. So each asset also gets a signed +# metadata sidecar binding the bytes to a version and a filename: +# +# .meta version=\nasset=\nsha256=\n +# .meta.sig ed25519 signature over the .meta bytes +# +# The launcher verifies the sidecar and refuses anything that is not strictly +# newer than itself, which is what stops a replay of an older org-signed build. +# # Usage: -# COLONY_SIGNING_KEY=/path/to/colony-release.pem ./scripts/sign-release.sh [ ...] +# COLONY_SIGNING_KEY=/path/to/colony-release.pem \ +# COLONY_RELEASE_VERSION=v1.2.3 ./scripts/sign-release.sh [ ...] # # In CI, provide the private key via a secret (e.g. write it to a temp file from -# a GitHub Actions secret) and set COLONY_SIGNING_KEY to its path. Upload each -# generated ".sig" as a release asset alongside its binary. +# a GitHub Actions secret) and set COLONY_SIGNING_KEY to its path. Upload every +# generated ".sig", ".meta" and ".meta.sig" as release +# assets alongside their binary. set -euo pipefail KEY="${COLONY_SIGNING_KEY:-$HOME/.config/colony/release-signing/colony-release.pem}" +VERSION="${COLONY_RELEASE_VERSION:-}" if [[ ! -f "$KEY" ]]; then echo "error: signing key not found at '$KEY'" >&2 @@ -23,26 +36,42 @@ if [[ ! -f "$KEY" ]]; then exit 1 fi if [[ $# -eq 0 ]]; then - echo "usage: COLONY_SIGNING_KEY= $0 [ ...]" >&2 + echo "usage: COLONY_SIGNING_KEY= COLONY_RELEASE_VERSION= $0 [ ...]" >&2 + exit 2 +fi +if [[ -z "$VERSION" ]]; then + echo "error: COLONY_RELEASE_VERSION is not set" >&2 + echo "it is bound into each signed .meta sidecar so the launcher can reject downgrades." >&2 exit 2 fi -for asset in "$@"; do - if [[ ! -f "$asset" ]]; then - echo "error: asset not found: $asset" >&2 - exit 1 - fi - sig="${asset}.sig" - openssl pkeyutl -sign -inkey "$KEY" -rawin -in "$asset" -out "$sig" - # Self-check: verify what we just produced before publishing it. +# Sign into .sig, then verify the signature before returning. +sign_and_verify() { + local file="$1" sig="$1.sig" pub + openssl pkeyutl -sign -inkey "$KEY" -rawin -in "$file" -out "$sig" pub="$(mktemp)" openssl pkey -in "$KEY" -pubout -out "$pub" 2>/dev/null - if openssl pkeyutl -verify -pubin -inkey "$pub" -rawin -in "$asset" -sigfile "$sig" >/dev/null 2>&1; then - echo "signed $asset -> $sig" - else - echo "error: self-verification failed for $asset" >&2 + if ! openssl pkeyutl -verify -pubin -inkey "$pub" -rawin -in "$file" -sigfile "$sig" >/dev/null 2>&1; then + echo "error: self-verification failed for $file" >&2 rm -f "$pub" - exit 1 + return 1 fi rm -f "$pub" +} + +for asset in "$@"; do + if [[ ! -f "$asset" ]]; then + echo "error: asset not found: $asset" >&2 + exit 1 + fi + # Detached signature over the asset bytes. + sign_and_verify "$asset" + echo "signed $asset -> ${asset}.sig" + + # Signed metadata binding those bytes to a version and a filename. + meta="${asset}.meta" + digest="$(openssl dgst -sha256 -r "$asset" | cut -d' ' -f1)" + printf 'version=%s\nasset=%s\nsha256=%s\n' "$VERSION" "$(basename "$asset")" "$digest" > "$meta" + sign_and_verify "$meta" + echo "signed $meta -> ${meta}.sig (version=$VERSION sha256=${digest:0:16}...)" done diff --git a/src/download.rs b/src/download.rs index 7dd16f7..04f384d 100644 --- a/src/download.rs +++ b/src/download.rs @@ -10,7 +10,7 @@ use std::path::PathBuf; use std::time::Duration; use crate::github::{APP_VERSION, CONNECT_TIMEOUT, GITHUB_ACCOUNT, LAUNCHER_OWNER, LAUNCHER_REPO}; -use crate::persistence::{colony_apps_dir, colony_data_dir}; +use crate::persistence::colony_data_dir; /// Build the HTTP client used for large asset downloads (longer read timeout /// than the API client). @@ -55,7 +55,8 @@ async fn download_to_file( use futures::StreamExt; use std::io::Write; - let mut file = std::fs::File::create(dest_path)?; + // Staging names are predictable, so never follow whatever sits there. + let mut file = create_new_file(dest_path)?; let mut stream = resp.bytes_stream(); let mut last_pct: u32 = 0; @@ -202,6 +203,52 @@ pub(crate) fn ensure_safe_component(name: &str) -> Result<()> { Ok(()) } +/// Normalized form of `raw` when it is an absolute `http`/`https` URL with a +/// host, else `None`. +/// +/// Gate for everything handed to the desktop's URI opener: link destinations in +/// remote Markdown (READMEs, release notes) reach it verbatim, and `open::that` +/// is not a browser call — it execs `xdg-open`/`gio open` on Linux and +/// `Start-Process` on Windows, both of which dispatch `file://`, UNC paths and +/// any registered `x-scheme-handler/*`. A `file://` or custom-scheme link in a +/// hostile README would otherwise be a one-click execution primitive that +/// bypasses every signature and digest check in this module. +/// +/// Returns the REPARSED string rather than a bool on purpose: the WHATWG parser +/// strips tabs, newlines and leading control characters anywhere in the input, so +/// validating `raw` and then opening `raw` would judge one string and execute a +/// different one (`"ht\ntps://x"` parses as `https://x`). Callers open what was +/// actually validated. +pub(crate) fn web_url(raw: &str) -> Option { + // Relative and scheme-less inputs (including protocol-relative + // `//host/share/x.exe`) fail to parse, which is the desired answer. + let parsed = reqwest::Url::parse(raw).ok()?; + // A host is required: `http:evil` and `http:/x` parse but address nothing, + // and an opener may fall back to treating them as a local path. + if !matches!(parsed.scheme(), "http" | "https") || !parsed.has_host() { + return None; + } + Some(parsed.into()) +} + +/// Create `path` fresh, never following an existing file or symlink. +/// +/// `File::create` truncates whatever it finds, and follows a symlink to write at +/// its target: a symlink pre-planted at a predictable staging name (`.new`, +/// `.part`) would redirect the write outside the install directory, and the +/// caller's `set_permissions` then chmods that target. Unlinking first and opening +/// with `create_new` makes the create fail rather than follow anything that races +/// in between. +fn create_new_file(path: &std::path::Path) -> Result { + // Removing our own leftover staging file is expected; a failure here is not + // fatal because create_new below is the actual guard. + let _ = std::fs::remove_file(path); + Ok(std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(path)?) +} + /// Extract a single file from a .zip archive. fn extract_from_zip( archive_path: &std::path::Path, @@ -230,7 +277,7 @@ fn extract_from_zip( // install so a failed extraction never leaves a truncated binary. let final_dest = dest_dir.join(binary_name); let tmp_dest = dest_dir.join(format!("{binary_name}.new")); - let mut out = std::fs::File::create(&tmp_dest)?; + let mut out = create_new_file(&tmp_dest)?; std::io::copy(&mut entry, &mut out)?; drop(out); std::fs::rename(&tmp_dest, &final_dest)?; @@ -288,6 +335,14 @@ fn extract_binary_from_archive( binary_name: &str, dest_dir: &std::path::Path, ) -> Result { + // `binary_name` is a manifest-supplied string that becomes a path component + // in EVERY branch below, so the traversal guard is hoisted here, before the + // dispatch. It used to live only inside the zip and tar extractors, which + // left the raw-binary branch able to write outside `dest_dir` — and, after + // the caller's chmod 0755 and desktop entry, to gain execution at next login + // — from a hostile manifest. The extractors keep their own call so they stay + // safe in isolation. + ensure_safe_component(binary_name)?; if asset_name.ends_with(".zip") { let result = extract_from_zip(archive_path, binary_name, dest_dir); let _ = std::fs::remove_file(archive_path); @@ -347,7 +402,7 @@ pub async fn download_release_asset( // `binary` guard. ensure_safe_component(&filename)?; - let dest_dir = colony_apps_dir()?.join(&repo_name); + let dest_dir = crate::persistence::colony_app_dir(&repo_name)?; std::fs::create_dir_all(&dest_dir)?; let dest_path = dest_dir.join(&filename); // Download to a temporary sibling so an interrupted or failed transfer @@ -361,6 +416,14 @@ pub async fn download_release_asset( let client = download_client()?; download_to_file(&client, &url, token.as_deref(), &temp_path, progress_tx).await?; + // `manifest.signed` lives in the very repo the signature protects, so a + // compromised repo could flip it to false and drop the `.sig` to install + // unsigned code silently. Pin it: once an install of this app has been + // signature-verified, later updates must stay signed whatever the manifest + // now claims. The pin only ever raises the bar. + let signature_pinned = crate::persistence::load_installed_signed(&repo_name); + let require_signature = require_signature || signature_pinned; + // Opportunistic app-signature verification: when the release publishes // `.sig`, it MUST verify against the org release key (the same // ed25519 key that signs the launcher, embedded in src/signing.rs). A @@ -377,6 +440,15 @@ pub async fn download_release_asset( }; if require_signature && signature.is_none() { let _ = std::fs::remove_file(&temp_path); + // Say WHICH rule refused, because the two have different remedies: the + // manifest is the repo's own declaration, whereas the pin is our memory + // of a previously verified install and can only be cleared by + // uninstalling the app. + if signature_pinned { + anyhow::bail!( + "{repo_name} was previously installed with a verified signature, but this release publishes no {filename}.sig - refusing to install an unsigned downgrade of a signed app (uninstall it first to opt back out)" + ); + } anyhow::bail!( "The manifest requires signed releases, but no {filename}.sig was published - refusing to install" ); @@ -394,6 +466,7 @@ pub async fn download_release_asset( let filename = filename.clone(); tokio::task::spawn_blocking(move || -> Result { + let was_signed = signature.is_some(); if let Some(sig) = signature { let bytes = std::fs::read(&temp_path)?; if let Err(e) = crate::signing::verify_release_signature(&bytes, &sig) { @@ -446,6 +519,13 @@ pub async fn download_release_asset( if record_asset { crate::persistence::save_installed_asset(&repo_name, &filename)?; } + // Pin the signature requirement for future updates: a repo that + // ships signatures today must not be able to stop tomorrow. Only + // ever raises the bar - the marker is written, never cleared, while + // the app stays installed. + if was_signed { + crate::persistence::save_installed_signed(&repo_name)?; + } // Desktop integration (Linux): index the installed app in the // desktop environment. Best-effort - a failure here must not fail // an otherwise complete install. @@ -505,15 +585,58 @@ pub async fn download_launcher_asset( let _ = std::fs::remove_file(&dest_path); anyhow::bail!("Refusing to self-update: {e}"); } + // The signature above proves the bytes came from the release key, but not + // WHICH artefact or version they are: an attacker controlling what the + // release host serves could replay an older, genuinely signed build. The + // signed metadata sidecar closes that by binding these bytes to a version + // and a filename. Fail-closed, exactly like the signature itself. + let meta_url = format!("{url}{}", crate::signing::METADATA_SUFFIX); + let meta_bytes = match fetch_bytes(&client, &meta_url, token.as_deref()).await { + Ok(bytes) => bytes, + Err(e) => { + let _ = std::fs::remove_file(&dest_path); + anyhow::bail!( + "Refusing to self-update: could not fetch the update metadata ({meta_url}): {e}" + ); + } + }; + let meta_sig = match fetch_bytes( + &client, + &format!("{meta_url}{}", crate::signing::SIGNATURE_SUFFIX), + token.as_deref(), + ) + .await + { + Ok(bytes) => bytes, + Err(e) => { + let _ = std::fs::remove_file(&dest_path); + anyhow::bail!("Refusing to self-update: could not fetch the metadata signature: {e}"); + } + }; + if let Err(e) = + verify_launcher_metadata(&meta_bytes, &meta_sig, &binary_bytes, &filename, Some(&tag)) + { + let _ = std::fs::remove_file(&dest_path); + anyhow::bail!("Refusing to self-update: {e}"); + } + // Persist the signature next to the staged binary so apply_launcher_update // can re-verify at install time — closing any window in which the staged - // file could be swapped between download and apply. + // file could be swapped between download and apply. The metadata sidecar is + // staged for the same reason. let sig_path = staged_signature_path(&dest_path); if let Err(e) = std::fs::write(&sig_path, &signature) { let _ = std::fs::remove_file(&dest_path); anyhow::bail!("Could not stage update signature: {e}"); } - tracing::info!("Launcher update signature verified for {filename}"); + let (meta_path, meta_sig_path) = staged_metadata_paths(&dest_path); + if let Err(e) = std::fs::write(&meta_path, &meta_bytes) + .and_then(|()| std::fs::write(&meta_sig_path, &meta_sig)) + { + let _ = std::fs::remove_file(&dest_path); + anyhow::bail!("Could not stage update metadata: {e}"); + } + tracing::info!("Launcher update signature and metadata verified for {filename} ({tag})"); // Make executable on Unix #[cfg(unix)] @@ -536,6 +659,86 @@ fn staged_signature_path(binary: &std::path::Path) -> PathBuf { )) } +/// Paths of the metadata sidecar and its signature, staged next to a binary. +fn staged_metadata_paths(binary: &std::path::Path) -> (PathBuf, PathBuf) { + let meta = format!("{}{}", binary.display(), crate::signing::METADATA_SUFFIX); + let sig = format!("{meta}{}", crate::signing::SIGNATURE_SUFFIX); + (PathBuf::from(meta), PathBuf::from(sig)) +} + +/// Verify a signed metadata sidecar and enforce what it binds. +/// +/// Checks, in order: the sidecar is signed by the release key; it describes the +/// asset we asked for; its digest matches the bytes in hand; it names the tag the +/// update check selected (`expected_tag`, unknown at apply time hence optional); +/// and its version is strictly newer than the running build. That last check is +/// the anti-rollback - without it, any older org-signed binary is a valid +/// "update". +fn verify_launcher_metadata( + meta_bytes: &[u8], + meta_sig: &[u8], + binary_bytes: &[u8], + expected_asset: &str, + expected_tag: Option<&str>, +) -> Result<()> { + crate::signing::verify_release_signature(meta_bytes, meta_sig) + .map_err(|e| anyhow::anyhow!("update metadata signature invalid: {e}"))?; + let meta = crate::signing::ReleaseMetadata::parse(meta_bytes)?; + check_metadata_bindings( + &meta, + binary_bytes, + expected_asset, + expected_tag, + APP_VERSION, + ) +} + +/// The binding rules of [`verify_launcher_metadata`], split out so they can be +/// tested without the release private key (`running` is injectable for the same +/// reason). Assumes the metadata signature has already been verified. +fn check_metadata_bindings( + meta: &crate::signing::ReleaseMetadata, + binary_bytes: &[u8], + expected_asset: &str, + expected_tag: Option<&str>, + running: &str, +) -> Result<()> { + anyhow::ensure!( + meta.asset == expected_asset, + "update metadata is for a different asset (signed '{}', expected '{expected_asset}')", + meta.asset + ); + + let digest = format!("{:x}", Sha256::digest(binary_bytes)); + anyhow::ensure!( + digest == meta.sha256, + "update metadata digest mismatch (signed {}, downloaded {digest})", + meta.sha256 + ); + + if let Some(tag) = expected_tag { + anyhow::ensure!( + meta.version == tag, + "update metadata is for a different release (signed '{}', expected '{tag}')", + meta.version + ); + } + + let new = crate::github::parse_version_tag(&meta.version).ok_or_else(|| { + anyhow::anyhow!( + "unrecognized version in update metadata: '{}'", + meta.version + ) + })?; + let current = crate::github::parse_version_tag(running) + .ok_or_else(|| anyhow::anyhow!("unparseable running version"))?; + anyhow::ensure!( + new > current, + "refusing a downgrade: signed update is {new} but the running build is {current}" + ); + Ok(()) +} + /// Replace the running Colony binary with the downloaded update. /// Returns the exe path for relaunch on success. Restores backup on failure. pub fn apply_launcher_update(new_binary: &std::path::Path) -> Result { @@ -557,6 +760,39 @@ pub fn apply_launcher_update(new_binary: &std::path::Path) -> Result { crate::signing::verify_release_signature(&staged_bytes, &signature) .map_err(|e| anyhow::anyhow!("Refusing to apply update: {e}"))?; + // Re-check the sidecar at install time too, for the same reason the signature + // is re-checked: the staging directory is writable, so the file could have + // been swapped after the download-time check. The requested tag is not in + // scope here, so this enforces the asset name, the digest and "strictly newer + // than the running build" - enough to stop a rollback, though a swap to + // another org-signed release that is ALSO newer than the running build would + // still pass. Closing that needs the tag threaded through the staged state. + let (meta_path, meta_sig_path) = staged_metadata_paths(new_binary); + let staged_meta = std::fs::read(&meta_path).map_err(|e| { + anyhow::anyhow!( + "Missing staged update metadata ({}): {e}", + meta_path.display() + ) + })?; + let staged_meta_sig = std::fs::read(&meta_sig_path).map_err(|e| { + anyhow::anyhow!( + "Missing staged update metadata signature ({}): {e}", + meta_sig_path.display() + ) + })?; + let expected_asset = new_binary + .file_name() + .and_then(|n| n.to_str()) + .ok_or_else(|| anyhow::anyhow!("staged update has no usable file name"))?; + verify_launcher_metadata( + &staged_meta, + &staged_meta_sig, + &staged_bytes, + expected_asset, + None, + ) + .map_err(|e| anyhow::anyhow!("Refusing to apply update: {e}"))?; + // Refuse to touch the running binary if the staged update is empty/missing. anyhow::ensure!( !staged_bytes.is_empty(), @@ -597,6 +833,10 @@ pub fn apply_launcher_update(new_binary: &std::path::Path) -> Result { Ok(()) => { let _ = std::fs::remove_file(new_binary); let _ = std::fs::remove_file(&sig_path); + // The sidecars are staged next to the binary, so they must be swept + // too or the staging directory never becomes empty. + let _ = std::fs::remove_file(&meta_path); + let _ = std::fs::remove_file(&meta_sig_path); let _ = std::fs::remove_dir(new_binary.parent().unwrap_or(new_binary)); Ok(current_exe) } @@ -619,6 +859,185 @@ pub fn apply_launcher_update(new_binary: &std::path::Path) -> Result { mod tests { use super::*; + /// The raw-binary branch of `extract_binary_from_archive` used to join the + /// manifest's `binary` field straight into the install dir, so a hostile + /// manifest could write anywhere (the caller then chmods 0755 and writes a + /// desktop entry pointing at it). The guard is now hoisted above the branch. + #[test] + fn raw_binary_branch_rejects_traversal_in_binary_name() { + // Every escape target below must ALREADY EXIST as a directory, so that + // without the guard the rename would genuinely succeed. A target whose + // parent is missing would make this test pass on ENOENT alone, and it + // would stay green with the fix reverted. + let root = std::env::temp_dir().join("colony_test_raw_traversal"); + let _ = std::fs::remove_dir_all(&root); + let dest_dir = root.join("apps").join("SomeApp"); + std::fs::create_dir_all(&dest_dir).unwrap(); + std::fs::create_dir_all(dest_dir.join("sub")).unwrap(); + let outside = root.join("apps").join("escaped"); + let staged = dest_dir.join("app.bin.part"); + + for hostile in ["../escaped", "../../apps/escaped", "sub/nested", ".."] { + std::fs::write(&staged, b"payload").unwrap(); + // `app.bin` has no archive extension, so this hits the raw branch. + let result = extract_binary_from_archive(&staged, "app.bin", hostile, &dest_dir); + assert!( + result.is_err(), + "traversal must be refused for binary name {hostile:?}" + ); + assert!( + staged.exists(), + "a refused extraction must leave staging intact ({hostile:?})" + ); + assert!( + !outside.exists(), + "nothing may be written outside the install dir ({hostile:?})" + ); + assert!( + !dest_dir.join("sub").join("nested").exists(), + "nothing may be written below the install dir ({hostile:?})" + ); + std::fs::remove_file(&staged).unwrap(); + } + + // Control: the same call with a plain name DOES install. Without it, the + // assertions above could be passing because of a broken fixture. + std::fs::write(&staged, b"payload").unwrap(); + let installed = extract_binary_from_archive(&staged, "app.bin", "app", &dest_dir).unwrap(); + assert_eq!(installed, dest_dir.join("app")); + assert!(installed.exists()); + + let _ = std::fs::remove_dir_all(&root); + } + + #[test] + fn only_http_and_https_urls_may_reach_the_uri_opener() { + for ok in [ + "https://github.com/Project-Colony/Colony", + "http://example.com/a?b=c#d", + "HTTPS://GitHub.com/x", + ] { + assert!(web_url(ok).is_some(), "{ok} should be allowed"); + } + for bad in [ + // The one-click execution vectors from a hostile README. + "file:///home/user/.local/share/applications/colony-app.desktop", + "//198.51.100.7/share/setup.exe", + "\\\\198.51.100.7\\share\\setup.exe", + "javascript:alert(1)", + "smb://host/share", + "steam://run/1", + "data:text/html,", + "mailto:a@b.c", + "", + " ", + "not a url", + "/etc/passwd", + ] { + assert!(web_url(bad).is_none(), "{bad:?} must be refused"); + } + } + + /// The WHATWG parser strips tabs, newlines and leading control characters + /// anywhere in the input, so a bool-returning check would validate one string + /// while `open::that` received a different one. CommonMark decodes character + /// references in link destinations, so `[x](ht tps://evil)` produces + /// exactly these shapes from a hostile README. + #[test] + fn opened_url_is_the_one_that_was_validated() { + for sneaky in [ + "ht\ntps://evil.example/x", + "ht\ttps://evil.example/x", + "\thttps://evil.example/x", + " https://evil.example/x", + "https://evil.example/\rx", + ] { + // These DO parse (that is the whole problem), so the contract is not + // "refused" but "what comes back is clean": a real http(s) URL with no + // control characters, never the raw string. + let normalized = web_url(sneaky).expect("parses as https"); + assert_ne!(normalized, sneaky, "the raw form must not be handed on"); + assert!(normalized.starts_with("https://"), "{normalized}"); + assert!( + !normalized.chars().any(|c| c.is_control()), + "{normalized:?} still carries control characters" + ); + } + // Scheme-only forms are normalized by the parser rather than rejected: + // for http(s) it fills in the host, which stays a plain web URL. + assert_eq!( + web_url("http:evil.example/x").as_deref(), + Some("http://evil.example/x") + ); + } + + fn meta_for(bytes: &[u8], version: &str, asset: &str) -> crate::signing::ReleaseMetadata { + crate::signing::ReleaseMetadata { + version: version.into(), + asset: asset.into(), + sha256: format!("{:x}", Sha256::digest(bytes)), + } + } + + #[test] + fn signed_metadata_accepts_a_genuine_newer_release() { + let bytes = b"new colony build"; + let meta = meta_for(bytes, "v1.2.0", "colony-linux"); + assert!( + check_metadata_bindings(&meta, bytes, "colony-linux", Some("v1.2.0"), "1.1.0").is_ok() + ); + } + + /// The point of the sidecar: an older but genuinely org-signed binary must + /// not be installable as an "update". + #[test] + fn signed_metadata_refuses_a_downgrade_or_replay() { + let bytes = b"old colony build"; + for older in ["v1.0.0", "v0.9.0", "v1.1.0"] { + let meta = meta_for(bytes, older, "colony-linux"); + let err = check_metadata_bindings(&meta, bytes, "colony-linux", Some(older), "1.1.0") + .unwrap_err() + .to_string(); + assert!( + err.contains("downgrade"), + "{older} vs running 1.1.0 should be refused as a downgrade, got: {err}" + ); + } + } + + #[test] + fn signed_metadata_refuses_a_substituted_artefact() { + let served = b"the macos binary"; + // Genuine, correctly signed metadata - but for a DIFFERENT asset. + let meta = meta_for(served, "v1.2.0", "colony-macos"); + assert!( + check_metadata_bindings(&meta, served, "colony-linux", Some("v1.2.0"), "1.1.0") + .is_err(), + "metadata naming another asset must not validate this download" + ); + } + + #[test] + fn signed_metadata_refuses_mismatched_digest_or_tag() { + let meta = meta_for(b"expected bytes", "v1.2.0", "colony-linux"); + assert!( + check_metadata_bindings(&meta, b"tampered bytes", "colony-linux", None, "1.1.0") + .is_err(), + "digest mismatch must fail" + ); + assert!( + check_metadata_bindings( + &meta, + b"expected bytes", + "colony-linux", + Some("v1.3.0"), + "1.1.0" + ) + .is_err(), + "metadata for another tag than the one resolved must fail" + ); + } + #[test] fn extract_from_zip_works() { use std::io::Write; diff --git a/src/oauth.rs b/src/oauth.rs index ec23ae7..abbd95a 100644 --- a/src/oauth.rs +++ b/src/oauth.rs @@ -67,9 +67,16 @@ pub async fn request_device_code() -> Result { device.verification_uri ); - // Open browser with pre-filled user code + // Open browser with pre-filled user code. The URI comes from the device-flow + // response, so it goes through the same http(s)-only gate as every other + // string that reaches the desktop URI opener. let url = format!("{}?user_code={}", device.verification_uri, device.user_code); - let _ = open::that(&url); + match crate::download::web_url(&url) { + Some(safe) => { + let _ = open::that(&safe); + } + None => tracing::warn!("refusing to open non-http(s) verification uri {url:?}"), + } Ok(DeviceCode { user_code: device.user_code, diff --git a/src/persistence.rs b/src/persistence.rs index a5ad07a..10afdc4 100644 --- a/src/persistence.rs +++ b/src/persistence.rs @@ -18,9 +18,27 @@ pub fn colony_data_dir() -> Result { Ok(base) } +/// Join a repo name onto `base` as a single directory component. +/// +/// `repo_name` is remote data (the `name` field of the GitHub API listing, also +/// rehydrated from `repos_cache.json`), and it is joined into a path in every +/// per-repo helper below plus the install directory and a `remove_dir_all`. The +/// leaf-filename guard was applied everywhere but here, so the containment +/// invariant "Colony only writes under `apps//`" rested on GitHub refusing +/// `/` and `..` in repo names. Enforce it locally instead of inheriting it. +fn join_repo_component(base: PathBuf, repo_name: &str) -> Result { + crate::download::ensure_safe_component(repo_name)?; + Ok(base.join(repo_name)) +} + +/// Per-repo directory under the apps root: `/Colony/apps/{repo_name}/` +pub(crate) fn colony_app_dir(repo_name: &str) -> Result { + join_repo_component(colony_apps_dir()?, repo_name) +} + /// Directory for cached repo documentation files: `~/.config/Colony/Colony/repo-docs/{repo_name}/` fn repo_docs_dir(repo_name: &str) -> Result { - let base = colony_data_dir()?.join("repo-docs").join(repo_name); + let base = join_repo_component(colony_data_dir()?.join("repo-docs"), repo_name)?; std::fs::create_dir_all(&base)?; Ok(base) } @@ -40,7 +58,7 @@ pub fn read_repo_doc(repo_name: &str, filename: &str) -> Option { /// Directory for the cached per-repo app icon: `~/.config/Colony/Colony/repo-icons/{repo_name}/` fn repo_icon_dir(repo_name: &str) -> Result { - let base = colony_data_dir()?.join("repo-icons").join(repo_name); + let base = join_repo_component(colony_data_dir()?.join("repo-icons"), repo_name)?; std::fs::create_dir_all(&base)?; Ok(base) } @@ -91,7 +109,7 @@ pub fn installed_app_path(repo: &ColonyRepo) -> Option { ); return None; } - let path = colony_apps_dir().ok()?.join(&repo.name).join(&filename); + let path = colony_app_dir(&repo.name).ok()?.join(&filename); if path.exists() { Some(path) } else { @@ -105,31 +123,76 @@ const VERSION_FILE: &str = ".colony_version"; /// Saved resolved asset name (when using filePattern). const ASSET_FILE: &str = ".colony_asset"; +/// Marker recording that this app was installed with a verified signature. +const SIGNED_FILE: &str = ".colony_signed"; + /// Save the installed version tag for a repo. pub fn save_installed_version(repo_name: &str, tag: &str) -> Result<()> { - let path = colony_apps_dir()?.join(repo_name).join(VERSION_FILE); + let path = colony_app_dir(repo_name)?.join(VERSION_FILE); std::fs::write(&path, tag)?; Ok(()) } /// Load the installed version tag for a repo. pub fn load_installed_version(repo_name: &str) -> Option { - let path = colony_apps_dir().ok()?.join(repo_name).join(VERSION_FILE); + let path = colony_app_dir(repo_name).ok()?.join(VERSION_FILE); std::fs::read_to_string(path) .ok() .map(|s| s.trim().to_string()) } +/// Record whether the installed build was signature-verified. +/// +/// Read back by the installer to pin the requirement: `manifest.signed` lives in +/// the very repo the signature is meant to protect, so an attacker with write +/// access could flip it to false and drop the `.sig` to install unsigned code +/// silently. Once an app has been installed with a verified signature, later +/// updates must stay signed regardless of what the manifest now claims. +/// Only ever sets the marker: the pin must not be clearable by a later unsigned +/// install, which is the whole point. Uninstalling removes the app directory and +/// with it the pin, and that is the deliberate way out. +pub fn save_installed_signed(repo_name: &str) -> Result<()> { + let path = colony_app_dir(repo_name)?.join(SIGNED_FILE); + std::fs::write(&path, "1")?; + Ok(()) +} + +/// True when a previous install of this repo was signature-verified. +/// +/// Matched case-insensitively: GitHub repo names are case-insensitive for lookup +/// while the install directory is not, so a rename from `Spotter` to `spotter` +/// would otherwise present as a brand-new app and silently drop the pin (while +/// still overwriting the same lowercased `.desktop` entry). +pub fn load_installed_signed(repo_name: &str) -> bool { + if colony_app_dir(repo_name) + .map(|d| d.join(SIGNED_FILE).exists()) + .unwrap_or(false) + { + return true; + } + let Ok(apps) = colony_apps_dir() else { + return false; + }; + let wanted = repo_name.to_lowercase(); + let Ok(entries) = std::fs::read_dir(apps) else { + return false; + }; + entries.flatten().any(|e| { + e.file_name().to_string_lossy().to_lowercase() == wanted + && e.path().join(SIGNED_FILE).exists() + }) +} + /// Save the resolved asset name for a repo (when using filePattern). pub fn save_installed_asset(repo_name: &str, filename: &str) -> Result<()> { - let path = colony_apps_dir()?.join(repo_name).join(ASSET_FILE); + let path = colony_app_dir(repo_name)?.join(ASSET_FILE); std::fs::write(&path, filename)?; Ok(()) } /// Load the saved resolved asset name for a repo. pub fn load_installed_asset(repo_name: &str) -> Option { - let path = colony_apps_dir().ok()?.join(repo_name).join(ASSET_FILE); + let path = colony_app_dir(repo_name).ok()?.join(ASSET_FILE); std::fs::read_to_string(path) .ok() .map(|s| s.trim().to_string()) @@ -270,20 +333,53 @@ pub fn write_desktop_entry(repo_name: &str, exec_path: &std::path::Path) -> Resu .ok_or_else(|| anyhow::anyhow!("Cannot determine data directory"))? .join("applications"); std::fs::create_dir_all(&dir)?; - let icon_line = repo_icon_dir(repo_name) + // `repo_name` and the install path both derive from remote data, and both are + // interpolated into a line-oriented key=value format where glib keeps the + // FIRST occurrence of a key. Unescaped, a newline in either injects arbitrary + // keys - including an `Exec=` that shadows the legitimate one - and a quote in + // the path closes the Exec quoting to append arguments. The icon path contains + // repo_name too, so it goes through the same check rather than relying on + // being emitted last. + let name = desktop_value(repo_name)?; + let exec = desktop_value(&exec_path.to_string_lossy())?; + let icon_line = match repo_icon_dir(repo_name) .ok() .map(|d| d.join("icon.png")) .filter(|p| p.exists()) - .map(|p| format!("Icon={}\n", p.display())) - .unwrap_or_default(); + { + Some(p) => format!("Icon={}\n", desktop_value(&p.to_string_lossy())?), + None => String::new(), + }; let entry = format!( - "[Desktop Entry]\nType=Application\nName={repo_name}\nExec=\"{}\"\nTerminal=false\nCategories=Utility;\nComment=Installed by Colony\nX-Colony-Managed=true\n{icon_line}", - exec_path.display() + "[Desktop Entry]\nType=Application\nName={name}\nExec=\"{exec}\"\nTerminal=false\nCategories=Utility;\nComment=Installed by Colony\nX-Colony-Managed=true\n{icon_line}" ); - std::fs::write(dir.join(desktop_entry_filename(repo_name)), entry)?; + std::fs::write(dir.join(desktop_entry_filename(repo_name)?), entry)?; Ok(()) } +/// Escape a string for use as a quoted Desktop Entry value. +/// +/// Per the Desktop Entry spec, a quoted argument must backslash-escape `"`, `` ` ``, +/// `$` and `\` - the last three because implementations may expand them. Control +/// characters have no valid escape and cannot appear in a value at all (a newline +/// would inject a whole new key, and glib keeps the FIRST occurrence of a key), so +/// they are rejected outright rather than mangled. +#[cfg(target_os = "linux")] +fn desktop_value(raw: &str) -> Result { + anyhow::ensure!( + !raw.chars().any(|c| c.is_control()), + "refusing to write a desktop entry containing control characters: {raw:?}" + ); + let mut out = String::with_capacity(raw.len()); + for c in raw.chars() { + if matches!(c, '\\' | '"' | '`' | '$') { + out.push('\\'); + } + out.push(c); + } + Ok(out) +} + #[cfg(not(target_os = "linux"))] pub fn write_desktop_entry(_repo_name: &str, _exec_path: &std::path::Path) -> Result<()> { Ok(()) @@ -293,10 +389,8 @@ pub fn write_desktop_entry(_repo_name: &str, _exec_path: &std::path::Path) -> Re /// absent or on non-Linux platforms). pub fn remove_desktop_entry(repo_name: &str) { #[cfg(target_os = "linux")] - if let Some(data) = dirs::data_dir() { - let path = data - .join("applications") - .join(desktop_entry_filename(repo_name)); + if let (Some(data), Ok(name)) = (dirs::data_dir(), desktop_entry_filename(repo_name)) { + let path = data.join("applications").join(name); if path.exists() { if let Err(e) = std::fs::remove_file(&path) { tracing::warn!("failed to remove desktop entry {}: {e}", path.display()); @@ -307,9 +401,13 @@ pub fn remove_desktop_entry(repo_name: &str) { let _ = repo_name; } +/// Filename of a repo's `.desktop` entry, guarded because `repo_name` is remote +/// data and this string is joined into `~/.local/share/applications`: a separator +/// or `..` in it would let a manifest choose which entry to write or delete. #[cfg(target_os = "linux")] -fn desktop_entry_filename(repo_name: &str) -> String { - format!("colony-{}.desktop", repo_name.to_lowercase()) +fn desktop_entry_filename(repo_name: &str) -> Result { + crate::download::ensure_safe_component(repo_name)?; + Ok(format!("colony-{}.desktop", repo_name.to_lowercase())) } /// Remove ALL store caches (docs + icons for every repo). Manual cache @@ -389,6 +487,38 @@ pub fn save_scan_cache(apps: &[CachedApp]) -> Result<()> { mod tests { use super::*; + /// `repo_name` is remote data (the GitHub API listing, and `repos_cache.json` + /// on top of it) and is joined into every per-repo path plus a + /// `remove_dir_all`, so containment must not depend on GitHub's own naming + /// rules. + #[test] + fn repo_component_refuses_anything_but_a_plain_name() { + let base = PathBuf::from("/tmp/colony-test"); + assert_eq!( + join_repo_component(base.clone(), "Colony").unwrap(), + base.join("Colony") + ); + for hostile in ["../../../../.local/bin", "..", "/etc", "a/b", "", "."] { + assert!( + join_repo_component(base.clone(), hostile).is_err(), + "repo name {hostile:?} must be refused" + ); + } + } + + #[cfg(target_os = "linux")] + #[test] + fn desktop_values_are_escaped_and_control_chars_refused() { + // Quotes and backslashes are escaped rather than closing our Exec="..." + assert_eq!(desktop_value(r#"a"b"#).unwrap(), r#"a\"b"#); + assert_eq!(desktop_value(r"a\b").unwrap(), r"a\\b"); + assert_eq!(desktop_value("plain-name").unwrap(), "plain-name"); + // A newline would inject a whole key; glib keeps the FIRST Exec= it sees. + assert!(desktop_value("app\nExec=sh").is_err()); + assert!(desktop_value("app\rExec=sh").is_err()); + assert!(desktop_value("app\0").is_err()); + } + #[test] fn colony_apps_dir_returns_path() { let dir = colony_apps_dir(); diff --git a/src/signing.rs b/src/signing.rs index 3c3277e..51fe5d7 100644 --- a/src/signing.rs +++ b/src/signing.rs @@ -28,6 +28,59 @@ const RELEASE_PUBLIC_KEY: [u8; 32] = [ /// Filename suffix of the detached signature published alongside each asset. pub const SIGNATURE_SUFFIX: &str = ".sig"; +/// Filename suffix of the signed metadata sidecar published alongside each asset +/// (itself signed as `.meta.sig`). +pub const METADATA_SUFFIX: &str = ".meta"; + +/// Contents of a signed `.meta` sidecar. +/// +/// A raw-bytes signature proves only that bytes came from the release key, not +/// WHICH artefact or version they are, so an attacker able to control what the +/// release host serves could replay an older, genuinely signed build. This +/// sidecar binds the bytes to a version and a filename, and is signed with the +/// same key; `scripts/sign-release.sh` emits it for every asset. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ReleaseMetadata { + /// Release tag the asset belongs to, e.g. `v1.2.3`. + pub version: String, + /// Basename of the asset the digest covers, e.g. `colony-linux`. + pub asset: String, + /// Lowercase hex SHA-256 of the asset bytes. + pub sha256: String, +} + +impl ReleaseMetadata { + /// Parse the `key=value` sidecar format. Unknown keys are ignored so future + /// fields do not break older launchers; the three known ones are required. + pub fn parse(bytes: &[u8]) -> Result { + let text = std::str::from_utf8(bytes) + .map_err(|_| anyhow::anyhow!("release metadata is not valid UTF-8"))?; + let mut version = None; + let mut asset = None; + let mut sha256 = None; + for line in text.lines() { + let line = line.trim(); + if line.is_empty() { + continue; + } + let Some((key, value)) = line.split_once('=') else { + anyhow::bail!("malformed release metadata line: {line:?}"); + }; + match key.trim() { + "version" => version = Some(value.trim().to_string()), + "asset" => asset = Some(value.trim().to_string()), + "sha256" => sha256 = Some(value.trim().to_ascii_lowercase()), + _ => {} + } + } + Ok(Self { + version: version.ok_or_else(|| anyhow::anyhow!("release metadata has no version"))?, + asset: asset.ok_or_else(|| anyhow::anyhow!("release metadata has no asset"))?, + sha256: sha256.ok_or_else(|| anyhow::anyhow!("release metadata has no sha256"))?, + }) + } +} + /// Verify a detached ed25519 signature over `data` against the embedded Colony /// release key. Returns Ok only if the signature is valid. pub fn verify_release_signature(data: &[u8], signature_bytes: &[u8]) -> Result<()> { @@ -115,4 +168,29 @@ mod tests { assert!(verify_with_key(&TEST_PUBKEY, TEST_MSG, &[0u8; 10]).is_err()); assert!(verify_with_key(&TEST_PUBKEY, TEST_MSG, b"not-base64-!!!").is_err()); } + + #[test] + fn metadata_parses_sign_release_output() { + // Byte-for-byte what scripts/sign-release.sh writes. + let raw = b"version=v0.9.1\nasset=colony-linux\nsha256=B1A5AF3D\n"; + let meta = ReleaseMetadata::parse(raw).unwrap(); + assert_eq!(meta.version, "v0.9.1"); + assert_eq!(meta.asset, "colony-linux"); + assert_eq!(meta.sha256, "b1a5af3d", "digest is normalized to lowercase"); + } + + #[test] + fn metadata_ignores_unknown_keys() { + let raw = b"version=v1.0.0\nasset=a\nsha256=ff\nfuture=whatever\n"; + assert!(ReleaseMetadata::parse(raw).is_ok()); + } + + #[test] + fn metadata_requires_every_known_field() { + assert!(ReleaseMetadata::parse(b"asset=a\nsha256=ff\n").is_err()); + assert!(ReleaseMetadata::parse(b"version=v1\nsha256=ff\n").is_err()); + assert!(ReleaseMetadata::parse(b"version=v1\nasset=a\n").is_err()); + assert!(ReleaseMetadata::parse(b"garbage\n").is_err()); + assert!(ReleaseMetadata::parse(&[0xff, 0xfe]).is_err()); + } } diff --git a/src/update.rs b/src/update.rs index 4a0bdfe..262b9ac 100644 --- a/src/update.rs +++ b/src/update.rs @@ -248,13 +248,28 @@ impl App { let launch_result = { #[cfg(windows)] { - std::process::Command::new("cmd") - .args(["/C", "start", "", &exec]) - .spawn() - .map(|_| ()) - .map_err(|error| { - i18n::t_fmt("launch_error", &[("error", &error.to_string())]) - }) + // A scanned Windows entry is often a `.lnk`, which + // CreateProcess cannot run, so this one genuinely needs + // `start`. Build the command line with raw_arg and quote + // the target ourselves: Rust only quotes arguments that + // contain whitespace, which would leave cmd to re-parse + // `&`, `|` and friends in the path as separators. A quote + // in the path would escape our quoting, so reject it. + use std::os::windows::process::CommandExt; + if exec.contains('"') || exec.chars().any(|c| c.is_control()) { + Err(i18n::t_fmt( + "launch_error", + &[("error", "unsupported characters in application path")], + )) + } else { + std::process::Command::new("cmd") + .raw_arg(format!("/C start \"\" \"{exec}\"")) + .spawn() + .map(|_| ()) + .map_err(|error| { + i18n::t_fmt("launch_error", &[("error", &error.to_string())]) + }) + } } #[cfg(not(windows))] @@ -474,6 +489,9 @@ impl App { let file_pattern = entry.file_pattern.clone(); let binary = entry.binary.clone(); let expected_sha256 = entry.sha256.clone(); + // Only what the manifest declares; the installer ORs in + // its own pin from any previously verified install, so + // that rule lives next to the check it feeds. let require_signature = repo.manifest.signed; let repo_name = repo.name.clone(); // API calls (release resolution) use the token for @@ -658,8 +676,8 @@ impl App { // The aborted task cannot clean up its staging file: sweep // the cancelled repo's *.part leftovers here. if let Some(repo) = self.downloading_repo.take() { - if let Ok(apps_dir) = crate::persistence::colony_apps_dir() { - if let Ok(entries) = std::fs::read_dir(apps_dir.join(&repo)) { + if let Ok(app_dir) = crate::persistence::colony_app_dir(&repo) { + if let Ok(entries) = std::fs::read_dir(&app_dir) { for entry in entries.flatten() { let name = entry.file_name().to_string_lossy().to_string(); if name.ends_with(".part") { @@ -678,12 +696,11 @@ impl App { self.push_notification(i18n::t("download_cancelled"), NotificationLevel::Warning) } Message::LaunchColonyApp(path) => { - #[cfg(windows)] - let result = std::process::Command::new("cmd") - .args(["/C", "start", "", &path.display().to_string()]) - .spawn() - .map(|_| ()); - #[cfg(not(windows))] + // Executed directly on every platform, never through `cmd /C`: + // Windows only quotes an argument containing a space or tab, so a + // manifest-derived path like `app&calc` reached cmd unquoted and + // its `&` was parsed as a command separator. A store install is + // always a real executable, so the shell buys nothing here. let result = std::process::Command::new(&path).spawn().map(|_| ()); match result { @@ -721,9 +738,8 @@ impl App { // cleanup happens on catalog refresh instead.) self.release_notes.remove(&repo_name); crate::persistence::remove_desktop_entry(&repo_name); - match crate::persistence::colony_apps_dir() { - Ok(apps_dir) => { - let app_dir = apps_dir.join(&repo_name); + match crate::persistence::colony_app_dir(&repo_name) { + Ok(app_dir) => { if app_dir.exists() { if let Err(e) = std::fs::remove_dir_all(&app_dir) { self.status_message = @@ -778,8 +794,16 @@ impl App { } Message::CopyToClipboard(value) => iced::clipboard::write(value), Message::OpenUrl(url) => { - if let Err(err) = open::that(&url) { - tracing::warn!("failed to open url {url:?}: {err}"); + // Single choke point for the three producers of this message + // (Markdown link clicks, README badge pills, "View on GitHub"), + // two of which carry remote attacker-influenced strings. Only + // http(s) may reach the desktop URI opener - see is_web_url. + let Some(safe) = crate::download::web_url(&url) else { + tracing::warn!("refusing to open non-http(s) url {url:?}"); + return Task::none(); + }; + if let Err(err) = open::that(&safe) { + tracing::warn!("failed to open url {safe:?}: {err}"); } Task::none() } @@ -1751,6 +1775,47 @@ mod tests { } #[cfg(target_os = "linux")] + /// The pin is what stops a compromised repo from flipping `signed` back to + /// false, so it must survive a manifest that no longer asks for signatures - + /// and a case-only rename of the repo, which creates a different directory. + #[test] + fn signature_pin_survives_and_is_case_insensitive() { + with_temp_dirs(|| { + use crate::persistence::{load_installed_signed, save_installed_signed}; + assert!( + !load_installed_signed("Spotter"), + "no pin before any install" + ); + + // The installer creates the app directory before recording anything; + // colony_app_dir deliberately does not, so mirror that order here. + std::fs::create_dir_all(crate::persistence::colony_app_dir("Spotter").unwrap()) + .unwrap(); + save_installed_signed("Spotter").unwrap(); + assert!(load_installed_signed("Spotter")); + assert!( + load_installed_signed("spotter"), + "a case-only rename must not drop the pin" + ); + assert!(load_installed_signed("SPOTTER")); + assert!( + !load_installed_signed("SpotterX"), + "the match must not be a prefix match" + ); + + // No API can clear it: only removing the app directory does, which is + // what uninstalling deliberately performs. + save_installed_signed("Spotter").unwrap(); + assert!(load_installed_signed("Spotter")); + let dir = crate::persistence::colony_app_dir("Spotter").unwrap(); + std::fs::remove_dir_all(&dir).unwrap(); + assert!( + !load_installed_signed("Spotter"), + "uninstall clears the pin" + ); + }); + } + #[test] fn toggle_favorite_persists_to_disk() { with_temp_dirs(|| { From f341ec3e65075ba24f30f4927e89b4c0b4017467 Mon Sep 17 00:00:00 2001 From: MotherSphere Date: Sat, 25 Jul 2026 11:52:26 +0200 Subject: [PATCH 2/2] fix(test): keep the Linux gate on disk-touching tests The new signature-pin test uses with_temp_dirs, which is cfg(target_os = "linux") because only Linux honors XDG for the data dir. Gate it, and restore the gate that inserting it above toggle_favorite_persists_to_disk had taken from that test - both broke the Windows and macOS builds. --- src/update.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/update.rs b/src/update.rs index 262b9ac..7582b8c 100644 --- a/src/update.rs +++ b/src/update.rs @@ -1778,6 +1778,7 @@ mod tests { /// The pin is what stops a compromised repo from flipping `signed` back to /// false, so it must survive a manifest that no longer asks for signatures - /// and a case-only rename of the repo, which creates a different directory. + #[cfg(target_os = "linux")] #[test] fn signature_pin_survives_and_is_case_insensitive() { with_temp_dirs(|| { @@ -1816,6 +1817,7 @@ mod tests { }); } + #[cfg(target_os = "linux")] #[test] fn toggle_favorite_persists_to_disk() { with_temp_dirs(|| {