Skip to content

Commit 8307ec3

Browse files
Merge pull request #13 from appdevforall/fix/phase1-security-d2-module-injection
fix(controller): allowlist module names to close command injection (D2)
2 parents 9cb9fcc + c460b5c commit 8307ec3

6 files changed

Lines changed: 191 additions & 2 deletions

File tree

‎CLAUDE.md‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,21 @@ validation, allowing config-directive/URL injection.
159159
`RsyncManager` re-checks defensively before writing the daemon config / building
160160
the client URL. Reusable by the remaining injection fixes (S4, D2) as they migrate.
161161

162+
**Slice (DONE) — deploy module-name allowlist (`org.iiab.controller.deploy.domain`)**
163+
Phase-1 security slice closing tech-debt **D2** (command injection). The install
164+
queue interpolates a module name into a `sed/echo/runrole` command run as root
165+
in the container; the name comes from the fixed `ModuleRegistry` catalog but
166+
round-trips through SharedPreferences.
167+
168+
- `domain/` — `ModuleName` (pure JVM): `isAllowed(name, knownKeys)` = the name is
169+
a known catalog key AND matches `[a-z0-9_-]` (no shell metacharacters).
170+
Unit-tested (`deploy/domain/ModuleNameTest`).
171+
- `ModuleRegistry.validYamlKeys()` is the single allowlist source (derived from
172+
`MASTER_ROSTER`).
173+
- **Legacy seam:** `DeployFragment.processNextInQueue()` validates the popped
174+
module and fails closed (logs + skips) before building the command. The command
175+
structure is unchanged for legitimate modules.
176+
162177
**Legacy (NOT yet layered)** — most of `org.iiab.controller` is still flat:
163178
god classes `MainActivity` and `DeployFragment` (~2.7k LOC), shared mutable
164179
state on public/static fields, hand-rolled `HttpURLConnection` calls duplicated

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
*/
99
package org.iiab.controller;
1010

11+
import org.iiab.controller.deploy.domain.ModuleName;
1112
import android.app.NotificationChannel;
1213
import android.app.NotificationManager;
1314
import android.app.PendingIntent;
@@ -18,6 +19,7 @@
1819
import android.content.res.ColorStateList;
1920
import android.graphics.Color;
2021
import android.graphics.Typeface;
22+
import android.util.Log;
2123
import android.net.Uri;
2224
import android.os.Bundle;
2325
import android.os.Environment;
@@ -1393,6 +1395,19 @@ private void processNextInQueue() {
13931395

13941396
String nextModule = installationQueue.remove(0);
13951397
saveQueueToPrefs();
1398+
1399+
// D2: nextModule is interpolated into a command run as root inside the
1400+
// container (sed/echo/runrole). Only allow names from the known catalog
1401+
// with no shell metacharacters; fail closed and skip anything else.
1402+
if (!ModuleName.isAllowed(nextModule, ModuleRegistry.validYamlKeys())) {
1403+
Log.e(TAG, "Refusing to install unrecognized/unsafe module name: " + nextModule);
1404+
if (getActivity() instanceof MainActivity) {
1405+
((MainActivity) getActivity()).addToLog("[Security] Skipped invalid module: " + nextModule);
1406+
}
1407+
processNextInQueue();
1408+
return;
1409+
}
1410+
13961411
btnLaunchInstall.setEnabled(false);
13971412
btnLaunchInstall.setText(getString(R.string.install_status_installing_module, nextModule));
13981413

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

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,9 @@
1010
package org.iiab.controller;
1111

1212
import java.util.Arrays;
13+
import java.util.HashSet;
1314
import java.util.List;
15+
import java.util.Set;
1416

1517
public class ModuleRegistry {
1618

@@ -41,4 +43,17 @@ public IiabModule(String endpoint, int nameResId, boolean requires64Bit, String
4143
// so the system doesn't break if it gets added in the future.
4244
new IiabModule("dashboard", R.string.dash_system, false, "dashboard")
4345
);
44-
}
46+
47+
/**
48+
* The set of valid module YAML keys. This is the single allowlist for any
49+
* value interpolated into a shell/Ansible command (see D2). Derived from
50+
* {@link #MASTER_ROSTER} so the catalog stays the single source of truth.
51+
*/
52+
public static Set<String> validYamlKeys() {
53+
Set<String> keys = new HashSet<>();
54+
for (IiabModule m : MASTER_ROSTER) {
55+
keys.add(m.yamlBaseKey);
56+
}
57+
return keys;
58+
}
59+
}
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
/*
2+
* ============================================================================
3+
* Name : ModuleName.java
4+
* Author : IIAB Project
5+
* Copyright : Copyright (c) 2026 IIAB Project
6+
* Description : Domain rule: a module name is only safe to install if it is a
7+
* known catalog entry AND contains no shell metacharacters.
8+
* Closes tech-debt item D2 (command injection via module name).
9+
* ============================================================================
10+
*/
11+
package org.iiab.controller.deploy.domain;
12+
13+
import java.util.Set;
14+
15+
/**
16+
* Pure (framework-free) guard for module names that get interpolated into a
17+
* shell/Ansible command executed <strong>as root inside the container</strong>
18+
* (see {@code DeployFragment}: {@code sed ... && echo ... && ./runrole NAME}).
19+
*
20+
* <p>A crafted name containing a quote, {@code ;}, {@code &&}, {@code $()} etc.
21+
* would break out of the command — tech-debt item <b>D2</b>. Module names come
22+
* from a fixed catalog ({@code ModuleRegistry.MASTER_ROSTER}) and round-trip
23+
* through SharedPreferences, so this enforces an <em>allowlist</em>: the name
24+
* must be one of the known catalog keys and also be well-formed.
25+
*
26+
* <p>No {@code android.*} here so it is unit-testable on a plain JVM and reusable
27+
* by any other code path that interpolates a module name.
28+
*/
29+
public final class ModuleName {
30+
31+
private static final int MAX_LEN = 64;
32+
33+
private ModuleName() {
34+
// Static utility; not instantiable.
35+
}
36+
37+
/**
38+
* True if {@code name} is one of the {@code known} catalog keys and is
39+
* well-formed. Fail-closed: anything not explicitly allowed is rejected.
40+
*/
41+
public static boolean isAllowed(String name, Set<String> known) {
42+
return name != null && known != null && known.contains(name) && isWellFormed(name);
43+
}
44+
45+
/**
46+
* True if {@code name} is a safe module identifier: a non-empty run of
47+
* {@code [a-z0-9_-]} (no uppercase, whitespace, quotes or shell
48+
* metacharacters), at most {@value #MAX_LEN} chars. Matches every key in the
49+
* current roster (e.g. {@code calibreweb}, {@code kiwix}, {@code maps}).
50+
*/
51+
public static boolean isWellFormed(String name) {
52+
if (name == null || name.isEmpty() || name.length() > MAX_LEN) {
53+
return false;
54+
}
55+
for (int i = 0; i < name.length(); i++) {
56+
char c = name.charAt(i);
57+
boolean ok = (c >= 'a' && c <= 'z')
58+
|| (c >= '0' && c <= '9')
59+
|| c == '_'
60+
|| c == '-';
61+
if (!ok) {
62+
return false;
63+
}
64+
}
65+
return true;
66+
}
67+
}
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
package org.iiab.controller.deploy.domain;
2+
3+
import static org.junit.Assert.assertFalse;
4+
import static org.junit.Assert.assertTrue;
5+
6+
import java.util.Arrays;
7+
import java.util.HashSet;
8+
import java.util.Set;
9+
10+
import org.junit.Test;
11+
12+
/**
13+
* Unit tests for {@link ModuleName} — the allowlist guard that closes D2
14+
* (command injection via a module name interpolated into a root shell command).
15+
* Pure JVM, no Android dependencies.
16+
*/
17+
public class ModuleNameTest {
18+
19+
private static final Set<String> ROSTER = new HashSet<>(Arrays.asList(
20+
"calibreweb", "code", "kiwix", "kolibri", "maps", "matomo", "dashboard"));
21+
22+
@Test
23+
public void acceptsEveryKnownRosterModule() {
24+
for (String m : ROSTER) {
25+
assertTrue("roster module rejected: " + m, ModuleName.isAllowed(m, ROSTER));
26+
}
27+
}
28+
29+
@Test
30+
public void rejectsShellInjectionEvenIfWellFormedPrefix() {
31+
assertFalse(ModuleName.isAllowed("kiwix; rm -rf /", ROSTER));
32+
assertFalse(ModuleName.isAllowed("kiwix' && reboot #", ROSTER));
33+
assertFalse(ModuleName.isAllowed("$(reboot)", ROSTER));
34+
assertFalse(ModuleName.isAllowed("maps`id`", ROSTER));
35+
assertFalse(ModuleName.isAllowed("maps\nmatomo", ROSTER));
36+
}
37+
38+
@Test
39+
public void rejectsWellFormedButUnknownModule() {
40+
// Charset-valid but not in the catalog -> still rejected (allowlist).
41+
assertFalse(ModuleName.isAllowed("evil", ROSTER));
42+
assertFalse(ModuleName.isAllowed("wordpress", ROSTER));
43+
}
44+
45+
@Test
46+
public void rejectsNulls() {
47+
assertFalse(ModuleName.isAllowed(null, ROSTER));
48+
assertFalse(ModuleName.isAllowed("kiwix", null));
49+
}
50+
51+
// --- isWellFormed (the charset rule on its own) ---
52+
53+
@Test
54+
public void wellFormedAcceptsCatalogStyleNames() {
55+
assertTrue(ModuleName.isWellFormed("calibreweb"));
56+
assertTrue(ModuleName.isWellFormed("kiwix"));
57+
assertTrue(ModuleName.isWellFormed("a-b_c1"));
58+
}
59+
60+
@Test
61+
public void wellFormedRejectsMetacharsAndCase() {
62+
assertFalse(ModuleName.isWellFormed("Kiwix")); // uppercase
63+
assertFalse(ModuleName.isWellFormed("a b")); // space
64+
assertFalse(ModuleName.isWellFormed("a;b")); // semicolon
65+
assertFalse(ModuleName.isWellFormed("a'b")); // quote
66+
assertFalse(ModuleName.isWellFormed("a/b")); // slash
67+
assertFalse(ModuleName.isWellFormed("")); // empty
68+
assertFalse(ModuleName.isWellFormed(null)); // null
69+
StringBuilder sb = new StringBuilder();
70+
for (int i = 0; i < 65; i++) sb.append('a');
71+
assertFalse(ModuleName.isWellFormed(sb.toString())); // too long
72+
}
73+
}

‎controller/docs/TECH_DEBT_PLAN.md‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,12 @@ _Last updated: 2026-06-17. Tracks remediation work against the findings below. I
4747
- **Integrity**: `--check-integrity=true` makes aria2 verify the SHA-256 checksums published in the `.meta4` (confirmed present: file-level + per-piece) during download; on mismatch aria2 exits non-zero, `onError` fires and the archive is never extracted. Uses the server's published hash + verified TLS, so no redundant on-device re-hash of the ~1.2 GB file.
4848
- Follow-up (separate items): ZIM content integrity (Kiwix publishes no embedded hash today; needs `.sha256` sidecars) and the cleartext OTA APK path in `MainActivity` (**F15**).
4949

50-
**Phase 1 — Security hardening: IN PROGRESS.** Done so far: **S1** (PR #9), **M4**, **S3** (PR #10), **D6**. Remaining: **D2**, **D12**, **S4**, **D11**.
50+
**D2 — Module-name command injection (Phase 1 security): DONE** (PR `fix/phase1-security-d2-module-injection`)
51+
- Closes **D2**: `DeployFragment.processNextInQueue()` interpolated the module name into a `sed ... && echo ... && ./runrole NAME` command executed as root in the container. Names come from the fixed `ModuleRegistry` catalog but round-trip through SharedPreferences, so a tampered/unknown value with shell metacharacters could inject commands.
52+
- New pure domain rule `org.iiab.controller.deploy.domain.ModuleName.isAllowed(name, known)`: the name must be a known catalog key (allowlist) AND match `[a-z0-9_-]` (no quotes/`;`/`&&`/`$()`). `ModuleRegistry.validYamlKeys()` is the single allowlist source. Unit-tested (`ModuleNameTest`).
53+
- The guard fails closed: an unrecognized/unsafe module is logged and skipped before the command is built; the command structure is unchanged for legitimate modules. Lower-risk on-device `sh -c` pipes (extract/backup, app-internal paths) are a documented follow-up.
5154

55+
**Phase 1 — Security hardening: IN PROGRESS.** Done so far: **S1** (PR #9), **M4**, **S3** (PR #10), **D6** (PR #12), **D2**. Remaining: **D12**, **S4**, **D11**.
5256
## 1. Executive summary
5357

5458
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:

0 commit comments

Comments
 (0)