upstream commit

eliminate fallback from untrusted X11 forwarding to trusted
 forwarding when the X server disables the SECURITY extension; Reported by
 Thomas Hoger; ok deraadt@

Upstream-ID: f76195bd2064615a63ef9674a0e4096b0713f938

1 files changed, 70 insertions, 44 deletions

diff --git a/clientloop.c b/clientloop.c
index f55545194..c0386d56b 100644
--- a/clientloop.c
+++ b/clientloop.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: clientloop.c,v 1.278 2015/12/26 07:46:03 semarie Exp $ */
+/* $OpenBSD: clientloop.c,v 1.279 2016/01/13 23:04:47 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -288,6 +288,9 @@ client_x11_display_valid(const char *display)
 {
         size_t i, dlen;
 
+        if (display == NULL)
+                return 0;
+
         dlen = strlen(display);
         for (i = 0; i < dlen; i++) {
                 if (!isalnum((u_char)display[i]) &&
@@ -301,34 +304,33 @@ client_x11_display_valid(const char *display)
 
 #define SSH_X11_PROTO           "MIT-MAGIC-COOKIE-1"
 #define X11_TIMEOUT_SLACK       60
-void
+int
 client_x11_get_proto(const char *display, const char *xauth_path,
     u_int trusted, u_int timeout, char **_proto, char **_data)
 {
-        char cmd[1024];
-        char line[512];
-        char xdisplay[512];
+        char cmd[1024], line[512], xdisplay[512];
+        char xauthfile[PATH_MAX], xauthdir[PATH_MAX];
         static char proto[512], data[512];
         FILE *f;
-        int got_data = 0, generated = 0, do_unlink = 0, i;
-        char xauthdir[PATH_MAX] = "", xauthfile[PATH_MAX] = "";
+        int got_data = 0, generated = 0, do_unlink = 0, i, r;
         struct stat st;
         u_int now, x11_timeout_real;
 
         *_proto = proto;
         *_data = data;
-        proto[0] = data[0] = '\0';
+        proto[0] = data[0] = xauthfile[0] = xauthdir[0] = '\0';
 
-        if (xauth_path == NULL ||(stat(xauth_path, &st) == -1)) {
-                debug("No xauth program.");
-        } else if (!client_x11_display_valid(display)) {
-                logit("DISPLAY '%s' invalid, falling back to fake xauth data",
+        if (!client_x11_display_valid(display)) {
+                logit("DISPLAY \"%s\" invalid; disabling X11 forwarding",
                     display);
-        } else {
-                if (display == NULL) {
-                        debug("x11_get_proto: DISPLAY not set");
-                        return;
-                }
+                return -1;
+        }
+        if (xauth_path != NULL && stat(xauth_path, &st) == -1) {
+                debug("No xauth program.");
+                xauth_path = NULL;
+        }
+
+        if (xauth_path != NULL) {
                 /*
                  * Handle FamilyLocal case where $DISPLAY does
                  * not match an authorization entry.  For this we
@@ -337,43 +339,60 @@ client_x11_get_proto(const char *display, const char *xauth_path,
                  *      is not perfect.
                  */
                 if (strncmp(display, "localhost:", 10) == 0) {
-                        snprintf(xdisplay, sizeof(xdisplay), "unix:%s",
-                            display + 10);
+                        if ((r = snprintf(xdisplay, sizeof(xdisplay), "unix:%s",
+                            display + 10)) < 0 ||
+                            (size_t)r >= sizeof(xdisplay)) {
+                                error("%s: display name too long", __func__);
+                                return -1;
+                        }
                         display = xdisplay;
                 }
                 if (trusted == 0) {
-                        mktemp_proto(xauthdir, PATH_MAX);
                         /*
+                         * Generate an untrusted X11 auth cookie.
+                         *
                          * The authentication cookie should briefly outlive
                          * ssh's willingness to forward X11 connections to
                          * avoid nasty fail-open behaviour in the X server.
                          */
+                        mktemp_proto(xauthdir, sizeof(xauthdir));
+                        if (mkdtemp(xauthdir) == NULL) {
+                                error("%s: mkdtemp: %s",
+                                    __func__, strerror(errno));
+                                return -1;
+                        }
+                        do_unlink = 1;
+                        if ((r = snprintf(xauthfile, sizeof(xauthfile),
+                            "%s/xauthfile", xauthdir)) < 0 ||
+                            (size_t)r >= sizeof(xauthfile)) {
+                                error("%s: xauthfile path too long", __func__);
+                                unlink(xauthfile);
+                                rmdir(xauthdir);
+                                return -1;
+                        }
+
                         if (timeout >= UINT_MAX - X11_TIMEOUT_SLACK)
                                 x11_timeout_real = UINT_MAX;
                         else
                                 x11_timeout_real = timeout + X11_TIMEOUT_SLACK;
-                        if (mkdtemp(xauthdir) != NULL) {
-                                do_unlink = 1;
-                                snprintf(xauthfile, PATH_MAX, "%s/xauthfile",
-                                    xauthdir);
-                                snprintf(cmd, sizeof(cmd),
-                                    "%s -f %s generate %s " SSH_X11_PROTO
-                                    " untrusted timeout %u 2>" _PATH_DEVNULL,
-                                    xauth_path, xauthfile, display,
-                                    x11_timeout_real);
-                                debug2("x11_get_proto: %s", cmd);
-                                if (x11_refuse_time == 0) {
-                                        now = monotime() + 1;
-                                        if (UINT_MAX - timeout < now)
-                                                x11_refuse_time = UINT_MAX;
-                                        else
-                                                x11_refuse_time = now + timeout;
-                                        channel_set_x11_refuse_time(
-                                            x11_refuse_time);
-                                }
-                                if (system(cmd) == 0)
-                                        generated = 1;
+                        if ((r = snprintf(cmd, sizeof(cmd),
+                            "%s -f %s generate %s " SSH_X11_PROTO
+                            " untrusted timeout %u 2>" _PATH_DEVNULL,
+                            xauth_path, xauthfile, display,
+                            x11_timeout_real)) < 0 ||
+                            (size_t)r >= sizeof(cmd))
+                                fatal("%s: cmd too long", __func__);
+                        debug2("%s: %s", __func__, cmd);
+                        if (x11_refuse_time == 0) {
+                                now = monotime() + 1;
+                                if (UINT_MAX - timeout < now)
+                                        x11_refuse_time = UINT_MAX;
+                                else
+                                        x11_refuse_time = now + timeout;
+                                channel_set_x11_refuse_time(x11_refuse_time);
                         }
+                        if (system(cmd) == 0)
+                                generated = 1;
                 }
 
                 /*
@@ -395,9 +414,7 @@ client_x11_get_proto(const char *display, const char *xauth_path,
                                 got_data = 1;
                         if (f)
                                 pclose(f);
-                } else
-                        error("Warning: untrusted X11 forwarding setup failed: "
-                            "xauth key data not generated");
+                }
         }
 
         if (do_unlink) {
@@ -405,6 +422,13 @@ client_x11_get_proto(const char *display, const char *xauth_path,
                 rmdir(xauthdir);
         }
 
+        /* Don't fall back to fake X11 data for untrusted forwarding */
+        if (!trusted && !got_data) {
+                error("Warning: untrusted X11 forwarding setup failed: "
+                    "xauth key data not generated");
+                return -1;
+        }
+
         /*
          * If we didn't get authentication data, just make up some
          * data.  The forwarding code will check the validity of the
@@ -427,6 +451,8 @@ client_x11_get_proto(const char *display, const char *xauth_path,
                         rnd >>= 8;
                 }
         }
+
+        return 0;
 }
 
 /*