After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 738573 - spice connection exposed to other users
spice connection exposed to other users
Status: RESOLVED FIXED
Product: gnome-boxes
Classification: Applications
Component: spice
3.15.x
Other Linux
: Normal normal
: 3.22
Assigned To: GNOME Boxes maintainer(s)
GNOME Boxes maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2014-10-15 12:19 UTC by Zeeshan Ali
Modified: 2016-04-21 16:27 UTC
See Also:
GNOME target: ---
GNOME version: 3.13/3.14


Attachments
spice-display: Micro readability improvement to a comment (913 bytes, patch)
2014-11-19 01:17 UTC, Zeeshan Ali
committed Details | Review
display: Allow connecting through FD (2.28 KB, patch)
2014-11-19 01:18 UTC, Zeeshan Ali
committed Details | Review
spice-display: Allow connecting through FD (1.61 KB, patch)
2014-11-19 01:18 UTC, Zeeshan Ali
committed Details | Review
libvirt-machine: Connect using FD if local (1.59 KB, patch)
2014-11-19 01:18 UTC, Zeeshan Ali
committed Details | Review
This patch will actually be coming before others: (823 bytes, patch)
2014-11-19 01:25 UTC, Zeeshan Ali
committed Details | Review
Spice connection exposed to other users (1.02 KB, patch)
2016-03-21 18:58 UTC, Ilie Nicolae Vlad
none Details | Review
vm-configurator: Add add_graphics_device() (1.93 KB, patch)
2016-04-21 16:22 UTC, Zeeshan Ali
committed Details | Review
vm-configurator: Don't autoport spice in new VMs (1.09 KB, patch)
2016-04-21 16:23 UTC, Zeeshan Ali
committed Details | Review
vm-configurator: Don't expose SPICE in existing VMs (1.58 KB, patch)
2016-04-21 16:23 UTC, Zeeshan Ali
committed Details | Review
spice-display: Special constructor for private connection (2.91 KB, patch)
2016-04-21 16:23 UTC, Zeeshan Ali
committed Details | Review
display: uri property now nullable (3.24 KB, patch)
2016-04-21 16:23 UTC, Zeeshan Ali
committed Details | Review
spice-display: No URI for local private case (1.22 KB, patch)
2016-04-21 16:24 UTC, Zeeshan Ali
committed Details | Review

Description Zeeshan Ali 2014-10-15 12:19:26 UTC
When we create a VM, we setup its display connection to be passwordless. That is fine on most hosts out there with single user setup but this is a serious security issue on multiseat hosts as any user on the host can connect to any running VM's display.
Comment 1 Zeeshan Ali 2014-10-15 12:22:43 UTC
See https://bugzilla.gnome.org/show_bug.cgi?id=667593#c0 for why we don't setup a password on connection and rest of the discussion on that bug is relevant too.
Comment 3 Marc-Andre Lureau 2014-11-13 17:31:24 UTC
The following patch is in queued for qemu to allow port-less servers:
http://lists.gnu.org/archive/html/qemu-devel/2014-11/msg01661.html

In the meatine, Boxes can learn to attach to local spice server via virDomainOpenGraphicsFD() and spice_session_open_fd() (not through spice-gtk port connect()).
Comment 4 Zeeshan Ali 2014-11-18 18:24:22 UTC
(In reply to comment #3)
>
> In the meatine, Boxes can learn to attach to local spice server via
> virDomainOpenGraphicsFD() and spice_session_open_fd() (not through spice-gtk
> port connect()).

I thought that is as easy as calling virDomainOpenGraphicsFD() replacing spice_session_connect with spice_session_open_fd(), passing it the fd from former call but seems its freaking rocket science that doesn't seem to be well documented at all. :(
Comment 5 Marc-Andre Lureau 2014-11-18 22:26:05 UTC
(In reply to comment #4)
> (In reply to comment #3)
> >
> > In the meatine, Boxes can learn to attach to local spice server via
> > virDomainOpenGraphicsFD() and spice_session_open_fd() (not through spice-gtk
> > port connect()).
> 
> I thought that is as easy as calling virDomainOpenGraphicsFD() replacing
> spice_session_connect with spice_session_open_fd(), passing it the fd from
> former call but seems its freaking rocket science that doesn't seem to be well
> documented at all. :(

Do you have specific question? This API can't be made more simple, because spice needs several file descriptor. You initiate the connection with session_open_fd(), this when channels are created SpiceSession::channel-new, you connect to the Channel::open-fd. When ::open-fd is emitted, you open a new connection to server and handle it back to the channel with spice_channel_open_fd().
Comment 6 Zeeshan Ali 2014-11-19 01:17:55 UTC
Created attachment 290948 [details] [review]
spice-display: Micro readability improvement to a comment

ctor => constructor
Comment 7 Zeeshan Ali 2014-11-19 01:18:02 UTC
Created attachment 290949 [details] [review]
display: Allow connecting through FD

Implementation classes can now support connecting through a file
descriptor.
Comment 8 Zeeshan Ali 2014-11-19 01:18:06 UTC
Created attachment 290950 [details] [review]
spice-display: Allow connecting through FD

Make use of the new superclass API to allow users of this class to
connect to display using a file descriptor.
Comment 9 Zeeshan Ali 2014-11-19 01:18:11 UTC
Created attachment 290951 [details] [review]
libvirt-machine: Connect using FD if local

If its a local libvirt machine, connect to SPICE display using file
descriptor. While the issue of spice connection being exposed to other
local users is not resolved by this patch[1], it prepares Boxes for the
real solution.

[1] Currently libvirt+Qemu mandate specifying a TCP port for the SPICE
display.
Comment 10 Zeeshan Ali 2014-11-19 01:25:39 UTC
Created attachment 290952 [details] [review]
This patch will actually be coming before others:

Require libvirt-glib >= 0.2.0

This is to be able to use the newly added gvir_domain_open_graphics_fd()
function.
Comment 11 Zeeshan Ali 2014-11-19 01:26:28 UTC
(In reply to comment #10)
> Created an attachment (id=290952) [details] [review]
> This patch will actually be coming before others:
> 
> Require libvirt-glib >= 0.2.0
> 
> This is to be able to use the newly added gvir_domain_open_graphics_fd()
> function.

newly added => just sent the patch to libvirt list. :)
Comment 12 Zeeshan Ali 2014-11-19 11:34:47 UTC
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > >
> > > In the meatine, Boxes can learn to attach to local spice server via
> > > virDomainOpenGraphicsFD() and spice_session_open_fd() (not through spice-gtk
> > > port connect()).
> > 
> > I thought that is as easy as calling virDomainOpenGraphicsFD() replacing
> > spice_session_connect with spice_session_open_fd(), passing it the fd from
> > former call but seems its freaking rocket science that doesn't seem to be well
> > documented at all. :(
> 
> Do you have specific question?

I asked on IRC but you started ignoring me (you were answering other people) and documentation is pretty bad, hence the outrage here. :)

> This API can't be made more simple, because
> spice needs several file descriptor. You initiate the connection with
> session_open_fd(), this when channels are created SpiceSession::channel-new,
> you connect to the Channel::open-fd. When ::open-fd is emitted, you open a new
> connection to server and handle it back to the channel with
> spice_channel_open_fd().

That is fine and as you can see from attached patches, I figured it out in the end (I missed your comment) but I think all this should be explained in the docs. Here is what docs say about spice_session_open_fd:

"Open the session using the provided fd socket file descriptor. This is useful if you create the fd yourself, for example to setup a SSH tunnel.

If fd is -1, a valid fd will be requested later via the SpiceChannel::open-fd signal."

This gives the impression that if you pass this call a valid fd, you don't need to deal with SpiceChannel::open-fd.
Comment 13 Marc-Andre Lureau 2014-11-19 11:43:54 UTC
(In reply to comment #12)
> > Do you have specific question?
> 
> I asked on IRC but you started ignoring me (you were answering other people)
> and documentation is pretty bad, hence the outrage here. :)

Eh? I don't recall that.

> "Open the session using the provided fd socket file descriptor. This is useful
> if you create the fd yourself, for example to setup a SSH tunnel.
> 
> If fd is -1, a valid fd will be requested later via the SpiceChannel::open-fd
> signal."
> 
> This gives the impression that if you pass this call a valid fd, you don't need
> to deal with SpiceChannel::open-fd.

Sorry if the documentation isn't giving you the solution to all your questions. I agree it could be improved. But complaining here is not helping much.
Comment 14 Marc-Andre Lureau 2014-11-19 11:47:33 UTC
Review of attachment 290948 [details] [review]:

trivial, ack
Comment 15 Marc-Andre Lureau 2014-11-19 11:48:45 UTC
Review of attachment 290949 [details] [review]:

ack
Comment 16 Marc-Andre Lureau 2014-11-19 11:49:04 UTC
Review of attachment 290950 [details] [review]:

ack
Comment 17 Marc-Andre Lureau 2014-11-19 11:49:36 UTC
Review of attachment 290951 [details] [review]:

you should bump libvirt dependency here
Comment 18 Marc-Andre Lureau 2014-11-19 11:49:55 UTC
Review of attachment 290952 [details] [review]:

should be merge with previous patch
Comment 19 Zeeshan Ali 2014-11-19 12:16:13 UTC
Review of attachment 290951 [details] [review]:

> you should bump libvirt dependency here

I tend to keep dep changes into separate commits so that I don't miss them in release notes.
Comment 20 Zeeshan Ali 2014-11-19 12:16:48 UTC
Review of attachment 290951 [details] [review]:

but yeah, I agree the version bump patch should be parent of this one.
Comment 21 Zeeshan Ali 2014-11-19 12:18:16 UTC
(In reply to comment #13)
> (In reply to comment #12)
> > > Do you have specific question?
> > 
> > I asked on IRC but you started ignoring me (you were answering other people)
> > and documentation is pretty bad, hence the outrage here. :)
> 
> Eh? I don't recall that.

Honest mistake then. nm :)

> > "Open the session using the provided fd socket file descriptor. This is useful
> > if you create the fd yourself, for example to setup a SSH tunnel.
> > 
> > If fd is -1, a valid fd will be requested later via the SpiceChannel::open-fd
> > signal."
> > 
> > This gives the impression that if you pass this call a valid fd, you don't need
> > to deal with SpiceChannel::open-fd.
> 
> Sorry if the documentation isn't giving you the solution to all your questions.
> I agree it could be improved. But complaining here is not helping much.

Agreed. I should send a patch instead.
Comment 22 Zeeshan Ali 2015-02-23 17:29:16 UTC
Attachment 290948 [details] pushed as cb00668 - spice-display: Micro readability improvement to a comment
Attachment 290949 [details] pushed as af7f876 - display: Allow connecting through FD
Attachment 290950 [details] pushed as 9268e02 - spice-display: Allow connecting through FD
Attachment 290951 [details] pushed as fb04e17 - libvirt-machine: Connect using FD if local

I'm resolving this bug since everything on our side is hopefully done with these patches.
Comment 23 Christophe Fergeau 2016-03-21 17:47:28 UTC
Something like

diff --git a/src/vm-configurator.vala b/src/vm-configurator.vala
index 9549682..027720b 100644
--- a/src/vm-configurator.vala
+++ b/src/vm-configurator.vala
@@ -72,7 +72,7 @@ public static Domain create_domain_config (InstallerMedia install_media, string
         install_media.setup_domain_config (domain);
         var graphics = new DomainGraphicsSpice ();
-        graphics.set_autoport (true);
+        graphics.set_autoport (false);
         graphics.set_image_compression (DomainGraphicsSpiceImageCompression.OFF);
         domain.add_device (graphics);


is missing as otherwise SPICE will still be listening on a local TCP port even though you are using a FD for the connection. I wonder if existing VMs created by Boxes should be upgraded to so that they stop listening on localhost.
Comment 24 Ilie Nicolae Vlad 2016-03-21 18:58:42 UTC
Created attachment 324497 [details] [review]
Spice connection exposed to other users

I put graphics.set_autoport(false);

This should make spice not listening on local TCP port.

Bug number 738573
Comment 25 Zeeshan Ali 2016-03-21 19:12:03 UTC
Review of attachment 324497 [details] [review]:

* Please read this guideline and try your level best to follow it: https://wiki.gnome.org/Git/CommitMessages

* "I put graphics.set_autoport(false);"

You want to describe the change(s) in English here, not repeat the code.

* "This should make". Since it's easy to test this, I don't think we need to speculate here. So you'd write "This makes".

* You also want to mention that some of the changes (with commit hash of the change, preferably) in this regard have already been merged but this change is still needed.

* In Boxes project, we use the bug URL at the end (not just the number) for reference. So it'll be:

https://bugzilla.gnome.org/show_bug.cgi?id=738573

::: src/vm-configurator.vala
@@ +71,3 @@
         set_target_media_config (domain, target_path, install_media);
         install_media.setup_domain_config (domain);
+	

Irrelevant change. Please remove this change.
Comment 26 Zeeshan Ali 2016-04-21 16:22:39 UTC
Created attachment 326505 [details] [review]
vm-configurator: Add add_graphics_device()

Refactor code that adds a new graphics device into a separate method.
Comment 27 Zeeshan Ali 2016-04-21 16:23:12 UTC
Created attachment 326506 [details] [review]
vm-configurator: Don't autoport spice in new VMs

If we set autoport=true on spice display in new VMs, spice will still
expose the VM's display on a local TCP port even if Boxes itself
connnects to FD socket.
Comment 28 Zeeshan Ali 2016-04-21 16:23:27 UTC
Created attachment 326507 [details] [review]
vm-configurator: Don't expose SPICE in existing VMs

Update configuration of existing local VMs on launch to not expose SPICE
connection on TCP so other users can't connect to the display.
Comment 29 Zeeshan Ali 2016-04-21 16:23:39 UTC
Created attachment 326508 [details] [review]
spice-display: Special constructor for private connection

If SPICE is not exposed on TCP, we end up with SpiceDisplay constructor
failing due to port and tls_port both being 0. Let's add a special
constructor for the case of local private SPICE connection.
Comment 30 Zeeshan Ali 2016-04-21 16:23:56 UTC
Created attachment 326509 [details] [review]
display: uri property now nullable

In some cases URI won't make sense so let's make it nullable. One such
case is that of local private VMs where we connect through an FD and a
following patch will modify uri in SpiceDisplay null for this case.
Comment 31 Zeeshan Ali 2016-04-21 16:24:07 UTC
Created attachment 326510 [details] [review]
spice-display: No URI for local private case

In case of local private connection, a URI is irrelevant so let's not
ask Spice.Session to create a URI in this case as it'll fail to do so.
Comment 32 Zeeshan Ali 2016-04-21 16:26:56 UTC
Attachment 326505 [details] pushed as c53d60c - vm-configurator: Add add_graphics_device()
Attachment 326506 [details] pushed as c723802 - vm-configurator: Don't autoport spice in new VMs
Attachment 326507 [details] pushed as 58c8e90 - vm-configurator: Don't expose SPICE in existing VMs
Attachment 326508 [details] pushed as 77a24a3 - spice-display: Special constructor for private connection
Attachment 326509 [details] pushed as 0a0a2c2 - display: uri property now nullable
Attachment 326510 [details] pushed as 0e74f8a - spice-display: No URI for local private case