diff --git a/src/main/ssh/ssh-known-hosts.test.ts b/src/main/ssh/ssh-known-hosts.test.ts index 4db946dd486..ad4c49b3699 100644 --- a/src/main/ssh/ssh-known-hosts.test.ts +++ b/src/main/ssh/ssh-known-hosts.test.ts @@ -253,6 +253,60 @@ describe('matching a presented key', () => { * client's verdict recorded from its own output. Everything else in this file states what we believe * ssh does; these state what it did. */ +describe('lines ssh itself refuses to parse', () => { + const ED = 'AAAAC3NzaC1lZDI1NTE5AAAAIKqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqq' + + // Buffer.from SKIPS invalid base64 characters instead of failing, so each of these decodes to + // something. Verified live against OpenSSH 10.2p1 on 127.0.0.1:2224: the valid control reached + // authentication and ALL THREE of these produced "No ED25519 host key is known" — ssh drops the + // line. Honouring them means granting trust from a line the user's own ssh ignores, and for the + // padded variant it means raising a CHANGED alarm from one. + it.each([ + ['trailing junk', `${ED}!!!`], + ['invalid characters spliced into the middle', ED.replace(/^(.{20})/, '$1@@')] + ])('drops a key field with %s, as ssh does', (_label, keyField) => { + expect(parseKnownHostsLine(`example.com ssh-ed25519 ${keyField}`)).toBeUndefined() + }) + + // Still VALID base64 — 68 characters plus 4 is a legal length — and the algorithm header still + // reads ssh-ed25519, so neither the base64 check nor the header check catches it. ssh parses the + // whole key structure and drops the line; we decoded 54 bytes where the key is 51 and reported a + // CHANGED alarm from an entry ssh ignores. + it('drops a key blob with trailing bytes its own structure does not account for', () => { + expect(parseKnownHostsLine(`example.com ssh-ed25519 ${ED}AAAA`)).toBeUndefined() + }) + + it('drops a key blob whose own length prefix overruns the buffer', () => { + const overrun = Buffer.concat([ + Buffer.from([0, 0, 0, 11]), + Buffer.from('ssh-ed25519'), + Buffer.from([0, 0, 0, 99]) + ]) + expect( + parseKnownHostsLine(`example.com ssh-ed25519 ${overrun.toString('base64')}`) + ).toBeUndefined() + }) + + it('still accepts the unmodified key field', () => { + expect(parseKnownHostsLine(`example.com ssh-ed25519 ${ED}`)).toBeDefined() + }) + + // ssh's extract_salt demands exactly one SHA1 digest: "expected salt len 20, got 16". A shorter + // salt is still a usable HMAC key for us, so we could match a hand-crafted line that is invisible + // to the user's ssh. ssh-keygen -H always writes 20 bytes, so refusing loses no real entry. + it('drops a hashed entry whose salt is not a full SHA1 digest', () => { + const shortSalt = Buffer.alloc(16, 1).toString('base64') + const hash = Buffer.alloc(20, 2).toString('base64') + expect(parseKnownHostsLine(`|1|${shortSalt}|${hash} ssh-ed25519 ${ED}`)).toBeUndefined() + }) + + it('accepts a hashed entry whose salt is a full SHA1 digest', () => { + const salt = Buffer.alloc(20, 1).toString('base64') + const hash = Buffer.alloc(20, 2).toString('base64') + expect(parseKnownHostsLine(`|1|${salt}|${hash} ssh-ed25519 ${ED}`)).toBeDefined() + }) +}) + describe('agreement with a live OpenSSH client', () => { const SERVER_KEY = 'AAAAC3NzaC1lZDI1NTE5AAAAIM2eSSqUqU9LERdg8qNjFiU59unM+JyfwFHLkMxR13oq' const OTHER_KEY = 'AAAAC3NzaC1lZDI1NTE5AAAAIKmVZ4Z+MJ3VYnZmZmZmZmZmZmZmZmZmZmZmZmZmZmZm' diff --git a/src/main/ssh/ssh-known-hosts.ts b/src/main/ssh/ssh-known-hosts.ts index c912b179528..bcb4430fd06 100644 --- a/src/main/ssh/ssh-known-hosts.ts +++ b/src/main/ssh/ssh-known-hosts.ts @@ -52,15 +52,52 @@ export function readHostKeyType(key: Buffer): string | undefined { return key.subarray(4, 4 + length).toString('utf8') } +/** + * Whether the blob is exactly consumed by its own SSH-wire structure. + * + * A public key blob is a run of length-prefixed fields — name, then the algorithm's parameters — and + * nothing else. Checking only the algorithm header leaves trailing bytes undetected, and ssh parses + * the whole structure: verified live against OpenSSH 10.2p1, a valid ed25519 key with four extra + * base64 characters appended is still valid base64 and still reports `ssh-ed25519`, but ssh reports + * "No ED25519 host key is known" and drops the line, where we decoded 54 bytes instead of 51 and + * raised a CHANGED alarm from an entry the user's own ssh ignores. + * + * Deliberately algorithm-agnostic: the field walk is the same for every key type, so a type we do + * not model is checked as well as one we do. + */ +function isWellFormedHostKeyBlob(key: Buffer): boolean { + let offset = 0 + while (offset < key.length) { + if (offset + 4 > key.length) { + return false + } + const fieldLength = key.readUInt32BE(offset) + offset += 4 + if (fieldLength > key.length - offset) { + return false + } + offset += fieldLength + } + return offset === key.length +} + /** `SHA256:...` exactly as `ssh-keygen -lf` prints it, base64 with padding stripped. */ export function formatHostKeyFingerprint(sha256Base64: string): string { return `SHA256:${sha256Base64.replace(/=+$/, '')}` } function decodeKey(raw: string): Buffer | undefined { - // Why the round-trip check: Buffer.from never throws on bad base64, it silently truncates. + // 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. const decoded = Buffer.from(raw, 'base64') - return decoded.length > 0 ? decoded : undefined + if (decoded.length === 0) { + return undefined + } + const canonical = decoded.toString('base64').replace(/=+$/, '') + return canonical === raw.replace(/=+$/, '') ? decoded : undefined } function parseHashedPatterns(field: string): KnownHostsEntry['hashed'] | undefined { @@ -71,7 +108,10 @@ function parseHashedPatterns(field: string): KnownHostsEntry['hashed'] | undefin } const salt = Buffer.from(parts[2] ?? '', 'base64') const hash = Buffer.from(parts[3] ?? '', 'base64') - if (salt.length === 0 || hash.length !== SHA1_DIGEST_BYTES) { + // 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) { return undefined } return { salt, hash } @@ -109,7 +149,7 @@ export function parseKnownHostsLine(line: string): KnownHostsEntry | undefined { } const key = decodeKey(keyBase64) - if (!key || readHostKeyType(key) !== keyType) { + if (!key || readHostKeyType(key) !== keyType || !isWellFormedHostKeyBlob(key)) { return undefined }