From 6b853fbea37a941d918ac0760a5492802df42b9b Mon Sep 17 00:00:00 2001 From: Carlos A Becker Date: Tue, 4 Mar 2025 14:03:42 +0000 Subject: ssh/knownhosts: check more than one key I believe this fixes https://github.com/golang/go/issues/36126 . The problem was that it was keeping only the first known key of each type found. If you have a server advertising multiple keys of the same type, you might get a missmatch key error. Per sshd(8) man page, it should allow reapeatable hosts with different host keys, although it don't specify anything about hosts being from different types: "It is permissible (but not recommended) to have several lines or different host keys for the same names. This will inevitably happen when short forms of host names from different domains are put in the file. It is possible that the files contain conflicting information; authentication is accepted if valid information can be found from either file." So, this changes knownhosts behavior to accept any of the keys for a given host, regardless of type. Fixes #36126 Change-Id: I3450ff954259a403f2471082d013a5f79def0e16 GitHub-Last-Rev: 361bd2bcd20348956aaf114ef159a5350397eaf4 GitHub-Pull-Request: golang/crypto#254 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/478535 Reviewed-by: Junyang Shao LUCI-TryBot-Result: Go LUCI Reviewed-by: Nicola Murino Reviewed-by: Michael Pratt Auto-Submit: Nicola Murino --- ssh/knownhosts/knownhosts.go | 36 +++++++++++------------------------- ssh/knownhosts/knownhosts_test.go | 24 +++++++++++++----------- 2 files changed, 24 insertions(+), 36 deletions(-) diff --git a/ssh/knownhosts/knownhosts.go b/ssh/knownhosts/knownhosts.go index 7376a8d..c022e41 100644 --- a/ssh/knownhosts/knownhosts.go +++ b/ssh/knownhosts/knownhosts.go @@ -302,8 +302,8 @@ func (k *KnownKey) String() string { // applications can offer an interactive prompt to the user. type KeyError struct { // Want holds the accepted host keys. For each key algorithm, - // there can be one hostkey. If Want is empty, the host is - // unknown. If Want is non-empty, there was a mismatch, which + // there can be multiple hostkeys. If Want is empty, the host + // is unknown. If Want is non-empty, there was a mismatch, which // can signify a MITM attack. Want []KnownKey } @@ -358,34 +358,20 @@ func (db *hostKeyDB) checkAddr(a addr, remoteKey ssh.PublicKey) error { // is just a key for the IP address, but not for the // hostname? - // Algorithm => key. - knownKeys := map[string]KnownKey{} - for _, l := range db.lines { - if l.match(a) { - typ := l.knownKey.Key.Type() - if _, ok := knownKeys[typ]; !ok { - knownKeys[typ] = l.knownKey - } - } - } - keyErr := &KeyError{} - for _, v := range knownKeys { - keyErr.Want = append(keyErr.Want, v) - } - // Unknown remote host. - if len(knownKeys) == 0 { - return keyErr - } + for _, l := range db.lines { + if !l.match(a) { + continue + } - // If the remote host starts using a different, unknown key type, we - // also interpret that as a mismatch. - if known, ok := knownKeys[remoteKey.Type()]; !ok || !keyEq(known.Key, remoteKey) { - return keyErr + keyErr.Want = append(keyErr.Want, l.knownKey) + if keyEq(l.knownKey.Key, remoteKey) { + return nil + } } - return nil + return keyErr } // The Read function parses file contents. diff --git a/ssh/knownhosts/knownhosts_test.go b/ssh/knownhosts/knownhosts_test.go index 464dd59..156576a 100644 --- a/ssh/knownhosts/knownhosts_test.go +++ b/ssh/knownhosts/knownhosts_test.go @@ -201,17 +201,6 @@ func TestHostNamePrecedence(t *testing.T) { } } -func TestDBOrderingPrecedenceKeyType(t *testing.T) { - str := fmt.Sprintf("server.org,%s %s\nserver.org,%s %s", testAddr, edKeyStr, testAddr, alternateEdKeyStr) - db := testDB(t, str) - - if err := db.check("server.org:22", testAddr, alternateEdKey); err == nil { - t.Errorf("check succeeded") - } else if _, ok := err.(*KeyError); !ok { - t.Errorf("got %T, want *KeyError", err) - } -} - func TestNegate(t *testing.T) { str := fmt.Sprintf("%s,!server.org %s", testAddr, edKeyStr) db := testDB(t, str) @@ -354,3 +343,16 @@ func TestHashedHostkeyCheck(t *testing.T) { t.Errorf("got error %v, want %v", got, want) } } + +func TestIssue36126(t *testing.T) { + str := fmt.Sprintf("server.org,%s %s\nserver.org,%s %s", testAddr, edKeyStr, testAddr, alternateEdKeyStr) + db := testDB(t, str) + + if err := db.check("server.org:22", testAddr, edKey); err != nil { + t.Errorf("should have passed the check, got %v", err) + } + + if err := db.check("server.org:22", testAddr, alternateEdKey); err != nil { + t.Errorf("should have passed the check, got %v", err) + } +} -- cgit v1.3