镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

fix(ssh): close host-certificate and Unix-socket hardening gaps - #332

Merged
inureyes merged 4 commits into
mainfrom
fix/issue-330-hostcert-socket-hardening
Oct 22, 2026
Merged

inureyes merged 4 commits into
mainfrom
fix/issue-330-hostcert-socket-hardening

Conversation

@inureyes

@inureyes inureyes commented Oct 22, 2026 •

Copy link
Copy Markdown
Member

Summary

  • KnownHostsCommand output is a certificate-authority source. For a host certificate under any method except NoCheck and pinned keys, the HOSTNAME invocation runs once with the certificate as %K/%t (%f is the certified key's fingerprint, as in OpenSSH). Its @cert-authority and @revoked lines go through the same text core as file lines (host scope, ! negation, hashed hosts, port retry), followed by the usual principal, validity, critical-option, CASignatureAlgorithms and RevokedHostKeys checks. A certificate from a command-trusted CA that fails any check is refused, not downgraded. The output is reused for the plain-key fallback. A command failure is still a hard error.
  • RevokedHostKeys takes a list of files. A key in any file is refused. An unreadable or KRL file, or an unexpandable path, anywhere in the list refuses every host key. none or /dev/null must be the only value. -G prints the expanded paths on one line.
  • On non-Linux Unix the listener is bound as s inside a private mkdtemp directory (.bsshXXXXXX, 0700) next to the target, narrowed to 0600 and verified there, then moved into place without replacing anything: renamex_np(RENAME_EXCL) on Apple, link(2) on other BSDs (via std::fs::hard_link, no extra unsafe). A completed link is the move; the private directory, including the temporary name, is removed on every path, so the configured path never holds an orphaned node after an error. A temporary path over sun_path fails closed with an error that names the limit. The umask is not touched.
  • Item 4 needed no code change: bssh advertises exactly the configured certificate-only list. The explanation is on fix(openssh): decide FIDO host-key coverage for the hostkey-agent sk certificate phases #328: fix(openssh): decide FIDO host-key coverage for the hostkey-agent sk certificate phases #328 (comment)
  • ARCHITECTURE.md and docs/architecture/ssh-client.md describe the current host-certificate behavior (the "never advertises certificates" claim was stale since fix(openssh): close residual regress compatibility gaps #314).

Changes during review

  • Security fix: @revoked marker lines were split on spaces only and matched without hashed |1| host fields, so a CA or certified key revoked by a hashed or tab-separated line was ignored next to a matching @cert-authority line. This predates the PR for known_hosts files and also reached the new KnownHostsCommand source. The scan now splits on any whitespace and matches hashed patterns; negated patterns never match and never veto, so revocation errs toward refusing. Covered by hashed_and_tab_separated_revoked_lines_refuse_the_certificate (files and command output, ports 22 and 2222).
  • Pre-existing race, fixed because it failed CI here: probe_known_hosts_path mistook a known_hosts file created by a concurrent first connection (between metadata and symlink_metadata) for a dangling symlink and refused the host. It reproduced on main at 2/720 under 12 parallel loops; after the fix 0/2400. A path that is not a symlink on the second look is probed again, and a real dangling symlink is still refused (test_accept_new_dangling_symlink_known_hosts_fails_closed).

Test plan

  • New tests fail on main or when their fix is reverted; item 2 has separate first-file and second-file tests, each exposed by a first-only or last-only mutation.
  • Full cargo fmt --check, cargo clippy -- -D warnings, cargo test --lib (1801 passed), and cargo test --tests -- --skip integration_test pass locally on the final head. Three tests/integration_test.rs cases that need a live localhost sshd fail identically on main 5b20ce5 on this host and are skipped in CI.
  • OpenSSH regress locally: keytype, limit-keytype, agent pass; hostkey-agent fails only the two sk-* certificate phases tracked in fix(openssh): decide FIDO host-key coverage for the hostkey-agent sk certificate phases #328.
  • macOS: the new openssh-regress.yml step ran cargo test --lib forwarding::streamlocal on macos-latest (run 39699945497): 5 passed, including macos_socket_node_is_owner_only_before_it_appears.

Closes #330

KnownHostsCommand output now feeds host-certificate validation. For a certificate and any method other than NoCheck or a pinned key, the HOSTNAME invocation runs once with the certificate as %K and %t, and its @cert-authority and @Revoked lines get the same checks as file lines. A certificate from a command-trusted CA that fails a check is refused, not downgraded. The output is reused for the plain-key fallback instead of running the command again. The marker scan and CA lookup are split into a file wrapper and a text core so both sources share one matcher.

RevokedHostKeys accepts several files, as OpenSSH 10.3 does. A key listed in any file is refused; an unreadable or KRL file, or an unexpandable path, anywhere in the list refuses every host key. none or /dev/null is valid only as the sole value. bssh -G prints the expanded paths on one line.

On macOS and the BSDs the Unix listener is bound and narrowed to 0600 inside a private mkdtemp directory beside the target, then moved into place with renamex_np(RENAME_EXCL) or link and unlink, so the configured path never holds a wider mode and no existing node is replaced. The macOS test runs in the macOS leg of openssh-regress.yml.

Validated with the narrow lib suites for tokio_client, ssh_config and streamlocal, the ssh_config_dump_test integration test, clippy and fmt. The Apple code path was compile-checked for aarch64-apple-ios in a scratch crate and exercised on Linux with the link and unlink move.

Closes #330
@inureyes inureyes added type:bug Something isn't working priority:low Low priority issue status:review Under review labels Oct 22, 2026
@inureyes

Copy link
Copy Markdown
Member Author

Implementation Review Summary

Intent

Close the four residual #314 findings from #330: KnownHostsCommand as a host-CA source, a list of RevokedHostKeys files, the macOS Unix-listener mode window, and the sk-* certificate-only negotiation question.

Findings Addressed

No CRITICAL or HIGH findings, so no code was changed. All four items meet their acceptance criteria. Each departure is sound: %f fingerprints the plain key as OpenSSH's sshkey_fingerprint does; OpenSSH 10.3 check_host_key also loads the command output once, before retry:, so the plain-key fallback reuses it; Unusable keeps the single-file fail-closed rule; std::fs::hard_link/remove_file are link(2)/unlink(2). The item 4 comment on #328 matches resolve_host_keys and russh 0.64.1 write_kex/read_kex.

Remaining Items

  • macOS cargo test --lib forwarding::streamlocal run is still pending in openssh-regress.yml; the PR has to cite it (acceptance criterion, item 3) (MEDIUM)
  • connection.rs:3863 comment says DNSSEC runs before any KnownHostsCommand lookup; for certificates the HOSTNAME invocation now runs first (LOW)
  • docs/architecture/ssh-client.md:46 does not mention KnownHostsCommand CA lines or the RevokedHostKeys list (LOW)
  • streamlocal.rs:224-227: if remove_file fails after hard_link, the call errors but leaves a socket node at the configured path (LOW)
  • streamlocal.rs:207: renamex_np(RENAME_EXCL) returns ENOTSUP on volumes without exclusive rename, where binding used to work; this fails closed (LOW)

Verification

  • All stated requirements implemented
  • No placeholder/mock code remaining
  • Integrated into project code flow
  • Project conventions followed
  • Existing modules reused where applicable
  • No unintended structural changes
  • Tests pass (narrow lib suites for tokio_client, ssh_config, streamlocal; ssh_config_dump_test; clippy --lib --tests; fmt). Full suites and macOS are pending.

The @Revoked marker scan split fields on spaces only and matched host fields only as globs, while @cert-authority lookup splits on any whitespace and matches hashed |1| fields. A revoked CA or certified key whose @Revoked line used a hashed host field or tab separators was therefore ignored next to a matching @cert-authority line, and the certificate was accepted. This applied to known_hosts files and to the KnownHostsCommand output that #330 now loads for certificates; OpenSSH refuses in both cases.

The scan now splits on ASCII whitespace and matches each non-negated pattern with host_pattern_matches, keeping the lenient bare-host retry. Negated patterns still never match and never veto, so revocation stays fail-safe. Plain-key marker checks share the same scan and gain the same coverage.

Validated with cargo test --lib for host_certificate, host_verification, connection, ssh_config, and forwarding::streamlocal, the ssh_config_dump_test integration test, clippy --lib --tests -D warnings, and fmt --check.

Refs #330
@inureyes

Copy link
Copy Markdown
Member Author

Security and Performance Review

Fixed

  • HIGH, revocation bypass (62fc1e6): scan_known_hosts_marker_text split fields on spaces only and marker_host_matches matched only globs, while @cert-authority lookup handles tabs and hashed |1| fields. A hashed or tab-separated @revoked line for a CA (or the certified key) was ignored next to a matching @cert-authority line, so the certificate was accepted. This affected known_hosts files (pre-existing) and the KnownHostsCommand output this PR newly loads for certificates. The scan now splits on any whitespace and matches hashed patterns, keeping the lenient bare-host retry. Negated patterns still neither match nor veto, so revocation stays fail-safe. New test hashed_and_tab_separated_revoked_lines_refuse_the_certificate covers files and command output on ports 22 and 2222. It failed on dd14a11 and passes now.

Checked, no finding

  • Fail-closed paths: command nonzero exit, timeout, oversize stdout or expanded argv, non-UTF-8 output, unexpandable or mixed none RevokedHostKeys entries, and an unreadable or KRL file at any position in the list all refuse the host key.
  • Command output gets the same CA checks as a file line (one shared matcher), plus the principal, validity, critical option, and CASignatureAlgorithms checks. It is consulted only after the files, and never for pinned-key methods or NoCheck.
  • %t/%K/%f/%H are substituted after the template is split into argv, without a shell, so they cannot inject arguments.
  • The command runs once per host-key check. The HOSTNAME output is reused for the plain-key fallback. For a certificate whose CA is in a file, the command now runs one extra time, as OpenSSH does.
  • streamlocal: the private mkdtemp directory is 0700, renamex_np(RENAME_EXCL)/link(2) never replace an existing node, and Drop removes the directory on every path. Swapping the private directory is possible only when the parent is world-writable without the sticky bit, and in that case the final socket can be replaced anyway, which is the same exposure OpenSSH has.

Remaining (LOW, not fixed)

  • streamlocal.rs:224-227: if remove_file fails after hard_link succeeds, the bind errors but the linked node stays at the configured path. Returning Ok after the link and letting PrivateBindDir::drop remove the temporary name would avoid that.
  • lookup_known_host_data counts a host-matching @revoked line for a different key as a recorded key, which can report a Conflict for the presented key. This predates the PR and fails closed.
  • The macOS forwarding::streamlocal CI step was still pending at review time.

Verification on 62fc1e6: cargo test --lib for host_certificate (15), host_verification (56), connection (73), ssh_config (312), and forwarding::streamlocal (5), plus cargo test --test ssh_config_dump_test (21), all pass. cargo clippy --lib --tests -- -D warnings and cargo fmt --check are clean.

On non-Apple BSDs the owner-only socket is hard-linked into place and the temporary name is then removed. If that removal failed after the link succeeded, the bind reported an error while a socket node stayed at the configured path. The link now counts as the completed move, and the private-directory drop guard removes the temporary name, so the configured path never holds an orphaned node after an error.

Also corrects the DNSSEC ordering comment in check_server_key (the certificate HOSTNAME invocation runs first), documents KnownHostsCommand @cert-authority and @Revoked lines and the RevokedHostKeys file list in docs/architecture/ssh-client.md, and rewrites the ARCHITECTURE.md host certificate section, which still said bssh never advertises certificates. No Korean docs exist in the repository.

Validated with cargo test --lib forwarding::streamlocal, clippy --lib --tests -D warnings, and cargo fmt. The BSD fallback and the macOS-only test cannot run on this host.

Refs #330
…link

probe_known_hosts_path called metadata, got NotFound, and then treated any successful symlink_metadata as a dangling symlink. When a concurrent first connection created known_hosts between the two calls, the probe refused the host with "exists but cannot be resolved" (ServerCheckFailed). This made test_accept_new_concurrent_first_connections_record_one_entry flaky under load: it failed CI on this PR and reproduced on main 5b20ce5 at 2/720 runs with 12 parallel loops.

A path that is not a symlink on the second look is now probed once more; a real dangling symlink is still refused. With the fix the same stress run gives 0/2400 failures. A new test pins the fail-closed dangling-symlink case, and it fails if a path present on the second look is treated as missing. The racer assertion now prints the result it got.

Refs #330
@inureyes inureyes added status:done Completed and removed status:review Under review labels Oct 22, 2026
@inureyes
inureyes merged commit 7e2130a into main Oct 22, 2026
5 checks passed
@inureyes
inureyes deleted the fix/issue-330-hostcert-socket-hardening branch October 22, 2026 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority:low Low priority issue status:done Completed type:bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(ssh): close residual host-certificate and Unix-socket hardening gaps

1 participant