Hiding keyboard state

[Antoine Martin]

On 24/11/15 02:04, 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.
This may cause us problems with xpra: we use XQueryKeymap to see which
keys are down when trying to synchronize remote and local keyboard
state, in particular for the modifiers.

The KeymapNotify handling in this patch just copies the new keymap,
can't that be used to see which keys are pressed? Or does setting a new
keymap also reset pressed keys?

The most important requirement we have is to be able to force unset all
the modifiers when a new client connects, so maybe we can just set a new
keymap every time to achieve that?

Since the actual keyboard state is still global and not per application,
what happens if:
* another app presses Shift_R, setting the "shift" modifier,
* we query the keymap and find that the "shift" modifier is not set,
* we press some other key (say "a") via XTest, expecting that
applications will see "a" but they may in fact see "(shift)a", right?
At present, we can spot that "shift" is (un)set and toggle it as needed,
using the correct key too (Shift_L vs Shift_R), will this no longer be
the case?

I understand the motivation though.. And we may be ok for most use
cases, but is there a chance of making this optional somehow?
A fair number of users rely on xpra + virtualization for application
isolation and therefore the security of the X11 keyboard within the
server is of no concern to them, but "correct" keyboard handling is.

From a later reply to this thread:
> Alternatively, we could restrict this request to "special"
applications using the technique I've outlined for access to the testing
extensions.
Is this the "Disabling RECORD by default" thread or another?

Cheers
Antoine


> 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>
> ---
>  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;
> 
> 
> 
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <https://lists.x.org/archives/xorg-devel/attachments/20160621/5676b217/attachment.sig>