[PATCH] allow indefinite ForwardX11Timeout by setting it to 0

[Damien Miller]

On Thu, 28 Jun 2018, Ian Sutton wrote:

> > Bump, ccing djm at openbsd.org as annotate indicates they committed most of the
> > code near these changes.
> >
> > If bumping patches is discouraged please let me know--I don't mean to be
> > rude but would like to have an r+ or r- for this small changeset.
> 
> This is reasonable and I'd like to see it in the tree, if someone gets
> a chance to review soon. Thanks

Sorry for being slow - I think that patch has one problem related to
setting x11_refuse_time (it would clobber an existing one) and IMO it
would be better to not rely on "timeout 0" on the xauth commandline
disabling the timeout, as that behaviour is not documented.

diff --git a/clientloop.c b/clientloop.c
index 68cc94b..bcd98bf 100644
--- a/clientloop.c
+++ b/clientloop.c
@@ -271,7 +271,7 @@ client_x11_get_proto(struct ssh *ssh, const char *display,
     const char *xauth_path, u_int trusted, u_int timeout,
     char **_proto, char **_data)
 {
-	char cmd[1024], line[512], xdisplay[512];
+	char *cmd, line[512], xdisplay[512];
 	char xauthfile[PATH_MAX], xauthdir[PATH_MAX];
 	static char proto[512], data[512];
 	FILE *f;
@@ -335,19 +335,31 @@ client_x11_get_proto(struct ssh *ssh, const char *display,
 				return -1;
 			}
 
-			if (timeout >= UINT_MAX - X11_TIMEOUT_SLACK)
-				x11_timeout_real = UINT_MAX;
-			else
-				x11_timeout_real = timeout + X11_TIMEOUT_SLACK;
-			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__);
+			if (timeout == 0) {
+				/* auth doesn't time out */
+				xasprintf(&cmd, "%s -f %s generate %s %s "
+				    "untrusted 2>%s",
+				    xauth_path, xauthfile, display,
+				    SSH_X11_PROTO, _PATH_DEVNULL);
+			} else {
+				/* Add some slack to requested expiry */
+				if (timeout < UINT_MAX - X11_TIMEOUT_SLACK)
+					x11_timeout_real = timeout +
+					    X11_TIMEOUT_SLACK;
+				else {
+					/* Don't overflow on long timeouts */
+					x11_timeout_real = UINT_MAX;
+				}
+				
+				xasprintf(&cmd, "%s -f %s generate %s %s "
+				    "untrusted timeout %u 2>%s",
+				    xauth_path, xauthfile, display,
+				    SSH_X11_PROTO, x11_timeout_real,
+				    _PATH_DEVNULL);
+			}
 			debug2("%s: %s", __func__, cmd);
-			if (x11_refuse_time == 0) {
+
+			if (timeout != 0 && x11_refuse_time == 0) {
 				now = monotime() + 1;
 				if (UINT_MAX - timeout < now)
 					x11_refuse_time = UINT_MAX;
@@ -358,6 +370,7 @@ client_x11_get_proto(struct ssh *ssh, const char *display,
 			}
 			if (system(cmd) == 0)
 				generated = 1;
+			free(cmd);
 		}
 
 		/*
@@ -366,7 +379,7 @@ client_x11_get_proto(struct ssh *ssh, const char *display,
 		 * above.
 		 */
 		if (trusted || generated) {
-			snprintf(cmd, sizeof(cmd),
+			xasprintf(&cmd,
 			    "%s %s%s list %s 2>" _PATH_DEVNULL,
 			    xauth_path,
 			    generated ? "-f " : "" ,
@@ -379,6 +392,7 @@ client_x11_get_proto(struct ssh *ssh, const char *display,
 				got_data = 1;
 			if (f)
 				pclose(f);
+			free(cmd);
 		}
 	}