From 1b77d23dc8afb26a494e331a3c637b6de88398d2 Mon Sep 17 00:00:00 2001 From: Nicola Murino Date: Sun, 24 May 2026 17:56:18 +0200 Subject: [PATCH] ssh/knownhosts: reject lines with multiple or unknown markers Previously, parseLine would accept lines with multiple markers (e.g., "@cert-authority @revoked ...") or unknown markers (e.g., "@unknown ..."). In these cases, the second or unknown marker was incorrectly parsed as the hostname. This often resulted in confusing downstream errors (like "illegal base64 data") because field positions were shifted. OpenSSH's hostfile.c strictly enforces a limit of one marker per line and validates that markers must be exactly "@cert-authority" or "@revoked". This change adds a check to ensure that the parsed hostname does not start with '@'. This effectively catches both multiple markers (where the second marker becomes the host) and unknown markers (which are not consumed as markers and thus become the host), enforcing strict compliance with the OpenSSH format. 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: I13bb04d1f2610483ad9c6d1020e5100b6feded90 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/782428 Reviewed-by: David Chase LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Mark Freeman Reviewed-by: Filippo Valsorda --- ssh/knownhosts/knownhosts.go | 6 +++++ ssh/knownhosts/knownhosts_test.go | 38 +++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/ssh/knownhosts/knownhosts.go b/ssh/knownhosts/knownhosts.go index c7eda05..cf520ad 100644 --- a/ssh/knownhosts/knownhosts.go +++ b/ssh/knownhosts/knownhosts.go @@ -188,6 +188,12 @@ func parseLine(line []byte) (marker, host string, key ssh.PublicKey, err error) } host, line = nextWord(line) + // If the extracted 'host' starts with '@', it means we either encountered + // a second marker (e.g., "@cert-authority @revoked") or an unknown marker + // (e.g., "@unknown"). Both are invalid. + if len(host) > 0 && host[0] == '@' { + return "", "", nil, fmt.Errorf("knownhosts: unexpected marker: %q", host) + } if len(line) == 0 { return "", "", nil, errors.New("knownhosts: missing host pattern") } diff --git a/ssh/knownhosts/knownhosts_test.go b/ssh/knownhosts/knownhosts_test.go index f3af2da..5e51447 100644 --- a/ssh/knownhosts/knownhosts_test.go +++ b/ssh/knownhosts/knownhosts_test.go @@ -462,3 +462,41 @@ func TestKeyTypeMismatch(t *testing.T) { t.Fatalf("got error %q, want to contain %q", err.Error(), expectedErr) } } + +func TestMultipleMarkers(t *testing.T) { + lineDouble := fmt.Sprintf("@cert-authority @revoked server.org %s %s", edKey.Type(), base64.StdEncoding.EncodeToString(edKey.Marshal())) + db := newHostKeyDB() + err := db.Read(bytes.NewBufferString(lineDouble), "testdb") + if err == nil { + t.Fatal("Read succeeded on line with multiple markers, expected error") + } + expectedError := "unexpected marker" + if !strings.Contains(err.Error(), expectedError) { + t.Errorf("got error %q, want it to contain %q", err.Error(), expectedError) + } + + lineUnknown := fmt.Sprintf("@unknown-marker server.org %s %s", edKey.Type(), base64.StdEncoding.EncodeToString(edKey.Marshal())) + err = db.Read(bytes.NewBufferString(lineUnknown), "testdb") + if err == nil { + t.Fatal("Read succeeded on line with unknown marker, expected error") + } + if !strings.Contains(err.Error(), expectedError) { + t.Errorf("got error %q, want it to contain %q", err.Error(), expectedError) + } +} + +func TestUnknownMarker(t *testing.T) { + line := fmt.Sprintf("@unknown-marker server.org %s %s", edKey.Type(), base64.StdEncoding.EncodeToString(edKey.Marshal())) + + db := newHostKeyDB() + err := db.Read(bytes.NewBufferString(line), "testdb") + + if err == nil { + t.Fatal("Read succeeded on line with unknown marker, expected error") + } + + expectedError := "unexpected marker" + if !strings.Contains(err.Error(), expectedError) { + t.Errorf("got error %q, want it to contain %q", err.Error(), expectedError) + } +}