ssh: verify declared key type matches decoded key in authorized_keys

ParseAuthorizedKey and ParseKnownHosts previously ignored the key type
field (e.g. "ssh-rsa") in each entry, relying solely on the type
information embedded within the base64-encoded key blob.

For ParseAuthorizedKey this also caused a single-token option to be
silently dropped: a line such as "restrict <key>" (with the key type
omitted) was parsed as an unrestricted key, because the option token
landed in the key type position and was discarded together with its
meaning. The same happens for no-pty, no-port-forwarding and the other
single-word options. OpenSSH's sshkey_read rejects such lines outright,
as the option token cannot be read as a key type.

OpenSSH's sshkey_read also explicitly verifies that the key type
declared in the text matches the type of the parsed key, returning
SSH_ERR_KEY_TYPE_MISMATCH if they differ.

This change adds, in both functions, a check that the declared key type
matches the decoded key's type, returning an error for malformed lines
where they diverge. This mirrors the fix already applied to the
ssh/knownhosts package in CL 782427.

Change-Id: I9163108d964de4be8a415fe0efd68e6b71f77990
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/792840
Reviewed-by: Filippo Valsorda <filippo@golang.org>
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Nicola Murino <nicola.murino@gmail.com>
Reviewed-by: Neal Patel <nealpatel@google.com>
Reviewed-by: Junyang Shao <shaojunyang@google.com>

ssh/keys.go

@@ -182,14 +182,19 @@ func ParseKnownHosts(in []byte) (marker string, hosts []string, pubKey PublicKey
 		}
 
 		hosts := string(keyFields[0])
-		// keyFields[1] contains the key type (e.g. “ssh-rsa”).
-		// However, that information is duplicated inside the
-		// base64-encoded key and so is ignored here.
+		// keyFields[1] contains the key type (e.g. "ssh-rsa"). This information
+		// is duplicated within the base64-encoded key blob. As OpenSSH's
+		// sshkey_read does, we verify that the declared key type matches the
+		// type embedded in the key blob.
+		wantType := string(keyFields[1])
 
 		key := bytes.Join(keyFields[2:], []byte(" "))
 		if pubKey, comment, err = parseAuthorizedKey(key); err != nil {
 			return "", nil, nil, "", nil, err
 		}
+		if pubKey.Type() != wantType {
+			return "", nil, nil, "", nil, fmt.Errorf("ssh: known hosts key type mismatch: human-readable type %q, encoded type %q", wantType, pubKey.Type())
+		}
 
 		return marker, strings.Split(hosts, ","), pubKey, comment, rest, nil
 	}
@@ -228,10 +233,17 @@ func ParseAuthorizedKey(in []byte) (out PublicKey, comment string, options []str
 		}
 
 		if out, comment, err = parseAuthorizedKey(in[i:]); err == nil {
-			return out, comment, options, rest, nil
-		} else {
-			lastErr = err
+			// The first field contains the declared key type. As OpenSSH's
+			// sshkey_read does, we verify that it matches the type embedded in
+			// the key blob. Without this check, a single-token option (e.g.
+			// "restrict") appearing in the key type position could be silently
+			// discarded along with its intended effect.
+			if string(in[:i]) == out.Type() {
+				return out, comment, options, rest, nil
+			}
+			err = fmt.Errorf("ssh: authorized keys key type mismatch: human-readable type %q, encoded type %q", in[:i], out.Type())
 		}
+		lastErr = err
 
 		// No key type recognised. Maybe there's an options field at
 		// the beginning.
@@ -271,11 +283,15 @@ func ParseAuthorizedKey(in []byte) (out PublicKey, comment string, options []str
 		}
 
 		if out, comment, err = parseAuthorizedKey(in[i:]); err == nil {
-			options = candidateOptions
-			return out, comment, options, rest, nil
-		} else {
-			lastErr = err
+			// As above, the declared key type (here following the options
+			// field) must match the type embedded in the key blob.
+			if string(in[:i]) == out.Type() {
+				options = candidateOptions
+				return out, comment, options, rest, nil
+			}
+			err = fmt.Errorf("ssh: authorized keys key type mismatch: human-readable type %q, encoded type %q", in[:i], out.Type())
 		}
+		lastErr = err
 
 		in = rest
 		continue

ssh/keys_test.go

@@ -848,6 +848,65 @@ func TestInvalidEntry(t *testing.T) {
 	}
 }
 
+func TestAuthorizedKeyOptionWithoutKeyType(t *testing.T) {
+	_, pubSerialized := getTestKey()
+	for _, opt := range []string{"restrict", "no-pty", "no-port-forwarding", "no-agent-forwarding"} {
+		// The key type ("ssh-rsa") is intentionally omitted.
+		line := opt + " " + pubSerialized
+		testAuthorizedKeys(t, []byte(line), []testAuthResult{
+			{nil, nil, "", "", false},
+		})
+	}
+}
+
+func TestAuthorizedKeyTypeMismatch(t *testing.T) {
+	_, pubSerialized := getTestKey() // an ssh-rsa key
+	lines := []string{
+		"ssh-ed25519 " + pubSerialized + " user@host",
+		"restrict ssh-ed25519 " + pubSerialized + " user@host",
+		"rsa-sha2-512 " + pubSerialized + " user@host",
+	}
+	for _, line := range lines {
+		testAuthorizedKeys(t, []byte(line), []testAuthResult{
+			{nil, nil, "", "", false},
+		})
+	}
+}
+
+func TestAuthorizedKeyCertificate(t *testing.T) {
+	certLine := testdata.SSHCertificates["rsa-user-testcertificate"]
+
+	key, _, options, _, err := ParseAuthorizedKey(certLine)
+	if err != nil {
+		t.Fatalf("ParseAuthorizedKey on certificate: %v", err)
+	}
+	if _, ok := key.(*Certificate); !ok {
+		t.Fatalf("got %T, want *Certificate", key)
+	}
+	if len(options) != 0 {
+		t.Errorf("got options %v, want none", options)
+	}
+
+	key, _, options, _, err = ParseAuthorizedKey(append([]byte("cert-authority "), certLine...))
+	if err != nil {
+		t.Fatalf("ParseAuthorizedKey on cert-authority certificate: %v", err)
+	}
+	if _, ok := key.(*Certificate); !ok {
+		t.Fatalf("got %T, want *Certificate", key)
+	}
+	if !reflect.DeepEqual(options, []string{"cert-authority"}) {
+		t.Errorf("got options %v, want [cert-authority]", options)
+	}
+}
+
+func TestAuthorizedKeyOptionWithKeyType(t *testing.T) {
+	pub, pubSerialized := getTestKey()
+	line := "restrict ssh-rsa " + pubSerialized + " user@host"
+	testAuthorizedKeys(t, []byte(line), []testAuthResult{
+		{pub, []string{"restrict"}, "user@host", "", true},
+	})
+}
+
 var knownHostsParseTests = []struct {
 	input string
 	err   string
@@ -927,6 +986,13 @@ var knownHostsParseTests = []struct {
 		"@marker \tlocalhost,[host2:123]\tssh-rsa aabbccdd",
 		"short read",
 
+		"", "", nil, "",
+	},
+	{
+		// Declared key type does not match the type embedded in the blob.
+		"localhost ssh-ed25519 {RSAPUB}",
+		"key type mismatch",
+
 		"", "", nil, "",
 	},
 }