[xserver] os: Treat ssh as a non-local client (v3)

Submitted by Michel Dänzer on Dec. 17, 2015, 7:41 a.m.

Details

Message ID 1450338066-26402-1-git-send-email-michel@daenzer.net
State Superseded
Headers show
Series "dri3: Refuse to work for remote clients" ( rev: 6 ) in X.org

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

Commit Message

Michel Dänzer Dec. 17, 2015, 7:41 a.m.
From: Adam Jackson <ajax@redhat.com>

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.

v2: (Michel Dänzer)
    * Only match "ssh" itself, not other executable names starting with
      that prefix.
    * Ignore executable path for the match.
v3: (Michel Dänzer)
    * Use GetClientCmdName (Mark Kettenis)
    * Perform check on Windows as well, but only ignore path on Cygwin
      (Martin Peres, Emil Velikov, Jon Turney)

Signed-off-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 os/access.c | 38 +++++++++++++++++++++++++++++++++++---
 1 file changed, 35 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/os/access.c b/os/access.c
index 10a48c3..3ea2e21 100644
--- a/os/access.c
+++ b/os/access.c
@@ -173,6 +173,10 @@  SOFTWARE.
 
 #endif                          /* WIN32 */
 
+#if !defined(WIN32) || defined(__CYGWIN__)
+#include <libgen.h>
+#endif
+
 #define X_INCLUDE_NETDB_H
 #include <X11/Xos_r.h>
 
@@ -1081,9 +1085,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 +1119,35 @@  ComputeLocalClient(ClientPtr client)
     return FALSE;
 }
 
+/* Is client on the local host */
+Bool
+ComputeLocalClient(ClientPtr client)
+{
+    const char *cmdname = GetClientCmdName(client);
+
+    if (!xtransLocalClient(client))
+        return FALSE;
+
+    /* If the executable name is "ssh", assume that this client connection
+     * is forwarded from another host via SSH
+     */
+    if (cmdname) {
+        Bool ret;
+
+#if !defined(WIN32) || defined(__CYGWIN__)
+        char *cmd = strdup(cmdname);
+        ret = strcmp(basename(cmd), "ssh") != 0;
+        free(cmd);
+#else
+        ret = strcmp(cmdname, "ssh") != 0;
+#endif
+
+        return ret;
+    }
+
+    return TRUE;
+}
+
 /*
  * Return the uid and all gids of a connected local client
  * Allocates a LocalClientCredRec - caller must call FreeLocalClientCreds

Comments

On 17.12.2015 16:41, Michel Dänzer wrote:
> From: Adam Jackson <ajax@redhat.com>
> 
> 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.
> 
> v2: (Michel Dänzer)
>     * Only match "ssh" itself, not other executable names starting with
>       that prefix.
>     * Ignore executable path for the match.
> v3: (Michel Dänzer)
>     * Use GetClientCmdName (Mark Kettenis)
>     * Perform check on Windows as well, but only ignore path on Cygwin
>       (Martin Peres, Emil Velikov, Jon Turney)
> 
> Signed-off-by: Adam Jackson <ajax@redhat.com>
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>

Martin, Mark, Jon, any other objections? If not, can we get a Reviewed-by?


> ---
>  os/access.c | 38 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/os/access.c b/os/access.c
> index 10a48c3..3ea2e21 100644
> --- a/os/access.c
> +++ b/os/access.c
> @@ -173,6 +173,10 @@ SOFTWARE.
>  
>  #endif                          /* WIN32 */
>  
> +#if !defined(WIN32) || defined(__CYGWIN__)
> +#include <libgen.h>
> +#endif
> +
>  #define X_INCLUDE_NETDB_H
>  #include <X11/Xos_r.h>
>  
> @@ -1081,9 +1085,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 +1119,35 @@ ComputeLocalClient(ClientPtr client)
>      return FALSE;
>  }
>  
> +/* Is client on the local host */
> +Bool
> +ComputeLocalClient(ClientPtr client)
> +{
> +    const char *cmdname = GetClientCmdName(client);
> +
> +    if (!xtransLocalClient(client))
> +        return FALSE;
> +
> +    /* If the executable name is "ssh", assume that this client connection
> +     * is forwarded from another host via SSH
> +     */
> +    if (cmdname) {
> +        Bool ret;
> +
> +#if !defined(WIN32) || defined(__CYGWIN__)
> +        char *cmd = strdup(cmdname);
> +        ret = strcmp(basename(cmd), "ssh") != 0;
> +        free(cmd);
> +#else
> +        ret = strcmp(cmdname, "ssh") != 0;
> +#endif
> +
> +        return ret;
> +    }
> +
> +    return TRUE;
> +}
> +
>  /*
>   * Return the uid and all gids of a connected local client
>   * Allocates a LocalClientCredRec - caller must call FreeLocalClientCreds
>
On 12/01/16 08:57, Michel Dänzer wrote:
> On 17.12.2015 16:41, Michel Dänzer wrote:
>> From: Adam Jackson <ajax@redhat.com>
>>
>> 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.
>>
>> v2: (Michel Dänzer)
>>      * Only match "ssh" itself, not other executable names starting with
>>        that prefix.
>>      * Ignore executable path for the match.
>> v3: (Michel Dänzer)
>>      * Use GetClientCmdName (Mark Kettenis)
>>      * Perform check on Windows as well, but only ignore path on Cygwin
>>        (Martin Peres, Emil Velikov, Jon Turney)
>>
>> Signed-off-by: Adam Jackson <ajax@redhat.com>
>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> Martin, Mark, Jon, any other objections? If not, can we get a Reviewed-by?

Oh, sorry, I already gave you my R-b and did not think you needed it 
again. In any case, thanks for the additional fixes!

Reviewed-by: Martin Peres <martin.peres@linux.intel.com>

>
>
>> ---
>>   os/access.c | 38 +++++++++++++++++++++++++++++++++++---
>>   1 file changed, 35 insertions(+), 3 deletions(-)
>>
>> diff --git a/os/access.c b/os/access.c
>> index 10a48c3..3ea2e21 100644
>> --- a/os/access.c
>> +++ b/os/access.c
>> @@ -173,6 +173,10 @@ SOFTWARE.
>>   
>>   #endif                          /* WIN32 */
>>   
>> +#if !defined(WIN32) || defined(__CYGWIN__)
>> +#include <libgen.h>
>> +#endif
>> +
>>   #define X_INCLUDE_NETDB_H
>>   #include <X11/Xos_r.h>
>>   
>> @@ -1081,9 +1085,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 +1119,35 @@ ComputeLocalClient(ClientPtr client)
>>       return FALSE;
>>   }
>>   
>> +/* Is client on the local host */
>> +Bool
>> +ComputeLocalClient(ClientPtr client)
>> +{
>> +    const char *cmdname = GetClientCmdName(client);
>> +
>> +    if (!xtransLocalClient(client))
>> +        return FALSE;
>> +
>> +    /* If the executable name is "ssh", assume that this client connection
>> +     * is forwarded from another host via SSH
>> +     */
>> +    if (cmdname) {
>> +        Bool ret;
>> +
>> +#if !defined(WIN32) || defined(__CYGWIN__)
>> +        char *cmd = strdup(cmdname);
>> +        ret = strcmp(basename(cmd), "ssh") != 0;
>> +        free(cmd);
>> +#else
>> +        ret = strcmp(cmdname, "ssh") != 0;
>> +#endif
>> +
>> +        return ret;
>> +    }
>> +
>> +    return TRUE;
>> +}
>> +
>>   /*
>>    * Return the uid and all gids of a connected local client
>>    * Allocates a LocalClientCredRec - caller must call FreeLocalClientCreds
>>
>
> From: =?UTF-8?Q?Michel_D=c3=a4nzer?= <michel@daenzer.net>
> 
> On 17.12.2015 16:41, Michel Dänzer wrote:
> > From: Adam Jackson <ajax@redhat.com>
> > 
> > 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.
> > 
> > v2: (Michel Dänzer)
> >     * Only match "ssh" itself, not other executable names starting with
> >       that prefix.
> >     * Ignore executable path for the match.
> > v3: (Michel Dänzer)
> >     * Use GetClientCmdName (Mark Kettenis)
> >     * Perform check on Windows as well, but only ignore path on Cygwin
> >       (Martin Peres, Emil Velikov, Jon Turney)
> > 
> > Signed-off-by: Adam Jackson <ajax@redhat.com>
> > Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> 
> Martin, Mark, Jon, any other objections? If not, can we get a Reviewed-by?

Must admit I'm not really thrilled by the diff.  SoI'll abstain.

> > ---
> >  os/access.c | 38 +++++++++++++++++++++++++++++++++++---
> >  1 file changed, 35 insertions(+), 3 deletions(-)
> > 
> > diff --git a/os/access.c b/os/access.c
> > index 10a48c3..3ea2e21 100644
> > --- a/os/access.c
> > +++ b/os/access.c
> > @@ -173,6 +173,10 @@ SOFTWARE.
> >  
> >  #endif                          /* WIN32 */
> >  
> > +#if !defined(WIN32) || defined(__CYGWIN__)
> > +#include <libgen.h>
> > +#endif
> > +
> >  #define X_INCLUDE_NETDB_H
> >  #include <X11/Xos_r.h>
> >  
> > @@ -1081,9 +1085,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 +1119,35 @@ ComputeLocalClient(ClientPtr client)
> >      return FALSE;
> >  }
> >  
> > +/* Is client on the local host */
> > +Bool
> > +ComputeLocalClient(ClientPtr client)
> > +{
> > +    const char *cmdname = GetClientCmdName(client);
> > +
> > +    if (!xtransLocalClient(client))
> > +        return FALSE;
> > +
> > +    /* If the executable name is "ssh", assume that this client connection
> > +     * is forwarded from another host via SSH
> > +     */
> > +    if (cmdname) {
> > +        Bool ret;
> > +
> > +#if !defined(WIN32) || defined(__CYGWIN__)
> > +        char *cmd = strdup(cmdname);
> > +        ret = strcmp(basename(cmd), "ssh") != 0;
> > +        free(cmd);
> > +#else
> > +        ret = strcmp(cmdname, "ssh") != 0;
> > +#endif
> > +
> > +        return ret;
> > +    }
> > +
> > +    return TRUE;
> > +}
> > +
> >  /*
> >   * Return the uid and all gids of a connected local client
> >   * Allocates a LocalClientCredRec - caller must call FreeLocalClientCreds
> > 
> 
> 
> -- 
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer
>
On 13.01.2016 03:43, Adam Jackson wrote:
> On Thu, 2015-12-17 at 16:41 +0900, Michel Dänzer wrote:
>> From: Adam Jackson <ajax@redhat.com>
>>
>> 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.
>>
>> v2: (Michel Dänzer)
>>     * Only match "ssh" itself, not other executable names starting with
>>       that prefix.
>>     * Ignore executable path for the match.
>> v3: (Michel Dänzer)
>>     * Use GetClientCmdName (Mark Kettenis)
>>     * Perform check on Windows as well, but only ignore path on Cygwin
>>       (Martin Peres, Emil Velikov, Jon Turney)
> 
> But this doesn't work reliably, which is why I used strncmp in the
> first place. See my original test report:
> 
> http://lists.freedesktop.org/archives/xorg-devel/2015-December/048164.html

I somehow missed that before, sorry.

Looks like this could be addressed by cutting of the colon and anything
after it using something like strtok(_r)?


> Note that cmdname is not always "ssh".

Indeed, that's one of the points which prompted my changes. E.g. it can
contain the executable path, which is stripped off with basename. That
might not work too well either with your example above without cutting
off at the colon.


> Are we honestly concerned that some client's name is prefixed with
> "ssh" and _isn't_ ssh or something acting like it?

That seems quite plausible. Debian is already shipping many more
executables starting with "ssh", that number will probably only increase
with time. Maybe none of them need DRI2/3 yet, but we shouldn't lightly
rely on that being the case for any future ones.
On 13/01/16 03:59, Michel Dänzer wrote:
> On 13.01.2016 03:43, Adam Jackson wrote:
>> On Thu, 2015-12-17 at 16:41 +0900, Michel Dänzer wrote:
>>> From: Adam Jackson <ajax@redhat.com>
>>>
>>> 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.
>>>
>>> v2: (Michel Dänzer)
>>>      * Only match "ssh" itself, not other executable names starting with
>>>        that prefix.
>>>      * Ignore executable path for the match.
>>> v3: (Michel Dänzer)
>>>      * Use GetClientCmdName (Mark Kettenis)
>>>      * Perform check on Windows as well, but only ignore path on Cygwin
>>>        (Martin Peres, Emil Velikov, Jon Turney)
>>
>> But this doesn't work reliably, which is why I used strncmp in the
>> first place. See my original test report:
>>
>> http://lists.freedesktop.org/archives/xorg-devel/2015-December/048164.html
>
> I somehow missed that before, sorry.
>
> Looks like this could be addressed by cutting of the colon and anything
> after it using something like strtok(_r)?
>
>
>> Note that cmdname is not always "ssh".
>
> Indeed, that's one of the points which prompted my changes. E.g. it can
> contain the executable path, which is stripped off with basename. That
> might not work too well either with your example above without cutting
> off at the colon.
>
>
>> Are we honestly concerned that some client's name is prefixed with
>> "ssh" and _isn't_ ssh or something acting like it?
>
> That seems quite plausible. Debian is already shipping many more
> executables starting with "ssh", that number will probably only increase
> with time. Maybe none of them need DRI2/3 yet, but we shouldn't lightly
> rely on that being the case for any future ones.

How about we also add a message in the logs to say that we treat this 
client as non-local? That seems to be a good idea, especially when we 
could have false positives.