ssh-workbench/docs/Audit.md

Audit — 2026-04-06

Comprehensive audit across all modules after the NumBlok / HW keyboard / VP2 rotation work in this session. Focus: security, bugs, dead code, file size, code quality. Debug infrastructure (TRACE logs, DevConfig receiver, SessionRecorder) is intentionally preserved and gated on the dev flavor.

Parallel scan by module (lib-terminal-keyboard, lib-ssh, lib-terminal-view, lib-vault-crypto, app). Many reported findings were false positives on re-inspection — logged below for clarity.

Fixes applied

lib-terminal-keyboard/src/main/kotlin/com/roundingmobile/keyboard/view/KeyboardPageView.kt

• Removed key.hint!! on lines 201/208. Captured key.hint into a local hintText val at the top of the render branches so Kotlin's smart-cast flows through the if (hintText != null && hasBelowHint) / if (hintText != null && hasCornerHint) checks. Drop-in safer replacement for the two !! assertions — no runtime behavior change.

lib-ssh/src/main/java/com/roundingmobile/ssh/SSHKeyLoader.kt

• Added require(seed.size == 32) in loadEd25519FromPkcs8. A malformed PKCS#8 blob with a non-32-byte OCTET STRING would previously crash inside EdDSAPrivateKeySpec(seed, …) with a cryptic stack. Now it fails fast with a clear message: "Ed25519 seed must be exactly 32 bytes, got $size".

lib-vault-crypto/src/main/cpp/vault_crypto.cpp

• Zero plaintext JNI buffer before ReleaseByteArrayElements in nativeEncrypt. The existing code was already zeroing the key buffer (secure_zero(keyPtr, keyLen)) before release with JNI_ABORT, but leaking the plaintext buffer through the same path. Added three secure_zero(ptPtr, ptLen) calls — one on the normal success path, two on error paths (urandom failure, keyPtr acquisition failure). This matches the key-handling pattern and closes a defence-in-depth gap for sensitive plaintext passing through encrypt. On Android ART, GetByteArrayElements returns a JVM-managed copy, so zeroing does not affect the caller's input array.

Findings that were NOT real issues (documented to avoid re-flagging)

lib-terminal-keyboard/TerminalKeyboard.kt

• "Comment syntax error / NumBlock… on lines 173/814" — false positive. Both lines use valid // comments; the agent misread the indentation.
• "_rawJson!! on line 1086" — false positive. Guarded by the enclosing if (kl == null && _rawJson != null) check; !! is safe.
• "Race condition on numBlockActive between UI and touch threads" — false positive. Both handleKeyAction (toggle) and KeyboardPageView rendering (read) run on the main thread.

lib-terminal-keyboard/KeyboardView.kt

• "Rapid rebuilds via onSizeChanged could lose state" — not a real issue. The rebuild is idempotent: it re-creates the ViewPager2 and re-applies cached lastModifierStates. Debouncing would hide legitimate resize events.

lib-terminal-keyboard/QuickBarView.kt

• "VelocityTracker.recycle() on line 747 is unreachable after return" — false positive. The recycles on 731 and 747 are in two separate if/else branches; each is reachable within its branch.
• "Empty catches around color parsing" — intentional. These are graceful fallbacks for user-provided custom color strings; failing silently to the theme default is the desired UX.

lib-terminal-keyboard/gesture/KeyRepeatHandler.kt

• "CoroutineScope not cancelled if destroy() never called" — not a real issue. TerminalKeyboard.detach() calls keyRepeatHandler.destroy(), and detach() is called from the DisposableEffect(keyboard) onDispose in MainActivity.

lib-ssh/SftpSession.kt

• "sortEntries sorts dirs by name even in size mode" — intentional. Directories in SFTP do not have a meaningful file size (usually 0 or filesystem-block size); alphabetical ordering is the sensible fallback.
• "Dispatcher leak on connect() failure" — false positive. SftpSessionManager.openSftpSession wraps the connect() call in try/catch and calls sftpSession.close() on failure (line 49), which shuts down the dispatcher.

lib-ssh/SSHSession.kt

• "Password in String" — known limitation documented in the existing code. Strings are immutable in the JVM and cannot be reliably zeroed; the app converts them to CharArray at the earliest opportunity in MainViewModel/TerminalService and zeros those. Not a regression.
• "Swallowed cleanup exceptions" — intentional. Cleanup paths swallow to avoid cascading failures that could mask the original disconnect reason.

lib-vault-crypto/aes256gcm.c

• "Table-based AES vulnerable to cache-timing side channels" — not in threat model. The app runs in its own Android process; there is no co-located untrusted code attempting cache-timing attacks against the vault crypto. Using a constant-time software AES would add complexity without addressing a real attack vector.
• "GHASH not constant-time" — same rationale.

lib-vault-crypto/VaultCrypto.kt

• "Plaintext ByteArray parameter not zeroed by the Kotlin API" — caller's responsibility. The Kotlin API is a thin JNI wrapper; the caller owns the input ByteArray and is expected to zero it after use if it contains secrets. Documenting this would be useful (follow-up doc task), but the code itself is not buggy.

lib-terminal-view/ScreenBuffer.kt

• "screenRow() returns buf[0] for out-of-bounds" — defensive, not a bug. Parser callers should never hit this path; the fallback prevents a crash in unexpected cases.

app/ui/MainActivity.kt, MainViewModel.kt, SessionTabBar.kt, EditConnectionScreen.kt, KeyboardSettingsDialog.kt, QuickBarCustomizer.kt

• All clean per the app-module agent. HW keyboard detection, theme picker, NumBlok serialization all correct.

app/src/dev/DevConfig.kt

• "Hardcoded test credentials" — dev flavor only, stripped from prod. Used for the internal test SSH servers (one.jima.cat, 10.10.0.39). Not a leak.

Build verification

• ./gradlew assembleDevDebug — BUILD SUCCESSFUL (v0.0.31)
• Native vault_crypto.cpp recompiled cleanly for all ABIs (armeabi-v7a, arm64-v8a, x86, x86_64) with the added secure_zero calls.
• Deployed to duero:/mnt/master/ssh-workbench.v0.0.31.dev.dbg.2026-04-06.apk.
• S23 was disconnected at deploy time, so no install/smoke test on device. Previous v0.0.30 is still installed on S23; the audit changes are all defence-in-depth tightening and should be transparent at runtime.