fix(ssh): validate hashed known_hosts fields through one strict base64 decoder (STA-4717) (#15345)

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', () => {

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 `<valid>!!!`
-  // 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; `<valid>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 `<valid>!!!` 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; `<valid>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
   }