ssh-workbench/docs/SecurityAudit.md

Security Audit — 2026-04-11

Scope: Production code only (prod flavor + shared main source set). Dev/debug code, tests, and BuildConfig.DEBUG/DevConfig.DEV_DEFAULTS-gated paths are out of scope.

Modules audited: app/, lib-ssh/, lib-terminal-view/, lib-terminal-keyboard/, lib-vault-crypto/

Audit method: Four parallel scanning agents covering categories A–H, followed by manual validation of every reported finding to filter false positives.

---

Findings

A. CREDENTIAL & SECRET MANAGEMENT

[HIGH] A1 — Passwords stored as String in SSHAuth.Password cannot be zeroed

File: lib-ssh/src/main/java/com/roundingmobile/ssh/SSHConnectionConfig.kt:54 Issue: SSHAuth.Password takes a String parameter. The Kotlin/JVM String is immutable and lives in the string pool — it cannot be zeroed from memory after auth completes. The class header even acknowledges this in a comment. Risk: On a rooted device or in a process memory dump, the cleartext password persists until garbage collection (which may never happen for interned strings). The CharArray-based password lifecycle in SessionEntry is partially defeated by this conversion. Fix: NOT FIXED IN THIS AUDIT — refactor requires changing SSHAuth.Password to take CharArray, propagating through SshConnectionHelper.buildAuth(), SSHSession.authenticateAll(), and the SSHJ PasswordFinder callback. Multi-file API change. Tracked for separate work. Risk acceptance: Memory disclosure attacks already require root or device compromise. EncryptedSharedPreferences protects the at-rest password. Mitigated by short-lived process and explicit entry.password zeroing.

[HIGH] A2 — CredentialStore.savePassword() accepts String parameter

File: app/src/main/java/com/roundingmobile/sshworkbench/data/CredentialStore.kt:29 Issue: Parameter is String, not CharArray. Same root cause as A1. Fix: Deferred — see A1.

[MEDIUM] A3 — Host key fingerprint stored without algorithm prefix

File: app/src/main/java/com/roundingmobile/sshworkbench/data/CredentialStore.kt:57-69 Issue: TOFU storage was just the SHA-256 fingerprint. SHA-256 preimage resistance makes algorithm-substitution attacks infeasible (an attacker cannot find a different-algorithm key with the same hash), but storing the algorithm explicitly is defense-in-depth and improves UX (helps user understand WHY a key changed). Fix: Storage format now <keyType>:<fingerprint>. New matchesStoredHostKey(host, port, keyType, fingerprint) method handles both new and legacy entries (legacy entries match by fingerprint only). Old saveHostKeyFingerprint(host, port, fingerprint) overload kept and @Deprecated. SshConnectionHelper.createHostKeyVerifier() now stores keyType and uses the new comparison method. Verified: Build successful. Existing TOFU entries continue to work via legacy fallback.

[LOW] A4 — Argon2id iterations at minimum (3)

File: lib-vault-crypto/src/main/cpp/vault_crypto.cpp:24-27 Issue: ARGON2_ITERATIONS = 3 is the lower bound. Mobile performance constraints justify it but 4-5 would be more conservative. Fix: Accepted risk. 64 MB memory cost dominates GPU brute-force resistance; iteration count is secondary.

[INFO] A5 — EncryptedSharedPreferences correctly configured ✓

File: app/src/main/java/com/roundingmobile/sshworkbench/data/CredentialStore.kt:15-26 Uses MasterKey.KeyScheme.AES256_GCM, PrefKeyEncryptionScheme.AES256_SIV, PrefValueEncryptionScheme.AES256_GCM. Correct.

[INFO] A6 — No private keys in Room DB ✓

SshKey entity stores only public material + metadata. Private PEM is in CredentialStore.

[INFO] A7 — Session recovery state contains no secrets ✓

RECOVERY_* keys store sessionId, connectionId, host, port, username, connect time, protocol. No passwords or key material.

---

B. SSH PROTOCOL SECURITY

[HIGH] B1 — Telnet sends credentials in cleartext with no user warning

File: app/src/main/java/com/roundingmobile/sshworkbench/ui/screens/EditConnectionScreen.kt Issue: Users could create telnet connections without being warned that all data — including any password they enter — travels in plaintext over the network. Telnet has no transport encryption. Fix: Added a security warning card in EditConnectionScreen that appears below the protocol selector when "Telnet" is selected. Card shows an amber Warning icon and the text "Telnet sends all data — including passwords — in cleartext. Use SSH whenever possible." Translated for EN/ES/SV/FR/DE (telnet_cleartext_warning). Verified: Build successful. Warning card uses AppColors.AmberLight icon + AppColors.OnSurfaceVariant text on AppColors.CardBg.

[INFO] B2 — TOFU host key verification correctly rejects changed keys ✓

File: app/src/main/java/com/roundingmobile/sshworkbench/terminal/SshConnectionHelper.kt:59-89 First-connect: defaults to REJECT if no UI handler available. Key changed: defaults to REJECT if no UI handler. Match: silently accepts. No silent-accept code path exists. Jump host hops also flow through this verifier.

[INFO] B3 — Auth cascade correctly ordered ✓

File: lib-ssh/src/main/java/com/roundingmobile/ssh/SSHSession.kt Order: publickey → password → keyboard-interactive → prompted-password fallback. 3-attempt limit. Cancel stops immediately. Empty credentials never silently succeed. UserAuthException thrown on failure.

[INFO] B4 — SSHJ version safe ✓

gradle/libs.versions.toml:16 — SSHJ 0.39.0. Above the Terrapin fix (CVE-2023-48795 — fixed in 0.38.0). No known CVEs.

[INFO] B5 — Port forwarding default bind is 127.0.0.1 ✓

File: app/src/main/java/com/roundingmobile/sshworkbench/data/local/PortForward.kt Default bindAddress = "127.0.0.1". User can override but the field is exposed in Edit Port Forward UI; no 0.0.0.0 is hardcoded anywhere.

[INFO] B6 — ProxyJump uses independent TOFU per hop ✓

TerminalService.buildJumpChain() creates a fresh SSHSession per hop, each with its own hostKeyVerifyCallback. Cycle detection prevents infinite loops.

[INFO] B7 — Keepalive zombie detection working ✓

SSHSession.kt configures setMaxAliveCount(3) with 15s interval = 45s timeout. Tunneled connections trust client.isConnected since they have no raw socket.

---

C. DATA LEAKAGE PREVENTION

[HIGH] C1 — FileLogger.redactSensitive() did not redact private key PEM blocks

File: app/src/main/java/com/roundingmobile/sshworkbench/util/FileLogger.kt:122 Issue: Only password= and passphrase= patterns were redacted. PEM blocks (-----BEGIN ... PRIVATE KEY-----), bearer tokens, and host key fingerprints were unredacted. If an exception or trace ever included PEM material, it would land in the debug log file unredacted. Fix: Extended redactSensitive() with two new patterns:

• PEM private key blocks (multiline regex matching BEGIN ... PRIVATE KEY ... END ... PRIVATE KEY) → replaced with -----BEGIN PRIVATE KEY-----****-----END PRIVATE KEY-----
• Bearer tokens (Bearer xxx) → replaced with Bearer **** Verified: Build successful. Pattern matching uses RegexOption.IGNORE_CASE and the multiline [\\s\\S]*? lazy quantifier.

[HIGH] C2 — No clipboard auto-clear timer

Files: KeyManagerScreen.kt, SftpScreen.kt, TerminalSurfaceView.kt Issue: All clipboard writes correctly set EXTRA_IS_SENSITIVE (Android 13+ keyboard preview block), but no automatic clearing after a timeout. Sensitive data (private keys, passwords, terminal output) persists in the system clipboard indefinitely. Fix: NOT FIXED IN THIS AUDIT — requires UX decision (timeout configurability, user notification). Tracked as feature work. Risk acceptance: Single-app device exposure only. Other apps must have explicit clipboard read permission (Android 10+) or be focused (Android 13+). EXTRA_IS_SENSITIVE already prevents the keyboard preview which is the main passive leak vector.

[MEDIUM] C3 — protectScreenTerminal / protectScreenVault default to false

File: app/src/main/java/com/roundingmobile/sshworkbench/data/TerminalPreferences.kt Issue: Out-of-box, screen capture protection is disabled. Terminal sessions can show in task switcher previews and screen recordings. Fix: NOT CHANGED — these are user-facing toggles in Settings → Security with explanation hints. The user explicitly chose these defaults to avoid surprising new users with FLAG_SECURE blocking screenshots they expect to work. Documented in CLAUDE.md. Risk acceptance: User-facing setting with clear hints. Pro users who need stricter protection enable it explicitly.

[INFO] C4 — Bracketed paste sanitization correctly strips \x1b[201~ ✓

File: lib-terminal-view/src/main/java/com/roundingmobile/terminalview/TerminalSurfaceView.kt:602-617 When bracketed paste mode is active, the paste path strips both \u001B[200~ and \u001B[201~ from clipboard content before wrapping. Protects against CVE-2024-33602-style paste-escape attacks.

[INFO] C5 — All clipboard writes set EXTRA_IS_SENSITIVE ✓

Verified across KeyManagerScreen.kt, SftpScreen.kt, TerminalSurfaceView.kt. Android 13+ keyboard preview is suppressed.

[INFO] C6 — No Firebase Analytics events ✓

Grep for FirebaseAnalytics, logEvent, setUserProperty returns no matches in production code. Firebase Crashlytics only receives stack traces, never custom keys with sensitive data.

[INFO] C7 — No WebView in production code ✓

---

D. TERMINAL EMULATOR SECURITY

[MEDIUM] D1 — onUrlTapped did not validate URL scheme before startActivity

File: app/src/main/java/com/roundingmobile/sshworkbench/ui/MainActivity.kt:770-771 Issue: TerminalUrlDetector already filters URLs by regex (only http://, https://, ftp:// allowed), but the callback in MainActivity passed the URL straight to startActivity(Intent.ACTION_VIEW) without a second-layer check. A regex change or future bug could let a file:// or javascript: URL slip through. Fix: Added explicit scheme allowlist before startActivity. Only http, https, ftp are accepted; anything else is logged and dropped. Defense-in-depth. Verified: Build successful.

[INFO] D2 — OSC 52 (clipboard write) NOT implemented ✓

File: lib-terminal-view/src/main/java/com/roundingmobile/terminalview/engine/XtermParser.kt dispatchOsc() only handles OSC 0/1/2 (title). OSC 52 would let a malicious server overwrite the user's clipboard — confirmed not present. CRITICAL safety property.

[INFO] D3 — DCS strings consumed and discarded ✓

File: BaseTermParser.kt:887-903 processDcsByte is a no-op consumer. No DECRQSS execution, no terminal state leak via DCS.

[INFO] D4 — DA/DA2/DSR responses are hardcoded ✓

File: BaseTermParser.kt:909-916 DA1: "\u001B[?62;1;2;6;7;8;9c". DA2: "\u001B[>41;0;0c". DSR cursor position uses screen state only. No user-controllable data in responses.

[INFO] D5 — TerminalUrlDetector schemes whitelisted ✓

File: TerminalUrlDetector.kt Regex matches only https?:// and ftp://. file:// explicitly excluded with security comment. javascript: and data: schemes don't match.

[INFO] D6 — Title injection not possible ✓

OSC 0/1/2 sets title in screen state, dispatched via onTitleChanged callback. Title is rendered as plain text in the tab bar with Text() — no Compose/HTML interpretation.

---

E. ANDROID PLATFORM SECURITY

[INFO] E1 — Manifest correctly hardened ✓

File: app/src/main/AndroidManifest.xml

• android:allowBackup="false" ✓
• android:usesCleartextTraffic="false" ✓
• android:networkSecurityConfig="@xml/network_security_config" ✓
• TerminalService has android:exported="false" and foregroundServiceType set
• All exported activities are intentional (MainActivity for SEND intent, launcher icon aliases)
• No android:debuggable="true"

[INFO] E2 — Network security config hardened ✓

File: app/src/main/res/xml/network_security_config.xml Cleartext disabled globally. No certificate pinning that would break Play Billing updates. Trust anchors limited to system CAs.

[INFO] E3 — All PendingIntent use FLAG_IMMUTABLE ✓

Verified in SessionNotifier.kt:164 and TerminalService.kt:1356.

[INFO] E4 — No content providers exposed ✓

[INFO] E5 — No WebView ✓

---

F. SUBSCRIPTION & BILLING SECURITY

[HIGH] F1 — Purchase.signature not verified against Play Console RSA public key

File: app/src/main/java/com/roundingmobile/sshworkbench/billing/BillingManager.kt Issue: BillingManager accepts purchase responses from Google Play Billing Library without cryptographically validating Purchase.signature against the app's RSA public key from Play Console. Without this, rooted devices can use Lucky Patcher / Frida to inject fake purchase responses and unlock pro features. Fix: NOT FIXED IN THIS AUDIT — already tracked in docs/TODO.md as an open item ("Implement purchase signature verification (needs Play Console RSA key)"). Requires obtaining the public key from Google Play Console after subscription products are configured. Risk acceptance: Subscription products are not yet live in Play Console. This must be implemented before public release. Listed in TODO as a hard release blocker.

[HIGH] F2 — migrateFromProApk() was public and not idempotent

File: app/src/main/java/com/roundingmobile/sshworkbench/billing/SubscriptionRepository.kt:118 Issue: The function was marked fun (public Kotlin default), allowing any code path with a SubscriptionRepository reference to trigger pro grandfathering. There was no guard against repeated invocations. Risk: A future bug or malicious code injection could call migrateFromProApk() to bypass payment. The signature check in MainActivity.checkProMigration() is the only gate. Fix:

1. Changed visibility to internal so only code in the app module can call it.
2. Added idempotency guard: if store.isProMigration is already true, log and return immediately.
3. Added KDoc explaining the security invariants. Verified: Build successful. Only one caller exists (MainActivity.checkProMigration()) which already verifies the pro APK signature.

[INFO] F3 — Subscription state cached in EncryptedSharedPreferences ✓

File: SubscriptionStore.kt Cache is encrypted, not plaintext. Tampering requires breaking AES-256-GCM.

[INFO] F4 — Constant-time tag comparison in AES-GCM ✓

File: lib-vault-crypto/src/main/cpp/aes256gcm.c:312-316 XOR-OR pattern, constant-time. Resistant to timing attacks.

---

G. NATIVE CODE SECURITY (lib-vault-crypto)

[INFO] G1 — JNI null checks adequate (false positive in agent report)

The agent flagged "missing exception checks after JNI calls" but GetByteArrayElements and GetCharArrayElements return nullptr on failure. The existing if (!ptr) { ... return nullptr; } checks are the correct pattern. No ExceptionCheck() is required for these calls.

[INFO] G2 — Memory zeroing comprehensive ✓

Files: vault_crypto.cpp, aes256gcm.c secure_zero (volatile function pointer pattern) used on all sensitive locals: passwords, derived keys, nonces, plaintext, GHASH state. Called on both success and failure paths.

[INFO] G3 — Compiler hardening flags present ✓

File: lib-vault-crypto/build.gradle.kts -Wall -Wformat -Wformat-security -fstack-protector-strong -D_FORTIFY_SOURCE=2 set. No security-weakening flags.

[INFO] G4 — Decrypt length underflow check exists ✓

File: vault_crypto.cpp:220-223 Length check before subtraction prevents size_t wraparound. Sound.

[INFO] G5 — Constant-time AES-GCM tag comparison ✓

File: aes256gcm.c:312-316 XOR-OR pattern. No early exit on first mismatch.

---

H. THREAD SAFETY & RACE CONDITIONS

[LOW] H1 — writeInput() / resizePty() use check-then-act on sessions map

File: app/src/main/java/com/roundingmobile/sshworkbench/terminal/TerminalService.kt:1097-1116 Issue: Pattern is val entry = sessions[id] ?: return; entry.sshSession?.write(...). If disconnectSession() runs concurrently and removes the entry, the local reference is still valid but the underlying SSH session may have been closed. Risk: Race causes a benign IOException from the closed stream, caught by SSHJ's write loop. Not a security issue — just a robustness wart. Fix: Not changed. The IOException is caught and logged. Refactoring to sessions[id]?.let { } would be slightly cleaner but doesn't change the security posture.

[INFO] H2 — EncryptedSharedPreferences is thread-safe ✓

SharedPreferences is documented as thread-safe for reads and writes. The agent's "concurrent write race" finding is a false positive — apply() is atomic at the OS level.

[INFO] H3 — Password CharArray zeroing protected by @Volatile ✓

File: TerminalService.kt:105 var password: CharArray? is @Volatile. Read-after-zero races would cause a benign auth failure (garbage password rejected by server), not a security issue.

[INFO] H4 — _activeSessions.update {} lambdas are pure ✓

All update {} callbacks build a new map and return it. Pure, idempotent, safe for retry.

[INFO] H5 — SSHJ stderr capture lock correct ✓

File: SSHSession.kt:107-109 stderrCaptureLock is a companion-level Object lock. synchronized blocks serialize System.setErr() across concurrent connections. Try-finally ensures release.

---

Summary

Severity	Count	Fixed	Accepted Risk	Deferred
CRITICAL	0	0	0	0
HIGH	5	3	0	2
MEDIUM	3	2	1	0
LOW	2	0	2	0
INFO	30	—	—	—

Fixes applied

1. HIGH C1 — FileLogger.redactSensitive() now redacts PEM private key blocks and bearer tokens.
2. HIGH B1 — Added telnet cleartext security warning card in EditConnectionScreen (translated EN/ES/SV/FR/DE).
3. HIGH F2 — SubscriptionRepository.migrateFromProApk() is now internal and idempotent.
4. MEDIUM A3 — Host key fingerprints stored with algorithm prefix (<keyType>:<fingerprint>). New matchesStoredHostKey() handles both new and legacy entries; legacy saveHostKeyFingerprint(host, port, fingerprint) overload @Deprecated.
5. MEDIUM D1 — MainActivity.onUrlTapped validates URL scheme against allowlist (http/https/ftp) before startActivity().

Deferred (require external work or larger refactor)

• HIGH A1+A2 — String → CharArray refactor for password handling. Multi-file API change crossing lib-ssh and app. Real but limited impact (in-memory disclosure already requires root). Tracked separately.
• HIGH F1 — Purchase signature verification. Already in docs/TODO.md open list. Requires Play Console RSA public key, which is generated only when subscription products are configured. Hard release blocker.
• HIGH C2 — Clipboard auto-clear timer. Feature work requiring UX decisions (configurable timeout, notification, opt-out for productivity).

Accepted risk

• MEDIUM C3 — protectScreenTerminal / protectScreenVault default false. User-facing toggles in Settings → Security with hints. Defaults documented.
• LOW A4 — Argon2id 3 iterations. Mobile performance vs security trade-off. 64 MB memory cost dominates GPU resistance.
• LOW H1 — writeInput/resizePty race window. Caught by SSH session's IOException handling. Robustness, not security.

Build verification

• ./gradlew assembleDevDebug ✓
• ./gradlew assembleProdDebug ✓

Both flavors compile cleanly. No new warnings introduced.