[Spice-devel] [RFC PATCH] [linux-vdagent] Lock screen on disconnect

[David Mansfield]

On 09/24/2015 12:46 PM, Victor Toso wrote:
> Hey,
>
> On Wed, Sep 23, 2015 at 10:05:40AM -0400, David Mansfield wrote:
>> Hi,
>>
>> The attached is a very simple patch, which is working but possibly not
>> suitable for inclusion at this point, that locks the x11 session when the
>> client disconnects.
>>
>> Locking is performed using "xdg-screensaver lock", which seems like an ok
>> implementation given that "xdg-open" is used in the file-transfer code.
>>
>> I looked at the ovirt-guest-agent code and that agent also locks the session
>> on disconnect unless specifically disabled.
>>
>> Citrix (ICAClient) sessions also automatically lock when the client
>> disconnects.
>>
>
> Not sure if I agree with the idea for vdagent... But it would need to be
> configurable by client-side IMHO. As Michal point out, the _security_
> when accesing remote VMs should be in the connection not _after_.
>
> Meaning: If one person can connect to the VM without permission, that's
> bad already, right?

I think there's a misunderstanding here - relevant users DO have to have 
permission to connect to the machine. It's possible (but far from easy 
BTW) to restrict them from being able to access when another user is 
connected. So far so good.

However, if a user disconnects (or is unexpectedly disconnected 
forcefully which can happen for a myriad of reasons beyond his/her 
control) and forgets to logout or lock screen, then there is no way to 
prevent the next user of the machine from connecting and seeing the 
other user's session.

Let's flip this around. Can anyone justify why a session should remain 
unlocked when no-one is connected to the VM? It seems like a pretty big 
security hole unless there's some way of forcing one-user-per-machine.

If the view of the spice team (or perhaps the RHEL and Fedora teams) is 
"any use of a VM by more than one user is inherently insecure but out of 
the box that's how we configure it" then I think there are other issues 
that need to be addressed. I personally don't think that use of a VM by 
more than one user is inherently insecure, provided the sessions get 
locked when the disconnect occurs (and yes, this SHOULD apply to 
consoles as well, but first things first).


>
>> 3) Is there any point checking the exit status of the lock command? (me: NO)
>
> why not?

The user has disconnected so we can't show a message to the user, we 
can't "fail the disconnect". What can actually be done here?  It could 
be logged I suppose.  Can you suggest any steps that should be taken?

>
>> 4) Should the lock command be configurable? (me: grumble)
>
> yes, preferable client-side

Ok.  That sounds somewhat reasonable: remote-viewer 
--lock-session-on-disconnect --lock-session-command="xdg-screensaver 
lock" spice://blah

(Or from the .ini file read by the remote-viewer).

How do we negotiate arbitrary or new options between the remote-viewer 
and the running agent? Any pointers?


>
>> diff -ur spice-vdagent-0.15.0.orig/src/vdagent-x11.c spice-vdagent-0.15.0/src/vdagent-x11.c
>> --- spice-vdagent-0.15.0.orig/src/vdagent-x11.c	2013-10-14 08:52:01.000000000 -0400
>> +++ spice-vdagent-0.15.0/src/vdagent-x11.c	2015-09-23 09:46:00.166210785 -0400
>> @@ -1308,11 +1308,17 @@
>>   void vdagent_x11_client_disconnected(struct vdagent_x11 *x11)
>>   {
>>       int sel;
>> +    int status;
>>
>>       for (sel = 0; sel < VD_AGENT_CLIPBOARD_SELECTION_SECONDARY; sel++) {
>>           if (x11->clipboard_owner[sel] == owner_client)
>>               vdagent_x11_clipboard_release(x11, sel);
>>       }
>> +
>> +    status = system("xdg-screensaver lock");
>> +    if (status != 0) {
>> +        /* exit status is not checked */
>> +    }
>>   }
>>
>>   /* Function used to determine the default location to save file-xfers,

Thanks,
David