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..7582b8c 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() } @@ -1750,6 +1774,49 @@ 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. + #[cfg(target_os = "linux")] + #[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" + ); + }); + } + #[cfg(target_os = "linux")] #[test] fn toggle_favorite_persists_to_disk() {