Audit 2026-04-12: C++ hardening, crash fix, dead code, quality

Fifth full codebase audit across all five modules (lib-ssh, lib-terminal-view,
lib-terminal-keyboard, lib-vault-crypto, app).

Security:
- Added cppFlags to lib-vault-crypto build — vault_crypto.cpp JNI bridge was
  missing all compiler hardening flags (-fstack-protector-strong, -D_FORTIFY_SOURCE=2)

Bugs fixed:
- SessionNotifier crash: first{} → firstOrNull to prevent NoSuchElementException
- Keyboard modifiers not consumed on SwitchPage/ToggleNumBlock — armed CTRL/ALT
  would persist and incorrectly modify the next key press
- KeyManagerViewModel silent exception swallow — now logs errors via FileLogger
- TelnetSession.sendTerminalType() variable shadowing fix

Dead code removed:
- Vt100Parser empty class (Vt220Parser now extends BaseTermParser directly)
- XtermParser.sendPrimaryDA() redundant override (identical to parent)
- TerminalKeyboard dead fields: menuPopupActive, menuPopupItems, miniContainer
- SpecialAction.SETTINGS_OPENED never emitted
- Deprecated 3-arg saveHostKeyFingerprint overload (no callers)

Code quality:
- Color(0xFF6E7979) → AppColors.Muted in ConnectionListScreen
- Hardcoded "v1.0.0" → BuildConfig.VERSION_NAME in SettingsScreen
- SubscriptionScreen back button contentDescription for accessibility
- TAG → companion const val in StartupCommandRunner, PortForwardManager, SftpSessionManager
- TerminalRenderer swapped KDoc comments fixed

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
jima 2026-04-12 15:09:05 +02:00
parent 56b875b9fc
commit 7b68e6404b
22 changed files with 43 additions and 123 deletions

View file

@ -50,8 +50,8 @@ class CredentialStoreInstrumentedTest {
@Test
fun storeAndRetrieveHostKeyFingerprint() {
store.saveHostKeyFingerprint("test.example.com", 22, "SHA256:abc123")
assertEquals("SHA256:abc123", store.getHostKeyFingerprint("test.example.com", 22))
store.saveHostKeyFingerprint("test.example.com", 22, "Ed25519", "SHA256:abc123")
assertEquals("Ed25519:SHA256:abc123", store.getHostKeyFingerprint("test.example.com", 22))
}
@Test

View file

@ -96,12 +96,6 @@ class CredentialStore @Inject constructor(
prefs.edit().putString(hostKeyId(host, port), "$keyType:$fingerprint").apply()
}
/** Legacy overload — kept for backwards compatibility, will be removed in v2. */
@Deprecated("Use the keyType variant for defense-in-depth", ReplaceWith("saveHostKeyFingerprint(host, port, keyType, fingerprint)"))
fun saveHostKeyFingerprint(host: String, port: Int, fingerprint: String) {
prefs.edit().putString(hostKeyId(host, port), fingerprint).apply()
}
fun deleteHostKeyFingerprint(host: String, port: Int) {
prefs.edit().remove(hostKeyId(host, port)).apply()
}

View file

@ -14,7 +14,9 @@ import kotlinx.coroutines.launch
*/
class PortForwardManager(private val context: Context, private val portForwardDao: PortForwardDao) {
private val TAG = "PortFwdMgr"
companion object {
private const val TAG = "PortFwdMgr"
}
/**
* Open all configured port forwards for a session.

View file

@ -126,7 +126,7 @@ class SessionNotifier(
flushed = pending.toList()
// Use the first pending session ID as navigation target
if (targetSessionId < 0 && pendingSessionIds.isNotEmpty()) {
targetSessionId = pendingSessionIds.first { it >= 0 }
targetSessionId = pendingSessionIds.firstOrNull { it >= 0 } ?: -1L
}
pending.clear()
pendingSessionIds.clear()

View file

@ -13,7 +13,9 @@ import java.util.concurrent.ConcurrentHashMap
*/
class SftpSessionManager {
private val TAG = "SftpMgr"
companion object {
private const val TAG = "SftpMgr"
}
private val sftpSessions = ConcurrentHashMap<String, SftpSession>()
private val sshSessions = ConcurrentHashMap<String, SSHSession>()

View file

@ -12,7 +12,9 @@ import kotlinx.coroutines.delay
*/
class StartupCommandRunner(private val savedConnectionDao: SavedConnectionDao) {
private val TAG = "StartupCmd"
companion object {
private const val TAG = "StartupCmd"
}
/**
* Execute startup commands from the SavedConnection after connect.

View file

@ -313,16 +313,16 @@ class TelnetSession {
}
private fun sendTerminalType() {
val termType = termType.toByteArray(Charsets.US_ASCII)
val termTypeBytes = termType.toByteArray(Charsets.US_ASCII)
// IAC SB TERMINAL-TYPE IS <type> IAC SE
val bytes = ByteArray(6 + termType.size)
val bytes = ByteArray(6 + termTypeBytes.size)
bytes[0] = IAC.toByte()
bytes[1] = SB.toByte()
bytes[2] = OPT_TTYPE.toByte()
bytes[3] = 0 // IS
termType.copyInto(bytes, 4)
bytes[4 + termType.size] = IAC.toByte()
bytes[5 + termType.size] = SE.toByte()
termTypeBytes.copyInto(bytes, 4)
bytes[4 + termTypeBytes.size] = IAC.toByte()
bytes[5 + termTypeBytes.size] = SE.toByte()
synchronized(writeLock) {
try {
outputStream?.write(bytes)

View file

@ -218,10 +218,10 @@ fun ConnectionListScreen(
}
}
IconButton(onClick = onNavigateToKeysVault) {
Icon(Icons.Filled.VpnKey, contentDescription = stringResource(R.string.keys_and_vault), tint = Color(0xFF6E7979))
Icon(Icons.Filled.VpnKey, contentDescription = stringResource(R.string.keys_and_vault), tint = AppColors.Muted)
}
IconButton(onClick = onNavigateToSettings) {
Icon(Icons.Filled.Settings, contentDescription = stringResource(R.string.settings), tint = Color(0xFF6E7979))
Icon(Icons.Filled.Settings, contentDescription = stringResource(R.string.settings), tint = AppColors.Muted)
}
}
}
@ -295,7 +295,7 @@ fun ConnectionListScreen(
singleLine = true,
trailingIcon = if (quickConnectText.isNotEmpty()) {
{ IconButton(onClick = { quickConnectText = ""; showQuickConnectHistory = false }) {
Icon(Icons.Filled.Close, contentDescription = null, modifier = Modifier.size(18.dp), tint = Color(0xFF6E7979))
Icon(Icons.Filled.Close, contentDescription = null, modifier = Modifier.size(18.dp), tint = AppColors.Muted)
} }
} else {
{ Icon(Icons.Filled.Terminal, contentDescription = null, modifier = Modifier.size(16.dp), tint = AppColors.Muted) }

View file

@ -761,7 +761,7 @@ fun SettingsScreen(
)
Spacer(modifier = Modifier.height(2.dp))
Text(
text = "v1.0.0",
text = "v${com.roundingmobile.sshworkbench.BuildConfig.VERSION_NAME}",
fontFamily = FontFamily.Monospace,
fontSize = 10.sp,
color = AppColors.OnSurfaceVariant,

View file

@ -79,7 +79,7 @@ fun SubscriptionScreen(
},
navigationIcon = {
IconButton(onClick = onBack) {
Icon(Icons.AutoMirrored.Filled.ArrowBack, contentDescription = null, tint = AppColors.Teal)
Icon(Icons.AutoMirrored.Filled.ArrowBack, contentDescription = stringResource(R.string.back), tint = AppColors.Teal)
}
},
modifier = Modifier.drawBehind {

View file

@ -10,6 +10,7 @@ import com.roundingmobile.sshworkbench.crypto.KeyImportStatus
import com.roundingmobile.sshworkbench.crypto.KeyValidationResult
import com.roundingmobile.sshworkbench.crypto.KeyZipHandler
import com.roundingmobile.sshworkbench.data.CredentialStore
import com.roundingmobile.sshworkbench.util.FileLogger
import com.roundingmobile.sshworkbench.data.local.SshKey
import com.roundingmobile.sshworkbench.data.local.SshKeyDao
import dagger.hilt.android.lifecycle.HiltViewModel
@ -48,6 +49,10 @@ class KeyManagerViewModel @Inject constructor(
@ApplicationContext private val appContext: Context
) : ViewModel() {
companion object {
private const val TAG = "KeyManagerVM"
}
val keys: StateFlow<List<SshKey>> = sshKeyDao.getAll()
.stateIn(viewModelScope, SharingStarted.WhileSubscribed(5000), emptyList())
@ -79,7 +84,8 @@ class KeyManagerViewModel @Inject constructor(
credentialStore.saveString("key_private_$keyId", generated.privateKeyPem)
}
} catch (_: Exception) {
} catch (e: Exception) {
FileLogger.logError(TAG, "Key generation failed", e)
} finally {
generating = false
}

View file

@ -1,64 +0,0 @@
# 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.

View file

@ -131,7 +131,7 @@ Terminal command snippets. `connectionId` null = global, non-null = per-connecti
- `savePassword(connectionId, password)` / `getPassword(connectionId)` / `deletePassword(connectionId)`
- `saveString(key, value)` / `getString(key)` — used for `key_private_$keyId`
- `saveHostKeyFingerprint(host, port, fp)` / `getHostKeyFingerprint(host, port)`
- `saveHostKeyFingerprint(host, port, keyType, fingerprint)` / `getHostKeyFingerprint(host, port)` / `matchesStoredHostKey(host, port, keyType, fingerprint)`
Passwords are only saved when explicitly requested (via "Remember password" checkbox in auth dialog or via the edit connection screen). `deletePassword()` is called when the password field is cleared in the connection editor.

View file

@ -17,13 +17,15 @@ The parser processes raw byte streams from SSH/Telnet/PTY connections and update
## Parser Hierarchy
```
BaseTermParser ← C0/C1, CSI dispatch, SGR, SM/RM, DSR
BaseTermParser ← C0/C1, CSI dispatch, SGR, SM/RM, DSR (all VT100 features)
└─ Vt220Parser ← DECSCA, DHDW, G2/G3 charsets, locking/single shifts
└─ XtermParser ← OSC (title/color), 256-color/RGB SGR, mouse, bracketed paste
Vt52Parser ← Standalone delegate (NOT in inheritance chain)
```
There is no intermediate `Vt100Parser` class — all VT100 features are implemented directly in `BaseTermParser`. `Vt220Parser` extends `BaseTermParser` directly.
`Vt52Parser` is a separate class used as a delegate by `BaseTermParser` when VT52 compatibility mode is active. It does not extend `BaseTermParser` — it has its own independent state machine.
---

View file

@ -5,6 +5,7 @@
## Recently Completed (2026-04-12)
- ~~Full codebase audit 2026-04-12~~ — fifth audit (all modules). Fixed: C++ compiler hardening via `cppFlags` in lib-vault-crypto; removed dead `Vt100Parser` class and redundant `XtermParser.sendPrimaryDA()`; fixed `SessionNotifier` crash (`first``firstOrNull`); fixed keyboard modifiers not consumed on SwitchPage/ToggleNumBlock; removed dead fields (`menuPopupActive`, `menuPopupItems`, `miniContainer`, `SETTINGS_OPENED`); removed deprecated `saveHostKeyFingerprint` 3-arg overload; TAG → companion const val; `Color(0xFF6E7979)``AppColors.Muted`; version string from `BuildConfig.VERSION_NAME`; `SubscriptionScreen` back button a11y; `KeyManagerViewModel` error logging; `TelnetSession.termType` shadowing fix.
- ~~Package name change~~ — applicationId changed from `com.roundingmobile.sshworkbench` to `com.roundingmobile.sshwb` (Firebase auto-key conflict blocked Play Console registration). Namespace stays `com.roundingmobile.sshworkbench`. Registered in Google Play Console.
- ~~Web platform scaffolding~~`www/` folder with Docker stack (nginx + Node.js/Express + MariaDB), landing page, login page (email/pw + OAuth), dashboard (vault + session logs). API routes for auth, vault CRUD, log management. MariaDB schema with users, vaults, logs, teams, permissions, snippets, audit_log.
- ~~Vault settings export/import~~ — optional "Include settings" checkbox (unchecked by default) in both Save Vault Locally and Export Vault. Exports 56 DataStore prefs (keyboard, display, QuickBar customization, HW actions). Import auto-restores settings. `EXPORTABLE_*_KEYS` lists in `TerminalPrefsKeys` define what's backed up.

View file

@ -72,8 +72,6 @@ class TerminalKeyboard private constructor(
private var modStateJob: Job? = null
// Menu popup state
private var menuPopupActive = false
private var menuPopupItems: List<MenuItem>? = null
private var menuOverlay: View? = null
// App shortcuts popup state
@ -262,7 +260,6 @@ class TerminalKeyboard private constructor(
// --- Mini number pad ---
private var miniPageView: KeyboardPageView? = null
private var miniContainer: ViewGroup? = null
/** The mini section definition from the layout, if any. */
val miniSection get() = layout.mini
@ -274,7 +271,6 @@ class TerminalKeyboard private constructor(
val mini = layout.mini ?: return
if (mini.rows.isEmpty()) return
this.miniContainer = container
val page = KeyboardPage(id = "mini", name = "Mini", rows = mini.rows)
val pv = KeyboardPageView(context, page, theme)
pv.setOnTouchListener { _, event -> handlePageTouch(pv, event) }
@ -302,7 +298,6 @@ class TerminalKeyboard private constructor(
longPressPopup = null
miniPageView = null
container = null
miniContainer = null
}
// --- Touch handling ---
@ -517,8 +512,6 @@ class TerminalKeyboard private constructor(
val rootContainer = container ?: return
hapticFeedback(qbView)
menuPopupActive = true
menuPopupItems = items
popup.theme = theme
popup.showMenu(items.map { it.label })
@ -590,8 +583,6 @@ class TerminalKeyboard private constructor(
private fun dismissMenuPopup() {
longPressPopup?.visibility = View.GONE
menuPopupActive = false
menuPopupItems = null
menuOverlay?.let { (it.parent as? ViewGroup)?.removeView(it) }
menuOverlay = null
}
@ -876,10 +867,14 @@ class TerminalKeyboard private constructor(
}
is KeyAction.SwitchPage -> {
keyboardView?.switchToPage(action.pageId)
modifierManager.consumeArmed()
keyboardView?.setModifierStates(modifierManager.states.value)
}
is KeyAction.ToggleNumBlock -> {
numBlockActive = !numBlockActive
miniPageView?.numBlockActive = numBlockActive
modifierManager.consumeArmed()
keyboardView?.setModifierStates(modifierManager.states.value)
}
is KeyAction.None -> {}
}

View file

@ -9,6 +9,5 @@ enum class SpecialAction {
KEYBOARD_HIDDEN,
KEYBOARD_SHOWN,
QUICKBAR_HIDDEN,
QUICKBAR_SHOWN,
SETTINGS_OPENED
QUICKBAR_SHOWN
}

View file

@ -99,10 +99,6 @@ class TerminalRenderer {
private var drawCount = 0L
/**
* Update font metrics for the given font size in pixels.
* Call whenever font size or display density changes.
*/
/** Set the terminal typeface. Call before updateFontMetrics. */
fun setTypeface(typeface: Typeface) {
terminalTypeface = typeface
@ -110,6 +106,7 @@ class TerminalRenderer {
indicatorPaint.typeface = typeface
}
/** Update font metrics for the given font size in pixels. Call whenever font size or display density changes. */
fun updateFontMetrics(fontSizePx: Float) {
textPaint.textSize = fontSizePx
textPaint.typeface = terminalTypeface

View file

@ -5,7 +5,7 @@ package com.roundingmobile.terminalview.engine
*
* Usage:
* val screen = ScreenBuffer(24, 80)
* val parser = Vt220Parser(screen) // or Vt100Parser, XtermParser
* val parser = XtermParser(screen) // or Vt220Parser for VT220-only
* parser.process(bytesFromSsh, 0, len) // feed it bytes, it updates the screen
*
* The parser is stateful it remembers partial escape sequences between calls.

View file

@ -1,20 +1,11 @@
package com.roundingmobile.terminalview.engine
// =================================================================================
// VT100 Parser — inherits base, this is the "standard" VT100 level
// Nothing extra needed beyond BaseTermParser — all VT100 features are in the base.
// The base was written at VT100 level already.
// =================================================================================
open class Vt100Parser(screen: ScreenBuffer) : BaseTermParser(screen)
// =================================================================================
// VT220 Parser — adds C1 controls, G2/G3 charsets, selective erase, DHDW
// Ported from: _ControlCharsC1, ESC #, ESC N/O, ESC ~/}/|/n/o, ESC SP F/G
// =================================================================================
open class Vt220Parser(screen: ScreenBuffer) : Vt100Parser(screen) {
open class Vt220Parser(screen: ScreenBuffer) : BaseTermParser(screen) {
// Whether to process 0x80-0x9F as C1 controls (vs ignoring them)
var process8BitControls: Boolean = true

View file

@ -76,12 +76,4 @@ open class XtermParser(screen: ScreenBuffer) : Vt220Parser(screen) {
}
}
// =========================================================================
// DA — xterm identifies differently
// =========================================================================
override fun sendPrimaryDA() {
// Identify as VT220 with xterm extensions
sendToHost("\u001B[?62;1;2;6;7;8;9c")
}
}

View file

@ -16,6 +16,7 @@ android {
externalNativeBuild {
cmake {
cFlags("-Wall", "-O2", "-fstack-protector-strong", "-D_FORTIFY_SOURCE=2", "-Wformat", "-Wformat-security")
cppFlags("-Wall", "-O2", "-fstack-protector-strong", "-D_FORTIFY_SOURCE=2", "-Wformat", "-Wformat-security")
}
}
}