From bf0ba0e00dd965bae0906f79a1623b8793ea0f1a Mon Sep 17 00:00:00 2001 From: Brian Joerger Date: Tue, 10 Dec 2024 10:30:20 -0800 Subject: [PATCH] Fix trusted x11 forwarding with client xauth data (#48937) * Fix trusted x11 forwarding with client xauth data; use slog. * Fix slog lint. --- lib/client/x11_session.go | 65 +++++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 26 deletions(-) diff --git a/lib/client/x11_session.go b/lib/client/x11_session.go index 61738795d5b95..d39bd14c752a0 100644 --- a/lib/client/x11_session.go +++ b/lib/client/x11_session.go @@ -20,6 +20,8 @@ package client import ( "context" + "fmt" + "log/slog" "os" "time" @@ -41,7 +43,7 @@ func (ns *NodeSession) handleX11Forwarding(ctx context.Context, sess *tracessh.S display, err := x11.GetXDisplay() if err != nil { - log.WithError(err).Info("X11 forwarding requested but $DISPLAY is invalid") + slog.InfoContext(ctx, "X11 forwarding requested but $DISPLAY is invalid", "err", err) return ns.rejectX11Channels(ctx) } @@ -60,8 +62,8 @@ func (ns *NodeSession) handleX11Forwarding(ctx context.Context, sess *tracessh.S if err := x11.RequestForwarding(sess.Session, ns.spoofedXAuthEntry); err != nil { // Notify the user that x11 forwarding request failed regardless of debug level - log.Print("X11 forwarding request failed") - log.WithError(err).Debug("X11 forwarding request error") + fmt.Fprintln(os.Stderr, "X11 forwarding request failed") + slog.DebugContext(ctx, "X11 forwarding request error", "err", err) // If the X11 forwarding request fails, we must reject all X11 channel requests. return ns.rejectX11Channels(ctx) } @@ -76,25 +78,35 @@ func (ns *NodeSession) handleX11Forwarding(ctx context.Context, sess *tracessh.S // This will be used during X11 forwarding to forward and authorize // XServer requests from the remote server to the client's XServer. func (ns *NodeSession) setXAuthData(ctx context.Context, display x11.Display) error { + checkXauthErr := x11.CheckXAuthPath() + if checkXauthErr != nil && !ns.nodeClient.TC.X11ForwardingTrusted { + slog.WarnContext(ctx, "Untrusted X11 forwarding requires xauth to be installed in order generated xauth key data") + return trace.Wrap(checkXauthErr) + } + if ns.nodeClient.TC.X11ForwardingTrusted { - // For trusted X11 forwarding, we can create a random cookie without xauth + slog.WarnContext(ctx, "Trusted X11 forwarding provides unmitigated access to your local XServer, use with caution") + + // Check for existing xauth data. If found, it must be used in order to connect to the client's XServer. + var err error + if checkXauthErr == nil { + ns.clientXAuthEntry, err = x11.NewXAuthCommand(ctx, "" /* xauthFile */).ReadEntry(display) + if !trace.IsNotFound(err) { + return trace.Wrap(err) + } + } + + // If no xauth data was found, we can create a random cookie without xauth // as it is only used to validate the server-client connection. Locally, // the client's XServer will ignore the cookie and use whatever authentication // mechanisms it would use as if the client made the request locally. - log.Info("Creating a fake xauth cookie for trusted X11 forwarding.") - log.Warn("Trusted X11 forwarding provides unmitigated access to your local XServer, use with caution") + slog.InfoContext(ctx, "No xauth data found, creating a fake xauth cookie for trusted X11 forwarding.") - var err error ns.clientXAuthEntry, err = x11.NewFakeXAuthEntry(display) return trace.Wrap(err) } - if err := x11.CheckXAuthPath(); err != nil { - log.Info("trusted X11 forwarding requested but xauth is not installed") - return trace.Wrap(err) - } - - // Generate the xauth entry in a temporary file so it only exists within the context of this request. + // Generate an untrusted xauth entry in a temporary file so it only exists within the context of this request. // The XServer will recognize the xauth data regardless of it's existence within the file system. xauthFile, err := os.CreateTemp("", "tsh-xauthfile-*") if err != nil { @@ -110,7 +122,7 @@ func (ns *NodeSession) setXAuthData(ctx context.Context, display x11.Display) er defer func() { if err := os.Remove(xauthFile.Name()); err != nil { - log.WithError(err).Debug("Failed to remove temporary xauth file") + slog.DebugContext(ctx, "Failed to remove temporary xauth file", "err", err) } }() @@ -123,7 +135,7 @@ func (ns *NodeSession) setXAuthData(ctx context.Context, display x11.Display) er ns.x11RefuseTime = time.Now().Add(ns.nodeClient.TC.X11ForwardingTimeout) } - log.Info("creating an untrusted xauth cookie for untrusted X11 forwarding") + slog.InfoContext(ctx, "creating an untrusted xauth cookie for untrusted X11 forwarding", "xauth_file", xauthFile.Name()) cmd := x11.NewXAuthCommand(ctx, xauthFile.Name()) if err := cmd.GenerateUntrustedCookie(display, ns.nodeClient.TC.X11ForwardingTimeout); err != nil { return trace.Wrap(err) @@ -131,6 +143,7 @@ func (ns *NodeSession) setXAuthData(ctx context.Context, display x11.Display) er ns.clientXAuthEntry, err = x11.NewXAuthCommand(ctx, xauthFile.Name()).ReadEntry(display) if err != nil { + slog.ErrorContext(ctx, "untrusted X11 forwarding setup failed: failed to generate xauth key data") return trace.Wrap(err) } @@ -142,21 +155,21 @@ func (ns *NodeSession) serveX11Channels(ctx context.Context, sess *tracessh.Sess err := x11.ServeChannelRequests(ctx, ns.nodeClient.Client.Client, func(ctx context.Context, nch ssh.NewChannel) { if !ns.x11RefuseTime.IsZero() && time.Now().After(ns.x11RefuseTime) { nch.Reject(ssh.Prohibited, "rejected X11 channel request after ForwardX11Timeout") - log.Warn("rejected X11 forwarding attempt after the ForwardX11Timeout") + slog.WarnContext(ctx, "rejected X11 forwarding attempt after the ForwardX11Timeout") return } var req x11.ChannelRequestPayload if err := ssh.Unmarshal(nch.ExtraData(), &req); err != nil { nch.Reject(ssh.Prohibited, "invalid payload") - log.WithError(err).Debug("rejected X11 channel request with invalid payload") + slog.DebugContext(ctx, "rejected X11 channel request with invalid payload", "err", err) return } - log.Debugf("received X11 channel request from %s:%d", req.OriginatorAddress, req.OriginatorPort) + slog.DebugContext(ctx, "received X11 channel request from %s:%d", req.OriginatorAddress, req.OriginatorPort) xchan, sin, err := nch.Accept() if err != nil { - log.WithError(err).Debug("failed to accept X11 channel request") + slog.DebugContext(ctx, "failed to accept X11 channel request", "err", err) return } defer xchan.Close() @@ -166,24 +179,24 @@ func (ns *NodeSession) serveX11Channels(ctx context.Context, sess *tracessh.Sess // client's xauth cookie. Otherwise, the request will be denied. authPacket, err := x11.ReadAndRewriteXAuthPacket(xchan, ns.spoofedXAuthEntry, ns.clientXAuthEntry) if trace.IsAccessDenied(err) { - log.Error("X11 connection rejected due to wrong authentication") + slog.ErrorContext(ctx, "X11 connection rejected due to wrong authentication", "err", err) return } else if err != nil { - log.WithError(err).Debug("Failed to read xauth packet from X11 channel request") + slog.DebugContext(ctx, "Failed to read xauth packet from X11 channel request", "err", err) return } // Dial a connection to the client's XServer. xconn, err := ns.clientXAuthEntry.Display.Dial() if err != nil { - log.WithError(err).Debug("Failed to connect to client's display") + slog.DebugContext(ctx, "Failed to connect to client's display", "err", err) return } defer xconn.Close() // Send the processed X11 auth packet to the client's XServer connection. if _, err := xconn.Write(authPacket); err != nil { - log.WithError(err).Debug("Failed to write xauth packet") + slog.DebugContext(ctx, "Failed to write xauth packet", "err", err) return } @@ -194,12 +207,12 @@ func (ns *NodeSession) serveX11Channels(ctx context.Context, sess *tracessh.Sess go func() { err := sshutils.ForwardRequests(ctx, sin, sess) if err != nil { - log.WithError(err).Debug("Failed to forward ssh request from server during X11 forwarding") + slog.DebugContext(ctx, "Failed to forward ssh request from server during X11 forwarding", "err", err) } }() if err := x11.Forward(ctx, xconn, xchan); err != nil { - log.WithError(err).Debug("Encountered error during X11 forwarding") + slog.DebugContext(ctx, "Encountered error during X11 forwarding", "err", err) } }) return trace.Wrap(err) @@ -211,7 +224,7 @@ func (ns *NodeSession) rejectX11Channels(ctx context.Context) error { // According to RFC 4254, client "implementations MUST reject any X11 channel // open requests if they have not requested X11 forwarding". Following openssh's // example, we treat such a request as a break in attempt and warn the user. - log.Warn("server tried X11 forwarding without client requesting it, this is likely a break-in attempt by a malicious user") + slog.WarnContext(ctx, "server tried X11 forwarding without client requesting it, this is likely a break-in attempt by a malicious user") nch.Reject(ssh.Prohibited, "") }) return trace.Wrap(err)