[Security] Native SSH hardening: zeroize secrets, SafeHandle, FFI abuse tests

[benyblack]

Area: src/NovaTerminal.App/native/rusty_ssh/, src/NovaTerminal.Platform/Ssh/Native/NativeSshInterop.cs

Summary

Hardening items for the native SSH stack. (Note: host-key verification was reviewed and is correctly implemented — TOFU with pinning via NativeKnownHostsStore, mismatch refusal in SshInteractionService.cs:141-163.)

1. No secret zeroization. zeroize is not in Cargo.toml; passwords/passphrases live in plain String/Vec<u8> (Rust) and GC-managed string (C#). Crash dumps/swap retain credentials. Add zeroize on the Rust side; minimize string retention on the C# side.
2. Raw IntPtr handles in NativeSshInterop.cs (no SafeHandle): an exception between allocation and storage leaks the native session; close/poll races mirror [Security] PTY FFI: use-after-free window between pty_close and concurrent calls #118. Wrap session handles in SafeHandle subclasses.
3. Audit error-string ownership across FFI — confirm every Rust-allocated error payload is freed when C# abandons a session mid-error (add a debug allocation counter).
4. ffi_contract.rs is thin (41 LOC) for a 13-export surface. Add abuse tests: call-after-close, double-close, concurrent poll+close, malformed/oversized JSON payloads. Confirm cargo test runs in CI.

[benyblack]

Scope update — re-verified all four items against main@29ae428

PR #210 (now merged) touched this file family, so I re-checked each item rather than trusting the 2026-07-27 triage. Two items are already done, one is done contrary to what the triage recorded, and one is half-done.

Item 2 — Raw IntPtr, no SafeHandle: DONE

src/NovaTerminal.Platform/Ssh/Native/NativeSshSafeHandle.cs defines:

public sealed class NovaSshSafeHandle : SafeHandleZeroOrMinusOneIsInvalid

and every session P/Invoke in NativeSshInterop.cs takes it rather than a raw nint.

Item 3 — FFI error-string ownership: DONE

lib.rs has static OUTSTANDING_FFI_STRINGS: AtomicI64, incremented on allocation and decremented in nova_ssh_string_free. The managed side frees in finally blocks, and alloc_balance_tests asserts the balance returns to zero — including on the malformed-input path, which is the case this item was worried about.

Item 4 — ffi_contract.rs abuse tests: effectively DONE — the triage was wrong here

All three requested cases already exist, in the handle_abuse_tests module in lib.rs rather than in tests/ffi_contract.rs:

Requested	Existing test
call-after-close	calls_after_close_fail_closed — 5 exports, including nova_ssh_channel_eof
double-close	double_close_is_rejected
concurrent poll+close	concurrent_poll_and_close_never_crashes — 200 iterations

The triage measured tests/ffi_contract.rs (67 LOC, 4 tests) and concluded the cases were missing; it didn't account for that module. cargo test does run in CI (ci.yml rust-ffi-tests), so this item's last sub-clause was already satisfied too.

Two real gaps did remain, and PR #212 covers them:

• the existing concurrency test races poll against a single closer, so two racing closers both reporting success wasn't covered
• nothing pinned that handle ids aren't reused after close — if they were, a stale handle could address a different live session instead of being rejected, quietly turning "fail closed" into "act on the wrong session"

Item 1 — Secret zeroization: Rust side done in #212, C# side still open

#212 declares zeroize directly and wraps the copies Rust owns: TransferAuthConfig::password, the interactive prompt results, and — easy to overlook — the raw JSON response payloads, which carry the secret in cleartext before parsing. It also replaces two password.clone() calls with a single take, so the credential exists as one allocation instead of two.

What's left, and why it's the bigger half. Rust can only wipe what Rust owns. The same credential also lives in a managed string, a C# JSON serialization buffer, the marshalled native string, and russh's protocol buffers. So #212 shortens the retention lifetime of the longest-lived copies but does not remove credentials from process memory — a crash dump can still contain the password. Finishing item 1 means char[]/pinned-buffer plumbing through NativeSshInterop plus a serializer that doesn't buffer the secret as a managed string, rippling into SshConnectionService, VaultService, and connection profiles. That is a materially larger change than the Rust half and worth scheduling on its own.

Suggested remaining scope for this issue

Items 2, 3, and 4 can be struck. What's left is the C#-side string retention in item 1 — worth either re-titling this issue to that, or closing it once #212 lands and opening a focused issue for the managed side.

[benyblack]

Triage 07-31: re-verified. The Rust half and item 4 are done; the managed half is genuinely open, and this issue describes it accurately. One gap it does not list.

Done (#212)

• Zeroize. zeroize = "1.8" (rusty_ssh/Cargo.toml:21), password: Option<Zeroizing<String>> (lib.rs:312, taken at :325), keyboard-interactive returns Zeroizing<String> / Zeroizing<Vec<String>> (:3183, :3194, :3200, :3205-3213), with three unit tests (:3980, :3993, :4003).
• SafeHandle. NovaSshSafeHandle : SafeHandleZeroOrMinusOneIsInvalid (NativeSshSafeHandle.cs:10), the P/Invoke return type for nova_ssh_connect (NativeSshInterop.cs:102), threaded through the whole interop surface (INativeSshInterop.cs:5,17-25), released under Interlocked.Exchange (NativeSshSession.cs:604).
• FFI abuse tests. Substantial: null handles (lib.rs:3286), null payload pointer (:3471), poll with null (:4359), panic guard (:4343, :4353), use-after-free (calls_after_close_fail_closed :4379), double-free (double_close_is_rejected :4407), concurrent poll/close (:4414), two concurrent closes (:4436), handle-id non-reuse (:4480), buffer-sizing abuse (:3395, :3416, :3449), plus tests/ffi_contract.rs (:11, :20, :46, :57). Gated at ci.yml:65-66 on both platforms.

Open — and this is the highest-value remaining item in the backlog

Managed code retains SSH secrets in plain string, which cannot be zeroed. The Rust half zeroizes; the managed half hands it a string that lives on in the GC heap until collection, and may be copied by compaction in the meantime.

The worst one, which the body does not call out specifically:

Services\Ssh\ActiveSshSessionRegistry.cs:11 — private readonly ConcurrentDictionary<Guid, string> _runtimePasswords, on a process-wide singleton (Instance :13), holding live plaintext for the whole session lifetime. Written :61, read :66, removed only on Unregister :45. That is a long-lived plaintext credential store in a process that also renders untrusted terminal output.

The rest of the retention chain:

Location	Member
Shell\TerminalProfile.cs:78	public string? Password { get; set; } — on the long-lived profile object
Platform\Ssh\Native\NativeSshConnectionOptions.cs:16	public string? Password { get; init; }
Platform\Ssh\Native\NativeSshInterop.cs:884	string? Password in the interop args record
Platform\Ssh\Interactions\SshInteractionResponse.cs:7,15-18	Secret, KeyboardResponses, FromSecret
ViewModels\Ssh\AuthPromptViewModel.cs:30,37	_value / Value — the typed password in a bindable string
Shell\VaultService.cs:37,62,88,98,188	string? password throughout the vault API
Shell\Win32CredentialManager.cs:54,83,96	returns (string Username, string Password)
Shell\LinuxSecretStore.cs:354	marshals as [MarshalAs(UnmanagedType.LPUTF8Str)] string password

None are SecureString, pooled char[], or pinned zeroable buffers.

A realistic scope, in priority order: (1) _runtimePasswords — either stop retaining it or hold a pinned byte[] that is zeroed on Unregister; (2) the prompt→interop hop, which is short-lived and the easiest to convert to a zeroable buffer; (3) TerminalProfile.Password, which is the awkward one because it is serialized. Worth being honest that .NET cannot make this airtight — the goal is shortening lifetimes and eliminating the long-lived copies, not perfection.

Gap this issue does not list

The abuse suite has no bad-UTF-8 input test. Closest are malformed_json_is_rejected_without_panic (ffi_contract.rs:46) and the oversized-JSON case (:57), which are structural rather than encoding. Given #152's history — the DllImport ANSI default silently mangling non-ASCII into U+FFFD — an invalid-UTF-8-bytes-across-the-boundary test belongs in this suite. Cheap, and squarely in scope.

[benyblack]

Item 1's managed half is done in #258 (bfdc8f2). Leaving this open for the rest, with a sharpened list.

What landed

ActiveSshSessionRegistry no longer retains session passwords as string. It now holds UTF-8 in a buffer allocated with GC.AllocateArray<byte>(n, pinned: true), zeroed with CryptographicOperations.ZeroMemory on overwrite and on Unregister.

Retention itself had to stay, which is worth recording because "just don't keep it" was my first instinct and it is wrong. Three consumers depend on it — SshInteractionService.cs:104 (re-auth in the same session), RemoteDirectoryBrowserService.cs:134 (the remote file browser's own SFTP connection), and path autocomplete via the same helper. Dropping it would re-prompt for a password on autocomplete.

Two properties moved, both about the long-lived copy specifically: it became clearable (window shrinks from process lifetime to session lifetime) and non-relocatable (compaction can no longer leave a stale copy that nothing references and nothing can clear).

Storage is a plain Dictionary under an explicit lock rather than a ConcurrentDictionary — wiping a buffer a reader is mid-decode would return a half-zeroed password and fail its auth for no discoverable reason.

Also closed the bad-UTF-8 abuse gap

The suite covered null pointers, double-close, use-after-free and malformed JSON but never invalid encoding — the one gap with history behind it, given #152.

The obvious assertion was too weak: assert_ne!(rc, 0) passes even with to_str() swapped for to_string_lossy(), because the mangled text then fails to parse as JSON and malformed JSON returns the same NOVA_SSH_RESULT_INVALID_ARGUMENT. It asserts on the rejection message now, verified by mutation.

A new finding, not fixed

read_c_string (rusty_ssh/src/lib.rs:1377) uses to_string_lossy(), and it backs every connect argument — host, user, term, identity_file, jump_host, jump_user, shell_detection_command and the three cwd bootstraps (:1185-1224, :860).

So invalid UTF-8 there is silently replaced with U+FFFD rather than rejected. Same class as #152, and inconsistent with the SFTP entry points, which reject it explicitly. The app cannot trigger it today (managed marshals as LPUTF8Str), but the boundary is pub extern "C", and silently substituting U+FFFD into bash_cwd_bootstrap — a shell command sent to the remote — is worse than rejecting it.

Not bundled into #258 because a naive fix changes semantics in several places at once: host/user would correctly become INVALID_ARGUMENT, but term and originator_address fall back to defaults via unwrap_or_else, and the cwd bootstraps would silently skip the feature instead of failing. Needs its own change with its own decision per call site.

Remaining, in the order I would do it

1. read_c_string above. Contained, and it is a correctness inconsistency rather than a design question.
2. The prompt → interop hop. AuthPromptViewModel._value (:30) is a bindable string holding the typed password, handed to SshInteractionResponse.Secret (:7) and on to NativeSshConnectionOptions.Password (:16) → NativeSshInterop.cs:884. All short-lived, so lower risk than the registry was, but this is the chain that would have to change to remove the transient copies fix(ssh): stop retaining session passwords as unclearable managed strings #258 leaves behind — and it means changing the FFI signature to accept a buffer rather than LPUTF8Str.
3. TerminalProfile.Password (:78). The awkward one: it is serialized, so it is not simply a lifetime question — it is about whether a password belongs on the profile object at all when the vault exists.
4. The vault chain. VaultService (:37, :62, :88, :98, :188), Win32CredentialManager (:54, :83, :96) returning (string, string), LinuxSecretStore.cs:354 marshalling as LPUTF8Str.

Worth being explicit that none of this reaches airtight: .NET cannot guarantee a secret is unrecoverable from a heap dump. The goal is shortening lifetimes and eliminating the copies that persist, which is why the registry was the right first slice — it was the only one whose lifetime was unbounded.