ssh: validate ECDSA curve matches expected algorithm

Previously, parseECDSA determined the curve purely based on the key
blob content, ignoring the algorithm identifier passed to parsePubKey.

This allowed a mismatch where a key could be encoded with an algorithm
type of "ecdsa-sha2-nistp256" but contain a NIST P-384 or P-521 curve.
The parser would succeed, returning a key with a type different from
the one indicated by the caller/wire format.

This change updates parseECDSA to accept the expected algorithm type
and verify that it matches the curve specified in the key data. This
matches the behavior of OpenSSH's ssh_ecdsa_deserialize_public in
ssh-ecdsa.c, which rejects a curve identifier that does not correspond
to the key algorithm name.

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: I9c748be948cca65e2f41089bb7510466d3bb316a
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/782425
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Junyang Shao <shaojunyang@google.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>

ssh/keys.go

diff --git a/ssh/keys.go b/ssh/keys.go
index 5deada7..334861b 100644
--- a/ssh/keys.go
+++ b/ssh/keys.go
@@ -76,7 +76,7 @@
 	case InsecureKeyAlgoDSA:
 		return parseDSA(in)
 	case KeyAlgoECDSA256, KeyAlgoECDSA384, KeyAlgoECDSA521:
-		return parseECDSA(in)
+		return parseECDSA(in, algo)
 	case KeyAlgoSKECDSA256:
 		return parseSKECDSA(in)
 	case KeyAlgoED25519:
@@ -806,7 +806,7 @@
 }
 
 // parseECDSA parses an ECDSA key according to RFC 5656, section 3.1.
-func parseECDSA(in []byte) (out PublicKey, rest []byte, err error) {
+func parseECDSA(in []byte, expectedType string) (out PublicKey, rest []byte, err error) {
 	var w struct {
 		Curve    string
 		KeyBytes []byte
@@ -817,6 +817,12 @@
 		return nil, nil, err
 	}
 
+	actualType := "ecdsa-sha2-" + w.Curve
+	if expectedType != actualType {
+		return nil, nil, fmt.Errorf("ssh: algorithm type mismatch: expected %q, found curve %q (type %q)",
+			expectedType, w.Curve, actualType)
+	}
+
 	key := new(ecdsa.PublicKey)
 
 	switch w.Curve {

ssh/keys_test.go

diff --git a/ssh/keys_test.go b/ssh/keys_test.go
index 9bd90a0..62ba549 100644
--- a/ssh/keys_test.go
+++ b/ssh/keys_test.go
@@ -1461,3 +1461,39 @@
 		t.Fatal("parsing an SSH certificate using another certificate as signature key succeeded; expected failure")
 	}
 }
+
+func TestParseECDSAAlgorithmMismatch(t *testing.T) {
+	cases := []struct {
+		keyName    string // key fixture in testPublicKeys
+		nativeAlgo string // algorithm actually carried in the key blob
+		askedAlgo  string // algorithm passed to parsePubKey
+	}{
+		{"ecdsap256", "ecdsa-sha2-nistp256", "ecdsa-sha2-nistp384"},
+		{"ecdsap256", "ecdsa-sha2-nistp256", "ecdsa-sha2-nistp521"},
+		{"ecdsap384", "ecdsa-sha2-nistp384", "ecdsa-sha2-nistp256"},
+		{"ecdsap384", "ecdsa-sha2-nistp384", "ecdsa-sha2-nistp521"},
+		{"ecdsap521", "ecdsa-sha2-nistp521", "ecdsa-sha2-nistp256"},
+		{"ecdsap521", "ecdsa-sha2-nistp521", "ecdsa-sha2-nistp384"},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.nativeAlgo+"_as_"+tc.askedAlgo, func(t *testing.T) {
+			pubKey := testPublicKeys[tc.keyName]
+			algo, in, ok := parseString(pubKey.Marshal())
+			if !ok {
+				t.Fatal("unable to parse public key wire format")
+			}
+			if string(algo) != tc.nativeAlgo {
+				t.Fatalf("test setup failed: expected %q, got %q", tc.nativeAlgo, algo)
+			}
+
+			_, _, err := parsePubKey(in, tc.askedAlgo)
+			if err == nil {
+				t.Fatal("expected error due to algorithm mismatch, but got nil")
+			}
+			if !strings.Contains(err.Error(), "algorithm type mismatch") {
+				t.Fatalf("unexpected error message: %v", err)
+			}
+		})
+	}
+}