Skip to content

fix(security): close remaining fail-open paths in host key verification #242

Description

@inureyes

Problem / Background

PR #241 (issue #239) replaced the no-op accept-new host key mode with real TOFU. Four fail-open or unsupported paths were identified during that work and deliberately left out of scope to keep an already-large security PR reviewable. They are collected here. Each one can result in a host key being accepted without meaningful verification, so they matter more now that users will reasonably assume accept-new provides real protection.

Technical Analysis

1. accept-new with no determinable home directory disables verification entirely

src/ssh/known_hosts.rs:65-77: when get_default_known_hosts_path() returns None, get_check_method(AcceptNew) returns ServerCheckMethod::NoCheck after printing a warning. NoCheck accepts any server key unconditionally. Because accept-new is the CLI default (src/cli/bssh.rs, where --strict-host-key-checking defaults to accept-new), this is a silent accept-all in the default mode. It is reachable in containers and service environments where HOME is unset and no passwd entry supplies a home directory. Strict yes mode was deliberately fixed to fail closed in this same situation and is not affected.

2. An unreadable known_hosts file is indistinguishable from an absent one

russh::keys::known_hosts::known_host_keys_path returns Ok(vec![]) whenever File::open fails, for any reason, so a file that exists but cannot be read (permission denied, a dangling symlink, a directory in its place) is reported as an empty known_hosts rather than an error. lookup_known_host in src/ssh/tokio_client/host_verification.rs is built on that function, so in accept-new mode such a file makes every host look like a genuine first use: the key is accepted, and the subsequent learn_known_hosts_path append will usually also fail, which is only a warning because recording is intentionally best effort. The net effect is a connection accepted unverified on every run, with no pinning ever established and no error exit code. A host key that changes between runs is never detected.

3. No cross-process locking, and a successfully injected entry becomes permanently trusted

KNOWN_HOSTS_LOCK in src/ssh/tokio_client/host_verification.rs is a process-wide tokio::sync::Mutex. It correctly prevents duplicate appends within one bssh invocation, but two concurrent bssh processes can both observe a host as unknown and both append. If one of them is talking to a man-in-the-middle, the file ends up holding two entries of the same algorithm for that host. This interacts badly with the acceptance rule: lookup_known_host deliberately implements OpenSSH's "any recorded key matches" semantics (itself a security fix, since the alternative rejects legitimate layouts such as a stale key kept beside a rotated one), so from then on the attacker's key is accepted silently and permanently, and the changed-key warning never fires again.

4. @cert-authority marker lines are not honored

src/ssh/tokio_client/host_verification.rs detects @cert-authority lines and prints a warning that the marker is unsupported, then falls through to ordinary TOFU. This was a deliberate choice: bssh has no CA signature validation to fall back on, and failing closed would break working CA-based setups with no workaround available to the user. It is recorded here so the limitation is tracked rather than rediscovered. Before PR #241 these lines were ignored completely and silently, so the warning is an improvement, not a regression.

Impact

Items 1 and 2 mean the documented and recommended default mode can silently provide zero MITM protection in specific but realistic environments. Item 3 means a single successful race, in a threat model where the attacker can already intercept one connection, converts into permanent trust. Item 4 means users relying on an SSH CA get TOFU semantics instead of certificate validation.

Proposed Solution

  1. Process-lifetime in-memory pin. Maintain a (host, port) -> PublicKey map for the lifetime of the process, populated on every successful verification and consulted alongside the file. This closes items 1 and 2 for the common fan-out case: even with no usable known_hosts file, all connections within a single run must agree on a host's key, so a mid-run substitution is caught. It also strengthens item 3, since a conflicting entry appended by another process is detected against the in-memory pin.
  2. Distinguish absent from unreadable. Before the lookup, check whether the path exists (for example Path::try_exists() plus a metadata or open probe). A path that is absent is a legitimate first-use situation. A path that exists but cannot be read, or is not a regular file, must fail closed with a distinct error rather than being treated as empty, in both accept-new and strict mode.
  3. Advisory cross-process file lock. Wrap the check-then-record critical section in an advisory lock on the known_hosts file (flock on Unix, LockFileEx on Windows, or a dedicated lock file) held for the duration the process-wide mutex is already held. Keep the existing in-process mutex; the two compose.
  4. Decide @cert-authority explicitly. Either implement certificate validation against the CA key, or make the behavior configurable so security-sensitive deployments can opt into failing closed. Do not change the default silently.

Acceptance Criteria

  • With accept-new and no determinable home directory, host keys are still pinned for the lifetime of the process, and a host presenting a different key later in the same run is rejected.
  • A known_hosts path that exists but cannot be read fails closed with a distinct error in both accept-new and yes modes, and is not reported as an unknown host.
  • A known_hosts path that is absent is still treated as a first-use situation and is created on recording, preserving current behavior.
  • Two concurrent bssh processes performing a first-time connection to the same new host record exactly one entry, verified by a test that spawns real processes or otherwise exercises the cross-process path.
  • A conflicting entry appended by another process while a run is in progress is detected rather than silently accepted.
  • @cert-authority behavior is either implemented or explicitly configurable, and the CLI help text plus docs/architecture/ssh-client.md state the actual guarantee.
  • Unit tests inject paths explicitly and none of them mutate the HOME environment variable (see the companion cleanup issue for the existing HOME-mutation race).

Metadata

Metadata

Assignees

No one assigned

    Labels

    priority:highHigh priority issuestatus:readyReady to be worked ontype:bugSomething isn't workingtype:securitySecurity vulnerability or fix

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions