Repository navigation
examples: check host keys against known_hosts - #1386
ejohnstown wants to merge 6 commits into
Conversation
ClientPublicKeyCheck() follows StrictHostKeyChecking=ask. A missing or empty known_hosts has no entries and prompts, creating ~/.ssh with mode 0700 on yes; one it cannot read refuses, as does EOF at the prompt. A line holding the key accepts wherever it is in the file. - Size the base64 host key buffer from the blob, so ML-DSA keys fit - Keep scanning past a stale same-type line for the host - Check the certificate IP only once known_hosts accepts, and let the override waive only the IP mismatch - Cover these in regress.test and sshclient.test Issue: F-14635, F-15505, F-15508, F-15664
3470a73 to
a89c603
Compare
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1386
Scan targets checked: wolfssh-src, wolfssh-bugs
Not reviewed (no scan target covers them): .github/workflows/interop-mlkem.yml, .github/workflows/network-contention-test.yml, .github/workflows/scp-test.yml, .github/workflows/sftp-test.yml, .github/workflows/test-fatfs.yml, .github/workflows/tpm-ssh.yml, .github/workflows/windows-cert-store-test.yml, .github/workflows/windows-sftp.yml, README.md, apps/wolfssh-options.c, apps/wolfsshd/test/error_return.sh, apps/wolfsshd/test/run_all_sshd_tests.sh, apps/wolfsshd/test/sshd_authkeys_none_test.sh, apps/wolfsshd/test/sshd_bad_sftp_test.sh, apps/wolfsshd/test/sshd_chroot_fail_test.sh, apps/wolfsshd/test/sshd_chroot_none_test.sh, apps/wolfsshd/test/sshd_empty_password_test.sh, apps/wolfsshd/test/sshd_exec_test.sh, apps/wolfsshd/test/sshd_forcedcmd_test.sh, apps/wolfsshd/test/sshd_fork_fail_test.sh and 38 more
Coverage: 11 of 11 in-scope changed file(s) opened by the reviewer
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Max
The client, scpclient and sftpclient accept a plain host key only when a "host type key" line of ~/.ssh/known_hosts holds it. An unknown or changed key is refused, printing its fingerprint and the line to add. -Y accepts any key, with a warning. - Check that an x509v3 host certificate names the host by DNS or IP subjectAltName, or by common name without one; -X waives a mismatch - Pass -Y in the tests and CI jobs that connect to a throwaway server - Seed known_hosts for openssh-interop.test from sshd's host keys - Add client-hostkey.test for unknown, known, stale and changed keys Issue: F-7620
portfwd links the example client's common code and registers its host key check, with the SSH server's host name as the context. -Y accepts any key, with a warning. - Pass -Y in fwd.test and fwd-bulk.test - Cover a refused unknown key in client-hostkey.test Issue: F-10587
tpmcertclient links the example client's common code and registers its host key check, with the -h host as the context. The library verifies the chain to the -A CA; the check requires the leaf to name the host. - Test the name check in testsuite.test with keys/ certificates - Add a tpm-ssh job refusing the certificate when asked for localhost Issue: F-10588
The test quoted the whole wolfsftp command line as one word, so the shell found no such program and its exit status 127 passed as the expected refusal. Quote each argument instead, and fail when the client can't be run.
a89c603 to
d2d5e21
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1386
Scan targets checked: wolfssh-src, wolfssh-bugs
Not reviewed (no scan target covers them): .github/workflows/interop-mlkem.yml, .github/workflows/network-contention-test.yml, .github/workflows/scp-test.yml, .github/workflows/sftp-test.yml, .github/workflows/test-fatfs.yml, .github/workflows/tpm-ssh.yml, .github/workflows/windows-cert-store-test.yml, .github/workflows/windows-sftp.yml, README.md, apps/wolfssh-options.c, apps/wolfsshd/test/error_return.sh, apps/wolfsshd/test/run_all_sshd_tests.sh, apps/wolfsshd/test/sshd_authkeys_none_test.sh, apps/wolfsshd/test/sshd_bad_sftp_test.sh, apps/wolfsshd/test/sshd_chroot_fail_test.sh, apps/wolfsshd/test/sshd_chroot_none_test.sh, apps/wolfsshd/test/sshd_empty_password_test.sh, apps/wolfsshd/test/sshd_exec_test.sh, apps/wolfsshd/test/sshd_forcedcmd_test.sh, apps/wolfsshd/test/sshd_fork_fail_test.sh and 38 more
Coverage: 2 of 2 in-scope changed file(s) opened by the reviewer
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Review tier: Lite
| int ipMatch = 0; | ||
| DNS_entry* current = dCert.altNames; | ||
| /* The library has already checked an x509v3 key's chain to the CA. */ | ||
| if (WSTRNCMP(keyType, "x509v3-", 7) == 0) { |
There was a problem hiding this comment.
Host certificate name check is chosen by the server's key-type label, not the negotiated algorithm · Authentication bypass
The ClientCertHostCheck branch runs only when the server's blob label starts with x509v3-. The library parses the cert by the negotiated pubKeyId, and ParseCertChain (src/internal.c:7978) skips that label without checking it. A server negotiating x509v3 can send a non-x509v3 label, so with -Y any CA-signed cert is accepted for any host.
Suggested fix: Reject a host key blob whose label does not match the negotiated algorithm (for example in ParseCertChain), so a relabelled certificate cannot skip ClientCertHostCheck.
Basis: RFC 4253 §6.6 and RFC 6187 §3: the key blob starts with the key format name, which identifies the negotiated public key algorithm.
Related known findings (similar but distinct; listed for context, not part of this finding)
- F-10588: F-10588 is the complete absence of a host-name check in tpmcertclient; this is a bypass of the new check, caused by trusting the attacker-supplied blob label.
- F-7620 (assigned): File/function: both are ClientPublicKeyCheck in examples/client/common.c, and both concern host verification, but F-7620 cites lines 383-459 and the candidate cites line 698, so the code differs. Operation: F-7620 is a callback that returns 0 (accept) unconditionally in the default build with no WOLFSSH_CERTS. The candidate is a WSTRNCMP on the attacker-supplied blob label that decides whether ClientCertHostCheck runs, in a certs build where the check exists. Root cause: F-7620 is a missing or stub
There was a problem hiding this comment.
Confirmed. With an echoserver serving keys/server-cert.der under x509v3-ecdsa-sha2-nistp256 but naming the blob ecdsa-sha2-nistp256, the client with -Y connected to localhost, which the certificate does not name; with the honest name the same connection was refused. The plain key parsers already check the blob's name against the negotiated algorithm, and the certificate parsers now do too, so a misnamed chain fails key exchange. test_ParseECCPubKeyCert covers a misnamed chain.
The plain host key parsers check the blob's name against the negotiated algorithm; the certificate parsers skipped it. A server could then name a certificate as a plain key, and a host key callback that reads the name, as the example clients do, never checked it against the host. - Test a misnamed chain in test_ParseECCPubKeyCert, and name the P-384 curve mismatch case for P-384
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1386
Scan targets checked: wolfssh-src, wolfssh-bugs
Not reviewed (no scan target covers them): .github/workflows/interop-mlkem.yml, .github/workflows/network-contention-test.yml, .github/workflows/scp-test.yml, .github/workflows/sftp-test.yml, .github/workflows/test-fatfs.yml, .github/workflows/tpm-ssh.yml, .github/workflows/windows-cert-store-test.yml, .github/workflows/windows-sftp.yml, README.md, apps/wolfssh-options.c, apps/wolfsshd/test/error_return.sh, apps/wolfsshd/test/run_all_sshd_tests.sh, apps/wolfsshd/test/sshd_authkeys_none_test.sh, apps/wolfsshd/test/sshd_bad_sftp_test.sh, apps/wolfsshd/test/sshd_chroot_fail_test.sh, apps/wolfsshd/test/sshd_chroot_none_test.sh, apps/wolfsshd/test/sshd_empty_password_test.sh, apps/wolfsshd/test/sshd_exec_test.sh, apps/wolfsshd/test/sshd_forcedcmd_test.sh, apps/wolfsshd/test/sshd_fork_fail_test.sh and 38 more
Coverage: 2 of 2 in-scope changed file(s) opened by the reviewer
No new issues found in the changed files.
💬 1 finding(s) from an earlier review are still open and were not re-posted:
- Host certificate name check is chosen by the server's key-type label, not the negotiated algorithm —
examples/client/common.c:698
This review was generated automatically by Fenrir. Reported findings require changes before merge.
Review tier: Lite
The example clients, portfwd and tpmcertclient refuse a host key that
~/.ssh/known_hosts does not hold, printing the line to add; -Y accepts
any key with a warning. Stacked on #1385; only the last three commits
are new here.
stale and changed keys
Issue: F-7620, F-10587, F-10588