7/19

SECURITY_REVIEW_2026-07-16.md

@@ -0,0 +1,130 @@
+# Security Review — libglacier-ng (dev branch, working tree)
+
+Date: 2026-07-16
+Scope: uncommitted working-tree changes (Makefile, config.mk, src/common.h,
+src/istoreutils.{c,h}, new src/transaction.{c,h}). The `dev` branch had no
+new commits vs. `origin/dev`, so the actual diff under review is the
+unstaged/untracked working-tree state.
+
+All three findings below share one root cause: package manifests
+(`PKG_NAME` / `PKG_REPO`, read from `manifest.gpm.cfg` inside the `.gpkg`
+archive being installed) are fully attacker-controlled and are never
+validated anywhere in the codebase. A full-repo search for
+`signature|verify|trusted|checksum|sha256|gpg|allowlist` returned zero
+hits — there is no package authenticity or provenance check at all.
+
+This PR's changes (`GL_SCOPE_SYS` / `gl_init_sys`, and the new
+`GL_BASE_REPO` → `/usr` symlink routing) newly plug that untrusted data
+into root-privileged, system-wide install paths, which is what escalates
+these from "attacker writes into their own store" to "attacker writes as
+root, or shadows real `/usr` binaries."
+
+---
+
+## Finding 1 — Authorization bypass via self-declared "base" repo
+
+* **File:** `src/istoreutils.c:742` (also `:859`, `:1404`)
+* **Severity:** High
+* **Category:** `authorization-bypass`
+* **Confidence:** High
+
+**Description:** System-scope symlink placement into `/usr` is gated only
+by a package's manifest self-declaring `PKG_REPO="base"`. There is no
+check that the package actually originated from a trusted/vetted base
+repository — the classification is just a string the package author
+wrote into their own manifest.
+
+**Exploit scenario:** An operator runs `gpkg -s malicious.gpkg` (a
+normal, documented system-scope install per `PACKAGE_SCOPES.txt`) on a
+package whose `manifest.gpm.cfg` the attacker fully controls. Setting
+`PKG_REPO="base"` causes `gl_link_pkg` (istoreutils.c:742) and
+`gl_relink_store` (istoreutils.c:1404) to route the package's symlinks
+into `GL_BASE_LINKS_DEST` (`/usr`) instead of the isolated
+`/glacier/sys/links` tree — the same privileged, real-FHS locations init
+scripts, systemd units, and shebang lines trust. Combined with Finding 3
+(unsanitized `PKG_NAME`), the attacker can target specific paths such as
+`/usr/bin/sudo`.
+
+**Recommendation:** Don't trust the manifest's self-declared repo for
+privilege routing. Either (a) determine repo/trust from the source the
+package was fetched from (signed repo metadata, not embedded manifest
+fields), or (b) require a separate signature/checksum check before a
+package is allowed to claim `base` classification and land in `/usr`.
+
+---
+
+## Finding 2 — Tar-slip path traversal during archive extraction
+
+* **File:** `src/istoreutils.c:1242` (loop starting ~1234, `dest_path` built at 1249)
+* **Severity:** High
+* **Category:** `path-traversal`
+* **Confidence:** High
+
+**Description:** `gl_install_pkg` extracts the package tar with a
+hand-rolled `open()`/`mkdir()` loop instead of libarchive's
+`archive_write_disk` (which offers `ARCHIVE_EXTRACT_SECURE_NODOTDOT` /
+`SECURE_SYMLINKS` protections). The per-entry relative path (`rel_path`,
+istoreutils.c:1242) is taken directly from the tar entry name after
+stripping a known prefix, and is concatenated into `dest_path` via
+`snprintf` with no rejection of `..` segments or absolute paths.
+
+**Exploit scenario:** Because the strip prefix is
+`"<pkg_name>-<ver>/files/"` and `pkg_name` is itself attacker-controlled
+(pulled verbatim from the manifest, no validation), the attacker fully
+controls the prefix match and can name a tar entry
+`"<pkg_name>-<ver>/files/../../../../etc/cron.d/pwn"`. After prefix
+stripping this becomes `../../../../etc/cron.d/pwn`, written wherever the
+installing process can write. This code path is reachable for both
+per-uid and (newly, via this PR) root-privileged system-scope installs,
+so the same bug now yields root-level arbitrary file write.
+
+**Recommendation:** Reject any entry path containing `..` components or a
+leading `/` before extraction, or switch to `archive_write_disk` with
+`ARCHIVE_EXTRACT_SECURE_NODOTDOT | ARCHIVE_EXTRACT_SECURE_SYMLINKS |
+ARCHIVE_EXTRACT_SECURE_NOABSOLUTEPATHS` set.
+
+---
+
+## Finding 3 — Path traversal via unsanitized PKG_NAME / PKG_REPO in store paths
+
+* **File:** `src/istoreutils.c:1196` (manifest parsing: `src/pkg.c:149,153`)
+* **Severity:** Medium–High
+* **Category:** `path-traversal`
+* **Confidence:** High
+
+**Description:** `gl_gpm2gpkg` (`pkg.c:149,153`) reads `PKG_NAME` and
+`PKG_REPO` straight out of the archive's manifest config with
+`config_lookup_string` + `strdup`, no validation. `gl_install_pkg` then
+builds the package's store directory as `"%s/%s/%s"` from
+`ctx->store_path`, `pkg.pkg_repo`, `pkg.pkg_name` (istoreutils.c:1196)
+and `mkdir -p`s it.
+
+**Exploit scenario:** A manifest with `PKG_REPO="../../../../tmp"` (or a
+`PKG_NAME` containing `../` segments) causes the package directory —
+and, via the extraction loop right after (Finding 2), arbitrary files —
+to be created outside `ctx->store_path` entirely. Previously this only
+affected a single user's own per-uid store; with this PR's new
+`GL_SCOPE_SYS` (`ctx->store_path == "/glacier/sys/store"`, root-owned),
+the same bug now lets an untrusted package write anywhere root can
+write.
+
+**Recommendation:** Validate `PKG_NAME` and `PKG_REPO` against an
+allowlist pattern (e.g. `^[A-Za-z0-9._-]+$`) immediately after parsing
+in `gl_gpm2gpkg`, rejecting the manifest outright if either field
+contains `/`, `..`, or is empty.
+
+---
+
+## Note — unrelated working-tree cleanup
+
+The working tree also contains what look like leftover failed-patch
+artifacts, not part of the reviewed logic but worth cleaning up before
+committing:
+
+```
+istoreutils.c.diff
+istoreutils.h.diff
+src/istoreutils.c.back
+src/istoreutils.c.rej
+src/istoreutils.h.back
+```