Bug 101354 - Minimal version of restricted, identifiable bus servers for containers (#100344)
Summary: Minimal version of restricted, identifiable bus servers for containers (#100344)
Status: ASSIGNED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: All All
: medium enhancement
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL: https://github.com/smcv/dbus/commits/...
Whiteboard: review?
Keywords: patch
Depends on: 101566 101567 101568 101569 101570 101362
Blocks: 100344
  Show dependency treegraph
 
Reported: 2017-06-08 16:58 UTC by Simon McVittie
Modified: 2017-06-23 16:44 UTC (History)
4 users (show)

See Also:
i915 platform:
i915 features:


Attachments
_dbus_asv_add_object_path: Add (2.67 KB, patch)
2017-06-23 16:17 UTC, Simon McVittie
Details | Splinter Review
bus_driver_send_ack_reply: Make available to other modules (3.71 KB, patch)
2017-06-23 16:24 UTC, Simon McVittie
Details | Splinter Review
spec: Document the initial Containers1 interface (18.26 KB, patch)
2017-06-23 16:25 UTC, Simon McVittie
Details | Splinter Review
driver: Add a stub implementation of the Containers1 interface (9.11 KB, patch)
2017-06-23 16:25 UTC, Simon McVittie
Details | Splinter Review
test/uid-permissions: Assert that AddContainerServer is privileged (3.44 KB, patch)
2017-06-23 16:26 UTC, Simon McVittie
Details | Splinter Review
test/containers: New test (5.53 KB, patch)
2017-06-23 16:26 UTC, Simon McVittie
Details | Splinter Review
containers: Do some basic checking on the parameters (3.69 KB, patch)
2017-06-23 16:26 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise the new parameter checking (4.00 KB, patch)
2017-06-23 16:27 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Build a global data structure for container instances (12.31 KB, patch)
2017-06-23 16:27 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Create a DBusServer and add it to the main loop (14.15 KB, patch)
2017-06-23 16:28 UTC, Simon McVittie
Details | Splinter Review
bus_context_add_incoming_connection: factor out (2.31 KB, patch)
2017-06-23 16:28 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Set up new connections to join the bus (862 bytes, patch)
2017-06-23 16:28 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise a successful call to AddContainerServer (5.94 KB, patch)
2017-06-23 16:28 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Require connecting uid to match caller of AddContainerServer (3.43 KB, patch)
2017-06-23 16:29 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Each connection to a container holds a reference (2.46 KB, patch)
2017-06-23 16:29 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Link each container to its initiating connection (7.94 KB, patch)
2017-06-23 16:30 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Shut down container servers when initiator goes away (3.39 KB, patch)
2017-06-23 16:30 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Give each instance a list of all its connections (3.46 KB, patch)
2017-06-23 16:31 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Implement methods to stop containers explicitly (6.43 KB, patch)
2017-06-23 16:31 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Don't allow stopping other users' containers (2.66 KB, patch)
2017-06-23 16:32 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise the various ways to stop a container (12.67 KB, patch)
2017-06-23 16:32 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Emit ContainerInstanceRemoved signal (4.97 KB, patch)
2017-06-23 16:32 UTC, Simon McVittie
Details | Splinter Review
test/containers: Assert that ContainerInstanceRemoved is emitted (7.63 KB, patch)
2017-06-23 16:32 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Indicate in loginfo whether connection is contained (3.58 KB, patch)
2017-06-23 16:33 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Treat connections from inside containers as unprivileged (2.54 KB, patch)
2017-06-23 16:33 UTC, Simon McVittie
Details | Splinter Review
test/containers: Check that containers can't make new containers (4.47 KB, patch)
2017-06-23 16:35 UTC, Simon McVittie
Details | Splinter Review
test/containers: Check that connections from containers are unprivileged (1.46 KB, patch)
2017-06-23 16:36 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Add a flag for methods that can't be invoked by containers (2.70 KB, patch)
2017-06-23 16:37 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Containers can't use the Verbose and Stats interfaces (1.80 KB, patch)
2017-06-23 16:38 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Add basic container info to GetConnectionCredentials() (1.56 KB, patch)
2017-06-23 16:38 UTC, Simon McVittie
Details | Splinter Review
test/dbus-daemon: Assert absence of Containers1 credentials (833 bytes, patch)
2017-06-23 16:38 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Add GetConnectionContainerInstance(), GetContainerInstanceInfo() (6.73 KB, patch)
2017-06-23 16:39 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise trivial and non-trivial container metadata (12.54 KB, patch)
2017-06-23 16:39 UTC, Simon McVittie
Details | Splinter Review
test/containers: Check that GetContainerInstanceInfo stops working (3.42 KB, patch)
2017-06-23 16:39 UTC, Simon McVittie
Details | Splinter Review
bus: Add (unused) settings for resource limits for containers (8.40 KB, patch)
2017-06-23 16:40 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Limit the size of metadata we will store (2.46 KB, patch)
2017-06-23 16:40 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Enforce max_containers limit (1.48 KB, patch)
2017-06-23 16:40 UTC, Simon McVittie
Details | Splinter Review
bus/containers: Enforce max_connections_per_container (1.43 KB, patch)
2017-06-23 16:40 UTC, Simon McVittie
Details | Splinter Review
containers: enforce max_containers_per_user (6.67 KB, patch)
2017-06-23 16:41 UTC, Simon McVittie
Details | Splinter Review
test/containers: Exercise the resource limits (16.80 KB, patch)
2017-06-23 16:41 UTC, Simon McVittie
Details | Splinter Review
bus/driver: Allow unprivileged connections to create app-containers (1.33 KB, patch)
2017-06-23 16:41 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2017-06-08 16:58:21 UTC
+++ This bug was initially created as a clone of Bug #100344 +++

In app-container projects like Flatpak, Snap, Firejail and AppImage, there's a need for a confined/contained/sandboxed app to be able to reach services on the normal session and system buses. However, unrestricted access to those buses is scary (and in particular, the session bus is not considered to be a security boundary, so various services there have APIs that are arbitrary code execution).

Bug #100344 outlines a plan to improve on that situation.

Here is what I think is necessary for a "minimum viable product" version of Bug #100344:

* Utility code to copy items from a message into a buffer, and from
  that buffer into a message (it will likely have to use
  _dbus_type_writer_write_reader)
    * Automated unit test

* Implement AddContainerServer(s: container_type, s: app_id,
  a{sv}: metadata, h: socket_to_accept,
  a{sv}: arguments) -> o: instance_id

    * Stub implementation that always fails
    * Call bus_driver_check_caller_is_privileged(), fail if not
    * Check that arguments is empty
    * Allocate a new instance ID, save the metadata in a data
      structure keyed by the instance ID
    * Create a new DBusServer and add it to the main loop
    * Watch for the calling connection to disconnect, and if it does,
      disconnect the DBusServer
    * Accept connections to the new DBusServer and add them to the
      bus as normal
    * Make bus_driver_check_caller_is_privileged() fail for 
      connections from the new DBusServer
    * Make connections from the new DBusServer unable to eavesdrop
    * Make sure connections from the new DBusServer can't call Stats
      methods, etc.

* Resource limit: make each new DBusServer "cost" one connection
  in the connections-per-uid quota

* Resource limit: maximum sandboxed servers per uid. Stop
  calling bus_driver_check_caller_is_privileged() from
  AddContainerServer, but still reject calls from a sandboxed
  connection, and enforce the resource limit
    * Automated test: Call AddContainerServer, wait for success,
      connect to the new socket
    * Automated test: Allow 2 sandboxed servers per uid. Call
      AddContainerServer 3 times from different connections; assert
      that the first two succeed and the third fails. Disconnect
      first connection, and wait for second connection to be notified
      that the name-owner has been lost. Call AddContainer a fourth
      time, assert it succeeds.

* Resource limit: maximum sandboxed connections per sandboxed server
    * Automated test: exceed the limit, assert that an extra
      attempt to connect is rejected. Disconnect enough sandboxed
      connections to be back below the limit. Assert that another
      attempt to connect succeeds.

* Associate basic metadata of the DBusServer with connections
  from the sandboxed server, and return it from
  GetConnectionCredentials
    * Automated test

* Implement GetConnectionContainerInstance
    * Automated test

* Implement ContainerInstanceRemoved signal
    * Automated test: close all sandboxed connections, then cause
      the server to be removed
    * Automated test: cause the server to be removed, then close
      all sandboxed connections

* Implement an 'as' property for the keys that are allowed in the
  arguments 'a{sv}' (initially an empty array)

    * Automated test

--------

Limitations to be lifted in separate patch serieses:

* no message filtering
* the container manager has to bind() and listen() on the socket,
  most likely in the host filesystem namespace, and then bind-mount
  it into the container; in future it should be able to pass in a
  socket that has not had bind() or listen() called on it yet,
  together with some way to tell the dbus-daemon when the socket
  is ready to be accept()ed
* the container manager has to stay on the bus until it's ready for
  the DBusServer to stop listening; in future it should be able to
   pass in some other way to stop the DBusServer
* no ability to virtualize the machine ID, do we want that?
* no ability to virtualize GetId(), do we want that?
Comment 1 Simon McVittie 2017-06-23 15:49:48 UTC
After a few tries at how the data structures and the order of implementation should look, I have an implementation of this. "Only" 49 commits - I'll split them up a bit into blocking bugs.

I haven't tested it "in real life", and I should check the test coverage with gcov, but I think the branch is ready for a first round of reviews.

My next step is to put a proof-of-concept implementation in Flatpak's D-Bus proxy, so that the D-Bus proxy is still responsible for filtering (and ensuring that the process has /proc/$pid/root/.flatpak-info for the older, more race-prone detection mechanism), but dbus-daemon has the opportunity to provide identification services.

(In reply to Simon McVittie from comment #0)
>     * Watch for the calling connection to disconnect, and if it does,
>       disconnect the DBusServer

We also need a method to stop listening explicitly. I added two: one which just stops, and one which also kills all the connections.

> * Resource limit: make each new DBusServer "cost" one connection
>   in the connections-per-uid quota

I decided against this. The limits I've implemented are:
max_containers
max_container_metadata_bytes
max_containers_per_user
max_connections_per_container
Comment 2 Simon McVittie 2017-06-23 16:17:34 UTC
Created attachment 132165 [details] [review]
_dbus_asv_add_object_path: Add
Comment 3 Simon McVittie 2017-06-23 16:24:09 UTC
Created attachment 132167 [details] [review]
bus_driver_send_ack_reply: Make available to other  modules
Comment 4 Simon McVittie 2017-06-23 16:24:57 UTC
Comment on attachment 132167 [details] [review]
bus_driver_send_ack_reply: Make available to other  modules

Moved to Bug #101567
Comment 5 Simon McVittie 2017-06-23 16:25:17 UTC
Created attachment 132169 [details] [review]
spec: Document the initial Containers1 interface
Comment 6 Simon McVittie 2017-06-23 16:25:33 UTC
Created attachment 132170 [details] [review]
driver: Add a stub implementation of the Containers1  interface

For now, this is considered to be a privileged operation, because the
resource-limiting isn't wired up yet. It only contains the bare minimum
of API.
Comment 7 Simon McVittie 2017-06-23 16:26:12 UTC
Created attachment 132171 [details] [review]
test/uid-permissions: Assert that AddContainerServer is  privileged

---

I'm not sure whether I've actually tested this as root yet.
Comment 8 Simon McVittie 2017-06-23 16:26:30 UTC
Created attachment 132172 [details] [review]
test/containers: New test

So far it only exercises SupportedArguments.
Comment 9 Simon McVittie 2017-06-23 16:26:52 UTC
Created attachment 132173 [details] [review]
containers: Do some basic checking on the parameters

In particular, we now fail early if we can't extract the file
descriptor, or if there are named parameters (none are supported yet).
Comment 10 Simon McVittie 2017-06-23 16:27:09 UTC
Created attachment 132174 [details] [review]
test/containers: Exercise the new parameter checking
Comment 11 Simon McVittie 2017-06-23 16:27:30 UTC
Created attachment 132175 [details] [review]
bus/containers: Build a global data structure for  container instances

We still don't actually create a DBusServer for incoming connections
at this point, much less accept incoming connections.
Comment 12 Simon McVittie 2017-06-23 16:28:01 UTC
Created attachment 132176 [details] [review]
bus/containers: Create a DBusServer and add it to the  main loop

This means we can accept connections on the new socket. For now, we
don't process them and they get closed.
Comment 13 Simon McVittie 2017-06-23 16:28:18 UTC
Created attachment 132177 [details] [review]
bus_context_add_incoming_connection: factor out
Comment 14 Simon McVittie 2017-06-23 16:28:36 UTC
Created attachment 132178 [details] [review]
bus/containers: Set up new connections to join the bus
Comment 15 Simon McVittie 2017-06-23 16:28:49 UTC
Created attachment 132179 [details] [review]
test/containers: Exercise a successful call to  AddContainerServer
Comment 16 Simon McVittie 2017-06-23 16:29:10 UTC
Created attachment 132180 [details] [review]
bus/containers: Require connecting uid to match caller  of AddContainerServer

If we're strict now, we can relax this later (either with a named
parameter or always); but if we're lenient now, we'll be stuck with it
forever, so be strict.
Comment 17 Simon McVittie 2017-06-23 16:29:30 UTC
Created attachment 132181 [details] [review]
bus/containers: Each connection to a container holds a  reference
Comment 18 Simon McVittie 2017-06-23 16:30:09 UTC
Created attachment 132182 [details] [review]
bus/containers: Link each container to its initiating  connection

We will need this to be able to shut down the container when its
creator vanishes.
Comment 19 Simon McVittie 2017-06-23 16:30:43 UTC
Created attachment 132183 [details] [review]
bus/containers: Shut down container servers when  initiator goes away

We will eventually want to have other ways to signal that a
container server should stop listening, so that the container manager
doesn't have to stay on D-Bus (fd-passing the read end of a pipe
whose write end will be closed by the container manager has been
suggested as easier to deal with for Flatpak/Bubblewrap), but for
now we're doing the simplest possible thing.
Comment 20 Simon McVittie 2017-06-23 16:31:01 UTC
Created attachment 132184 [details] [review]
bus/containers: Give each instance a list of all its  connections
Comment 21 Simon McVittie 2017-06-23 16:31:48 UTC
Created attachment 132185 [details] [review]
bus/containers: Implement methods to stop containers  explicitly

---

We don't really *need* these, but they follow the rule of thumb "if something can be gc'd by falling off the bus, then it should also be possible to remove it early".
Comment 22 Simon McVittie 2017-06-23 16:32:16 UTC
Created attachment 132186 [details] [review]
bus/containers: Don't allow stopping other users'  containers

On the system bus, that would be a denial of service, assuming we
relax the access-control from METHOD_FLAG_PRIVILEGED to a new
METHOD_FLAG_NOT_CONTAINERS later.
Comment 23 Simon McVittie 2017-06-23 16:32:28 UTC
Created attachment 132187 [details] [review]
test/containers: Exercise the various ways to stop a  container
Comment 24 Simon McVittie 2017-06-23 16:32:44 UTC
Created attachment 132188 [details] [review]
bus/containers: Emit ContainerInstanceRemoved signal
Comment 25 Simon McVittie 2017-06-23 16:32:59 UTC
Created attachment 132189 [details] [review]
test/containers: Assert that ContainerInstanceRemoved  is emitted
Comment 26 Simon McVittie 2017-06-23 16:33:13 UTC
Created attachment 132190 [details] [review]
bus/containers: Indicate in loginfo whether connection  is contained
Comment 27 Simon McVittie 2017-06-23 16:33:37 UTC
Created attachment 132191 [details] [review]
bus/driver: Treat connections from inside containers as  unprivileged

Even if the uid matches, a contained app shouldn't count as the owner
of the bus.
Comment 28 Simon McVittie 2017-06-23 16:35:17 UTC
Created attachment 132192 [details] [review]
test/containers: Check that containers can't make new  containers

We should prevent containers from trying to putting a container in our
container so we can contain while we contain; the implementation doesn't
actually have any concept of nesting or layering, so that would potentially
be privilege escalation.

At the moment, this is just prevented by METHOD_FLAG_PRIVILEGED. If we
remove that flag when we've introduced better resource limits, then
we can specifically restrict this method to not be called by
containers instead: this test will make sure we do.
Comment 29 Simon McVittie 2017-06-23 16:36:45 UTC
Created attachment 132193 [details] [review]
test/containers: Check that connections from containers  are unprivileged

---

I hope the difference between

                                       g_variant_new ("(a{ss})", NULL),

and

  dbus_message_iter_init_append (m, &args_iter);

  /* Append an empty a{ss} (string => string dictionary). */
  if (!dbus_message_iter_open_container (&args_iter, DBUS_TYPE_ARRAY,
        "{ss}", &arr_iter) ||
      !dbus_message_iter_close_container (&args_iter, &arr_iter))
    g_error ("OOM");

illustrates why I'm using GVariant for this test :-)
Comment 30 Simon McVittie 2017-06-23 16:37:05 UTC
Created attachment 132194 [details] [review]
bus/driver: Add a flag for methods that can't be  invoked by containers

We can consider relaxing AddContainerServer() from PRIVILEGED to
NOT_CONTAINERS when we've put resource limits in place.
Comment 31 Simon McVittie 2017-06-23 16:38:01 UTC
Created attachment 132195 [details] [review]
bus/driver: Containers can't use the Verbose and Stats  interfaces

These are debugging interfaces, which are essentially read-only.
By default, Verbose is not available on the system bus at all and
Stats is only available to uid 0, but both are available on the
session bus, and they can be allowed for other uids by configuring
the system bus.

---

We document how to enable Stats on the system bus, so I didn't want to just make it METHOD_FLAG_PRIVILEGED.
Comment 32 Simon McVittie 2017-06-23 16:38:15 UTC
Created attachment 132196 [details] [review]
bus/driver: Add basic container info to  GetConnectionCredentials()
Comment 33 Simon McVittie 2017-06-23 16:38:32 UTC
Created attachment 132197 [details] [review]
test/dbus-daemon: Assert absence of Containers1  credentials

These connections are not to a container server.
Comment 34 Simon McVittie 2017-06-23 16:39:00 UTC
Created attachment 132198 [details] [review]
bus/driver: Add GetConnectionContainerInstance(),  GetContainerInstanceInfo()

---

I am not particularly happy with the length of these names.
Comment 35 Simon McVittie 2017-06-23 16:39:31 UTC
Created attachment 132199 [details] [review]
test/containers: Exercise trivial and non-trivial  container metadata
Comment 36 Simon McVittie 2017-06-23 16:39:49 UTC
Created attachment 132200 [details] [review]
test/containers: Check that GetContainerInstanceInfo  stops working

After the container instance is removed, the method should not work.
Comment 37 Simon McVittie 2017-06-23 16:40:08 UTC
Created attachment 132201 [details] [review]
bus: Add (unused) settings for resource limits for  containers
Comment 38 Simon McVittie 2017-06-23 16:40:23 UTC
Created attachment 132202 [details] [review]
bus/containers: Limit the size of metadata we will  store
Comment 39 Simon McVittie 2017-06-23 16:40:39 UTC
Created attachment 132203 [details] [review]
bus/containers: Enforce max_containers limit
Comment 40 Simon McVittie 2017-06-23 16:40:53 UTC
Created attachment 132204 [details] [review]
bus/containers: Enforce max_connections_per_container
Comment 41 Simon McVittie 2017-06-23 16:41:07 UTC
Created attachment 132205 [details] [review]
containers: enforce max_containers_per_user
Comment 42 Simon McVittie 2017-06-23 16:41:23 UTC
Created attachment 132206 [details] [review]
test/containers: Exercise the resource limits
Comment 43 Simon McVittie 2017-06-23 16:41:44 UTC
Created attachment 132207 [details] [review]
bus/driver: Allow unprivileged connections to create  app-containers

This lets ordinary users create a limited number of app-containers
on the system bus.
Comment 44 Simon McVittie 2017-06-23 16:43:23 UTC
Also available at: https://github.com/smcv/dbus containers-minimum-101354
Comment 45 Simon McVittie 2017-06-23 16:44:50 UTC
Comment on attachment 132196 [details] [review]
bus/driver: Add basic container info to  GetConnectionCredentials()

Review of attachment 132196 [details] [review]:
-----------------------------------------------------------------

This should also be described in the commit that documents this stuff in the spec, of course.

I'm using the fully qualified (long) name to set a good example for others.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct.