diff --git a/CLAUDE.md b/CLAUDE.md index 56b2f10..ee88af2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -174,6 +174,16 @@ round-trips through SharedPreferences. module and fails closed (logs + skips) before building the command. The command structure is unchanged for legitimate modules. +**Slice (DONE) — archive extraction guard (`org.iiab.controller.deploy.domain`)** +Phase-1 security slice closing tech-debt **D11** (tar path-traversal). An imported +backup is untrusted; `TarExtractor` extracted without validating member names. + +- `domain/` — `ArchiveEntry.escapesRoot(name)` (pure JVM): rejects absolute paths + and `..` climbing above the destination root. Unit-tested (`ArchiveEntryTest`). +- **Legacy seam:** `TarExtractor` pre-lists entries (`tar -t`) and fails closed + before extracting if any member escapes; the backup-creation `sh -c` pipe was + single-quoted. Applies to all extractions. + **Legacy (NOT yet layered)** — most of `org.iiab.controller` is still flat: god classes `MainActivity` and `DeployFragment` (~2.7k LOC), shared mutable state on public/static fields, hand-rolled `HttpURLConnection` calls duplicated diff --git a/controller/app/src/main/java/org/iiab/controller/DeployFragment.java b/controller/app/src/main/java/org/iiab/controller/DeployFragment.java index 0b1ade5..f5a7a0f 100644 --- a/controller/app/src/main/java/org/iiab/controller/DeployFragment.java +++ b/controller/app/src/main/java/org/iiab/controller/DeployFragment.java @@ -1893,7 +1893,10 @@ private void bindBackupButtonLogic(MainActivity mainAct, File backupsDir, File i String tarBin = staticTar.exists() ? staticTar.getAbsolutePath() : "tar"; String gzipBin = staticGzip.exists() ? staticGzip.getAbsolutePath() : "gzip"; - String cmd = tarBin + " -cf - -C " + iiabRootDir.getAbsolutePath() + " installed-rootfs | " + gzipBin + " > " + backupFile.getAbsolutePath(); + // D11: single-quote the interpolated paths so the backup pipe is robust + // even if a path ever contains spaces/metacharacters (app-internal today). + String cmd = "'" + tarBin + "' -cf - -C '" + iiabRootDir.getAbsolutePath() + + "' installed-rootfs | '" + gzipBin + "' > '" + backupFile.getAbsolutePath() + "'"; // D12: ProcessRunner drains stderr so a large backup with tar warnings // cannot deadlock on a full pipe buffer. ProcessRunner.Result backupResult = ProcessRunner.run(new String[]{"/system/bin/sh", "-c", cmd}); diff --git a/controller/app/src/main/java/org/iiab/controller/TarExtractor.java b/controller/app/src/main/java/org/iiab/controller/TarExtractor.java index 2cd57e7..b977d12 100644 --- a/controller/app/src/main/java/org/iiab/controller/TarExtractor.java +++ b/controller/app/src/main/java/org/iiab/controller/TarExtractor.java @@ -9,6 +9,8 @@ package org.iiab.controller; +import org.iiab.controller.deploy.domain.ArchiveEntry; + import android.content.Context; import android.os.Handler; import android.os.Looper; @@ -51,6 +53,16 @@ public void startExtraction(Context context, String archivePath, String destDir, boolean isGzip = archivePath.toLowerCase().endsWith(".gz"); + // D11: refuse path-traversal. List the archive members first and + // bail out (without extracting anything) if any member is absolute + // or climbs out of destDir via "..". An imported/restored backup is + // untrusted, so this runs for every extraction. + for (String entry : listEntries(tarBinary, archivePath, isGzip)) { + if (ArchiveEntry.escapesRoot(entry)) { + throw new Exception("Unsafe archive entry (path traversal): " + entry); + } + } + // 2. BUILD THE COMMAND List command = new ArrayList<>(); command.add(tarBinary); @@ -125,6 +137,60 @@ public void startExtraction(Context context, String archivePath, String destDir, }).start(); } + + /** + * D11: enumerate the archive's member names without extracting, so we can + * reject path-traversal before any file is written. Mirrors the extraction + * invocation (gzip is decompressed in Java and piped to {@code tar -t}). + */ + private List listEntries(String tarBinary, String archivePath, boolean isGzip) throws Exception { + List names = new ArrayList<>(); + List listCmd = new ArrayList<>(); + listCmd.add(tarBinary); + if (isGzip) { + listCmd.add("-t"); + listCmd.add("-f"); + listCmd.add("-"); + } else { + listCmd.add("-tf"); + listCmd.add(archivePath); + } + + Process listProcess = new ProcessBuilder(listCmd).start(); + + Thread feeder = null; + if (isGzip) { + feeder = new Thread(() -> { + try (GZIPInputStream gis = new GZIPInputStream(new FileInputStream(archivePath)); + OutputStream os = listProcess.getOutputStream()) { + byte[] buffer = new byte[8192]; + int read; + while ((read = gis.read(buffer)) != -1) { + os.write(buffer, 0, read); + } + os.flush(); + } catch (Exception ignored) { + } + }); + feeder.start(); + } + + try (BufferedReader reader = new BufferedReader(new InputStreamReader(listProcess.getInputStream()))) { + String line; + while ((line = reader.readLine()) != null) { + names.add(line); + } + } + + int exitCode = listProcess.waitFor(); + if (feeder != null) feeder.join(); + if (exitCode != 0) { + // Could not verify the archive -> fail closed rather than extract blind. + throw new Exception("Could not read archive listing for verification (tar exit " + exitCode + ")"); + } + return names; + } + public void stopExtraction() { if (tarProcess != null) { tarProcess.destroy(); diff --git a/controller/app/src/main/java/org/iiab/controller/deploy/domain/ArchiveEntry.java b/controller/app/src/main/java/org/iiab/controller/deploy/domain/ArchiveEntry.java new file mode 100644 index 0000000..fc92220 --- /dev/null +++ b/controller/app/src/main/java/org/iiab/controller/deploy/domain/ArchiveEntry.java @@ -0,0 +1,70 @@ +/* + * ============================================================================ + * Name : ArchiveEntry.java + * Author : IIAB Project + * Copyright : Copyright (c) 2026 IIAB Project + * Description : Domain rule: is an archive member name safe to extract, i.e. + * does it stay inside the destination directory? Closes + * tech-debt item D11 (tar path-traversal on extraction). + * ============================================================================ + */ +package org.iiab.controller.deploy.domain; + +/** + * Pure (framework-free) guard against archive path-traversal ("Zip Slip" for + * tar). An imported/restored backup is untrusted: a crafted {@code .tar.gz} + * whose members use {@code ../} or absolute paths could write outside + * the extraction directory and overwrite app files — tech-debt item D11. + * + *

No {@code android.*} here, so it is unit-testable on a plain JVM and + * reusable by any extractor. + */ +public final class ArchiveEntry { + + private ArchiveEntry() { + // Static utility; not instantiable. + } + + /** + * True if extracting {@code name} could escape the destination root, i.e. + * the member is an absolute path or uses {@code ..} segments that climb + * above the root. Benign relative paths (including a leading {@code ./} and + * an internal {@code a/../b} that stays within the tree) return false. + * + *

Mirrors the safe model "strip leading slash, forbid climbing out": a + * legitimate distro rootfs / backup uses relative members and never needs + * to escape, so this rejects attacks without rejecting real archives. + */ + public static boolean escapesRoot(String name) { + if (name == null) { + return false; // ignore blank listing lines, not an escape + } + String n = name.replace('\\', '/').trim(); + if (n.isEmpty()) { + return false; + } + if (n.startsWith("/")) { + return true; // absolute path — would ignore the -C destination + } + int depth = 0; + int start = 0; + for (int i = 0; i <= n.length(); i++) { + if (i == n.length() || n.charAt(i) == '/') { + String seg = n.substring(start, i); + start = i + 1; + if (seg.isEmpty() || seg.equals(".")) { + continue; + } + if (seg.equals("..")) { + depth--; + if (depth < 0) { + return true; // climbed above the root + } + } else { + depth++; + } + } + } + return false; + } +} diff --git a/controller/app/src/test/java/org/iiab/controller/deploy/domain/ArchiveEntryTest.java b/controller/app/src/test/java/org/iiab/controller/deploy/domain/ArchiveEntryTest.java new file mode 100644 index 0000000..dffab67 --- /dev/null +++ b/controller/app/src/test/java/org/iiab/controller/deploy/domain/ArchiveEntryTest.java @@ -0,0 +1,49 @@ +package org.iiab.controller.deploy.domain; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import org.junit.Test; + +/** + * Unit tests for {@link ArchiveEntry} — the path-traversal guard that closes + * D11 (malicious tar member names escaping the extraction directory). + */ +public class ArchiveEntryTest { + + @Test + public void allowsNormalRelativeEntries() { + assertFalse(ArchiveEntry.escapesRoot("etc/hosts")); + assertFalse(ArchiveEntry.escapesRoot("./etc/hosts")); + assertFalse(ArchiveEntry.escapesRoot("installed-rootfs/usr/bin/bash")); + assertFalse(ArchiveEntry.escapesRoot("a/b/c/")); // directory entry + assertFalse(ArchiveEntry.escapesRoot("a/../b")); // dips in then stays inside + } + + @Test + public void blocksParentTraversal() { + assertTrue(ArchiveEntry.escapesRoot("../etc/passwd")); + assertTrue(ArchiveEntry.escapesRoot("a/../../b")); + assertTrue(ArchiveEntry.escapesRoot("foo/../../../bar")); + assertTrue(ArchiveEntry.escapesRoot("..")); // bare parent + } + + @Test + public void blocksAbsolutePaths() { + assertTrue(ArchiveEntry.escapesRoot("/etc/passwd")); + assertTrue(ArchiveEntry.escapesRoot("/")); + } + + @Test + public void blocksBackslashTraversal() { + // Normalize backslashes so a Windows-style payload can't sneak past. + assertTrue(ArchiveEntry.escapesRoot("..\\..\\x")); + } + + @Test + public void ignoresBlankOrNull() { + assertFalse(ArchiveEntry.escapesRoot(null)); + assertFalse(ArchiveEntry.escapesRoot("")); + assertFalse(ArchiveEntry.escapesRoot(" ")); + } +} diff --git a/controller/docs/TECH_DEBT_PLAN.md b/controller/docs/TECH_DEBT_PLAN.md index fa101a3..c0ac91a 100644 --- a/controller/docs/TECH_DEBT_PLAN.md +++ b/controller/docs/TECH_DEBT_PLAN.md @@ -61,8 +61,13 @@ _Last updated: 2026-06-17. Tracks remediation work against the findings below. I - `:app:syncNativeArtifacts` (`preBuild` dependency) called the GitHub API **unauthenticated on every build**; since `jniLibs/*.so` are gitignored, CI always downloads and hit GitHub's 60/hr unauthenticated limit -> intermittent **HTTP 403** ("tag not found"), failing `assembleDebug` on unrelated PRs. - Fix: (1) **cache-first** — check the local tracker/binaries/manifest before any network call, so builds with artifacts present skip the API entirely; (2) **fallback auth** — fetch release metadata UNAUTHENTICATED first (so forks without a token still build), and only on failure retry with `GITHUB_TOKEN`/`GH_TOKEN` from the env; (3) clearer rate-limit error. CI passes `secrets.GITHUB_TOKEN` to the gradle steps (job-level env). No token is ever *required*. -**Phase 1 — Security hardening: IN PROGRESS.** Done so far: **S1** (PR #9), **M4**, **S3** (PR #10), **D6** (PR #12), **D2** (PR #13), **D12**. Remaining: **D11**, **S4**, **F15**. -## 1. Executive summary +**D11 — Archive path-traversal on extraction (Phase 1 security): DONE** (PR `fix/phase1-security-d11-archive-traversal`) +- Closes **D11**: `TarExtractor` extracted with `tar -xf -C destDir` without validating member names. An **imported/restored backup is untrusted** (`Import backup` accepts any file and `restore` extracts it into `filesDir/rootfs`), so a crafted `.tar.gz` using `../` or absolute members could escape the destination and overwrite app files. +- New pure domain rule `org.iiab.controller.deploy.domain.ArchiveEntry.escapesRoot(name)` (rejects absolute paths and `..` that climb above the root; allows benign `./` and internal `a/../b`). Unit-tested (`ArchiveEntryTest`). +- `TarExtractor` now **pre-lists** entries (`tar -t`, gzip decompressed in Java) and **fails closed** if any member escapes — for *every* extraction (the verified rootfs install included, defense in depth). Also single-quoted the paths in the backup-creation `sh -c` pipe (the "unquoted backup pipe" half of D11). +- Verify on a real install/restore that legitimate rootfs/backup members are relative (they are by convention) so the guard does not false-positive. + +**Phase 1 — Security hardening: IN PROGRESS.** Done so far: **S1** (PR #9), **M4**, **S3** (PR #10), **D6** (PR #12), **D2** (PR #13), **D12** (PR #16), **D11**. Remaining: **S4**, **F15**.## 1. Executive summary The Controller is functional and shows real security intent (it SHA256-audits native binaries at build time, scrubs the keystore in CI, and scopes most broadcasts). But it carries debt on four fronts that scale badly toward the README's "millions of users" goal: