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

diff --git a/clientloop.c b/clientloop.c
index f55545194055d9f382d01e7b4f4aabf8cdde6c7a..c0386d56b3e6a93b08bc6f8fa00037e1acd15047 100644 (file)
--- 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;
 }
 
 /*

diff --git a/clientloop.h b/clientloop.h
index 338d45186f8b0fda199c647e19044f72073abf2e..f4d4c69b73a0780d65a8ddeb05e7055a976b5b48 100644 (file)
--- a/clientloop.h
+++ b/clientloop.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: clientloop.h,v 1.31 2013/06/02 23:36:29 dtucker Exp $ */
+/* $OpenBSD: clientloop.h,v 1.32 2016/01/13 23:04:47 djm Exp $ */
 
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
@@ -39,7 +39,7 @@
 
 /* Client side main loop for the interactive session. */
 int     client_loop(int, int, int);
-void    client_x11_get_proto(const char *, const char *, u_int, u_int,
+int     client_x11_get_proto(const char *, const char *, u_int, u_int,
            char **, char **);
 void    client_global_request_reply_fwd(int, u_int32_t, void *);
 void    client_session2_setup(int, int, int, const char *, struct termios *,

diff --git a/mux.c b/mux.c
index f9c3af651c700fc507fc058537f118f77db08cc3..6bf53ebd9a32e25a024f05518821d302cb2387d9 100644 (file)
--- a/mux.c
+++ b/mux.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: mux.c,v 1.57 2015/12/26 07:46:03 semarie Exp $ */
+/* $OpenBSD: mux.c,v 1.58 2016/01/13 23:04:47 djm Exp $ */
 /*
  * Copyright (c) 2002-2008 Damien Miller <djm@openbsd.org>
  *
@@ -1354,16 +1354,18 @@ mux_session_confirm(int id, int success, void *arg)
                char *proto, *data;
 
                /* Get reasonable local authentication information. */
-               client_x11_get_proto(display, options.xauth_location,
+               if (client_x11_get_proto(display, options.xauth_location,
                    options.forward_x11_trusted, options.forward_x11_timeout,
-                   &proto, &data);
-               /* Request forwarding with authentication spoofing. */
-               debug("Requesting X11 forwarding with authentication "
-                   "spoofing.");
-               x11_request_forwarding_with_spoofing(id, display, proto,
-                   data, 1);
-               client_expect_confirm(id, "X11 forwarding", CONFIRM_WARN);
-               /* XXX exit_on_forward_failure */
+                   &proto, &data) == 0) {
+                       /* Request forwarding with authentication spoofing. */
+                       debug("Requesting X11 forwarding with authentication "
+                           "spoofing.");
+                       x11_request_forwarding_with_spoofing(id, display, proto,
+                           data, 1);
+                       /* XXX exit_on_forward_failure */
+                       client_expect_confirm(id, "X11 forwarding",
+                           CONFIRM_WARN);
+               }
        }
 
        if (cctx->want_agent_fwd && options.forward_agent) {

diff --git a/ssh.c b/ssh.c
index 81704ab317976b0424f88a7b3afc43dc1a5afd09..096c5b5d74f6b70eae52ccd7881b3758b9b5e714 100644 (file)
--- a/ssh.c
+++ b/ssh.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssh.c,v 1.432 2015/12/11 03:20:09 djm Exp $ */
+/* $OpenBSD: ssh.c,v 1.433 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
@@ -1626,6 +1626,7 @@ ssh_session(void)
        struct winsize ws;
        char *cp;
        const char *display;
+       char *proto = NULL, *data = NULL;
 
        /* Enable compression if requested. */
        if (options.compression) {
@@ -1696,13 +1697,9 @@ ssh_session(void)
        display = getenv("DISPLAY");
        if (display == NULL && options.forward_x11)
                debug("X11 forwarding requested but DISPLAY not set");
-       if (options.forward_x11 && display != NULL) {
-               char *proto, *data;
-               /* Get reasonable local authentication information. */
-               client_x11_get_proto(display, options.xauth_location,
-                   options.forward_x11_trusted,
-                   options.forward_x11_timeout,
-                   &proto, &data);
+       if (options.forward_x11 && client_x11_get_proto(display,
+           options.xauth_location, options.forward_x11_trusted,
+           options.forward_x11_timeout, &proto, &data) == 0) {
                /* Request forwarding with authentication spoofing. */
                debug("Requesting X11 forwarding with authentication "
                    "spoofing.");
@@ -1792,6 +1789,7 @@ ssh_session2_setup(int id, int success, void *arg)
        extern char **environ;
        const char *display;
        int interactive = tty_flag;
+       char *proto = NULL, *data = NULL;
 
        if (!success)
                return; /* No need for error message, channels code sens one */
@@ -1799,12 +1797,9 @@ ssh_session2_setup(int id, int success, void *arg)
        display = getenv("DISPLAY");
        if (display == NULL && options.forward_x11)
                debug("X11 forwarding requested but DISPLAY not set");
-       if (options.forward_x11 && display != NULL) {
-               char *proto, *data;
-               /* Get reasonable local authentication information. */
-               client_x11_get_proto(display, options.xauth_location,
-                   options.forward_x11_trusted,
-                   options.forward_x11_timeout, &proto, &data);
+       if (options.forward_x11 && client_x11_get_proto(display,
+           options.xauth_location, options.forward_x11_trusted,
+           options.forward_x11_timeout, &proto, &data) == 0) {
                /* Request forwarding with authentication spoofing. */
                debug("Requesting X11 forwarding with authentication "
                    "spoofing.");