ssh/agent: validate ed25519 private key length in Add insertKey and insertCert assume an ed25519.PrivateKey is exactly 64 bytes long: a 32-byte seed followed by a 32-byte public key. The expression []byte(k)[32:] panics with "slice bounds out of range" when a shorter slice is passed, which is reachable through the exported Add API and crashes the caller process. Reject ed25519 keys whose length is not ed25519.PrivateKeySize in all four branches (value and pointer variants of insertKey and insertCert), using the same error wording already used by parseEd25519Cert in server.go. This issue was found during a security audit by NCC Group Cryptography Services, sponsored by Teleport, and was assessed and is being fixed as a non-security bug. Change-Id: I4a053835588aad4c3482fe1ca8045542cd960500 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/782423 Reviewed-by: David Chase <drchase@google.com> Reviewed-by: Filippo Valsorda <filippo@golang.org> Reviewed-by: Junyang Shao <shaojunyang@google.com> LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
diff --git a/ssh/agent/client.go b/ssh/agent/client.go index 4276002..eb96184 100644 --- a/ssh/agent/client.go +++ b/ssh/agent/client.go
@@ -621,6 +621,9 @@ Constraints: constraints, }) case ed25519.PrivateKey: + if len(k) != ed25519.PrivateKeySize { + return fmt.Errorf("agent: bad ED25519 key size: %d", len(k)) + } req = ssh.Marshal(ed25519KeyMsg{ Type: ssh.KeyAlgoED25519, Pub: []byte(k)[32:], @@ -632,6 +635,9 @@ // general idiom is to pass ed25519.PrivateKey by value, not by pointer. // We still support the pointer variant for backwards compatibility. case *ed25519.PrivateKey: + if len(*k) != ed25519.PrivateKeySize { + return fmt.Errorf("agent: bad ED25519 key size: %d", len(*k)) + } req = ssh.Marshal(ed25519KeyMsg{ Type: ssh.KeyAlgoED25519, Pub: []byte(*k)[32:], @@ -756,6 +762,9 @@ Constraints: constraints, }) case ed25519.PrivateKey: + if len(k) != ed25519.PrivateKeySize { + return fmt.Errorf("agent: bad ED25519 key size: %d", len(k)) + } req = ssh.Marshal(ed25519CertMsg{ Type: cert.Type(), CertBytes: cert.Marshal(), @@ -768,6 +777,9 @@ // general idiom is to pass ed25519.PrivateKey by value, not by pointer. // We still support the pointer variant for backwards compatibility. case *ed25519.PrivateKey: + if len(*k) != ed25519.PrivateKeySize { + return fmt.Errorf("agent: bad ED25519 key size: %d", len(*k)) + } req = ssh.Marshal(ed25519CertMsg{ Type: cert.Type(), CertBytes: cert.Marshal(),
diff --git a/ssh/agent/client_test.go b/ssh/agent/client_test.go index 567c0d9..3b925c3 100644 --- a/ssh/agent/client_test.go +++ b/ssh/agent/client_test.go
@@ -6,6 +6,7 @@ import ( "bytes" + "crypto/ed25519" "crypto/rand" "encoding/binary" "errors" @@ -654,3 +655,36 @@ } } } + +func TestAddRejectsShortEd25519Key(t *testing.T) { + short := ed25519.PrivateKey(make([]byte, 16)) + // insertCert reaches its ed25519 branch only when AddedKey.Certificate + // is non-nil. The length check returns before cert.Type/Marshal run, so + // an empty certificate is sufficient to drive the path. + emptyCert := &ssh.Certificate{} + + cases := []struct { + name string + add AddedKey + }{ + {"insertKey value", AddedKey{PrivateKey: short}}, + {"insertKey pointer", AddedKey{PrivateKey: &short}}, + {"insertCert value", AddedKey{PrivateKey: short, Certificate: emptyCert}}, + {"insertCert pointer", AddedKey{PrivateKey: &short, Certificate: emptyCert}}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + client, cleanup := startKeyringAgent(t) + defer cleanup() + + err := client.Add(tc.add) + if err == nil { + t.Fatal("Add accepted ed25519 key shorter than 64 bytes") + } + if !strings.Contains(err.Error(), "bad ED25519 key size") { + t.Errorf("got error %q, want substring %q", err.Error(), "bad ED25519 key size") + } + }) + } +}