diff --git a/ssh/keys.go b/ssh/keys.go index 334861b..6437771 100644 --- a/ssh/keys.go +++ b/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 diff --git a/ssh/keys_test.go b/ssh/keys_test.go index 62ba549..a16f7cd 100644 --- a/ssh/keys_test.go +++ b/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, "", }, }