From f98a09cacff7baad8748c9aa217afd155a4d493f Mon Sep 17 00:00:00 2001

From: "mmcc@openbsd.org" <mmcc@openbsd.org>

Date: Tue, 20 Oct 2015 03:36:35 +0000

Subject: upstream commit

Replace a function-local allocation with stack memory.

ok djm@

Upstream-ID: c09fbbab637053a2ab9f33ca142b4e20a4c5a17e

---

 clientloop.c | 9 ++-------

 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/clientloop.c b/clientloop.c

index 87ceb3d..1e05cba 100644

--- a/clientloop.c

+++ b/clientloop.c

@@ -311,11 +311,10 @@ client_x11_get_proto(const char *display, const char *xauth_path,

 	static char proto[512], data[512];

 	FILE *f;

 	int got_data = 0, generated = 0, do_unlink = 0, i;

-	char *xauthdir, *xauthfile;

+	char xauthdir[MAXPATHLEN] = "", xauthfile[MAXPATHLEN] = "";

 	struct stat st;

 	u_int now, x11_timeout_real;

-	xauthdir = xauthfile = NULL;

 	*_proto = proto;

 	*_data = data;

 	proto[0] = data[0] = '\0';

@@ -343,8 +342,6 @@ client_x11_get_proto(const char *display, const char *xauth_path,

 			display = xdisplay;

 		}

 		if (trusted == 0) {

-			xauthdir = xmalloc(MAXPATHLEN);

-			xauthfile = xmalloc(MAXPATHLEN);

 			mktemp_proto(xauthdir, MAXPATHLEN);

 			/*

 			 * The authentication cookie should briefly outlive

@@ -407,8 +404,6 @@ client_x11_get_proto(const char *display, const char *xauth_path,

 		unlink(xauthfile);

 		rmdir(xauthdir);

 	}

-	free(xauthdir);

-	free(xauthfile);

 	/*

 	 * If we didn't get authentication data, just make up some

-- 

cgit v0.11.2

From ed4ce82dbfa8a3a3c8ea6fa0db113c71e234416c Mon Sep 17 00:00:00 2001

From: "djm@openbsd.org" <djm@openbsd.org>

Date: Wed, 13 Jan 2016 23:04:47 +0000

Subject: 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

---

 clientloop.c | 114 ++++++++++++++++++++++++++++++++++++-----------------------

 clientloop.h |   4 +--

 mux.c        |  22 ++++++------

 ssh.c        |  23 +++++-------

 4 files changed, 93 insertions(+), 70 deletions(-)

diff --git a/clientloop.c b/clientloop.c

index f555451..c0386d5 100644

--- a/clientloop.c

+++ b/clientloop.c

@@ -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[MAXPATHLEN], xauthdir[MAXPATHLEN];

 	static char proto[512], data[512];

 	FILE *f;

-	int got_data = 0, generated = 0, do_unlink = 0, i;

-	char xauthdir[MAXPATHLEN] = "", xauthfile[MAXPATHLEN] = "";

+	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, 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;

 		}

 		/*

@@ -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 338d451..f4d4c69 100644

--- a/clientloop.h

+++ b/clientloop.h

@@ -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 f9c3af6..6bf53eb 100644

--- a/mux.c

+++ b/mux.c

@@ -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 81704ab..096c5b5 100644

--- a/ssh.c

+++ b/ssh.c

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

 	}

 	/* 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.");

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

 		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.");

-- 

cgit v0.11.2

From 5658ef2501e785fbbdf5de2dc33b1ff7a4dca73a Mon Sep 17 00:00:00 2001

From: "millert@openbsd.org" <millert@openbsd.org>

Date: Mon, 1 Feb 2016 21:18:17 +0000

Subject: upstream commit

Avoid ugly "DISPLAY "(null)" invalid; disabling X11

 forwarding" message when DISPLAY is not set.  This could also result in a

 crash on systems with a printf that doesn't handle NULL.  OK djm@

Upstream-ID: 20ee0cfbda678a247264c20ed75362042b90b412

---

 clientloop.c | 7 ++++---

 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/clientloop.c b/clientloop.c

index f8f9a3f..f0a08f2 100644

--- a/clientloop.c

+++ b/clientloop.c

@@ -318,8 +318,9 @@ client_x11_get_proto(const char *display, const char *xauth_path,

 	proto[0] = data[0] = xauthfile[0] = xauthdir[0] = '\0';

 	if (!client_x11_display_valid(display)) {

-		logit("DISPLAY \"%s\" invalid; disabling X11 forwarding",

-		    display);

+		if (display != NULL)

+			logit("DISPLAY \"%s\" invalid; disabling X11 forwarding",

+			    display);

 		return -1;

 	}

 	if (xauth_path != NULL && stat(xauth_path, &st) == -1) {

-- 

cgit v0.11.2