[xserver,v2.1] os: Treat ssh as a non-local client (v2)

Submitted by Michel Dänzer on Dec. 11, 2015, 2:34 a.m.

Details

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

Not browsing as part of any series.

Commit Message

Michel Dänzer Dec. 11, 2015, 2:34 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.

Signed-off-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---

v2.1: Slightly extended code comment

 os/access.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/os/access.c b/os/access.c
index 10a48c3..3e32128 100644
--- a/os/access.c
+++ b/os/access.c
@@ -101,6 +101,7 @@  SOFTWARE.
 #include <sys/socket.h>
 #include <sys/ioctl.h>
 #include <ctype.h>
+#include <libgen.h>
 
 #ifndef NO_LOCAL_CLIENT_CRED
 #include <pwd.h>
@@ -1081,9 +1082,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 +1116,29 @@  ComputeLocalClient(ClientPtr client)
     return FALSE;
 }
 
+/* Is client on the local host */
+Bool
+ComputeLocalClient(ClientPtr client)
+{
+    if (!xtransLocalClient(client))
+        return FALSE;
+
+#ifndef WIN32
+    /* If the executable name is "ssh", assume that this client connection
+     * is forwarded from another host via SSH
+     */
+    if (client->clientIds->cmdname) {
+        char *cmdname = strdup(client->clientIds->cmdname);
+        Bool ret = strcmp(basename(cmdname), "ssh") != 0;
+
+        free(cmdname);
+        return ret;
+    }
+#endif
+
+    return TRUE;
+}
+
 /*
  * Return the uid and all gids of a connected local client
  * Allocates a LocalClientCredRec - caller must call FreeLocalClientCreds

Comments

On 11/12/15 04:34, 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.
>
> Signed-off-by: Adam Jackson <ajax@redhat.com>
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
>
> v2.1: Slightly extended code comment
>
>   os/access.c | 29 ++++++++++++++++++++++++++---
>   1 file changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/os/access.c b/os/access.c
> index 10a48c3..3e32128 100644
> --- a/os/access.c
> +++ b/os/access.c
> @@ -101,6 +101,7 @@ SOFTWARE.
>   #include <sys/socket.h>
>   #include <sys/ioctl.h>
>   #include <ctype.h>
> +#include <libgen.h>
>   
>   #ifndef NO_LOCAL_CLIENT_CRED
>   #include <pwd.h>
> @@ -1081,9 +1082,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 +1116,29 @@ ComputeLocalClient(ClientPtr client)
>       return FALSE;
>   }
>   
> +/* Is client on the local host */
> +Bool
> +ComputeLocalClient(ClientPtr client)
> +{
> +    if (!xtransLocalClient(client))
> +        return FALSE;
> +
> +#ifndef WIN32
Why add this ifndef? And why all clients of an xserver running on 
windows should be considered local?

Seems like this function is implementing the logic "Can we safely use 
the features of posix-provided extensions?" instead of sticking to what 
the name suggests. If it is a hack, it would be nice to document it at 
least :)

Thanks for spending time on this!
> +    /* If the executable name is "ssh", assume that this client connection
> +     * is forwarded from another host via SSH
> +     */
> +    if (client->clientIds->cmdname) {
> +        char *cmdname = strdup(client->clientIds->cmdname);
> +        Bool ret = strcmp(basename(cmdname), "ssh") != 0;
> +
> +        free(cmdname);
> +        return ret;
> +    }
> +#endif
> +
> +    return TRUE;
> +}
> +
>   /*
>    * Return the uid and all gids of a connected local client
>    * Allocates a LocalClientCredRec - caller must call FreeLocalClientCreds
On 11.12.2015 18:17, Martin Peres wrote:
> On 11/12/15 04:34, 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.
>>
>> Signed-off-by: Adam Jackson <ajax@redhat.com>
>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
>> ---
>>
>> v2.1: Slightly extended code comment
>>
>>   os/access.c | 29 ++++++++++++++++++++++++++---
>>   1 file changed, 26 insertions(+), 3 deletions(-)
>>
>> diff --git a/os/access.c b/os/access.c
>> index 10a48c3..3e32128 100644
>> --- a/os/access.c
>> +++ b/os/access.c
>> @@ -101,6 +101,7 @@ SOFTWARE.
>>   #include <sys/socket.h>
>>   #include <sys/ioctl.h>
>>   #include <ctype.h>
>> +#include <libgen.h>
>>     #ifndef NO_LOCAL_CLIENT_CRED
>>   #include <pwd.h>
>> @@ -1081,9 +1082,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 +1116,29 @@ ComputeLocalClient(ClientPtr client)
>>       return FALSE;
>>   }
>>   +/* Is client on the local host */
>> +Bool
>> +ComputeLocalClient(ClientPtr client)
>> +{
>> +    if (!xtransLocalClient(client))
>> +        return FALSE;
>> +
>> +#ifndef WIN32
> Why add this ifndef?

Because libgen.h and basename() aren't available on Windows AFAICT. I
figured this use case probably isn't common enough on Windows anyway to
bother making it work there as well.


> And why all clients of an xserver running on windows should be
> considered local?

You misread the patch. There's no change in behaviour on Windows,
because xtransLocalClient and by extension ComputeLocalClient returns
FALSE in all the same cases ComputeLocalClient did before the patch.


>> +    /* If the executable name is "ssh", assume that this client
>> connection
>> +     * is forwarded from another host via SSH
>> +     */
>> +    if (client->clientIds->cmdname) {
>> +        char *cmdname = strdup(client->clientIds->cmdname);
>> +        Bool ret = strcmp(basename(cmdname), "ssh") != 0;
>> +
>> +        free(cmdname);
>> +        return ret;
>> +    }
>> +#endif
>> +
>> +    return TRUE;
>> +}
>> +
>>   /*
>>    * Return the uid and all gids of a connected local client
>>    * Allocates a LocalClientCredRec - caller must call
>> FreeLocalClientCreds
On 11/12/15 11:30, Michel Dänzer wrote:
> On 11.12.2015 18:17, Martin Peres wrote:
>> On 11/12/15 04:34, 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.
>>>
>>> Signed-off-by: Adam Jackson <ajax@redhat.com>
>>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
>>> ---
>>>
>>> v2.1: Slightly extended code comment
>>>
>>>    os/access.c | 29 ++++++++++++++++++++++++++---
>>>    1 file changed, 26 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/os/access.c b/os/access.c
>>> index 10a48c3..3e32128 100644
>>> --- a/os/access.c
>>> +++ b/os/access.c
>>> @@ -101,6 +101,7 @@ SOFTWARE.
>>>    #include <sys/socket.h>
>>>    #include <sys/ioctl.h>
>>>    #include <ctype.h>
>>> +#include <libgen.h>
>>>      #ifndef NO_LOCAL_CLIENT_CRED
>>>    #include <pwd.h>
>>> @@ -1081,9 +1082,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 +1116,29 @@ ComputeLocalClient(ClientPtr client)
>>>        return FALSE;
>>>    }
>>>    +/* Is client on the local host */
>>> +Bool
>>> +ComputeLocalClient(ClientPtr client)
>>> +{
>>> +    if (!xtransLocalClient(client))
>>> +        return FALSE;
>>> +
>>> +#ifndef WIN32
>> Why add this ifndef?
> Because libgen.h and basename() aren't available on Windows AFAICT. I
> figured this use case probably isn't common enough on Windows anyway to
> bother making it work there as well.

You are right, basename is a POSIX function and mingw does expose it, 
but it turns out msvc does not. As you said, it is probably not worth 
the effort to add msvc support that no-one will be able to test anyway.

>
>> And why all clients of an xserver running on windows should be
>> considered local?
> You misread the patch. There's no change in behaviour on Windows,
> because xtransLocalClient and by extension ComputeLocalClient returns
> FALSE in all the same cases ComputeLocalClient did before the patch.

Yes, indeed. The added code is just filtering one more case.

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

>
>
>>> +    /* If the executable name is "ssh", assume that this client
>>> connection
>>> +     * is forwarded from another host via SSH
>>> +     */
>>> +    if (client->clientIds->cmdname) {
>>> +        char *cmdname = strdup(client->clientIds->cmdname);
>>> +        Bool ret = strcmp(basename(cmdname), "ssh") != 0;
>>> +
>>> +        free(cmdname);
>>> +        return ret;
>>> +    }
>>> +#endif
>>> +
>>> +    return TRUE;
>>> +}
>>> +
>>>    /*
>>>     * Return the uid and all gids of a connected local client
>>>     * Allocates a LocalClientCredRec - caller must call
>>> FreeLocalClientCreds
>
On 11 December 2015 at 10:20, Martin Peres <martin.peres@free.fr> wrote:
> On 11/12/15 11:30, Michel Dänzer wrote:
>>
>> On 11.12.2015 18:17, Martin Peres wrote:
>>>
>>> On 11/12/15 04:34, 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.
>>>>
>>>> Signed-off-by: Adam Jackson <ajax@redhat.com>
>>>> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
>>>> ---
>>>>
>>>> v2.1: Slightly extended code comment
>>>>
>>>>    os/access.c | 29 ++++++++++++++++++++++++++---
>>>>    1 file changed, 26 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/os/access.c b/os/access.c
>>>> index 10a48c3..3e32128 100644
>>>> --- a/os/access.c
>>>> +++ b/os/access.c
>>>> @@ -101,6 +101,7 @@ SOFTWARE.
>>>>    #include <sys/socket.h>
>>>>    #include <sys/ioctl.h>
>>>>    #include <ctype.h>
>>>> +#include <libgen.h>
>>>>      #ifndef NO_LOCAL_CLIENT_CRED
>>>>    #include <pwd.h>
>>>> @@ -1081,9 +1082,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 +1116,29 @@ ComputeLocalClient(ClientPtr client)
>>>>        return FALSE;
>>>>    }
>>>>    +/* Is client on the local host */
>>>> +Bool
>>>> +ComputeLocalClient(ClientPtr client)
>>>> +{
>>>> +    if (!xtransLocalClient(client))
>>>> +        return FALSE;
>>>> +
>>>> +#ifndef WIN32
>>>
>>> Why add this ifndef?
>>
>> Because libgen.h and basename() aren't available on Windows AFAICT. I
>> figured this use case probably isn't common enough on Windows anyway to
>> bother making it work there as well.
>
>
> You are right, basename is a POSIX function and mingw does expose it, but it
> turns out msvc does not. As you said, it is probably not worth the effort to
> add msvc support that no-one will be able to test anyway.
>
There's no (intentional) MSVC or mingw support in xserver that I know
of - only Cygwin. There is also the GNU version of the function
(#define _GNU_SOURCE + #include <string.h>) although I'm not sure if
Cygwin has either of them. I'd assume Jon can follow up as needed.

-Emil
On 11/12/2015 10:31, Emil Velikov wrote:
> On 11 December 2015 at 10:20, Martin Peres <martin.peres@free.fr> wrote:
>> On 11/12/15 11:30, Michel Dänzer wrote:
>>> On 11.12.2015 18:17, Martin Peres wrote:
>>>> On 11/12/15 04:34, Michel Dänzer wrote:
>>>>> From: Adam Jackson <ajax@redhat.com>
[...]
>>>>> +#ifndef WIN32

Side note:  WIN32 just means that the Win32 API is available, and may be 
defined even on Cygwin after including windows.h (so it doesn't really 
indicate what the target is)

This is why the circumlocution "!defined(WIN32) || defined(__CYGWIN__)" 
(or it's inverse) is used in places, and to be strictly correct should 
probably be used here.

>>>> Why add this ifndef?
>>>
>>> Because libgen.h and basename() aren't available on Windows AFAICT. I
>>> figured this use case probably isn't common enough on Windows anyway to
>>> bother making it work there as well.
>>
>> You are right, basename is a POSIX function and mingw does expose it, but it
>> turns out msvc does not. As you said, it is probably not worth the effort to
>> add msvc support that no-one will be able to test anyway.
>>
> There's no (intentional) MSVC or mingw support in xserver that I know
> of - only Cygwin. There is also the GNU version of the function
> (#define _GNU_SOURCE + #include <string.h>) although I'm not sure if
> Cygwin has either of them. I'd assume Jon can follow up as needed.

No, there should be support in the X server for building it for MinGW. 
doc/c-extension touches upon this subject briefly.

I suspect it may be broken in master due to recent changes, but I do try 
to upstream patches from Xming to keep it building for MinGW, even 
though my primary interest is building xserver for Cygwin.

(There also exists VcXsrv, which uses a much larger set of patches and 
some special tools to build the X.Org Xserver with MSVC)
On 12.12.2015 00:47, Jon Turney wrote:
> On 11/12/2015 10:31, Emil Velikov wrote:
>> On 11 December 2015 at 10:20, Martin Peres <martin.peres@free.fr> wrote:
>>> On 11/12/15 11:30, Michel Dänzer wrote:
>>>> On 11.12.2015 18:17, Martin Peres wrote:
>>>>> On 11/12/15 04:34, Michel Dänzer wrote:
>>>>>> From: Adam Jackson <ajax@redhat.com>
> [...]
>>>>>> +#ifndef WIN32
> 
> Side note:  WIN32 just means that the Win32 API is available, and may be
> defined even on Cygwin after including windows.h (so it doesn't really
> indicate what the target is)
> 
> This is why the circumlocution "!defined(WIN32) || defined(__CYGWIN__)"
> (or it's inverse) is used in places, and to be strictly correct should
> probably be used here.

Changed accordingly in v3, thanks.