openssh unix dev - Jun 2018 - [PATCH] allow indefinite ForwardX11Timeout by setting it to 0

This change allows use of untrusted X11 forwarding (which is more 
secure) without
requiring users to choose a finite timeout after which to refuse new 
connections.

This matches the semantics of the X11 security extension itself, which 
also treat a
validity timeout of 0 on an authentication cookie as indefinite.

Signed-off-by: Trixie Able <table at inventati.org>
---
  clientloop.c | 12 +++++++++---
  ssh_config.5 |  1 +
  2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/clientloop.c b/clientloop.c
index 7bcf22e3..99dcec89 100644
--- a/clientloop.c
+++ b/clientloop.c
@@ -342,11 +342,17 @@ client_x11_get_proto(struct ssh *ssh, const char 
*display,
                  rmdir(xauthdir);
                  return -1;
              }
-
-            if (timeout >= UINT_MAX - X11_TIMEOUT_SLACK)
+            /* add (at most) X11_TIMEOUT_SLACK to timeout to get
+             * x11_timeout_real, but do not adjust a timeout of 0 or
+             * overflow integers.
+             */
+            if (timeout == 0)
+                x11_timeout_real = 0;
+            else 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,
@@ -355,7 +361,7 @@ client_x11_get_proto(struct ssh *ssh, const char 
*display,
                  (size_t)r >= sizeof(cmd))
                  fatal("%s: cmd too long", __func__);
              debug2("%s: %s", __func__, cmd);
-            if (x11_refuse_time == 0) {
+            if (timeout != 0) {
                  now = monotime() + 1;
                  if (UINT_MAX - timeout < now)
                      x11_refuse_time = UINT_MAX;
diff --git a/ssh_config.5 b/ssh_config.5
index 71705cab..cdc407ed 100644
--- a/ssh_config.5
+++ b/ssh_config.5
@@ -683,6 +683,7 @@ X11 connections received by
  after this time will be refused.
  The default is to disable untrusted X11 forwarding after twenty minutes 
has
  elapsed.
+A timeout of zero allows untrusted X11 forwarding indefinitely.
  .It Cm ForwardX11Trusted
  If this option is set to
  .Cm yes ,
-- 
2.17.0

On 2018-04-27 16:21, table at inventati.org wrote:
> This change allows use of untrusted X11 forwarding (which is more
> secure) without
> requiring users to choose a finite timeout after which to refuse new
> connections.
> 
> This matches the semantics of the X11 security extension itself, which
> also treat a
> validity timeout of 0 on an authentication cookie as indefinite.
> 
> Signed-off-by: Trixie Able <table at inventati.org>
> ---
>  clientloop.c | 12 +++++++++---
>  ssh_config.5 |  1 +
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/clientloop.c b/clientloop.c
> index 7bcf22e3..99dcec89 100644
> --- a/clientloop.c
> +++ b/clientloop.c
> @@ -342,11 +342,17 @@ client_x11_get_proto(struct ssh *ssh, const char 
> *display,
>                  rmdir(xauthdir);
>                  return -1;
>              }
> -
> -            if (timeout >= UINT_MAX - X11_TIMEOUT_SLACK)
> +            /* add (at most) X11_TIMEOUT_SLACK to timeout to get
> +             * x11_timeout_real, but do not adjust a timeout of 0 or
> +             * overflow integers.
> +             */
> +            if (timeout == 0)
> +                x11_timeout_real = 0;
> +            else 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,
> @@ -355,7 +361,7 @@ client_x11_get_proto(struct ssh *ssh, const char 
> *display,
>                  (size_t)r >= sizeof(cmd))
>                  fatal("%s: cmd too long", __func__);
>              debug2("%s: %s", __func__, cmd);
> -            if (x11_refuse_time == 0) {
> +            if (timeout != 0) {
>                  now = monotime() + 1;
>                  if (UINT_MAX - timeout < now)
>                      x11_refuse_time = UINT_MAX;
> diff --git a/ssh_config.5 b/ssh_config.5
> index 71705cab..cdc407ed 100644
> --- a/ssh_config.5
> +++ b/ssh_config.5
> @@ -683,6 +683,7 @@ X11 connections received by
>  after this time will be refused.
>  The default is to disable untrusted X11 forwarding after twenty 
> minutes has
>  elapsed.
> +A timeout of zero allows untrusted X11 forwarding indefinitely.
>  .It Cm ForwardX11Trusted
>  If this option is set to
>  .Cm yes ,
r?

On 2018-05-28 22:12, table at inventati.org wrote:
> On 2018-04-27 16:21, table at inventati.org wrote:
>> This change allows use of untrusted X11 forwarding (which is more
>> secure) without
>> requiring users to choose a finite timeout after which to refuse new
>> connections.
>> 
>> This matches the semantics of the X11 security extension itself, which
>> also treat a
>> validity timeout of 0 on an authentication cookie as indefinite.
>> 
>> Signed-off-by: Trixie Able <table at inventati.org>
>> ---
>>  clientloop.c | 12 +++++++++---
>>  ssh_config.5 |  1 +
>>  2 files changed, 10 insertions(+), 3 deletions(-)
>> 
>> diff --git a/clientloop.c b/clientloop.c
>> index 7bcf22e3..99dcec89 100644
>> --- a/clientloop.c
>> +++ b/clientloop.c
>> @@ -342,11 +342,17 @@ client_x11_get_proto(struct ssh *ssh, const char 
>> *display,
>>                  rmdir(xauthdir);
>>                  return -1;
>>              }
>> -
>> -            if (timeout >= UINT_MAX - X11_TIMEOUT_SLACK)
>> +            /* add (at most) X11_TIMEOUT_SLACK to timeout to get
>> +             * x11_timeout_real, but do not adjust a timeout of 0 or
>> +             * overflow integers.
>> +             */
>> +            if (timeout == 0)
>> +                x11_timeout_real = 0;
>> +            else 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,
>> @@ -355,7 +361,7 @@ client_x11_get_proto(struct ssh *ssh, const char 
>> *display,
>>                  (size_t)r >= sizeof(cmd))
>>                  fatal("%s: cmd too long", __func__);
>>              debug2("%s: %s", __func__, cmd);
>> -            if (x11_refuse_time == 0) {
>> +            if (timeout != 0) {
>>                  now = monotime() + 1;
>>                  if (UINT_MAX - timeout < now)
>>                      x11_refuse_time = UINT_MAX;
>> diff --git a/ssh_config.5 b/ssh_config.5
>> index 71705cab..cdc407ed 100644
>> --- a/ssh_config.5
>> +++ b/ssh_config.5
>> @@ -683,6 +683,7 @@ X11 connections received by
>>  after this time will be refused.
>>  The default is to disable untrusted X11 forwarding after twenty 
>> minutes has
>>  elapsed.
>> +A timeout of zero allows untrusted X11 forwarding indefinitely.
>>  .It Cm ForwardX11Trusted
>>  If this option is set to
>>  .Cm yes ,
> 
> r?
> _______________________________________________
> openssh-unix-dev mailing list
> openssh-unix-dev at mindrot.org
> https://lists.mindrot.org/mailman/listinfo/openssh-unix-dev
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.

> On 2018-05-28 22:12, table at inventati.org wrote:
>>
>> On 2018-04-27 16:21, table at inventati.org wrote:
>>>
>>> This change allows use of untrusted X11 forwarding (which is more
>>> secure) without
>>> requiring users to choose a finite timeout after which to refuse
new
>>> connections.
>>>
>>> This matches the semantics of the X11 security extension itself,
which
>>> also treat a
>>> validity timeout of 0 on an authentication cookie as indefinite.
>>>
>>> Signed-off-by: Trixie Able <table at inventati.org>
>>> ---
>>>  clientloop.c | 12 +++++++++---
>>>  ssh_config.5 |  1 +
>>>  2 files changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/clientloop.c b/clientloop.c
>>> index 7bcf22e3..99dcec89 100644
>>> --- a/clientloop.c
>>> +++ b/clientloop.c
>>> @@ -342,11 +342,17 @@ client_x11_get_proto(struct ssh *ssh, const
char
>>> *display,
>>>                  rmdir(xauthdir);
>>>                  return -1;
>>>              }
>>> -
>>> -            if (timeout >= UINT_MAX - X11_TIMEOUT_SLACK)
>>> +            /* add (at most) X11_TIMEOUT_SLACK to timeout to get
>>> +             * x11_timeout_real, but do not adjust a timeout of 0
or
>>> +             * overflow integers.
>>> +             */
>>> +            if (timeout == 0)
>>> +                x11_timeout_real = 0;
>>> +            else 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,
>>> @@ -355,7 +361,7 @@ client_x11_get_proto(struct ssh *ssh, const
char
>>> *display,
>>>                  (size_t)r >= sizeof(cmd))
>>>                  fatal("%s: cmd too long", __func__);
>>>              debug2("%s: %s", __func__, cmd);
>>> -            if (x11_refuse_time == 0) {
>>> +            if (timeout != 0) {
>>>                  now = monotime() + 1;
>>>                  if (UINT_MAX - timeout < now)
>>>                      x11_refuse_time = UINT_MAX;
>>> diff --git a/ssh_config.5 b/ssh_config.5
>>> index 71705cab..cdc407ed 100644
>>> --- a/ssh_config.5
>>> +++ b/ssh_config.5
>>> @@ -683,6 +683,7 @@ X11 connections received by
>>>  after this time will be refused.
>>>  The default is to disable untrusted X11 forwarding after twenty
minutes
>>> has
>>>  elapsed.
>>> +A timeout of zero allows untrusted X11 forwarding indefinitely.
>>>  .It Cm ForwardX11Trusted
>>>  If this option is set to
>>>  .Cm yes ,
>>
>>
>> r?
>
>
> 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

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);
 		}
 	}