Sitelet https://github.com/golang/crypto/commit/0471e7969e6740594dfe354646bf03e5e89de52d
Skip to content

Commit 0471e79

Browse files
drakkandrakkan
committed
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>
1 parent 6435c37 commit 0471e79

2 files changed

Lines changed: 117 additions & 6 deletions

File tree

‎ssh/agent/server.go‎

Lines changed: 47 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -304,19 +304,60 @@ func parseEd25519Key(req []byte) (*AddedKey, error) {
304304
return addedKey, nil
305305
}
306306

307+
func checkDSAParams(param *dsa.Parameters) error {
308+
// SSH specifies FIPS 186-2, which only provided a single size
309+
// (1024 bits) DSA key. FIPS 186-3 allows for larger key
310+
// sizes, which would confuse SSH.
311+
if l := param.P.BitLen(); l != 1024 {
312+
return fmt.Errorf("ssh: unsupported DSA key size %d", l)
313+
}
314+
315+
// FIPS 186-2 specifies that Q must be exactly 160 bits. We must enforce
316+
// this to prevent DoS attacks where an attacker sends a huge Q which makes
317+
// verification slow.
318+
if l := param.Q.BitLen(); l != 160 {
319+
return fmt.Errorf("ssh: unsupported DSA sub-prime size %d", l)
320+
}
321+
322+
// The generator G is an element of the group, so it must be strictly less
323+
// than the modulus P.
324+
if param.G.Cmp(param.P) >= 0 {
325+
return errors.New("ssh: DSA generator larger than modulus")
326+
}
327+
328+
// G must be positive.
329+
if param.G.Sign() <= 0 {
330+
return errors.New("ssh: DSA generator must be positive")
331+
}
332+
333+
return nil
334+
}
335+
307336
func parseDSAKey(req []byte) (*AddedKey, error) {
308337
var k dsaKeyMsg
309338
if err := ssh.Unmarshal(req, &k); err != nil {
310339
return nil, err
311340
}
341+
params := dsa.Parameters{
342+
P: k.P,
343+
Q: k.Q,
344+
G: k.G,
345+
}
346+
if err := checkDSAParams(&params); err != nil {
347+
return nil, err
348+
}
349+
350+
// The public value Y must be a non-zero element of the group, i.e. strictly
351+
// between 0 and P, to prevent a maliciously oversized Y from slowing
352+
// signature operations.
353+
if k.Y.Sign() <= 0 || k.Y.Cmp(k.P) >= 0 {
354+
return nil, errors.New("agent: DSA public value Y out of range")
355+
}
356+
312357
priv := &dsa.PrivateKey{
313358
PublicKey: dsa.PublicKey{
314-
Parameters: dsa.Parameters{
315-
P: k.P,
316-
Q: k.Q,
317-
G: k.G,
318-
},
319-
Y: k.Y,
359+
Parameters: params,
360+
Y: k.Y,
320361
},
321362
X: k.X,
322363
}

‎ssh/agent/server_test.go‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"crypto/rand"
1010
"fmt"
1111
"io"
12+
"math/big"
1213
pseudorand "math/rand"
1314
"reflect"
1415
"strings"
@@ -245,6 +246,75 @@ func addKeyToAgent(key crypto.PrivateKey) error {
245246
return verifyKey(sshAgent)
246247
}
247248

249+
func TestParseDSAKeyHugeQ(t *testing.T) {
250+
P := new(big.Int).Lsh(big.NewInt(1), 1023)
251+
Q := new(big.Int).Lsh(big.NewInt(1), 20000) // very large
252+
// G and Y: dummy values, just needs to be < P to pass that specific check.
253+
G := big.NewInt(2)
254+
Y := big.NewInt(5)
255+
256+
req := ssh.Marshal(dsaKeyMsg{
257+
Type: ssh.InsecureKeyAlgoDSA,
258+
P: P,
259+
Q: Q,
260+
G: G,
261+
Y: Y,
262+
X: big.NewInt(1),
263+
})
264+
265+
_, err := parseDSAKey(req)
266+
if err == nil {
267+
t.Fatal("parseDSAKey accepted a DSA key with large Q")
268+
}
269+
270+
expectedError := "unsupported DSA sub-prime size"
271+
if !strings.Contains(err.Error(), expectedError) {
272+
t.Errorf("unexpected error message: got %q, want substring %q", err.Error(), expectedError)
273+
}
274+
}
275+
276+
func TestParseDSAKeyYOutOfRange(t *testing.T) {
277+
// Valid 1024/160 parameters (values don't need to be a real DSA group,
278+
// they only need to pass the checkDSAParams bit-length checks and the
279+
// G < P / G > 0 checks).
280+
P := new(big.Int).Lsh(big.NewInt(1), 1023)
281+
P.SetBit(P, 0, 1) // make P odd so it can pass as a prime candidate shape
282+
Q := new(big.Int).Lsh(big.NewInt(1), 159)
283+
Q.SetBit(Q, 0, 1)
284+
G := big.NewInt(2)
285+
286+
for _, tc := range []struct {
287+
name string
288+
Y *big.Int
289+
}{
290+
{"Y_zero", big.NewInt(0)},
291+
{"Y_negative", big.NewInt(-1)},
292+
{"Y_equals_P", new(big.Int).Set(P)},
293+
{"Y_greater_than_P", new(big.Int).Add(P, big.NewInt(1))},
294+
{"Y_much_greater_than_P", new(big.Int).Lsh(big.NewInt(1), 20000)},
295+
} {
296+
t.Run(tc.name, func(t *testing.T) {
297+
req := ssh.Marshal(dsaKeyMsg{
298+
Type: ssh.InsecureKeyAlgoDSA,
299+
P: P,
300+
Q: Q,
301+
G: G,
302+
Y: tc.Y,
303+
X: big.NewInt(1),
304+
})
305+
306+
_, err := parseDSAKey(req)
307+
if err == nil {
308+
t.Fatalf("parseDSAKey accepted a DSA key with Y=%s (P=%s)", tc.Y, P)
309+
}
310+
expectedError := "DSA public value Y out of range"
311+
if !strings.Contains(err.Error(), expectedError) {
312+
t.Errorf("unexpected error message: got %q, want substring %q", err.Error(), expectedError)
313+
}
314+
})
315+
}
316+
}
317+
248318
func TestKeyTypes(t *testing.T) {
249319
for k, v := range testPrivateKeys {
250320
if err := addKeyToAgent(v); err != nil {

0 commit comments

Comments
 (0)