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 {