diff --git a/CLAUDE.md b/CLAUDE.md index 53a2752..af7de61 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -144,6 +144,21 @@ app's ABI, so a 32-bit app on a 64-bit phone reported 32-bit). use case; `getTermuxArch()` (app/content ABI) is unchanged for modules, termux and debian arch. +**Slice (DONE) — sync credential validation (`org.iiab.controller.sync.domain`)** +First Phase-1 *security* slice (refactor-by-feature while closing tech-debt **S1**). +The peer-to-peer sync handshake carries host/port/user/pass in a scanned QR code; +those values were interpolated into `rsyncd.conf` and a `rsync://` URL with no +validation, allowing config-directive/URL injection. + +- `domain/` — `SyncCredentialValidator` (pure JVM, no Android): the single rule + for "what is a safe sync credential" — strict charsets for user/host, range + check for port, control-char-free password, and an `isSafeConfigValue` guard + for `rsyncd.conf` values. Unit-tested (`sync/domain/SyncCredentialValidatorTest`). +- **Legacy seam:** `SyncHandshakeHelper.parsePayload()` now validates at the QR + parse boundary (returns `null` -> existing "invalid QR" toast), and + `RsyncManager` re-checks defensively before writing the daemon config / building + the client URL. Reusable by the remaining injection fixes (S4, D2) as they migrate. + **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/RsyncManager.java b/controller/app/src/main/java/org/iiab/controller/RsyncManager.java index 1487437..5857376 100644 --- a/controller/app/src/main/java/org/iiab/controller/RsyncManager.java +++ b/controller/app/src/main/java/org/iiab/controller/RsyncManager.java @@ -14,6 +14,8 @@ import android.os.Looper; import android.util.Log; +import org.iiab.controller.sync.domain.SyncCredentialValidator; + import java.io.BufferedReader; import java.io.File; import java.io.FileOutputStream; @@ -42,6 +44,18 @@ public boolean startServer(Context context, int port, String user, String pass, stop(); isCancelled = false; + // S1 (defence in depth): never write attacker-controllable text into + // rsyncd.conf without validation. user/pass are app-generated and + // dirToShare is app-controlled, but a stray CR/LF here would let new + // config directives or module sections be injected. + if (!SyncCredentialValidator.isValidUsername(user) + || !SyncCredentialValidator.isValidPassword(pass) + || !SyncCredentialValidator.isValidPort(port) + || !SyncCredentialValidator.isSafeConfigValue(dirToShare)) { + Log.e(TAG, "Refusing to start rsync daemon: invalid credentials or share path"); + return false; + } + try { File nativeLibDir = new File(context.getApplicationInfo().nativeLibraryDir); File rsyncBin = new File(nativeLibDir, "librsync.so"); @@ -122,6 +136,13 @@ public void startClient(Context context, String hostIp, int port, String user, S throw new Exception(context.getString(R.string.rsync_error_binary_missing)); } + // S1: reject credentials that could break out of the rsync:// URL. + if (!SyncCredentialValidator.validateCredentials(hostIp, port, user, pass).valid) { + mainHandler.post(() -> listener.onError( + context.getString(R.string.rsync_error_invalid_credentials))); + return; + } + File passFile = new File(context.getCacheDir(), "rsync_client.pass"); writeTextToFile(passFile, pass); @@ -220,6 +241,13 @@ public void calculateTransferPlan(Context context, String hostIp, int port, Stri if (!rsyncBin.exists()) throw new Exception(context.getString(R.string.rsync_error_binary_missing)); + // S1: reject credentials that could break out of the rsync:// URL. + if (!SyncCredentialValidator.validateCredentials(hostIp, port, user, pass).valid) { + mainHandler.post(() -> listener.onError( + context.getString(R.string.rsync_error_invalid_credentials))); + return; + } + File passFile = new File(context.getCacheDir(), "rsync_client.pass"); writeTextToFile(passFile, pass); passFile.setExecutable(false, false); diff --git a/controller/app/src/main/java/org/iiab/controller/SyncHandshakeHelper.java b/controller/app/src/main/java/org/iiab/controller/SyncHandshakeHelper.java index 659ba67..bca1a14 100644 --- a/controller/app/src/main/java/org/iiab/controller/SyncHandshakeHelper.java +++ b/controller/app/src/main/java/org/iiab/controller/SyncHandshakeHelper.java @@ -14,6 +14,8 @@ import org.json.JSONObject; +import org.iiab.controller.sync.domain.SyncCredentialValidator; + import java.security.SecureRandom; import com.google.zxing.BarcodeFormat; @@ -76,11 +78,26 @@ public static SyncCredentials parsePayload(String scannedJson) { Log.w(TAG, "Scanned QR is not an IIAB Sync code."); return null; } + String ip = json.getString("ip"); + int port = json.getInt("port"); + String user = json.getString("user"); + String pass = json.getString("pass"); + + // S1: the QR payload is untrusted input that is later interpolated + // into rsyncd.conf and a rsync:// URL. Reject anything that could + // inject config directives or break out of the URL. + SyncCredentialValidator.Result check = + SyncCredentialValidator.validateCredentials(ip, port, user, pass); + if (!check.valid) { + Log.w(TAG, "Rejecting scanned credentials: " + check.reason); + return null; + } + return new SyncCredentials( - json.getString("ip"), - json.getInt("port"), - json.getString("user"), - json.getString("pass"), + ip, + port, + user, + pass, json.optBoolean("has_rootfs", true), // Default to true if missing for legacy compatibility json.optInt("a", 0) ); diff --git a/controller/app/src/main/java/org/iiab/controller/sync/domain/SyncCredentialValidator.java b/controller/app/src/main/java/org/iiab/controller/sync/domain/SyncCredentialValidator.java new file mode 100644 index 0000000..29c0a96 --- /dev/null +++ b/controller/app/src/main/java/org/iiab/controller/sync/domain/SyncCredentialValidator.java @@ -0,0 +1,184 @@ +/* + * ============================================================================ + * Name : SyncCredentialValidator.java + * Author : IIAB Project + * Copyright : Copyright (c) 2026 IIAB Project + * Description : Domain rule: what counts as a *safe* peer-to-peer sync + * credential. Closes tech-debt item S1 (rsyncd.conf / rsync:// + * URL injection from QR-scanned credentials). + * ============================================================================ + */ +package org.iiab.controller.sync.domain; + +/** + * Pure (framework-free) validation of peer-to-peer sync credentials. + * + *

The sync handshake transports a host IP, port, username and password + * inside a QR code that the receiving device scans. Those values are + * untrusted input: they are later interpolated into an + * {@code rsyncd.conf} file (server side) and into a {@code rsync://user@host:port/...} + * URL (client side). Without validation, a crafted QR code could inject extra + * {@code rsyncd.conf} directives (e.g. a newline followed by a new module + * section) or break out of the URL — see tech-debt item S1. + * + *

This class is the single source of truth for "what is a valid credential". + * It lives in the domain layer: no {@code android.*}, no networking, no I/O, so + * it is fully unit-testable on a plain JVM and reusable by any other injection + * fix (e.g. S4/D2) that needs to validate or quote shell/config input. + * + *

The policy is intentionally strict and fail-closed. Legitimate credentials + * are produced by {@code SyncHandshakeHelper.generateSecurePassword()} + * (12 alphanumeric chars) and a fixed username, so a conservative character set + * rejects attacks without rejecting any real value. + */ +public final class SyncCredentialValidator { + + /** Max username length accepted in {@code auth users} / URL userinfo. */ + private static final int MAX_USERNAME_LEN = 64; + /** Max password length accepted in the rsync password file. */ + private static final int MAX_PASSWORD_LEN = 128; + /** Max host/IP length (DNS label limit is 253). */ + private static final int MAX_HOST_LEN = 253; + + private SyncCredentialValidator() { + // Static utility; not instantiable. + } + + /** Outcome of a validation, with a machine-readable reason on failure. */ + public static final class Result { + public final boolean valid; + /** Null when {@link #valid}; otherwise a short, log-safe reason. */ + public final String reason; + + private Result(boolean valid, String reason) { + this.valid = valid; + this.reason = reason; + } + + public static Result ok() { + return new Result(true, null); + } + + public static Result fail(String reason) { + return new Result(false, reason); + } + } + + /** + * Validate a full set of scanned sync credentials. Call this at the parse + * boundary (QR decode); reject the whole handshake if it returns invalid. + */ + public static Result validateCredentials(String ip, int port, String user, String pass) { + if (!isValidHost(ip)) { + return Result.fail("invalid host/ip"); + } + if (!isValidPort(port)) { + return Result.fail("invalid port"); + } + if (!isValidUsername(user)) { + return Result.fail("invalid username"); + } + if (!isValidPassword(pass)) { + return Result.fail("invalid password"); + } + return Result.ok(); + } + + /** + * A username safe for both the {@code rsync://USER@host} URL userinfo and + * the {@code auth users = USER} directive: a non-empty run of + * {@code [A-Za-z0-9_-]} starting with a letter, digit or underscore. + * Matches the fixed {@code iiab_peer} account. + */ + public static boolean isValidUsername(String user) { + if (user == null || user.isEmpty() || user.length() > MAX_USERNAME_LEN) { + return false; + } + for (int i = 0; i < user.length(); i++) { + char c = user.charAt(i); + boolean ok = (c >= 'A' && c <= 'Z') + || (c >= 'a' && c <= 'z') + || (c >= '0' && c <= '9') + || c == '_' + || (c == '-' && i > 0); // '-' allowed, but not as the first char + if (!ok) { + return false; + } + } + return true; + } + + /** + * A password safe to write as a single line in the rsync password / secrets + * file: printable ASCII only, no whitespace and no control characters (so it + * cannot add a second secrets line or a stray {@code rsyncd.conf} directive). + * The app's generated alphanumeric passwords satisfy this. + */ + public static boolean isValidPassword(String pass) { + if (pass == null || pass.isEmpty() || pass.length() > MAX_PASSWORD_LEN) { + return false; + } + for (int i = 0; i < pass.length(); i++) { + char c = pass.charAt(i); + // Printable ASCII excluding space (0x20) and DEL (0x7f). + if (c <= 0x20 || c >= 0x7f) { + return false; + } + } + return true; + } + + /** + * A host that is either a dotted IPv4 literal or a DNS hostname, using only + * {@code [A-Za-z0-9.-]}. Rejects anything containing {@code @ : / \\}, + * whitespace or control characters that could break out of the URL. + */ + public static boolean isValidHost(String host) { + if (host == null || host.isEmpty() || host.length() > MAX_HOST_LEN) { + return false; + } + for (int i = 0; i < host.length(); i++) { + char c = host.charAt(i); + boolean ok = (c >= 'A' && c <= 'Z') + || (c >= 'a' && c <= 'z') + || (c >= '0' && c <= '9') + || c == '.' + || c == '-'; + if (!ok) { + return false; + } + } + // Reject leading/trailing dot or dash and empty labels (".." / "a..b"). + if (host.charAt(0) == '.' || host.charAt(0) == '-' + || host.charAt(host.length() - 1) == '.' + || host.charAt(host.length() - 1) == '-' + || host.contains("..")) { + return false; + } + return true; + } + + /** TCP port in the valid 1..65535 range. */ + public static boolean isValidPort(int port) { + return port >= 1 && port <= 65535; + } + + /** + * True if {@code value} is safe to embed verbatim on a single line of an + * {@code rsyncd.conf} file: no control characters (notably CR/LF, which + * would let an attacker append new directives or module sections) and no + * NUL. Use this for app-controlled values such as the shared directory path. + */ + public static boolean isSafeConfigValue(String value) { + if (value == null) { + return false; + } + for (int i = 0; i < value.length(); i++) { + char c = value.charAt(i); + if (c == '\n' || c == '\r' || c == '\0' || c < 0x20 && c != '\t') { + return false; + } + } + return true; + } +} diff --git a/controller/app/src/main/res/values-es/strings.xml b/controller/app/src/main/res/values-es/strings.xml index 9b909ca..ee11369 100644 --- a/controller/app/src/main/res/values-es/strings.xml +++ b/controller/app/src/main/res/values-es/strings.xml @@ -499,4 +499,5 @@ App Arch: %1$d-bit Hardware compatible. Iniciando conexión... + Credenciales de sincronización inválidas o no seguras. Escanea un código QR de sincronización IIAB válido. \ No newline at end of file diff --git a/controller/app/src/main/res/values/strings.xml b/controller/app/src/main/res/values/strings.xml index 6cbe95d..bef9101 100644 --- a/controller/app/src/main/res/values/strings.xml +++ b/controller/app/src/main/res/values/strings.xml @@ -514,4 +514,5 @@ App Arch: %1$d-bit Hardware compatible. Starting connection... + Invalid or unsafe sync credentials. Please scan a valid IIAB sync QR code. \ No newline at end of file diff --git a/controller/app/src/test/java/org/iiab/controller/SyncHandshakeHelperTest.java b/controller/app/src/test/java/org/iiab/controller/SyncHandshakeHelperTest.java index b2ac4bd..56093de 100644 --- a/controller/app/src/test/java/org/iiab/controller/SyncHandshakeHelperTest.java +++ b/controller/app/src/test/java/org/iiab/controller/SyncHandshakeHelperTest.java @@ -79,4 +79,27 @@ public void parsePayload_defaultsHasRootfsTrueForLegacyPayload() { assertTrue(creds.hasRootfs); assertEquals(0, creds.archBits); } + // --- S1: malicious payloads are rejected at the parse boundary --- + + @Test + public void parsePayload_rejectsRsyncdConfInjectionInUsername() { + // A username carrying a newline + a new rsyncd.conf section. + String malicious = "{\"app\":\"iiab_sync\",\"ip\":\"10.0.0.1\",\"port\":8730," + + "\"user\":\"iiab\\n[evil]\\npath = /\",\"pass\":\"p\"}"; + assertNull(SyncHandshakeHelper.parsePayload(malicious)); + } + + @Test + public void parsePayload_rejectsUrlBreakoutInHost() { + String malicious = "{\"app\":\"iiab_sync\",\"ip\":\"10.0.0.1/evil\",\"port\":8730," + + "\"user\":\"iiab_peer\",\"pass\":\"p\"}"; + assertNull(SyncHandshakeHelper.parsePayload(malicious)); + } + + @Test + public void parsePayload_rejectsOutOfRangePort() { + String malicious = "{\"app\":\"iiab_sync\",\"ip\":\"10.0.0.1\",\"port\":70000," + + "\"user\":\"iiab_peer\",\"pass\":\"p\"}"; + assertNull(SyncHandshakeHelper.parsePayload(malicious)); + } } diff --git a/controller/app/src/test/java/org/iiab/controller/sync/domain/SyncCredentialValidatorTest.java b/controller/app/src/test/java/org/iiab/controller/sync/domain/SyncCredentialValidatorTest.java new file mode 100644 index 0000000..0cc5c9b --- /dev/null +++ b/controller/app/src/test/java/org/iiab/controller/sync/domain/SyncCredentialValidatorTest.java @@ -0,0 +1,137 @@ +package org.iiab.controller.sync.domain; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import org.iiab.controller.SyncHandshakeHelper; +import org.junit.Test; + +/** + * Unit tests for {@link SyncCredentialValidator} — the domain rule that closes + * tech-debt item S1 (rsyncd.conf / rsync:// URL injection from scanned QR + * credentials). Pure JVM, no Android dependencies. + */ +public class SyncCredentialValidatorTest { + + // --- happy path --- + + @Test + public void acceptsTypicalLanCredentials() { + assertTrue(SyncCredentialValidator + .validateCredentials("192.168.1.50", 8730, "iiab_peer", "s3cretPass").valid); + } + + @Test + public void acceptsGeneratedPassword() { + // The validator must never reject a password the app itself produces. + for (int i = 0; i < 200; i++) { + String pwd = SyncHandshakeHelper.generateSecurePassword(); + assertTrue("rejected a generated password: " + pwd, + SyncCredentialValidator.isValidPassword(pwd)); + } + } + + // --- username --- + + @Test + public void rejectsUsernameWithNewlineInjection() { + // The classic S1 attack: smuggle a new rsyncd.conf directive. + assertFalse(SyncCredentialValidator.isValidUsername("iiab\n[evil]\npath = /")); + } + + @Test + public void rejectsUsernameWithUrlMetacharacters() { + assertFalse(SyncCredentialValidator.isValidUsername("a@b")); + assertFalse(SyncCredentialValidator.isValidUsername("a:b")); + assertFalse(SyncCredentialValidator.isValidUsername("a/b")); + assertFalse(SyncCredentialValidator.isValidUsername("a b")); + } + + @Test + public void rejectsEmptyOrOverlongOrLeadingDashUsername() { + assertFalse(SyncCredentialValidator.isValidUsername("")); + assertFalse(SyncCredentialValidator.isValidUsername(null)); + assertFalse(SyncCredentialValidator.isValidUsername("-leadingdash")); + StringBuilder sb = new StringBuilder(); + for (int i = 0; i < 65; i++) sb.append('a'); + assertFalse(SyncCredentialValidator.isValidUsername(sb.toString())); + } + + @Test + public void acceptsPlainUsernames() { + assertTrue(SyncCredentialValidator.isValidUsername("iiab_peer")); + assertTrue(SyncCredentialValidator.isValidUsername("u")); + assertTrue(SyncCredentialValidator.isValidUsername("host-1_2")); + } + + // --- password --- + + @Test + public void rejectsPasswordWithControlChars() { + assertFalse(SyncCredentialValidator.isValidPassword("good\npart")); + assertFalse(SyncCredentialValidator.isValidPassword("good\rpart")); + assertFalse(SyncCredentialValidator.isValidPassword("has space")); + assertFalse(SyncCredentialValidator.isValidPassword("tab\tinside")); + assertFalse(SyncCredentialValidator.isValidPassword("")); + assertFalse(SyncCredentialValidator.isValidPassword(null)); + } + + // --- host --- + + @Test + public void rejectsHostWithInjectionOrMetacharacters() { + assertFalse(SyncCredentialValidator.isValidHost("10.0.0.1/extra")); + assertFalse(SyncCredentialValidator.isValidHost("10.0.0.1@evil")); + assertFalse(SyncCredentialValidator.isValidHost("10.0.0.1 8730")); + assertFalse(SyncCredentialValidator.isValidHost("host\nname")); + assertFalse(SyncCredentialValidator.isValidHost(".bad")); + assertFalse(SyncCredentialValidator.isValidHost("a..b")); + assertFalse(SyncCredentialValidator.isValidHost("")); + assertFalse(SyncCredentialValidator.isValidHost(null)); + } + + @Test + public void acceptsIpv4AndHostnames() { + assertTrue(SyncCredentialValidator.isValidHost("192.168.1.50")); + assertTrue(SyncCredentialValidator.isValidHost("iiab-host.local")); + } + + // --- port --- + + @Test + public void rejectsOutOfRangePorts() { + assertFalse(SyncCredentialValidator.isValidPort(0)); + assertFalse(SyncCredentialValidator.isValidPort(-1)); + assertFalse(SyncCredentialValidator.isValidPort(70000)); + assertTrue(SyncCredentialValidator.isValidPort(8730)); + assertTrue(SyncCredentialValidator.isValidPort(1)); + assertTrue(SyncCredentialValidator.isValidPort(65535)); + } + + // --- rsyncd.conf value safety (server side, e.g. shared path) --- + + @Test + public void rejectsConfigValueWithNewline() { + assertFalse(SyncCredentialValidator.isSafeConfigValue("/data\nread only = no")); + assertFalse(SyncCredentialValidator.isSafeConfigValue("/data\rx")); + assertFalse(SyncCredentialValidator.isSafeConfigValue(null)); + } + + @Test + public void acceptsNormalPath() { + assertTrue(SyncCredentialValidator.isSafeConfigValue( + "/data/user/0/org.iiab.controller/files/rootfs")); + } + + // --- aggregate --- + + @Test + public void validateCredentialsFailsOnAnyBadField() { + assertFalse(SyncCredentialValidator + .validateCredentials("10.0.0.1", 8730, "ok", "bad\npass").valid); + assertFalse(SyncCredentialValidator + .validateCredentials("10.0.0.1", 0, "ok", "ok").valid); + assertFalse(SyncCredentialValidator + .validateCredentials("bad host", 8730, "ok", "ok").valid); + } +} diff --git a/controller/docs/TECH_DEBT_PLAN.md b/controller/docs/TECH_DEBT_PLAN.md index 573aa79..28b82f7 100644 --- a/controller/docs/TECH_DEBT_PLAN.md +++ b/controller/docs/TECH_DEBT_PLAN.md @@ -30,7 +30,12 @@ _Last updated: 2026-06-17. Tracks remediation work against the findings below. I - Bug: the dashboard "device architecture" field reported the *app's* ABI (via `nativeLibraryDir`), so a 32-bit build on a 64-bit device wrongly showed 32-bit (we install the 32-bit app on 64-bit hardware to test the 32-bit path). - Fix: new layered slice — `domain` (`DeviceAbiProvider` port + `GetDeviceArchUseCase`, prefer-64-bit rule, pure JVM) and `data` (`BuildDeviceAbiProvider` reading device-level `Build.SUPPORTED_*_ABIS`). `DashboardFragment` now shows the real device arch; `getTermuxArch()` stays for app/content arch (modules, termux, debian). Unit test `GetDeviceArchUseCaseTest` covers the 32-bit-app-on-64-bit-device case. -**Phases 1–4: NOT STARTED.** Next: Phase 1 security — **D2**, **D6**, **S1**, **S3**, **M4**, **D12**. +**S1 — Sync credential injection (Phase 1 security): DONE** (PR `feat/phase1-security-sync-credential-validation`) +- Closes **S1**: QR-scanned sync credentials (host/port/user/pass) were interpolated into `rsyncd.conf` and the `rsync://` client URL unescaped, enabling config-directive and URL injection. +- New pure domain rule `org.iiab.controller.sync.domain.SyncCredentialValidator` (strict user/host charsets, port range, control-char-free password, `isSafeConfigValue` for config lines). Unit-tested (`SyncCredentialValidatorTest`) plus three new malicious-payload cases in `SyncHandshakeHelperTest`. +- Validation applied at the untrusted boundary (`SyncHandshakeHelper.parsePayload` -> `null` on invalid) and defensively in `RsyncManager` (server config + client URL paths), with a new `rsync_error_invalid_credentials` string (en + es). The validator is the reusable contract the remaining injection fixes (**S4**, **D2**) can build on. + +**Phase 1 — Security hardening: IN PROGRESS.** **S1** done (above); remaining Phase 1 targets: **D2**, **D6**, **S3**, **M4**, **D12**, **S4**. ## 1. Executive summary