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 <nsh@golang.org>
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: Nicola Murino <nicola.murino@gmail.com>
Reviewed-by: Nicholas Husin <husin@google.com>

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{}

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)
 }
 

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.

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())

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.

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) {

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)
 	}

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 {