Skip to content

Commit 882f32f

Browse files
Merge pull request #9 from appdevforall/feat/phase1-security-sync-credential-validation
feat(controller): validate sync credentials to close rsync injection (S1)
2 parents 2594dd5 + bf8b621 commit 882f32f

9 files changed

Lines changed: 416 additions & 5 deletions

File tree

‎CLAUDE.md‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,21 @@ app's ABI, so a 32-bit app on a 64-bit phone reported 32-bit).
144144
use case; `getTermuxArch()` (app/content ABI) is unchanged for modules, termux
145145
and debian arch.
146146

147+
**Slice (DONE) — sync credential validation (`org.iiab.controller.sync.domain`)**
148+
First Phase-1 *security* slice (refactor-by-feature while closing tech-debt **S1**).
149+
The peer-to-peer sync handshake carries host/port/user/pass in a scanned QR code;
150+
those values were interpolated into `rsyncd.conf` and a `rsync://` URL with no
151+
validation, allowing config-directive/URL injection.
152+
153+
- `domain/` — `SyncCredentialValidator` (pure JVM, no Android): the single rule
154+
for "what is a safe sync credential" — strict charsets for user/host, range
155+
check for port, control-char-free password, and an `isSafeConfigValue` guard
156+
for `rsyncd.conf` values. Unit-tested (`sync/domain/SyncCredentialValidatorTest`).
157+
- **Legacy seam:** `SyncHandshakeHelper.parsePayload()` now validates at the QR
158+
parse boundary (returns `null` -> existing "invalid QR" toast), and
159+
`RsyncManager` re-checks defensively before writing the daemon config / building
160+
the client URL. Reusable by the remaining injection fixes (S4, D2) as they migrate.
161+
147162
**Legacy (NOT yet layered)** — most of `org.iiab.controller` is still flat:
148163
god classes `MainActivity` and `DeployFragment` (~2.7k LOC), shared mutable
149164
state on public/static fields, hand-rolled `HttpURLConnection` calls duplicated

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
import android.os.Looper;
1515
import android.util.Log;
1616

17+
import org.iiab.controller.sync.domain.SyncCredentialValidator;
18+
1719
import java.io.BufferedReader;
1820
import java.io.File;
1921
import java.io.FileOutputStream;
@@ -42,6 +44,18 @@ public boolean startServer(Context context, int port, String user, String pass,
4244
stop();
4345
isCancelled = false;
4446

47+
// S1 (defence in depth): never write attacker-controllable text into
48+
// rsyncd.conf without validation. user/pass are app-generated and
49+
// dirToShare is app-controlled, but a stray CR/LF here would let new
50+
// config directives or module sections be injected.
51+
if (!SyncCredentialValidator.isValidUsername(user)
52+
|| !SyncCredentialValidator.isValidPassword(pass)
53+
|| !SyncCredentialValidator.isValidPort(port)
54+
|| !SyncCredentialValidator.isSafeConfigValue(dirToShare)) {
55+
Log.e(TAG, "Refusing to start rsync daemon: invalid credentials or share path");
56+
return false;
57+
}
58+
4559
try {
4660
File nativeLibDir = new File(context.getApplicationInfo().nativeLibraryDir);
4761
File rsyncBin = new File(nativeLibDir, "librsync.so");
@@ -122,6 +136,13 @@ public void startClient(Context context, String hostIp, int port, String user, S
122136
throw new Exception(context.getString(R.string.rsync_error_binary_missing));
123137
}
124138

139+
// S1: reject credentials that could break out of the rsync:// URL.
140+
if (!SyncCredentialValidator.validateCredentials(hostIp, port, user, pass).valid) {
141+
mainHandler.post(() -> listener.onError(
142+
context.getString(R.string.rsync_error_invalid_credentials)));
143+
return;
144+
}
145+
125146
File passFile = new File(context.getCacheDir(), "rsync_client.pass");
126147
writeTextToFile(passFile, pass);
127148

@@ -220,6 +241,13 @@ public void calculateTransferPlan(Context context, String hostIp, int port, Stri
220241
if (!rsyncBin.exists())
221242
throw new Exception(context.getString(R.string.rsync_error_binary_missing));
222243

244+
// S1: reject credentials that could break out of the rsync:// URL.
245+
if (!SyncCredentialValidator.validateCredentials(hostIp, port, user, pass).valid) {
246+
mainHandler.post(() -> listener.onError(
247+
context.getString(R.string.rsync_error_invalid_credentials)));
248+
return;
249+
}
250+
223251
File passFile = new File(context.getCacheDir(), "rsync_client.pass");
224252
writeTextToFile(passFile, pass);
225253
passFile.setExecutable(false, false);

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

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414

1515
import org.json.JSONObject;
1616

17+
import org.iiab.controller.sync.domain.SyncCredentialValidator;
18+
1719
import java.security.SecureRandom;
1820

1921
import com.google.zxing.BarcodeFormat;
@@ -76,11 +78,26 @@ public static SyncCredentials parsePayload(String scannedJson) {
7678
Log.w(TAG, "Scanned QR is not an IIAB Sync code.");
7779
return null;
7880
}
81+
String ip = json.getString("ip");
82+
int port = json.getInt("port");
83+
String user = json.getString("user");
84+
String pass = json.getString("pass");
85+
86+
// S1: the QR payload is untrusted input that is later interpolated
87+
// into rsyncd.conf and a rsync:// URL. Reject anything that could
88+
// inject config directives or break out of the URL.
89+
SyncCredentialValidator.Result check =
90+
SyncCredentialValidator.validateCredentials(ip, port, user, pass);
91+
if (!check.valid) {
92+
Log.w(TAG, "Rejecting scanned credentials: " + check.reason);
93+
return null;
94+
}
95+
7996
return new SyncCredentials(
80-
json.getString("ip"),
81-
json.getInt("port"),
82-
json.getString("user"),
83-
json.getString("pass"),
97+
ip,
98+
port,
99+
user,
100+
pass,
84101
json.optBoolean("has_rootfs", true), // Default to true if missing for legacy compatibility
85102
json.optInt("a", 0)
86103
);
Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,184 @@
1+
/*
2+
* ============================================================================
3+
* Name : SyncCredentialValidator.java
4+
* Author : IIAB Project
5+
* Copyright : Copyright (c) 2026 IIAB Project
6+
* Description : Domain rule: what counts as a *safe* peer-to-peer sync
7+
* credential. Closes tech-debt item S1 (rsyncd.conf / rsync://
8+
* URL injection from QR-scanned credentials).
9+
* ============================================================================
10+
*/
11+
package org.iiab.controller.sync.domain;
12+
13+
/**
14+
* Pure (framework-free) validation of peer-to-peer sync credentials.
15+
*
16+
* <p>The sync handshake transports a host IP, port, username and password
17+
* inside a QR code that the receiving device scans. Those values are
18+
* <strong>untrusted input</strong>: they are later interpolated into an
19+
* {@code rsyncd.conf} file (server side) and into a {@code rsync://user@host:port/...}
20+
* URL (client side). Without validation, a crafted QR code could inject extra
21+
* {@code rsyncd.conf} directives (e.g. a newline followed by a new module
22+
* section) or break out of the URL — see tech-debt item <b>S1</b>.
23+
*
24+
* <p>This class is the single source of truth for "what is a valid credential".
25+
* It lives in the domain layer: no {@code android.*}, no networking, no I/O, so
26+
* it is fully unit-testable on a plain JVM and reusable by any other injection
27+
* fix (e.g. S4/D2) that needs to validate or quote shell/config input.
28+
*
29+
* <p>The policy is intentionally strict and fail-closed. Legitimate credentials
30+
* are produced by {@code SyncHandshakeHelper.generateSecurePassword()}
31+
* (12 alphanumeric chars) and a fixed username, so a conservative character set
32+
* rejects attacks without rejecting any real value.
33+
*/
34+
public final class SyncCredentialValidator {
35+
36+
/** Max username length accepted in {@code auth users} / URL userinfo. */
37+
private static final int MAX_USERNAME_LEN = 64;
38+
/** Max password length accepted in the rsync password file. */
39+
private static final int MAX_PASSWORD_LEN = 128;
40+
/** Max host/IP length (DNS label limit is 253). */
41+
private static final int MAX_HOST_LEN = 253;
42+
43+
private SyncCredentialValidator() {
44+
// Static utility; not instantiable.
45+
}
46+
47+
/** Outcome of a validation, with a machine-readable reason on failure. */
48+
public static final class Result {
49+
public final boolean valid;
50+
/** Null when {@link #valid}; otherwise a short, log-safe reason. */
51+
public final String reason;
52+
53+
private Result(boolean valid, String reason) {
54+
this.valid = valid;
55+
this.reason = reason;
56+
}
57+
58+
public static Result ok() {
59+
return new Result(true, null);
60+
}
61+
62+
public static Result fail(String reason) {
63+
return new Result(false, reason);
64+
}
65+
}
66+
67+
/**
68+
* Validate a full set of scanned sync credentials. Call this at the parse
69+
* boundary (QR decode); reject the whole handshake if it returns invalid.
70+
*/
71+
public static Result validateCredentials(String ip, int port, String user, String pass) {
72+
if (!isValidHost(ip)) {
73+
return Result.fail("invalid host/ip");
74+
}
75+
if (!isValidPort(port)) {
76+
return Result.fail("invalid port");
77+
}
78+
if (!isValidUsername(user)) {
79+
return Result.fail("invalid username");
80+
}
81+
if (!isValidPassword(pass)) {
82+
return Result.fail("invalid password");
83+
}
84+
return Result.ok();
85+
}
86+
87+
/**
88+
* A username safe for both the {@code rsync://USER@host} URL userinfo and
89+
* the {@code auth users = USER} directive: a non-empty run of
90+
* {@code [A-Za-z0-9_-]} starting with a letter, digit or underscore.
91+
* Matches the fixed {@code iiab_peer} account.
92+
*/
93+
public static boolean isValidUsername(String user) {
94+
if (user == null || user.isEmpty() || user.length() > MAX_USERNAME_LEN) {
95+
return false;
96+
}
97+
for (int i = 0; i < user.length(); i++) {
98+
char c = user.charAt(i);
99+
boolean ok = (c >= 'A' && c <= 'Z')
100+
|| (c >= 'a' && c <= 'z')
101+
|| (c >= '0' && c <= '9')
102+
|| c == '_'
103+
|| (c == '-' && i > 0); // '-' allowed, but not as the first char
104+
if (!ok) {
105+
return false;
106+
}
107+
}
108+
return true;
109+
}
110+
111+
/**
112+
* A password safe to write as a single line in the rsync password / secrets
113+
* file: printable ASCII only, no whitespace and no control characters (so it
114+
* cannot add a second secrets line or a stray {@code rsyncd.conf} directive).
115+
* The app's generated alphanumeric passwords satisfy this.
116+
*/
117+
public static boolean isValidPassword(String pass) {
118+
if (pass == null || pass.isEmpty() || pass.length() > MAX_PASSWORD_LEN) {
119+
return false;
120+
}
121+
for (int i = 0; i < pass.length(); i++) {
122+
char c = pass.charAt(i);
123+
// Printable ASCII excluding space (0x20) and DEL (0x7f).
124+
if (c <= 0x20 || c >= 0x7f) {
125+
return false;
126+
}
127+
}
128+
return true;
129+
}
130+
131+
/**
132+
* A host that is either a dotted IPv4 literal or a DNS hostname, using only
133+
* {@code [A-Za-z0-9.-]}. Rejects anything containing {@code @ : / \\},
134+
* whitespace or control characters that could break out of the URL.
135+
*/
136+
public static boolean isValidHost(String host) {
137+
if (host == null || host.isEmpty() || host.length() > MAX_HOST_LEN) {
138+
return false;
139+
}
140+
for (int i = 0; i < host.length(); i++) {
141+
char c = host.charAt(i);
142+
boolean ok = (c >= 'A' && c <= 'Z')
143+
|| (c >= 'a' && c <= 'z')
144+
|| (c >= '0' && c <= '9')
145+
|| c == '.'
146+
|| c == '-';
147+
if (!ok) {
148+
return false;
149+
}
150+
}
151+
// Reject leading/trailing dot or dash and empty labels (".." / "a..b").
152+
if (host.charAt(0) == '.' || host.charAt(0) == '-'
153+
|| host.charAt(host.length() - 1) == '.'
154+
|| host.charAt(host.length() - 1) == '-'
155+
|| host.contains("..")) {
156+
return false;
157+
}
158+
return true;
159+
}
160+
161+
/** TCP port in the valid 1..65535 range. */
162+
public static boolean isValidPort(int port) {
163+
return port >= 1 && port <= 65535;
164+
}
165+
166+
/**
167+
* True if {@code value} is safe to embed verbatim on a single line of an
168+
* {@code rsyncd.conf} file: no control characters (notably CR/LF, which
169+
* would let an attacker append new directives or module sections) and no
170+
* NUL. Use this for app-controlled values such as the shared directory path.
171+
*/
172+
public static boolean isSafeConfigValue(String value) {
173+
if (value == null) {
174+
return false;
175+
}
176+
for (int i = 0; i < value.length(); i++) {
177+
char c = value.charAt(i);
178+
if (c == '\n' || c == '\r' || c == '\0' || c < 0x20 && c != '\t') {
179+
return false;
180+
}
181+
}
182+
return true;
183+
}
184+
}

‎controller/app/src/main/res/values-es/strings.xml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -499,4 +499,5 @@
499499
<string name="sync_app_arch_label">App Arch: %1$d-bit</string>
500500
<string name="sync_msg_arch_compatible">Hardware compatible. Iniciando conexión...</string>
501501

502+
<string name="rsync_error_invalid_credentials">Credenciales de sincronización inválidas o no seguras. Escanea un código QR de sincronización IIAB válido.</string>
502503
</resources>

‎controller/app/src/main/res/values/strings.xml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -514,4 +514,5 @@
514514
<string name="sync_app_arch_label">App Arch: %1$d-bit</string>
515515
<string name="sync_msg_arch_compatible">Hardware compatible. Starting connection...</string>
516516

517+
<string name="rsync_error_invalid_credentials">Invalid or unsafe sync credentials. Please scan a valid IIAB sync QR code.</string>
517518
</resources>

‎controller/app/src/test/java/org/iiab/controller/SyncHandshakeHelperTest.java‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,4 +79,27 @@ public void parsePayload_defaultsHasRootfsTrueForLegacyPayload() {
7979
assertTrue(creds.hasRootfs);
8080
assertEquals(0, creds.archBits);
8181
}
82+
// --- S1: malicious payloads are rejected at the parse boundary ---
83+
84+
@Test
85+
public void parsePayload_rejectsRsyncdConfInjectionInUsername() {
86+
// A username carrying a newline + a new rsyncd.conf section.
87+
String malicious = "{\"app\":\"iiab_sync\",\"ip\":\"10.0.0.1\",\"port\":8730,"
88+
+ "\"user\":\"iiab\\n[evil]\\npath = /\",\"pass\":\"p\"}";
89+
assertNull(SyncHandshakeHelper.parsePayload(malicious));
90+
}
91+
92+
@Test
93+
public void parsePayload_rejectsUrlBreakoutInHost() {
94+
String malicious = "{\"app\":\"iiab_sync\",\"ip\":\"10.0.0.1/evil\",\"port\":8730,"
95+
+ "\"user\":\"iiab_peer\",\"pass\":\"p\"}";
96+
assertNull(SyncHandshakeHelper.parsePayload(malicious));
97+
}
98+
99+
@Test
100+
public void parsePayload_rejectsOutOfRangePort() {
101+
String malicious = "{\"app\":\"iiab_sync\",\"ip\":\"10.0.0.1\",\"port\":70000,"
102+
+ "\"user\":\"iiab_peer\",\"pass\":\"p\"}";
103+
assertNull(SyncHandshakeHelper.parsePayload(malicious));
104+
}
82105
}

0 commit comments

Comments
 (0)