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

[Victor Toso]

Hi David,

On Thu, Sep 24, 2015 at 01:21:53PM -0400, David Mansfield wrote:
> On 09/24/2015 12:46 PM, Victor Toso wrote:
> >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.
>

Understanding how RHEVM/ovirt does it would probably be a better
solution.

I'm not against your request (I'm happy with patches actually). I'm not
sure you will achieve the security you seek by tyring to lock the screen
with vdagent.

> 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).
>

This is just my opinion, really.

>
> >
> >>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?
>

Yeah, the problem is that if it fails for some reason you still would
have the issues you want to address... Logging at least is a must.

> >
> >>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?

Sure. The last protocol change + vdagent was due volume synchronization

linux-vdagent:
http://cgit.freedesktop.org/spice/linux/vd_agent/commit/?id=9b0eb8b1246ccb422ccecc3679b0bb6b477ba6cb
spice-gtk:
http://cgit.freedesktop.org/spice/spice-gtk/commit/?id=8c50e1a2b9f243a7da9b538cc1438b6a9d8e5671
spice-protocol:
http://cgit.freedesktop.org/spice/spice-protocol/commit/?id=9acfaa66df90cb1475db7c09da09b6e9b5f5dd00

>
> >
> >>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

cheers,
  Victor Toso