Skip to content

Commit b4f1988

Browse files
rolandshoemakergopherbot
authored andcommitted
ssh: make the public key cache a 1-entry FIFO cache
Users of the the ssh package seem to extremely commonly misuse the PublicKeyCallback API, assuming that the key passed in the last call before a connection is established is the key used for authentication. Some users then make authorization decisions based on this key. This property is not documented, and may not be correct, due to the caching behavior of the package, resulting in users making incorrect authorization decisions about the connection. This change makes the cache a one entry FIFO cache, making the assumed property, that the last call to PublicKeyCallback represents the key actually used for authentication, actually hold. Thanks to Damien Tournoud, Patrick Dawkins, Vince Parker, and Jules Duvivier from the Platform.sh / Upsun engineering team for reporting this issue. Fixes golang/go#70779 Fixes CVE-2024-45337 Change-Id: Ife7c7b4045d8b6bcd7e3a417bdfae370c709797f Reviewed-on: https://go-review.googlesource.com/c/crypto/+/635315 Reviewed-by: Roland Shoemaker <[email protected]> Auto-Submit: Gopher Robot <[email protected]> Reviewed-by: Damien Neil <[email protected]> Reviewed-by: Nicola Murino <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
1 parent 7042ebc commit b4f1988

File tree

2 files changed

+60
-4
lines changed

2 files changed

+60
-4
lines changed

ssh/server.go

+11-4
Original file line numberDiff line numberDiff line change
@@ -149,15 +149,21 @@ func (s *ServerConfig) AddHostKey(key Signer) {
149149
}
150150

151151
// cachedPubKey contains the results of querying whether a public key is
152-
// acceptable for a user.
152+
// acceptable for a user. This is a FIFO cache.
153153
type cachedPubKey struct {
154154
user string
155155
pubKeyData []byte
156156
result error
157157
perms *Permissions
158158
}
159159

160-
const maxCachedPubKeys = 16
160+
// maxCachedPubKeys is the number of cache entries we store.
161+
//
162+
// Due to consistent misuse of the PublicKeyCallback API, we have reduced this
163+
// to 1, such that the only key in the cache is the most recently seen one. This
164+
// forces the behavior that the last call to PublicKeyCallback will always be
165+
// with the key that is used for authentication.
166+
const maxCachedPubKeys = 1
161167

162168
// pubKeyCache caches tests for public keys. Since SSH clients
163169
// will query whether a public key is acceptable before attempting to
@@ -179,9 +185,10 @@ func (c *pubKeyCache) get(user string, pubKeyData []byte) (cachedPubKey, bool) {
179185

180186
// add adds the given tuple to the cache.
181187
func (c *pubKeyCache) add(candidate cachedPubKey) {
182-
if len(c.keys) < maxCachedPubKeys {
183-
c.keys = append(c.keys, candidate)
188+
if len(c.keys) >= maxCachedPubKeys {
189+
c.keys = c.keys[1:]
184190
}
191+
c.keys = append(c.keys, candidate)
185192
}
186193

187194
// ServerConn is an authenticated SSH connection, as seen from the

ssh/server_test.go

+49
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
package ssh
66

77
import (
8+
"bytes"
89
"errors"
910
"fmt"
1011
"io"
@@ -299,6 +300,54 @@ func TestBannerError(t *testing.T) {
299300
}
300301
}
301302

303+
func TestPublicKeyCallbackLastSeen(t *testing.T) {
304+
var lastSeenKey PublicKey
305+
306+
c1, c2, err := netPipe()
307+
if err != nil {
308+
t.Fatalf("netPipe: %v", err)
309+
}
310+
defer c1.Close()
311+
defer c2.Close()
312+
serverConf := &ServerConfig{
313+
PublicKeyCallback: func(conn ConnMetadata, key PublicKey) (*Permissions, error) {
314+
lastSeenKey = key
315+
fmt.Printf("seen %#v\n", key)
316+
if _, ok := key.(*dsaPublicKey); !ok {
317+
return nil, errors.New("nope")
318+
}
319+
return nil, nil
320+
},
321+
}
322+
serverConf.AddHostKey(testSigners["ecdsap256"])
323+
324+
done := make(chan struct{})
325+
go func() {
326+
defer close(done)
327+
NewServerConn(c1, serverConf)
328+
}()
329+
330+
clientConf := ClientConfig{
331+
User: "user",
332+
Auth: []AuthMethod{
333+
PublicKeys(testSigners["rsa"], testSigners["dsa"], testSigners["ed25519"]),
334+
},
335+
HostKeyCallback: InsecureIgnoreHostKey(),
336+
}
337+
338+
_, _, _, err = NewClientConn(c2, "", &clientConf)
339+
if err != nil {
340+
t.Fatal(err)
341+
}
342+
<-done
343+
344+
expectedPublicKey := testSigners["dsa"].PublicKey().Marshal()
345+
lastSeenMarshalled := lastSeenKey.Marshal()
346+
if !bytes.Equal(lastSeenMarshalled, expectedPublicKey) {
347+
t.Errorf("unexpected key: got %#v, want %#v", lastSeenKey, testSigners["dsa"].PublicKey())
348+
}
349+
}
350+
302351
type markerConn struct {
303352
closed uint32
304353
used uint32

0 commit comments

Comments
 (0)