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 <roland@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>

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(&params); 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,
 	}

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 {