Skip to content

Commit ec1660f

Browse files
Merge pull request #10 from appdevforall/fix/phase1-security-m4-s3
fix(controller): scope log broadcast (M4) + ADB key sign/verify-only (S3)
2 parents 882f32f + 969c78c commit ec1660f

3 files changed

Lines changed: 16 additions & 5 deletions

File tree

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,14 +51,16 @@ private IIABAdbManager(Context context) {
5151
Log.i(TAG, "Generating new RSA V3 key pair in AndroidKeyStore...");
5252
KeyPairGenerator kpg = KeyPairGenerator.getInstance(KeyProperties.KEY_ALGORITHM_RSA, "AndroidKeyStore");
5353

54+
// S3: the ADB identity key is only ever used to sign/verify the
55+
// connection handshake (no Cipher uses this alias), so scope it to
56+
// SIGN | VERIFY. The previous spec also granted ENCRYPT | DECRYPT
57+
// with non-randomized encryption padding, which is unused attack
58+
// surface and a crypto anti-pattern.
5459
KeyGenParameterSpec spec = new KeyGenParameterSpec.Builder(
5560
KEY_ALIAS,
56-
KeyProperties.PURPOSE_SIGN | KeyProperties.PURPOSE_VERIFY |
57-
KeyProperties.PURPOSE_ENCRYPT | KeyProperties.PURPOSE_DECRYPT)
61+
KeyProperties.PURPOSE_SIGN | KeyProperties.PURPOSE_VERIFY)
5862
.setDigests(KeyProperties.DIGEST_NONE, KeyProperties.DIGEST_SHA256, KeyProperties.DIGEST_SHA512)
5963
.setSignaturePaddings(KeyProperties.SIGNATURE_PADDING_RSA_PKCS1)
60-
.setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE, KeyProperties.ENCRYPTION_PADDING_RSA_PKCS1)
61-
.setRandomizedEncryptionRequired(false)
6264
.setKeySize(2048)
6365
.setCertificateSubject(new X500Principal("CN=" + this.deviceName))
6466
.setCertificateSerialNumber(BigInteger.ONE)

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,10 @@ private static void setRapidGrowthFlag(Context context, boolean enabled) {
225225
private static void broadcastLog(Context context, String message) {
226226
Intent intent = new Intent(ACTION_LOG_MESSAGE);
227227
intent.putExtra(EXTRA_MESSAGE, message);
228+
// M4: scope to our own package. An implicit broadcast would leak the log
229+
// text to any installed app, and broadcasting to dynamic receivers without
230+
// a package is rejected on API 34+ (would crash). Matches the other senders.
231+
intent.setPackage(context.getPackageName());
228232
context.sendBroadcast(intent);
229233
}
230234
}

‎controller/docs/TECH_DEBT_PLAN.md‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,12 @@ _Last updated: 2026-06-17. Tracks remediation work against the findings below. I
3535
- 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`.
3636
- 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.
3737

38-
**Phase 1 — Security hardening: IN PROGRESS.** **S1** done (above); remaining Phase 1 targets: **D2**, **D6**, **S3**, **M4**, **D12**, **S4**.
38+
**M4 + S3 — Phase 1 security one-liners: DONE** (PR `fix/phase1-security-m4-s3`)
39+
- **M4** (`IIABWatchdog.broadcastLog`): the `ACTION_LOG_MESSAGE` broadcast carrying log text was implicit (no `setPackage`), leaking it to any installed app and crashing on API 34+. Now scoped to our own package, matching every other broadcast in the app.
40+
- **S3** (`IIABAdbManager` key spec): the ADB identity key was created with `ENCRYPT | DECRYPT` + non-randomized encryption padding on top of sign/verify. Verified no `Cipher` uses this alias, so the spec is now `SIGN | VERIFY` only (encryption paddings / `setRandomizedEncryptionRequired(false)` removed). Alias kept at `v3` (non-disruptive: hardens new key generation without forcing existing devices to re-pair ADB).
41+
- No new unit tests: both are framework-bound legacy line-fixes with no extractable pure logic; verified by inspection + compile + CI lint.
42+
43+
**Phase 1 — Security hardening: IN PROGRESS.** Done so far: **S1** (PR #9), **M4**, **S3**. Remaining: **D2**, **D6**, **D12**, **S4**, **D11**.
3944

4045
## 1. Executive summary
4146

0 commit comments

Comments
 (0)