os: Treat ssh as a non-local client

Submitted by Adam Jackson on Dec. 8, 2015, 6:55 p.m.

Details

Message ID 1449600902-16151-1-git-send-email-ajax@redhat.com
State Superseded
Headers show
Series "dri3: Refuse to work for remote clients" ( rev: 2 ) in X.org

Browsing this patch as part of:
"dri3: Refuse to work for remote clients" rev 2 in X.org
<< prev patch [1/1] next patch >>

Commit Message

Adam Jackson Dec. 8, 2015, 6:55 p.m.
By the time we get to ComputeLocalClient, we've already done
NextAvailableClient → ReserveClientIds → DetermineClientCmd (assuming
we're built with #define CLIENTIDS), so we can look up the name of the
client process and refuse to treat ssh's X forwarding as if it were
local.

Signed-off-by: Adam Jackson <ajax@redhat.com>
---
 os/access.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/os/access.c b/os/access.c
index 10a48c3..f9619ae 100644
--- a/os/access.c
+++ b/os/access.c
@@ -1081,9 +1081,8 @@  ResetHosts(const char *display)
     }
 }
 
-/* Is client on the local host */
-Bool
-ComputeLocalClient(ClientPtr client)
+static Bool
+xtransLocalClient(ClientPtr client)
 {
     int alen, family, notused;
     Xtransaddr *from = NULL;
@@ -1116,6 +1115,20 @@  ComputeLocalClient(ClientPtr client)
     return FALSE;
 }
 
+/* Is client on the local host */
+Bool
+ComputeLocalClient(ClientPtr client)
+{
+    if (!xtransLocalClient(client))
+        return FALSE;
+
+    if (client->clientIds)
+        if (!strncmp(client->clientIds->cmdname, "ssh", 3))
+            return FALSE;
+
+    return TRUE;
+}
+
 /*
  * Return the uid and all gids of a connected local client
  * Allocates a LocalClientCredRec - caller must call FreeLocalClientCreds

Comments

Am 08.12.2015 19:55, schrieb Adam Jackson:
> By the time we get to ComputeLocalClient, we've already done
> NextAvailableClient → ReserveClientIds → DetermineClientCmd (assuming
> we're built with #define CLIENTIDS), so we can look up the name of the
> client process and refuse to treat ssh's X forwarding as if it were
> local.
> 
> Signed-off-by: Adam Jackson <ajax@redhat.com>
> ---
>  os/access.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/os/access.c b/os/access.c
> index 10a48c3..f9619ae 100644
> --- a/os/access.c
> +++ b/os/access.c
> @@ -1081,9 +1081,8 @@ ResetHosts(const char *display)
>      }
>  }
>  
> -/* Is client on the local host */
> -Bool
> -ComputeLocalClient(ClientPtr client)
> +static Bool
> +xtransLocalClient(ClientPtr client)
>  {
>      int alen, family, notused;
>      Xtransaddr *from = NULL;
> @@ -1116,6 +1115,20 @@ ComputeLocalClient(ClientPtr client)
>      return FALSE;
>  }
>  

unfortunately, that means if my client is called
"the_exceptional_secure_shell" it would break, right ?

there i would ask, could that be made on option ?
(default value e.g. ssh) other programms may benefit
also.


re,
 wh



> +/* Is client on the local host */
> +Bool
> +ComputeLocalClient(ClientPtr client)
> +{
> +    if (!xtransLocalClient(client))
> +        return FALSE;
> +
> +    if (client->clientIds)
> +        if (!strncmp(client->clientIds->cmdname, "ssh", 3))
> +            return FALSE;
> +
> +    return TRUE;
> +}
> +
>  /*
>   * Return the uid and all gids of a connected local client
>   * Allocates a LocalClientCredRec - caller must call FreeLocalClientCreds
On 2015-12-08 13:55, Adam Jackson wrote:
> By the time we get to ComputeLocalClient, we've already done
> NextAvailableClient → ReserveClientIds → DetermineClientCmd (assuming
> we're built with #define CLIENTIDS), so we can look up the name of the
> client process and refuse to treat ssh's X forwarding as if it were
> local.

Was sshd never updated to take advantage of "dix: Extend initial
connection handshake for forwarding proxies"?

http://cgit.freedesktop.org/xorg/xserver/commit/?id=78fa121f4097d29458e5453c13473595df06e26e

Peter Harris
On Tue, 2015-12-08 at 20:09 +0100, walter harms wrote:

> unfortunately, that means if my client is called
> "the_exceptional_secure_shell" it would break, right ?

"Break" in the sense of "be exactly as broken as it already is", sure.

> there i would ask, could that be made on option ?
> (default value e.g. ssh) other programms may benefit
> also.

What I'd really prefer is that forwarding proxies (including ssh)
implement support for the extended connection handshake, which allows
the client to ask to be treated as non-local:

http://cgit.freedesktop.org/xorg/xserver/commit/?id=e2c7d70e5ddb8b17676a13ceebfbb87d14d63243

But since ssh is to a first-order approximation the only forwarding X
proxy anybody uses, we might as well special case it here too.

- ajax
On Tue, 2015-12-08 at 14:14 -0500, Peter Harris wrote:

> Was sshd never updated to take advantage of "dix: Extend initial
> connection handshake for forwarding proxies"?

Nope. I'm quite sure I had a patch for that once upon a time, which
I've not been able to find. I should rewrite that too, I suppose, but
there's certainly no harm in fixing it in both places.

- ajax
On 12/ 8/15 11:31 AM, Adam Jackson wrote:
> What I'd really prefer is that forwarding proxies (including ssh)
> implement support for the extended connection handshake, which allows
> the client to ask to be treated as non-local:
>
> http://cgit.freedesktop.org/xorg/xserver/commit/?id=e2c7d70e5ddb8b17676a13ceebfbb87d14d63243
>
> But since ssh is to a first-order approximation the only forwarding X
> proxy anybody uses, we might as well special case it here too.

I suppose we could implement that in app/xfwp as an example, but I have no idea
if anyone would notice or still uses it.  (I'm in the process of removing all
the X.Org proxying programs from Solaris in favor of ssh tunneling, since I
agree that it's all we've seen anyone use in years.)
On 09.12.2015 03:55, Adam Jackson wrote:
> By the time we get to ComputeLocalClient, we've already done
> NextAvailableClient → ReserveClientIds → DetermineClientCmd (assuming
> we're built with #define CLIENTIDS), so we can look up the name of the
> client process and refuse to treat ssh's X forwarding as if it were
> local.
> 
> Signed-off-by: Adam Jackson <ajax@redhat.com>

The only argument I can come up with against this is that it
artificially prevents some things such as DRI2 direct rendering via ssh
to localhost. Probably not a big loss.

However, while the idea makes sense to me, the implementation needs some
refinement:


> +    if (client->clientIds)
> +        if (!strncmp(client->clientIds->cmdname, "ssh", 3))
> +            return FALSE;

This would treat any client whose cmdname starts with "ssh" as
non-local, right? There's quite a few executables in Debian matching
that, we probably can't guarantee that none of them (not to mention any
others or future ones) ever need anything which requires client->local
to be TRUE.

OTOH this doesn't match ssh invoked as [path]/ssh, which I imagine could
happen e.g. if ssh is launched from some kind of frontend / script.


P.S. xserver patches should now also use the explicit [PATCH xserver]
prefix.