Hiding keyboard state

[Peter Hutterer]

On Mon, Nov 23, 2015 at 11:04:10AM -0800, Keith Packard wrote:
> 
> One of the many security holes in X is that any application can monitor
> the state of the keyboard device by querying the list of pressed keys on
> a regular basis. Here's a simple patch which makes that request report
> only key state which the client itself has already seen through X
> events.
> 
> With this patch in place, grabbing the keyboard should be sufficient to
> hide key presses from other clients.
> 
> I think we need to try to fix some of these issues, even if the fixes
> break existing applications. The next thing I'd like to try is to to
> deliver input events to only one client (owner first, then
> others). After that, apply the same rules to the input extension.
> 
> In general, there are three areas that I'm wondering if we can fix:
> 
>  1) input monitoring. This seems fairly "safe" as far as existing apps
>     go.
> 
>  2) output monitoring. This seems much harder as so many useful hacks
>     and extensions take advantage of being able to get contents from
>     other windows.
> 
>  3) breaking screen saver security. We've got an extension, let's make
>     it work.
> 
> -keith
> 
> From 627815391d2d6845f7e0a66d447c6b379be9d3cb Mon Sep 17 00:00:00 2001
> From: Keith Packard <keithp at keithp.com>
> Date: Mon, 23 Nov 2015 10:01:10 -0800
> Subject: [PATCH xserver] Track keystate per client for QueryKeymap
> 
> This adds a per-client key state vector and uses that for
> ProcQueryKeymap instead of the device keymap.
> 
> Signed-off-by: Keith Packard <keithp at keithp.com>

I _think_ this is correct, but you really want a lot of test cases for this
to make sure you're not missing out on any shortcuts the event delivery code
may take somewhere. I don't think there are any, but...

Reviewed-by: Peter Hutterer <peter.hutterer at who-t.net>

also, as I wrote in the other email this breaks syndaemon's
disable-while-typing feature, especially together with RECORD disabled.

Cheers,
   Peter

> ---
>  dix/devices.c       |  2 +-
>  dix/events.c        | 13 +++++++++++++
>  include/dixstruct.h |  1 +
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/dix/devices.c b/dix/devices.c
> index 9b0c7d2..49b6994 100644
> --- a/dix/devices.c
> +++ b/dix/devices.c
> @@ -2405,7 +2405,7 @@ ProcQueryKeymap(ClientPtr client)
>      xQueryKeymapReply rep;
>      int rc, i;
>      DeviceIntPtr keybd = PickKeyboard(client);
> -    CARD8 *down = keybd->key->down;
> +    CARD8 *down = client->down;
>  
>      REQUEST_SIZE_MATCH(xReq);
>      rep = (xQueryKeymapReply) {
> diff --git a/dix/events.c b/dix/events.c
> index efaf91d..4114471 100644
> --- a/dix/events.c
> +++ b/dix/events.c
> @@ -2006,6 +2006,19 @@ TryClientEvents(ClientPtr client, DeviceIntPtr dev, xEvent *pEvents,
>          }
>      }
>  
> +    /* Track keyboard state per client */
> +    switch (type) {
> +    case KeyPress:
> +        SetBit(client->down, pEvents->u.u.detail);
> +        break;
> +    case KeyRelease:
> +        ClearBit(client->down, pEvents->u.u.detail);
> +        break;
> +    case KeymapNotify:
> +        memcpy(client->down+1, ((xKeymapEvent *) pEvents)->map, 31);
> +        break;
> +    }
> +
>      if (BitIsOn(criticalEvents, type)) {
>          if (client->smart_priority < SMART_MAX_PRIORITY)
>              client->smart_priority++;
> diff --git a/include/dixstruct.h b/include/dixstruct.h
> index 8e70ae1..1e9f69e 100644
> --- a/include/dixstruct.h
> +++ b/include/dixstruct.h
> @@ -103,6 +103,7 @@ typedef struct _Client {
>      unsigned short newKeyboardNotifyMask;
>      unsigned short vMajor, vMinor;
>      KeyCode minKC, maxKC;
> +    CARD8 down[DOWN_LENGTH];    /* track key state for QueryKeymap */
>  
>      int smart_start_tick;
>      int smart_stop_tick;
> -- 
> 2.6.1