From beb2a9779c3b677077c41673505f150149fce895 Mon Sep 17 00:00:00 2001 From: Han-Wen Nienhuys Date: Thu, 5 Apr 2018 16:16:06 +0200 Subject: [PATCH] ssh/knownhosts: disregard IP address if the hostname is available This fixes the following vulnerability scenario: * Victim logs into SAFE-HOST on SAFE-IP-ADDRESS regularly. * Victim is cajoled into connecting to attacker controlled ATTACK-HOST, on ATTACK-IP-ADDRESS. ATTACK-HOST uses a different host key type (e.g. Ed25519 vs RSA). The new key is added at the end of known_hosts. * Attacker makes DNS system return ATTACK-IP-ADDRESS for SAFE-HOST. * Victim logs into SAFE-HOST, but is not warned because the host key matches ATTACK-IP-ADDRESS. For this attack to work, the key type has to be different, because knownhosts gives precedence to the first key found for each type. Add a test that asserts this behavior. The new semantics simplify the code, but callers that modify .ssh/known_host interactviely must now take an extra step to remain OpenSSH compatible: on successful login, the IP address must be checked without hostname, and if it is not known, added separately to the known_hosts file, so future logins that use an IP address only will be protected too. Thanks to Daniel Parks for finding this vulnerability. Change-Id: I62b1b60ceb02e2f583a4657213feac1a8885dd42 Reviewed-on: https://go-review.googlesource.com/104939 Reviewed-by: Adam Langley Run-TryBot: Han-Wen Nienhuys TryBot-Result: Gobot Gobot --- ssh/knownhosts/knownhosts.go | 58 ++++++++++++++----------------- ssh/knownhosts/knownhosts_test.go | 29 +++++++++++++++- 2 files changed, 54 insertions(+), 33 deletions(-) diff --git a/ssh/knownhosts/knownhosts.go b/ssh/knownhosts/knownhosts.go index 46dad14..bc3db73 100644 --- a/ssh/knownhosts/knownhosts.go +++ b/ssh/knownhosts/knownhosts.go @@ -2,8 +2,9 @@ // Use of this source code is governed by a BSD-style // license that can be found in the LICENSE file. -// Package knownhosts implements a parser for the OpenSSH -// known_hosts host key database. +// Package knownhosts implements a parser for the OpenSSH known_hosts +// host key database, and provides utility functions for writing +// OpenSSH compliant known_hosts files. package knownhosts import ( @@ -38,7 +39,7 @@ func (a *addr) String() string { } type matcher interface { - match([]addr) bool + match(addr) bool } type hostPattern struct { @@ -57,19 +58,16 @@ func (p *hostPattern) String() string { type hostPatterns []hostPattern -func (ps hostPatterns) match(addrs []addr) bool { +func (ps hostPatterns) match(a addr) bool { matched := false for _, p := range ps { - for _, a := range addrs { - m := p.match(a) - if !m { - continue - } - if p.negate { - return false - } - matched = true + if !p.match(a) { + continue } + if p.negate { + return false + } + matched = true } return matched } @@ -122,8 +120,8 @@ func serialize(k ssh.PublicKey) string { return k.Type() + " " + base64.StdEncoding.EncodeToString(k.Marshal()) } -func (l *keyDBLine) match(addrs []addr) bool { - return l.matcher.match(addrs) +func (l *keyDBLine) match(a addr) bool { + return l.matcher.match(a) } type hostKeyDB struct { @@ -153,7 +151,7 @@ func (db *hostKeyDB) IsHostAuthority(remote ssh.PublicKey, address string) bool a := addr{host: h, port: p} for _, l := range db.lines { - if l.cert && keyEq(l.knownKey.Key, remote) && l.match([]addr{a}) { + if l.cert && keyEq(l.knownKey.Key, remote) && l.match(a) { return true } } @@ -338,26 +336,24 @@ func (db *hostKeyDB) check(address string, remote net.Addr, remoteKey ssh.Public return fmt.Errorf("knownhosts: SplitHostPort(%s): %v", remote, err) } - addrs := []addr{ - {host, port}, - } - + hostToCheck := addr{host, port} if address != "" { + // Give preference to the hostname if available. host, port, err := net.SplitHostPort(address) if err != nil { return fmt.Errorf("knownhosts: SplitHostPort(%s): %v", address, err) } - addrs = append(addrs, addr{host, port}) + hostToCheck = addr{host, port} } - return db.checkAddrs(addrs, remoteKey) + return db.checkAddr(hostToCheck, remoteKey) } // checkAddrs checks if we can find the given public key for any of // the given addresses. If we only find an entry for the IP address, // or only the hostname, then this still succeeds. -func (db *hostKeyDB) checkAddrs(addrs []addr, remoteKey ssh.PublicKey) error { +func (db *hostKeyDB) checkAddr(a addr, remoteKey ssh.PublicKey) error { // TODO(hanwen): are these the right semantics? What if there // is just a key for the IP address, but not for the // hostname? @@ -365,7 +361,7 @@ func (db *hostKeyDB) checkAddrs(addrs []addr, remoteKey ssh.PublicKey) error { // Algorithm => key. knownKeys := map[string]KnownKey{} for _, l := range db.lines { - if l.match(addrs) { + if l.match(a) { typ := l.knownKey.Key.Type() if _, ok := knownKeys[typ]; !ok { knownKeys[typ] = l.knownKey @@ -414,7 +410,10 @@ func (db *hostKeyDB) Read(r io.Reader, filename string) error { // New creates a host key callback from the given OpenSSH host key // files. The returned callback is for use in -// ssh.ClientConfig.HostKeyCallback. +// ssh.ClientConfig.HostKeyCallback. By preference, the key check +// operates on the hostname if available, i.e. if a server changes its +// IP address, the host key check will still succeed, even though a +// record of the new IP address is not available. func New(files ...string) (ssh.HostKeyCallback, error) { db := newHostKeyDB() for _, fn := range files { @@ -536,11 +535,6 @@ func newHashedHost(encoded string) (*hashedHost, error) { return &hashedHost{salt: salt, hash: hash}, nil } -func (h *hashedHost) match(addrs []addr) bool { - for _, a := range addrs { - if bytes.Equal(hashHost(Normalize(a.String()), h.salt), h.hash) { - return true - } - } - return false +func (h *hashedHost) match(a addr) bool { + return bytes.Equal(hashHost(Normalize(a.String()), h.salt), h.hash) } diff --git a/ssh/knownhosts/knownhosts_test.go b/ssh/knownhosts/knownhosts_test.go index be7cc0e..464dd59 100644 --- a/ssh/knownhosts/knownhosts_test.go +++ b/ssh/knownhosts/knownhosts_test.go @@ -166,7 +166,7 @@ func TestBasic(t *testing.T) { str := fmt.Sprintf("#comment\n\nserver.org,%s %s\notherhost %s", testAddr, edKeyStr, ecKeyStr) db := testDB(t, str) if err := db.check("server.org:22", testAddr, edKey); err != nil { - t.Errorf("got error %q, want none", err) + t.Errorf("got error %v, want none", err) } want := KnownKey{ @@ -185,6 +185,33 @@ func TestBasic(t *testing.T) { } } +func TestHostNamePrecedence(t *testing.T) { + var evilAddr = &net.TCPAddr{ + IP: net.IP{66, 66, 66, 66}, + Port: 22, + } + + str := fmt.Sprintf("server.org,%s %s\nevil.org,%s %s", testAddr, edKeyStr, evilAddr, ecKeyStr) + db := testDB(t, str) + + if err := db.check("server.org:22", evilAddr, ecKey); err == nil { + t.Errorf("check succeeded") + } else if _, ok := err.(*KeyError); !ok { + t.Errorf("got %T, want *KeyError", err) + } +} + +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)