Sitelet https://github.com/golang/crypto/commit/7626c5025624025bb44739a805f431cf93c06d6e
Skip to content

Commit 7626c50

Browse files
drakkangopherbot
drakkan
authored and
gopherbot
committed
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>
1 parent 0471e79 commit 7626c50

2 files changed

Lines changed: 92 additions & 10 deletions

File tree

‎ssh/keys.go‎

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -182,14 +182,19 @@ func ParseKnownHosts(in []byte) (marker string, hosts []string, pubKey PublicKey
182182
}
183183

184184
hosts := string(keyFields[0])
185-
// keyFields[1] contains the key type (e.g. “ssh-rsa”).
186-
// However, that information is duplicated inside the
187-
// base64-encoded key and so is ignored here.
185+
// keyFields[1] contains the key type (e.g. "ssh-rsa"). This information
186+
// is duplicated within the base64-encoded key blob. As OpenSSH's
187+
// sshkey_read does, we verify that the declared key type matches the
188+
// type embedded in the key blob.
189+
wantType := string(keyFields[1])
188190

189191
key := bytes.Join(keyFields[2:], []byte(" "))
190192
if pubKey, comment, err = parseAuthorizedKey(key); err != nil {
191193
return "", nil, nil, "", nil, err
192194
}
195+
if pubKey.Type() != wantType {
196+
return "", nil, nil, "", nil, fmt.Errorf("ssh: known hosts key type mismatch: human-readable type %q, encoded type %q", wantType, pubKey.Type())
197+
}
193198

194199
return marker, strings.Split(hosts, ","), pubKey, comment, rest, nil
195200
}
@@ -228,10 +233,17 @@ func ParseAuthorizedKey(in []byte) (out PublicKey, comment string, options []str
228233
}
229234

230235
if out, comment, err = parseAuthorizedKey(in[i:]); err == nil {
231-
return out, comment, options, rest, nil
232-
} else {
233-
lastErr = err
236+
// The first field contains the declared key type. As OpenSSH's
237+
// sshkey_read does, we verify that it matches the type embedded in
238+
// the key blob. Without this check, a single-token option (e.g.
239+
// "restrict") appearing in the key type position could be silently
240+
// discarded along with its intended effect.
241+
if string(in[:i]) == out.Type() {
242+
return out, comment, options, rest, nil
243+
}
244+
err = fmt.Errorf("ssh: authorized keys key type mismatch: human-readable type %q, encoded type %q", in[:i], out.Type())
234245
}
246+
lastErr = err
235247

236248
// No key type recognised. Maybe there's an options field at
237249
// the beginning.
@@ -271,11 +283,15 @@ func ParseAuthorizedKey(in []byte) (out PublicKey, comment string, options []str
271283
}
272284

273285
if out, comment, err = parseAuthorizedKey(in[i:]); err == nil {
274-
options = candidateOptions
275-
return out, comment, options, rest, nil
276-
} else {
277-
lastErr = err
286+
// As above, the declared key type (here following the options
287+
// field) must match the type embedded in the key blob.
288+
if string(in[:i]) == out.Type() {
289+
options = candidateOptions
290+
return out, comment, options, rest, nil
291+
}
292+
err = fmt.Errorf("ssh: authorized keys key type mismatch: human-readable type %q, encoded type %q", in[:i], out.Type())
278293
}
294+
lastErr = err
279295

280296
in = rest
281297
continue

‎ssh/keys_test.go‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -848,6 +848,65 @@ func TestInvalidEntry(t *testing.T) {
848848
}
849849
}
850850

851+
func TestAuthorizedKeyOptionWithoutKeyType(t *testing.T) {
852+
_, pubSerialized := getTestKey()
853+
for _, opt := range []string{"restrict", "no-pty", "no-port-forwarding", "no-agent-forwarding"} {
854+
// The key type ("ssh-rsa") is intentionally omitted.
855+
line := opt + " " + pubSerialized
856+
testAuthorizedKeys(t, []byte(line), []testAuthResult{
857+
{nil, nil, "", "", false},
858+
})
859+
}
860+
}
861+
862+
func TestAuthorizedKeyTypeMismatch(t *testing.T) {
863+
_, pubSerialized := getTestKey() // an ssh-rsa key
864+
lines := []string{
865+
"ssh-ed25519 " + pubSerialized + " user@host",
866+
"restrict ssh-ed25519 " + pubSerialized + " user@host",
867+
"rsa-sha2-512 " + pubSerialized + " user@host",
868+
}
869+
for _, line := range lines {
870+
testAuthorizedKeys(t, []byte(line), []testAuthResult{
871+
{nil, nil, "", "", false},
872+
})
873+
}
874+
}
875+
876+
func TestAuthorizedKeyCertificate(t *testing.T) {
877+
certLine := testdata.SSHCertificates["rsa-user-testcertificate"]
878+
879+
key, _, options, _, err := ParseAuthorizedKey(certLine)
880+
if err != nil {
881+
t.Fatalf("ParseAuthorizedKey on certificate: %v", err)
882+
}
883+
if _, ok := key.(*Certificate); !ok {
884+
t.Fatalf("got %T, want *Certificate", key)
885+
}
886+
if len(options) != 0 {
887+
t.Errorf("got options %v, want none", options)
888+
}
889+
890+
key, _, options, _, err = ParseAuthorizedKey(append([]byte("cert-authority "), certLine...))
891+
if err != nil {
892+
t.Fatalf("ParseAuthorizedKey on cert-authority certificate: %v", err)
893+
}
894+
if _, ok := key.(*Certificate); !ok {
895+
t.Fatalf("got %T, want *Certificate", key)
896+
}
897+
if !reflect.DeepEqual(options, []string{"cert-authority"}) {
898+
t.Errorf("got options %v, want [cert-authority]", options)
899+
}
900+
}
901+
902+
func TestAuthorizedKeyOptionWithKeyType(t *testing.T) {
903+
pub, pubSerialized := getTestKey()
904+
line := "restrict ssh-rsa " + pubSerialized + " user@host"
905+
testAuthorizedKeys(t, []byte(line), []testAuthResult{
906+
{pub, []string{"restrict"}, "user@host", "", true},
907+
})
908+
}
909+
851910
var knownHostsParseTests = []struct {
852911
input string
853912
err string
@@ -927,6 +986,13 @@ var knownHostsParseTests = []struct {
927986
"@marker \tlocalhost,[host2:123]\tssh-rsa aabbccdd",
928987
"short read",
929988

989+
"", "", nil, "",
990+
},
991+
{
992+
// Declared key type does not match the type embedded in the blob.
993+
"localhost ssh-ed25519 {RSAPUB}",
994+
"key type mismatch",
995+
930996
"", "", nil, "",
931997
},
932998
}

0 commit comments

Comments
 (0)