# HG changeset patch
# Parent  6c50ec85ae480cf2aa8ebce8c9f4ff17265dde6e
Prevent X clients from circumventing X SECURITY restrictions by falling back
to implicit authentication upon MIT-MAGIC-COOKIE authentication fail by
disallowing X11 authentication after ForwardX11Timeout.

CVE-2016-1908
bsc#962313

c.f. https://thejh.net/written-stuff/openssh-6.8-xsecurity

Backport of upstream:

 commit f98a09cacff7baad8748c9aa217afd155a4d493f
 Author: mmcc@openbsd.org <mmcc@openbsd.org>
 Date:   Tue Oct 20 03:36:35 2015 +0000

     Replace a function-local allocation with stack memory.

     ok djm@

     Upstream-ID: c09fbbab637053a2ab9f33ca142b4e20a4c5a17e

 commit ed4ce82dbfa8a3a3c8ea6fa0db113c71e234416c
 Author: djm@openbsd.org <djm@openbsd.org>
 Date:   Wed Jan 13 23:04:47 2016 +0000

     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/openssh-6.6p1/clientloop.c b/openssh-6.6p1/clientloop.c
--- a/openssh-6.6p1/clientloop.c
+++ b/openssh-6.6p1/clientloop.c
@@ -286,105 +286,121 @@ set_control_persist_exit_time(void)
 }
 
 #define SSH_X11_VALID_DISPLAY_CHARS ":/.-_"
 static int
 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]) &&
 		    strchr(SSH_X11_VALID_DISPLAY_CHARS, display[i]) == NULL) {
 			debug("Invalid character '%c' in DISPLAY", display[i]);
 			return 0;
 		}
 	}
 	return 1;
 }
 
 #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, *xauthfile;
+	int got_data = 0, generated = 0, do_unlink = 0, i, r;
 	struct stat st;
 	u_int now, x11_timeout_real;
 
-	xauthdir = xauthfile = NULL;
 	*_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)) {
+	if (!client_x11_display_valid(display)) {
+		logit("DISPLAY \"%s\" invalid; disabling X11 forwarding",
+		    display);
+		return -1;
+	}
+	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",
-		    display);
-	} else {
-		if (display == NULL) {
-			debug("x11_get_proto: DISPLAY not set");
-			return;
-		}
+		xauth_path = NULL;
+	}
+
+	if (xauth_path != NULL) {
 		/*
 		 * Handle FamilyLocal case where $DISPLAY does
 		 * not match an authorization entry.  For this we
 		 * just try "xauth list unix:displaynum.screennum".
 		 * XXX: "localhost" match to determine FamilyLocal
 		 *      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) {
-			xauthdir = xmalloc(MAXPATHLEN);
-			xauthfile = xmalloc(MAXPATHLEN);
-			mktemp_proto(xauthdir, MAXPATHLEN);
 			/*
+			 * 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, MAXPATHLEN, "%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;
 		}
 
 		/*
 		 * When in untrusted mode, we read the cookie only if it was
 		 * successfully generated as an untrusted one in the step
 		 * above.
 		 */
 		if (trusted || generated) {
@@ -396,27 +412,30 @@ client_x11_get_proto(const char *display
 			    display);
 			debug2("x11_get_proto: %s", cmd);
 			f = popen(cmd, "r");
 			if (f && fgets(line, sizeof(line), f) &&
 			    sscanf(line, "%*s %511s %511s", proto, data) == 2)
 				got_data = 1;
 			if (f)
 				pclose(f);
-		} else
-			error("Warning: untrusted X11 forwarding setup failed: "
-			    "xauth key data not generated");
+		}
 	}
 
 	if (do_unlink) {
 		unlink(xauthfile);
 		rmdir(xauthdir);
 	}
-	free(xauthdir);
-	free(xauthfile);
+
+	/* 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
 	 * response anyway, and substitute this data.  The X11
 	 * server, however, will ignore this fake data and use
 	 * whatever authentication mechanisms it was using otherwise
 	 * for the local connection.
@@ -430,16 +449,18 @@ client_x11_get_proto(const char *display
 		for (i = 0; i < 16; i++) {
 			if (i % 4 == 0)
 				rnd = arc4random();
 			snprintf(data + 2 * i, sizeof data - 2 * i, "%02x",
 			    rnd & 0xff);
 			rnd >>= 8;
 		}
 	}
+
+	return 0;
 }
 
 /*
  * This is called when the interactive is entered.  This checks if there is
  * an EOF coming on stdin.  We must check this explicitly, as select() does
  * not appear to wake up when redirecting from /dev/null.
  */
 
diff --git a/openssh-6.6p1/clientloop.h b/openssh-6.6p1/clientloop.h
--- a/openssh-6.6p1/clientloop.h
+++ b/openssh-6.6p1/clientloop.h
@@ -1,9 +1,9 @@
-/* $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>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
  *                    All rights reserved
  *
  * As far as I am concerned, the code I have written for this software
  * can be used freely for any purpose.  Any derived versions of this
@@ -34,17 +34,17 @@
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
  * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include <termios.h>
 
 /* 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 *,
 	    int, Buffer *, char **);
 int	 client_request_tun_fwd(int, int, int);
 void	 client_stop_mux(void);
 
 /* Escape filter for protocol 2 sessions */
diff --git a/openssh-6.6p1/mux.c b/openssh-6.6p1/mux.c
--- a/openssh-6.6p1/mux.c
+++ b/openssh-6.6p1/mux.c
@@ -1256,26 +1256,28 @@ mux_session_confirm(int id, int success,
 		goto done;
 	}
 
 	display = getenv("DISPLAY");
 	if (cctx->want_x_fwd && options.forward_x11 && display != NULL) {
 		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) {
 		debug("Requesting authentication agent forwarding.");
 		channel_request_start(id, "auth-agent-req@openssh.com", 0);
 		packet_send();
 	}
 
diff --git a/openssh-6.6p1/ssh.c b/openssh-6.6p1/ssh.c
--- a/openssh-6.6p1/ssh.c
+++ b/openssh-6.6p1/ssh.c
@@ -1413,16 +1413,17 @@ static int
 ssh_session(void)
 {
 	int type;
 	int interactive = 0;
 	int have_tty = 0;
 	struct winsize ws;
 	char *cp;
 	const char *display;
+	char *proto = NULL, *data = NULL;
 
 	/* Enable compression if requested. */
 	if (options.compression) {
 		debug("Requesting compression at level %d.",
 		    options.compression_level);
 
 		if (options.compression_level < 1 ||
 		    options.compression_level > 9)
@@ -1481,23 +1482,19 @@ ssh_session(void)
 			logit("Warning: Remote host failed or refused to "
 			    "allocate a pseudo tty.");
 		else
 			packet_disconnect("Protocol error waiting for pty "
 			    "request response.");
 	}
 	/* Request X11 forwarding if enabled and DISPLAY is set. */
 	display = getenv("DISPLAY");
-	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.");
 		x11_request_forwarding_with_spoofing(0, display, proto,
 		    data, 0);
 		/* Read response from the server. */
 		type = packet_read();
 		if (type == SSH_SMSG_SUCCESS) {
@@ -1577,27 +1574,25 @@ ssh_session(void)
 
 /* request pty/x11/agent/tcpfwd/shell for channel */
 static void
 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 */
 
 	display = getenv("DISPLAY");
-	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.");
 		x11_request_forwarding_with_spoofing(id, display, proto,
 		    data, 1);
 		client_expect_confirm(id, "X11 forwarding", CONFIRM_WARN);
 		/* XXX exit_on_forward_failure */
 		interactive = 1;
