From 0471e7969e6740594dfe354646bf03e5e89de52d Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Mon, 29 Jun 2026 19:45:55 +0200 Subject: [PATCH] ssh/agent: enforce strict limits on DSA key parameters The parseDSAKey function constructed a *dsa.PrivateKey directly from the add-identity request without validating the key parameters. Unlike DSA certificates, whose parameters are checked by the ssh package when the certificate's public key is parsed, raw DSA keys added to the agent were not validated at all. Align the raw DSA key parsing with the validation already performed by the main ssh package. Fixes golang/go#79725 Change-Id: I537cc2175d35c19848c90c68739cf94ba7b50e10 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/795422 Reviewed-by: Roland Shoemaker Reviewed-by: Junyang Shao LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com --- ssh/agent/server.go | 53 ++++++++++++++++++++++++++---- ssh/agent/server_test.go | 70 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+), 6 deletions(-) diff --git a/ssh/agent/server.go b/ssh/agent/server.go index f05d22f..782c54e 100644 --- a/ssh/agent/server.go +++ b/ssh/agent/server.go @@ -304,19 +304,60 @@ func parseEd25519Key(req []byte) (*AddedKey, error) { return addedKey, nil } +func checkDSAParams(param *dsa.Parameters) error { + // SSH specifies FIPS 186-2, which only provided a single size + // (1024 bits) DSA key. FIPS 186-3 allows for larger key + // sizes, which would confuse SSH. + if l := param.P.BitLen(); l != 1024 { + return fmt.Errorf("ssh: unsupported DSA key size %d", l) + } + + // FIPS 186-2 specifies that Q must be exactly 160 bits. We must enforce + // this to prevent DoS attacks where an attacker sends a huge Q which makes + // verification slow. + if l := param.Q.BitLen(); l != 160 { + return fmt.Errorf("ssh: unsupported DSA sub-prime size %d", l) + } + + // The generator G is an element of the group, so it must be strictly less + // than the modulus P. + if param.G.Cmp(param.P) >= 0 { + return errors.New("ssh: DSA generator larger than modulus") + } + + // G must be positive. + if param.G.Sign() <= 0 { + return errors.New("ssh: DSA generator must be positive") + } + + return nil +} + func parseDSAKey(req []byte) (*AddedKey, error) { var k dsaKeyMsg if err := ssh.Unmarshal(req, &k); err != nil { return nil, err } + params := dsa.Parameters{ + P: k.P, + Q: k.Q, + G: k.G, + } + if err := checkDSAParams(¶ms); err != nil { + return nil, err + } + + // The public value Y must be a non-zero element of the group, i.e. strictly + // between 0 and P, to prevent a maliciously oversized Y from slowing + // signature operations. + if k.Y.Sign() <= 0 || k.Y.Cmp(k.P) >= 0 { + return nil, errors.New("agent: DSA public value Y out of range") + } + priv := &dsa.PrivateKey{ PublicKey: dsa.PublicKey{ - Parameters: dsa.Parameters{ - P: k.P, - Q: k.Q, - G: k.G, - }, - Y: k.Y, + Parameters: params, + Y: k.Y, }, X: k.X, } diff --git a/ssh/agent/server_test.go b/ssh/agent/server_test.go index f57e039..be6ba15 100644 --- a/ssh/agent/server_test.go +++ b/ssh/agent/server_test.go @@ -9,6 +9,7 @@ import ( "crypto/rand" "fmt" "io" + "math/big" pseudorand "math/rand" "reflect" "strings" @@ -245,6 +246,75 @@ func addKeyToAgent(key crypto.PrivateKey) error { return verifyKey(sshAgent) } +func TestParseDSAKeyHugeQ(t *testing.T) { + P := new(big.Int).Lsh(big.NewInt(1), 1023) + Q := new(big.Int).Lsh(big.NewInt(1), 20000) // very large + // G and Y: dummy values, just needs to be < P to pass that specific check. + G := big.NewInt(2) + Y := big.NewInt(5) + + req := ssh.Marshal(dsaKeyMsg{ + Type: ssh.InsecureKeyAlgoDSA, + P: P, + Q: Q, + G: G, + Y: Y, + X: big.NewInt(1), + }) + + _, err := parseDSAKey(req) + if err == nil { + t.Fatal("parseDSAKey accepted a DSA key with large Q") + } + + expectedError := "unsupported DSA sub-prime size" + if !strings.Contains(err.Error(), expectedError) { + t.Errorf("unexpected error message: got %q, want substring %q", err.Error(), expectedError) + } +} + +func TestParseDSAKeyYOutOfRange(t *testing.T) { + // Valid 1024/160 parameters (values don't need to be a real DSA group, + // they only need to pass the checkDSAParams bit-length checks and the + // G < P / G > 0 checks). + P := new(big.Int).Lsh(big.NewInt(1), 1023) + P.SetBit(P, 0, 1) // make P odd so it can pass as a prime candidate shape + Q := new(big.Int).Lsh(big.NewInt(1), 159) + Q.SetBit(Q, 0, 1) + G := big.NewInt(2) + + for _, tc := range []struct { + name string + Y *big.Int + }{ + {"Y_zero", big.NewInt(0)}, + {"Y_negative", big.NewInt(-1)}, + {"Y_equals_P", new(big.Int).Set(P)}, + {"Y_greater_than_P", new(big.Int).Add(P, big.NewInt(1))}, + {"Y_much_greater_than_P", new(big.Int).Lsh(big.NewInt(1), 20000)}, + } { + t.Run(tc.name, func(t *testing.T) { + req := ssh.Marshal(dsaKeyMsg{ + Type: ssh.InsecureKeyAlgoDSA, + P: P, + Q: Q, + G: G, + Y: tc.Y, + X: big.NewInt(1), + }) + + _, err := parseDSAKey(req) + if err == nil { + t.Fatalf("parseDSAKey accepted a DSA key with Y=%s (P=%s)", tc.Y, P) + } + expectedError := "DSA public value Y out of range" + if !strings.Contains(err.Error(), expectedError) { + t.Errorf("unexpected error message: got %q, want substring %q", err.Error(), expectedError) + } + }) + } +} + func TestKeyTypes(t *testing.T) { for k, v := range testPrivateKeys { if err := addKeyToAgent(v); err != nil {