Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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});
Expand Down
66 changes: 66 additions & 0 deletions controller/app/src/main/java/org/iiab/controller/TarExtractor.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<String> command = new ArrayList<>();
command.add(tarBinary);
Expand Down Expand Up @@ -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<String> listEntries(String tarBinary, String archivePath, boolean isGzip) throws Exception {
List<String> names = new ArrayList<>();
List<String> 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();
Expand Down
Original file line number Diff line number Diff line change
@@ -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 <em>outside</em>
* the extraction directory and overwrite app files — tech-debt item <b>D11</b>.
*
* <p>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.
*
* <p>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;
}
}
Original file line number Diff line number Diff line change
@@ -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(" "));
}
}
9 changes: 7 additions & 2 deletions controller/docs/TECH_DEBT_PLAN.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down
Loading