diff --git a/src/main/ssh/ssh-known-hosts.test.ts b/src/main/ssh/ssh-known-hosts.test.ts index 0a204be6aa0..d8f8d0283df 100644 --- a/src/main/ssh/ssh-known-hosts.test.ts +++ b/src/main/ssh/ssh-known-hosts.test.ts @@ -349,6 +349,52 @@ describe('lines ssh itself refuses to parse', () => { const hash = Buffer.alloc(20, 2).toString('base64') expect(parseKnownHostsLine(`|1|${salt}|${hash} ssh-ed25519 ${ED}`)).toBeDefined() }) + + const [, , REAL_SALT = '', REAL_HASH = ''] = HASHED_EXAMPLE_COM.split('|') + const hashedLine = (salt: string, hash: string): string => `|1|${salt}|${hash} ssh-ed25519 ${ED}` + + // Non-obvious: every mutation below still decodes to exactly 20 bytes, so the length check above + // passes and the entry would be trusted — Buffer.from just skips the junk. Verified live against + // OpenSSH 10.2p1 with `ssh-keygen -vvv -F` on a real `ssh-keygen -H` file: the salt cases print + // "extract_salt: salt decode error" / "bad host hash", and the hash cases find nothing silently + // because ssh regenerates the canonical `|1|salt|hash` string and byte-compares it. + it.each([ + ['salt with invalid characters spliced into the middle', REAL_SALT.replace(/^(.{6})/, '$1@@')], + ['salt with trailing junk', `${REAL_SALT}!!!`], + // Buffer.from accepts the base64url alphabet and yields the same 20 bytes; ssh does not. + ['salt in the base64url alphabet', REAL_SALT.replace(/\//g, '_').replace(/\+/g, '-')], + // Right length and it looks like base64, but the final character's leftover bits are set — the + // case a "does it look like base64" check misses and b64_pton rejects as a subliminal channel. + ['salt whose final character carries non-zero padding bits', `${REAL_SALT.slice(0, -2)}9=`], + ['salt with its base64 padding stripped', REAL_SALT.replace(/=+$/, '')] + ])('drops a hashed entry with a %s, as ssh does', (_label, salt) => { + expect(parseKnownHostsLine(hashedLine(salt, REAL_HASH))).toBeUndefined() + }) + + it.each([ + ['invalid characters spliced into the middle', REAL_HASH.replace(/^(.{6})/, '$1@@')], + ['trailing junk', `${REAL_HASH}!!!`] + ])('drops a hashed entry whose hash has %s, as ssh does', (_label, hash) => { + expect(parseKnownHostsLine(hashedLine(REAL_SALT, hash))).toBeUndefined() + }) + + // Guards the cases above from passing by rejecting everything. + it('still accepts the unmutated ssh-keygen -H vector', () => { + expect(parseKnownHostsLine(hashedLine(REAL_SALT, REAL_HASH))).toBeDefined() + }) + + // Extra padding is a b64_pton error too, so the key field is held to the same exact-encoding rule. + it('drops a key field with extra base64 padding', () => { + expect(parseKnownHostsLine(`example.com ssh-ed25519 ${ED}==`)).toBeUndefined() + }) + + // The parse result IS the trust decision: a junk-salt line previously produced a full `match`. + it('reports unknown for a junk-salt hashed line instead of matching it', () => { + expect(verdict(hashedLine(REAL_SALT.replace(/^(.{6})/, '$1@@'), REAL_HASH), { key: ED })).toBe( + 'unknown' + ) + expect(verdict(hashedLine(REAL_SALT, REAL_HASH), { key: ED })).toBe('match') + }) }) describe('agreement with a live OpenSSH client', () => { diff --git a/src/main/ssh/ssh-known-hosts.ts b/src/main/ssh/ssh-known-hosts.ts index 6f0765ae1a4..6545d615023 100644 --- a/src/main/ssh/ssh-known-hosts.ts +++ b/src/main/ssh/ssh-known-hosts.ts @@ -86,18 +86,29 @@ export function formatHostKeyFingerprint(sha256Base64: string): string { return `SHA256:${sha256Base64.replace(/=+$/, '')}` } -function decodeKey(raw: string): Buffer | undefined { - // Buffer.from never throws on bad base64 — it silently SKIPS invalid characters, so `!!!` - // and a blob with `@@` spliced into it both decode to the same correct key. ssh rejects those - // lines outright ("parse error in hostkeys file"), so accepting them grants trust from a line the - // user's own ssh ignores; `AAAA` is worse still, decoding to different bytes that still - // parse, which reads as a CHANGED key. Re-encoding and comparing is what makes us as strict. +/** + * The ONE way this file turns a base64 field into bytes. Every base64 field on a known_hosts line — + * key blob, hash salt, host hash — must come through here, so a field added later cannot quietly + * skip the rule. + * + * Why re-encode and compare: Buffer.from never throws on bad base64, it silently SKIPS invalid + * characters, so `!!!` and a field with `@@` spliced into it both decode to the same correct + * bytes. ssh rejects those lines outright ("parse error in hostkeys file", "salt decode error"), so + * accepting them grants trust from a line the user's own ssh ignores; `AAAA` is worse still, + * decoding to different bytes that still parse, which reads as a CHANGED key. + * + * The comparison is EXACT, padding included, which is what OpenSSH's b64_pton does: it rejects a + * missing `=`, a stray one, the base64url alphabet, and a final character whose leftover bits are + * non-zero. Verified live against OpenSSH 10.2p1 — each of those mutations on a real `ssh-keygen -H` + * salt makes ssh refuse to find the host at all. + */ +function decodeCanonicalBase64(raw: string): Buffer | undefined { const decoded = Buffer.from(raw, 'base64') + // Empty would re-encode to '' and pass the comparison; `|1||hash` must not survive as an entry. if (decoded.length === 0) { return undefined } - const canonical = decoded.toString('base64').replace(/=+$/, '') - return canonical === raw.replace(/=+$/, '') ? decoded : undefined + return decoded.toString('base64') === raw ? decoded : undefined } function parseHashedPatterns(field: string): KnownHostsEntry['hashed'] | undefined { @@ -106,12 +117,13 @@ function parseHashedPatterns(field: string): KnownHostsEntry['hashed'] | undefin if (parts.length !== 4 || parts[0] !== '' || parts[1] !== '1') { return undefined } - const salt = Buffer.from(parts[2] ?? '', 'base64') - const hash = Buffer.from(parts[3] ?? '', 'base64') - // ssh requires BOTH to be exactly one SHA1 digest — extract_salt rejects anything else with - // "expected salt len 20, got N". Accepting a shorter salt would let us match a line ssh treats as - // a parse error, so the entry would be invisible to the user's own ssh but trusted by us. - if (salt.length !== SHA1_DIGEST_BYTES || hash.length !== SHA1_DIGEST_BYTES) { + const salt = decodeCanonicalBase64(parts[2] ?? '') + const hash = decodeCanonicalBase64(parts[3] ?? '') + // Length is orthogonal to canonicality, so both checks are needed: ssh requires BOTH fields to be + // exactly one SHA1 digest — extract_salt rejects anything else with "expected salt len 20, got N". + // Accepting a shorter salt would let us match a line ssh treats as a parse error, so the entry + // would be invisible to the user's own ssh but trusted by us. + if (!salt || !hash || salt.length !== SHA1_DIGEST_BYTES || hash.length !== SHA1_DIGEST_BYTES) { return undefined } return { salt, hash } @@ -148,7 +160,7 @@ export function parseKnownHostsLine(line: string): KnownHostsEntry | undefined { return undefined } - const key = decodeKey(keyBase64) + const key = decodeCanonicalBase64(keyBase64) if (!key || readHostKeyType(key) !== keyType || !isWellFormedHostKeyBlob(key)) { return undefined }