security extension

[Ángel González]

On 06/01/14 22:51, Peter Hutterer wrote:
> On Sun, Jan 05, 2014 at 07:34:23PM -0500, Mouse wrote:
>>> [...the security extension...]  A lot of people use X remotely with
>>> ssh which is unsafe without something like this.
>>
>> What's unsafe about that?  I don't see anything offhand, but I haven't
>> thought about it for long enough for that to mean much.
>
> ssh merely encrypts the connections between the two hosts, but the usual
> issues with X itself still apply (every client has access to every other
> client, more-or-less).
>
> Cheers,
>     Peter

That's not (completely) correct.

In openssh, ssh -X gives you an untrusted client, and you need ssh -Y in 
order to get a X11 client with access to the other, local, clients.


What I would like to see are mutually untrusted clients, so they can't 
snoop each other and eventually you could have an environment with every 
application ‘untrusted’.

The changes to Xext/security.c seem minor (see below) and for the common 
case where the X11 client connects to the server using a local socket, 
the pid of the process which opened the socket (usually available in 
some OS-specific way) seem a good identifier (would need to take care on 
pid reuse). Another option would be to assign the id from a counter.

The piece which is more delicate in my opinion is defining the 
authorization protocol appropiately. The pid could be stored there, 
there could be a magic value making the X server untrust the client and 
fetch the id itself…

What do you think?


diff --git a/Xext/securitysrv.h b/Xext/securitysrv.h
index 8904242..d007f19 100644
--- a/Xext/securitysrv.h
+++ b/Xext/securitysrv.h
@@ -63,7 +63,7 @@ extern RESTYPE SecurityAuthorizationResType;
  typedef struct {
      XID id;                     /* resource ID */
      CARD32 timeout;             /* how long to live in seconds after 
refcnt == 0 */
-    unsigned int trustLevel;    /* trusted/untrusted */
+    pid_t trustLevel;    /* trusted/untrusted */
      XID group;                  /* see embedding extension */
      unsigned int refcnt;        /* how many clients connected with 
this auth */
      unsigned int secondsRemaining;      /* overflow time amount for 
 >49 days */
diff --git a/Xext/security.c b/Xext/security.c
index 7bf6cc4..80a2e3c 100644
--- a/Xext/security.c
+++ b/Xext/security.c
@@ -59,7 +59,7 @@ static DevPrivateKeyRec stateKeyRec;
  typedef struct {
      unsigned int haveState  :1;
      unsigned int live       :1;
-    unsigned int trustLevel :2;
+    pid_t trustLevel;
      XID authId;
  } SecurityStateRec;

@@ -122,7 +122,7 @@ SecurityDoCheck(SecurityStateRec * subj, 
SecurityStateRec * obj,
          return Success;
      if (subj->trustLevel == XSecurityClientTrusted)
          return Success;
-    if (obj->trustLevel != XSecurityClientTrusted)
+    if (obj->trustLevel == subj->trustLevel)
          return Success;
      if ((requested | allowed) == allowed)
          return Success;
@@ -401,7 +401,7 @@ ProcSecurityGenerateAuthorization(ClientPtr client)
      int err;                    /* error to return from this function */
      XID authId;                 /* authorization ID assigned by os 
layer */
      xSecurityGenerateAuthorizationReply rep;    /* reply struct */
-    unsigned int trustLevel;    /* trust level of new auth */
+    pid_t trustLevel;    /* trust level of new auth */
      XID group;                  /* group of new auth */
      CARD32 timeout;             /* timeout of new auth */
      CARD32 *values;             /* list of supplied attributes */
@@ -438,6 +438,7 @@ ProcSecurityGenerateAuthorization(ClientPtr client)
      trustLevel = XSecurityClientUntrusted;
      if (stuff->valueMask & XSecurityTrustLevel) {
          trustLevel = *values++;
+        /* Store the pid here */
          if (trustLevel != XSecurityClientTrusted &&
              trustLevel != XSecurityClientUntrusted) {
              client->errorValue = trustLevel;