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 <security@demonhorse.net> for finding this
vulnerability.

Change-Id: I62b1b60ceb02e2f583a4657213feac1a8885dd42
Reviewed-on: https://go-review.googlesource.com/104939
Reviewed-by: Adam Langley <agl@golang.org>
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>

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)
 }

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)