From 0f039f3a4ad8093b3587dd32a675f6f101f902ed Mon Sep 17 00:00:00 2001 From: "Nicholas S. Husin" Date: Wed, 8 Jul 2026 12:15:26 -0400 Subject: [PATCH] ssh: sanitize various error messages Similar to CL 780180, sanitize other error messages that are controlled by the remote peer, to avoid potential injection of control characters. Also remove sanitizeString usage as it is not strictly needed to escape error messages safely, and for consistency with how we sanitize other error messages throughout the codebase. BannerDisplayStderr is left unsanitized for now, as it is one instance where we might actually want control characters for valid reasons (see golang/go#80302). Updates golang/go#80302 Fixes golang/go#80301 Change-Id: Ie40f4ac2f76afed740dceb548a4b0fa96a6a6964 Reviewed-on: https://go-review.googlesource.com/c/crypto/+/798420 Auto-Submit: Nicholas Husin LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com Reviewed-by: Michael Pratt Reviewed-by: Nicola Murino Reviewed-by: Nicholas Husin --- ssh/client_auth.go | 2 +- ssh/common.go | 2 +- ssh/connection.go | 2 +- ssh/handshake_test.go | 4 ++-- ssh/messages.go | 11 +---------- ssh/messages_test.go | 8 ++++---- ssh/mux_test.go | 2 +- ssh/server.go | 2 +- 8 files changed, 12 insertions(+), 21 deletions(-) diff --git a/ssh/client_auth.go b/ssh/client_auth.go index 60af2fc..138cc90 100644 --- a/ssh/client_auth.go +++ b/ssh/client_auth.go @@ -798,7 +798,7 @@ func (g *gssAPIWithMICCallback) auth(session []byte, user string, c packetConn, return authFailure, nil, fmt.Errorf("GSS-API Error:\n"+ "Major Status: %d\n"+ "Minor Status: %d\n"+ - "Error Message: %s\n", userAuthGSSAPIErrorResp.MajorStatus, userAuthGSSAPIErrorResp.MinorStatus, + "Error Message: %q\n", userAuthGSSAPIErrorResp.MajorStatus, userAuthGSSAPIErrorResp.MinorStatus, userAuthGSSAPIErrorResp.Message) case msgUserAuthGSSAPIToken: userAuthGSSAPITokenReq := &userAuthGSSAPIToken{} diff --git a/ssh/common.go b/ssh/common.go index 2e44e9c..4024712 100644 --- a/ssh/common.go +++ b/ssh/common.go @@ -419,7 +419,7 @@ type AlgorithmNegotiationError struct { } func (a *AlgorithmNegotiationError) Error() string { - return fmt.Sprintf("ssh: no common algorithm for %s; we offered: %v, peer offered: %v", + return fmt.Sprintf("ssh: no common algorithm for %s; we offered: %q, peer offered: %q", a.What, a.SupportedAlgorithms, a.RequestedAlgorithms) } diff --git a/ssh/connection.go b/ssh/connection.go index 378f640..9e0ed08 100644 --- a/ssh/connection.go +++ b/ssh/connection.go @@ -17,7 +17,7 @@ type OpenChannelError struct { } func (e *OpenChannelError) Error() string { - return fmt.Sprintf("ssh: rejected: %s (%s)", e.Reason, e.Message) + return fmt.Sprintf("ssh: rejected: %s (%q)", e.Reason, e.Message) } // ConnMetadata holds metadata for the connection. diff --git a/ssh/handshake_test.go b/ssh/handshake_test.go index 56f230c..1322db4 100644 --- a/ssh/handshake_test.go +++ b/ssh/handshake_test.go @@ -1336,7 +1336,7 @@ func TestAlgorithmNegotiationError(t *testing.T) { if !errors.As(err, &negotiationError) { t.Fatalf("expected algorithm negotiation error, got %v", err) } - expectedErrorString := fmt.Sprintf("ssh: handshake failed: ssh: no common algorithm for client to server cipher; we offered: %v, peer offered: %v", + expectedErrorString := fmt.Sprintf("ssh: handshake failed: ssh: no common algorithm for client to server cipher; we offered: %q, peer offered: %q", clientConf.Ciphers, serverConf.Ciphers) if err.Error() != expectedErrorString { t.Fatalf("expected error string %q, got %q", expectedErrorString, err.Error()) @@ -1352,7 +1352,7 @@ func TestAlgorithmNegotiationError(t *testing.T) { if !errors.As(err, &negotiationError) { t.Fatalf("expected algorithm negotiation error, got %v", err) } - expectedErrorString = fmt.Sprintf("ssh: no common algorithm for client to server cipher; we offered: %v, peer offered: %v", + expectedErrorString = fmt.Sprintf("ssh: no common algorithm for client to server cipher; we offered: %q, peer offered: %q", serverConf.Ciphers, clientConf.Ciphers) if err.Error() != expectedErrorString { t.Fatalf("expected error string %q, got %q", expectedErrorString, err.Error()) diff --git a/ssh/messages.go b/ssh/messages.go index de86f71..1d23dde 100644 --- a/ssh/messages.go +++ b/ssh/messages.go @@ -44,16 +44,7 @@ type disconnectMsg struct { } func (d *disconnectMsg) Error() string { - return fmt.Sprintf("ssh: disconnect, reason %d: %q", d.Reason, sanitizeString(d.Message)) -} - -func sanitizeString(s string) string { - return strings.Map(func(r rune) rune { - if r == '\t' || (r >= ' ' && r < 0x7f) { - return r - } - return -1 - }, s) + return fmt.Sprintf("ssh: disconnect, reason %d: %q", d.Reason, d.Message) } // See RFC 4253, section 7.1. diff --git a/ssh/messages_test.go b/ssh/messages_test.go index 90f2f4a..c333edc 100644 --- a/ssh/messages_test.go +++ b/ssh/messages_test.go @@ -269,10 +269,10 @@ func TestDisconnectMsgSanitizesMessage(t *testing.T) { want string }{ {"clean message", `"clean message"`}, - {"line1\nline2\nline3", `"line1line2line3"`}, - {"has\x00null\x00bytes", `"hasnullbytes"`}, - {"\x1b[31mred\x1b[0m", `"[31mred[0m"`}, - {"newline\r\nCRLF", `"newlineCRLF"`}, + {"line1\nline2\nline3", `"line1\nline2\nline3"`}, + {"has\x00null\x00bytes", `"has\x00null\x00bytes"`}, + {"\x1b[31mred\x1b[0m", `"\x1b[31mred\x1b[0m"`}, + {"newline\r\nCRLF", `"newline\r\nCRLF"`}, {"tab\there", `"tab\there"`}, } { t.Run(tc.in, func(t *testing.T) { diff --git a/ssh/mux_test.go b/ssh/mux_test.go index 5196b33..4996878 100644 --- a/ssh/mux_test.go +++ b/ssh/mux_test.go @@ -299,7 +299,7 @@ func TestMuxReject(t *testing.T) { t.Errorf("got %#v, want {Reason: 42, Message: %q}", ocf, "message") } - want := "ssh: rejected: unknown reason 42 (message)" + want := "ssh: rejected: unknown reason 42 (\"message\")" if err.Error() != want { t.Errorf("got %q, want %q", err.Error(), want) } diff --git a/ssh/server.go b/ssh/server.go index 18992a5..07d2405 100644 --- a/ssh/server.go +++ b/ssh/server.go @@ -710,7 +710,7 @@ userAuthLoop: } if userAuthReq.Service != serviceSSH { - return nil, errors.New("ssh: client attempted to negotiate for unknown service: " + userAuthReq.Service) + return nil, fmt.Errorf("ssh: client attempted to negotiate for unknown service: %q", userAuthReq.Service) } if s.user != userAuthReq.User && partialSuccessReturned {