From 39262d0a2913fc8ee951beb3d0241720abf651c0 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 2 Jun 2017 14:52:03 +0100 Subject: [PATCH 01/44] spec: Document the initial Containers1 interface Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- doc/dbus-specification.xml | 552 +++++++++++++++++++++++++++++++++++++ 1 file changed, 552 insertions(+) diff --git a/doc/dbus-specification.xml b/doc/dbus-specification.xml index c33f0a0c..abafb819 100644 --- a/doc/dbus-specification.xml +++ b/doc/dbus-specification.xml @@ -6416,6 +6416,44 @@ + + org.freedesktop.DBus.Containers1.Instance + OBJECT_PATH + + The container instance object path of the server through + which this connection is connected, as returned by + AddServer. Omitted from the + dictionary if this connection is not via a container's + server. + + + + + org.freedesktop.DBus.Containers1.Type + STRING + + The container technology that created the container + instance, as passed to AddServer. + Omitted from the dictionary if this connection is not + via a container's server. For example, a typical value + for a Flatpak application would be + org.flatpak. + + + + + org.freedesktop.DBus.Containers1.Name + STRING + + The contained app name for the container instance, + as passed to AddServer. Omitted + from the dictionary if this connection is not via a + container's server. For example, a typical value for + a Flatpak application would be + org.gnome.Weather. + + + @@ -6647,6 +6685,520 @@ + + <literal>org.freedesktop.DBus.Containers1.AddServer</literal> + + As a method: + + AddServer (in STRING container_type, + in STRING container_name, + in DICT<STRING,VARIANT> metadata, + in DICT<STRING,VARIANT> named_arguments, + out OBJECT_PATH container_instance, + out ARRAY<BYTE> socket_path, + out STRING connectable_address) + + Message arguments: + + + + + Argument + Type + Description + + + + + 0 + STRING + + Reversed domain name identifying a container manager + or container technology, such as + org.flatpak or + io.snapcraft. It must follow the + same syntactic rules as a + D-Bus + interface name. + + + + 1 + STRING + + Some unique identifier for an application or + container, whose meaning is defined by the + maintainers of the container type. If the container + type does not have a concept of identifying or + naming its applications or containers by a string, + using the empty string here is suggested. + + + + 2 + DICT<STRING,VARIANT> + + Metadata describing the application or container, with the + keys and values defined by the maintainers of the container + type. + + + + 3 + DICT<STRING,VARIANT> + + Additional arguments that extend this method. + The only named arguments that are allowed are the ones + listed in the + org.freedesktop.DBus.Containers1.SupportedArguments + property. All other named arguments are an error. + + + + 4 + OBJECT_PATH + + An opaque object path identifying the new container + instance. + + + + 5 + ARRAY<BYTE> + + The absolute filesystem path of the resulting + AF_UNIX socket, followed by a single + zero (nul) byte, for example + /run/user/1000/dbus-12345vwxyz\x00 + where \x00 represents the terminating zero byte. + + + + 6 + STRING + + A connectable D-Bus address, for example + unix:path=/run/user/1000/dbus-12345vwxyz. + + + + + + + + + Set up a new server socket for use in an application container. + Clients can connect to that socket, and the result will be as + though they had connected to the message bus, with a few differences. + The intention is that a container manager will call this method + to get a new server socket, bind-mount the server socket + into a location that is accessible by the confined (sandboxed) + application, and ensure that the normal message bus socket is + not accessible by the confined application. + + + + Each call to this method creates a new + container instance, identified by an object + path. Even if the specified container type and name are the + same as for a pre-existing container instance, each call + creates a new server with a new unique object path, because + the new container instance might represent a different + version of the confined application with different + characteristics and restrictions. The message bus may provide + an object at the given object path, but is not required to + do so. + + + + Metadata from the first three arguments is stored by the message + bus, but not interpreted; software that interacts with the + container manager can use this metadata. + + The method call may fail with the error + org.freedesktop.DBus.Error.LimitsExceeded + if the caller provides more metadata than the message bus + is willing to store. + + + + The last argument (the named arguments) + will be used in future versions of this specification to modify + the behaviour of this method call; all keys in this dictionary not + containing . are reserved by this specification + for this purpose. It can also contain implementation-specific + arguments for a particular message bus implementation, which + should start with a reversed domain name in the same way as + interface names. For example, GLib's gdbus-daemon might use + org.gtk.GDBus.SomeArgument if it used this + extension point. + + + + In the most basic form of a call to this method, the new server + socket is an AF_UNIX socket at a path chosen + by the message bus and returned from the method. Future versions + of this specification are likely to provide a way for the + message bus implementation to receive a socket from the caller + and arrange for it to listen for connections, using the named + arguments dictionary. + + + + Connections to the new server socket are said to be + confined. The message bus may prevent + confined connections from calling certain security-sensitive methods, + and may apply lower limits to these connections. However, container + managers and services must not rely on confined connections having + any particular filtering applied to them by the message bus. + + + + In the most basic form of a call to this method, the server + socket will cease to listen for new connections (in the same + way as if StopListening had been called + successfully) when the caller of this method disconnects from + the bus. Any confined connections that are already active are + unaffected. Future versions of this specification are likely + to provide a way for the caller to alter this behaviour by + specifying named arguments. + + + + The container instance object path remains valid for as + long as one or more confined connection via the same server + socket remain open, or there is a way for the server socket + to produce new connections in future (in other words, + either it is preparing to listen for new connections, or + it is currently listening for new connections). When the + container instance has ceased to listen for new connections + and no longer has any confined connections, the object path + becomes invalid, and API calls that specify it will fail with + the org.freedesktop.DBus.Error.NotContainer + error. + + + + + <literal>org.freedesktop.DBus.Containers1.StopInstance</literal> + + As a method: + + StopInstance (in OBJECT_PATH container_instance) + + Message arguments: + + + + + Argument + Type + Description + + + + + 0 + OBJECT_PATH + + The opaque object path that was returned from the + AddServer method, identifying a + container instance. + + + + + + + + + Terminate the container instance. The server will stop + listening for new connections, and any existing connections to + that server will be disconnected. When all connections have + been disconnected, the instance will cease to exist. + + + If the given object path does not represent a valid container + instance (see + ), the + org.freedesktop.DBus.Error.NotContainer + error is returned. In particular, if this method is called + twice, the second call will fail in this way. + + + If the given container instance exists but the caller of this + method is not allowed to stop it (for example because the + caller is in a container instance or because its user ID does + not match the user ID of the creator of the container + instance), + the org.freedesktop.DBus.Error.AccessDenied + error is returned and nothing is stopped. + + + + + <literal>org.freedesktop.DBus.Containers1.StopListening</literal> + + As a method: + + StopListening (in OBJECT_PATH container_instance) + + Message arguments: + + + + + Argument + Type + Description + + + + + 0 + OBJECT_PATH + + The opaque object path that was returned from the + AddServer method, identifying a + container instance. + + + + + + + + + Stop listening for new connections to the server in the given + container instance, but do not disconnect any existing + connections. + + + If this method is called more than once, while the container + instance still exists because connections to it are still + open, the second and subsequent calls will be successful but + will have no practical effect. + + + If the given object path does not represent a valid container + instance (see + ), the + org.freedesktop.DBus.Error.NotContainer + error is returned. In particular, this will happen if the + container instance already stopped listening, and all + connections to it (if any) were already closed. + + + If the given container instance exists but the caller of this + method is not allowed to stop it (for example because the + caller is in a container instance or because its user ID does + not match the user ID of the creator of the container + instance), + the org.freedesktop.DBus.Error.AccessDenied + error is returned and nothing is stopped. + + + + + <literal>org.freedesktop.DBus.Containers1.GetConnectionInstance</literal> + + As a method: + + GetConnectionInstance (in STRING bus_name, + out OBJECT_PATH container_instance, + out STRING container_type, + out STRING container_name, + out DICT<STRING,VARIANT> metadata) + + Message arguments: + + + + + Argument + Type + Description + + + + + 0 + STRING + + Unique or well-known bus name of the connection to + query, such as :12.34 or + com.example.tea. + + + + 1 + OBJECT_PATH + + The opaque object path that was returned from the + AddServer method, identifying a + container instance. + + + + 2 + STRING + + Reversed domain name identifying a container + manager or container technology, as passed to the + AddServer method, such as + org.flatpak or + io.snapcraft. + + + + 3 + STRING + + Some unique identifier for an application or container, + whose meaning is defined by the maintainers of the + container type. + + + + 4 + DICT<STRING,VARIANT> + + Metadata describing the application or container, with the + keys and values defined by the maintainers of the container + type. + + + + + + + + + If the given unique or well-known bus name is a connection to a + container server, return information about that container instance. + If the bus name exists but is not confined (in other words, if it + is a direct connection to the main message bus socket), the + org.freedesktop.DBus.Error.NotContainer error + is returned instead. If the given bus name has no owner at all, the + org.freedesktop.DBus.Error.NameHasNoOwner error + is returned instead. + + + + + <literal>org.freedesktop.DBus.Containers1.GetInstanceInfo</literal> + + As a method: + + GetInstanceInfo (in OBJECT_PATH container_instance, + out STRING container_type, + out STRING container_name, + out DICT<STRING,VARIANT> metadata) + + Message arguments: + + + + + Argument + Type + Description + + + + + 0 + OBJECT_PATH + + The opaque object path that was returned from the + AddServer method, identifying a + container instance. + + + + 1 + STRING + + Reversed domain name identifying a container + manager or container technology, as passed to the + AddServer method, such as + org.flatpak or + io.snapcraft. + + + + 2 + STRING + + Some unique identifier for an application or container, + whose meaning is defined by the maintainers of the + container type. + + + + 3 + DICT<STRING,VARIANT> + + Metadata describing the application or container, with the + keys and values defined by the maintainers of the container + type. + + + + + + + + + If the given object path represents a valid container instance, + (see ), + return information about it. Otherwise, the + org.freedesktop.DBus.Error.NotContainer error + is returned. + + + + + <literal>org.freedesktop.DBus.Containers1.InstanceRemoved</literal> + + As a signal emitted by the message bus: + + InstanceRemoved (OBJECT_PATH container_instance) + + Message arguments: + + + + + Argument + Type + Description + + + + + 0 + OBJECT_PATH + + The opaque object path that was returned from the + AddServer method, identifying a + container instance. + + + + + + + + + Emitted when a container instance ceases to exist. A container + instance continues to exist as long as it is listening for + new connections, or as long as connections to the instance + are open, whichever is longer. + + + <literal>org.freedesktop.DBus.Monitoring.BecomeMonitor</literal> From 88b3c319281d988d70253b17d59f7ba687042006 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 2 Jun 2017 14:51:54 +0100 Subject: [PATCH 02/44] 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. Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- README.cmake | 3 +++ bus/Makefile.am | 2 ++ bus/containers.c | 55 ++++++++++++++++++++++++++++++++++++++++ bus/containers.h | 35 +++++++++++++++++++++++++ bus/driver.c | 17 +++++++++++++ cmake/CMakeLists.txt | 1 + cmake/bus/CMakeLists.txt | 2 ++ cmake/config.h.cmake | 1 + configure.ac | 11 ++++++++ dbus/dbus-shared.h | 2 ++ 10 files changed, 129 insertions(+) create mode 100644 bus/containers.c create mode 100644 bus/containers.h diff --git a/README.cmake b/README.cmake index 69012fbe..6d5621fd 100644 --- a/README.cmake +++ b/README.cmake @@ -117,6 +117,9 @@ DBUS_ENABLE_DOXYGEN_DOCS:BOOL=OFF // enable bus daemon usage statistics DBUS_ENABLE_STATS:BOOL=OFF +// enable restricted servers for app containers +DBUS_ENABLE_CONTAINERS:BOOL=OFF + // support verbose debug mode DBUS_ENABLE_VERBOSE_MODE:BOOL=ON diff --git a/bus/Makefile.am b/bus/Makefile.am index 9ae30716..33751412 100644 --- a/bus/Makefile.am +++ b/bus/Makefile.am @@ -97,6 +97,8 @@ BUS_SOURCES= \ config-parser-common.h \ connection.c \ connection.h \ + containers.c \ + containers.h \ desktop-file.c \ desktop-file.h \ $(DIR_WATCH_SOURCE) \ diff --git a/bus/containers.c b/bus/containers.c new file mode 100644 index 00000000..e8935490 --- /dev/null +++ b/bus/containers.c @@ -0,0 +1,55 @@ +/* containers.c - restricted bus servers for containers + * + * Copyright © 2017 Collabora Ltd. + * + * Licensed under the Academic Free License version 2.1 + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA + * 02110-1301 USA + */ + +#include +#include "containers.h" + +#ifdef DBUS_ENABLE_CONTAINERS + +#ifndef DBUS_UNIX +# error DBUS_ENABLE_CONTAINERS requires DBUS_UNIX +#endif + +dbus_bool_t +bus_containers_handle_add_server (DBusConnection *connection, + BusTransaction *transaction, + DBusMessage *message, + DBusError *error) +{ + dbus_set_error (error, DBUS_ERROR_NOT_SUPPORTED, "Not yet implemented"); + return FALSE; +} + +dbus_bool_t +bus_containers_supported_arguments_getter (BusContext *context, + DBusMessageIter *var_iter) +{ + DBusMessageIter arr_iter; + + /* There are none so far */ + return dbus_message_iter_open_container (var_iter, DBUS_TYPE_ARRAY, + DBUS_TYPE_STRING_AS_STRING, + &arr_iter) && + dbus_message_iter_close_container (var_iter, &arr_iter); +} + +#endif /* DBUS_ENABLE_CONTAINERS */ diff --git a/bus/containers.h b/bus/containers.h new file mode 100644 index 00000000..3564bbd2 --- /dev/null +++ b/bus/containers.h @@ -0,0 +1,35 @@ +/* containers.h - restricted bus servers for containers + * + * Copyright © 2017 Collabora Ltd. + * + * Licensed under the Academic Free License version 2.1 + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA + * 02110-1301 USA + */ + +#ifndef BUS_CONTAINERS_H +#define BUS_CONTAINERS_H + +#include "bus.h" + +dbus_bool_t bus_containers_handle_add_server (DBusConnection *connection, + BusTransaction *transaction, + DBusMessage *message, + DBusError *error); +dbus_bool_t bus_containers_supported_arguments_getter (BusContext *context, + DBusMessageIter *var_iter); + +#endif /* multiple-inclusion guard */ diff --git a/bus/driver.c b/bus/driver.c index cd0a714d..9529b07c 100644 --- a/bus/driver.c +++ b/bus/driver.c @@ -26,6 +26,7 @@ #include "activation.h" #include "apparmor.h" #include "connection.h" +#include "containers.h" #include "driver.h" #include "dispatch.h" #include "services.h" @@ -2517,6 +2518,18 @@ static const MessageHandler introspectable_message_handlers[] = { { NULL, NULL, NULL, NULL } }; +#ifdef DBUS_ENABLE_CONTAINERS +static const MessageHandler containers_message_handlers[] = { + { "AddServer", "ssa{sv}a{sv}", "oays", bus_containers_handle_add_server, + METHOD_FLAG_PRIVILEGED }, + { NULL, NULL, NULL, NULL } +}; +static const PropertyHandler containers_property_handlers[] = { + { "SupportedArguments", "as", bus_containers_supported_arguments_getter }, + { NULL, NULL, NULL } +}; +#endif + static const MessageHandler monitoring_message_handlers[] = { { "BecomeMonitor", "asu", "", bus_driver_handle_become_monitor, METHOD_FLAG_PRIVILEGED }, @@ -2620,6 +2633,10 @@ static InterfaceHandler interface_handlers[] = { #ifdef DBUS_ENABLE_STATS { BUS_INTERFACE_STATS, stats_message_handlers, NULL, INTERFACE_FLAG_NONE }, +#endif +#ifdef DBUS_ENABLE_CONTAINERS + { DBUS_INTERFACE_CONTAINERS1, containers_message_handlers, NULL, + INTERFACE_FLAG_NONE, containers_property_handlers }, #endif { DBUS_INTERFACE_PEER, peer_message_handlers, NULL, /* Not in the Interfaces property because it's a pseudo-interface diff --git a/cmake/CMakeLists.txt b/cmake/CMakeLists.txt index 3ac71a5a..cebf8169 100644 --- a/cmake/CMakeLists.txt +++ b/cmake/CMakeLists.txt @@ -128,6 +128,7 @@ endif(NOT WIN32) option (DBUS_DISABLE_ASSERT "Disable assertion checking" OFF) option (DBUS_ENABLE_STATS "enable bus daemon usage statistics" OFF) +option (DBUS_ENABLE_CONTAINERS "enable restricted servers for app-containers" OFF) if(WIN32) set(FD_SETSIZE "8192" CACHE STRING "The maximum number of connections that can be handled at once") diff --git a/cmake/bus/CMakeLists.txt b/cmake/bus/CMakeLists.txt index 4c5bdcf2..9e806c4f 100644 --- a/cmake/bus/CMakeLists.txt +++ b/cmake/bus/CMakeLists.txt @@ -52,6 +52,8 @@ set (BUS_SOURCES # ${BUS_DIR}/config-parser-trivial.c ${BUS_DIR}/connection.c ${BUS_DIR}/connection.h + ${BUS_DIR}/containers.c + ${BUS_DIR}/containers.h ${BUS_DIR}/desktop-file.c ${BUS_DIR}/desktop-file.h ${BUS_DIR}/dir-watch.h diff --git a/cmake/config.h.cmake b/cmake/config.h.cmake index 202c0ab0..efba76d1 100644 --- a/cmake/config.h.cmake +++ b/cmake/config.h.cmake @@ -28,6 +28,7 @@ #cmakedefine DBUS_RUNSTATEDIR "@DBUS_RUNSTATEDIR@" #cmakedefine DBUS_ENABLE_STATS +#cmakedefine DBUS_ENABLE_CONTAINERS #define TEST_LISTEN "@TEST_LISTEN@" diff --git a/configure.ac b/configure.ac index ce1f2c56..2ab704b3 100644 --- a/configure.ac +++ b/configure.ac @@ -1762,6 +1762,16 @@ AC_ARG_ENABLE([user-session], AM_CONDITIONAL([DBUS_ENABLE_USER_SESSION], [test "x$enable_user_session" = xyes]) +AC_ARG_ENABLE([containers], + [AS_HELP_STRING([--enable-containers], + [enable restricted servers for app containers])], + [], [enable_containers=no]) +AS_IF([test "x$enable_containers" = xyes && test "x$dbus_unix" != xyes], + [AC_MSG_ERROR([Restricted servers for app containers require Unix])]) +AS_IF([test "x$enable_containers" = xyes], + [AC_DEFINE([DBUS_ENABLE_CONTAINERS], [1], + [Define to enable restricted servers for app containers])]) + AC_CONFIG_FILES([ Doxyfile dbus/Version @@ -1842,6 +1852,7 @@ echo " Building assertions: ${enable_asserts} Building checks: ${enable_checks} Building bus stats API: ${enable_stats} + Building container API: ${enable_containers} Building SELinux support: ${have_selinux} Building AppArmor support: ${have_apparmor} Building inotify support: ${have_inotify} diff --git a/dbus/dbus-shared.h b/dbus/dbus-shared.h index 7ab91035..f20c72ad 100644 --- a/dbus/dbus-shared.h +++ b/dbus/dbus-shared.h @@ -86,6 +86,8 @@ typedef enum */ /** The interface exported by the object with #DBUS_SERVICE_DBUS and #DBUS_PATH_DBUS */ #define DBUS_INTERFACE_DBUS "org.freedesktop.DBus" +/** The restricted container interface exported by the dbus-daemon */ +#define DBUS_INTERFACE_CONTAINERS1 "org.freedesktop.DBus.Containers1" /** The monitoring interface exported by the dbus-daemon */ #define DBUS_INTERFACE_MONITORING "org.freedesktop.DBus.Monitoring" From 333558d67ede6bcf6b8281cb3dcdde336e715450 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 25 Jul 2017 12:43:40 +0100 Subject: [PATCH 03/44] travis-ci: Do at least one build with and one without containers Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- tools/ci-build.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/ci-build.sh b/tools/ci-build.sh index d0938ee2..33d5ffe1 100755 --- a/tools/ci-build.sh +++ b/tools/ci-build.sh @@ -127,6 +127,7 @@ case "$ci_buildsys" in set "$@" --enable-developer --enable-tests # Enable optional features that are off by default if [ "$ci_host" != mingw ]; then + set "$@" --enable-containers set "$@" --enable-user-session fi shift @@ -153,6 +154,7 @@ case "$ci_buildsys" in set "$@" --disable-libaudit --without-valgrind # Disable optional features, some of which are on by # default + set "$@" --disable-containers set "$@" --disable-stats set "$@" --disable-user-session shift From b37fa3e8ca7891ad50d27eb004190c9111a3b19a Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 20 Jun 2017 17:36:59 +0100 Subject: [PATCH 04/44] test/uid-permissions: Assert that AddServer is privileged Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- test/uid-permissions.c | 81 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/test/uid-permissions.c b/test/uid-permissions.c index 6ad66e62..061333f5 100644 --- a/test/uid-permissions.c +++ b/test/uid-permissions.c @@ -187,6 +187,78 @@ test_monitor (Fixture *f, dbus_clear_message (&m); } +/* + * Assert that AddServer() can be called by the owner of the bus + * (TEST_USER_MESSAGEBUS) or by root, but cannot be called by other + * users for now. + */ +static void +test_containers (Fixture *f, + gconstpointer context) +{ +#ifdef DBUS_ENABLE_CONTAINERS + const Config *config = context; +#endif + DBusMessage *m; + DBusPendingCall *pc; + + if (f->skip) + return; + + /* We cheat and pass the wrong arguments, because passing an a{sv} with + * the libdbus API is really long-winded. The bus driver code checks + * for privileged or unprivileged access before it checks the arguments + * anyway. */ + m = dbus_message_new_method_call (DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, DBUS_INTERFACE_CONTAINERS1, "AddServer"); + + if (m == NULL) + g_error ("OOM"); + + if (!dbus_connection_send_with_reply (f->conn, m, &pc, + DBUS_TIMEOUT_USE_DEFAULT) || + pc == NULL) + g_error ("OOM"); + + dbus_clear_message (&m); + + if (dbus_pending_call_get_completed (pc)) + test_pending_call_store_reply (pc, &m); + else if (!dbus_pending_call_set_notify (pc, test_pending_call_store_reply, + &m, NULL)) + g_error ("OOM"); + + while (m == NULL) + test_main_context_iterate (f->ctx, TRUE); + +#ifdef DBUS_ENABLE_CONTAINERS + if (config->expect_success) + { + /* It would have succeeded if we'd passed the right arguments! */ + g_assert_cmpint (dbus_message_get_type (m), ==, DBUS_MESSAGE_TYPE_ERROR); + g_assert_cmpstr (dbus_message_get_error_name (m), ==, + DBUS_ERROR_INVALID_ARGS); + g_assert_cmpstr (dbus_message_get_signature (m), ==, "s"); + } + else + { + /* It fails, yielding an error message with one string argument */ + g_assert_cmpint (dbus_message_get_type (m), ==, DBUS_MESSAGE_TYPE_ERROR); + g_assert_cmpstr (dbus_message_get_error_name (m), ==, + DBUS_ERROR_ACCESS_DENIED); + g_assert_cmpstr (dbus_message_get_signature (m), ==, "s"); + } +#else + g_assert_cmpint (dbus_message_get_type (m), ==, DBUS_MESSAGE_TYPE_ERROR); + g_assert_cmpstr (dbus_message_get_error_name (m), ==, + DBUS_ERROR_UNKNOWN_INTERFACE); + g_assert_cmpstr (dbus_message_get_signature (m), ==, "s"); +#endif + + dbus_clear_pending_call (&pc); + dbus_clear_message (&m); +} + static void teardown (Fixture *f, gconstpointer context G_GNUC_UNUSED) @@ -252,5 +324,14 @@ main (int argc, g_test_add ("/uid-permissions/monitor/other", Fixture, &other_fail_config, setup, test_monitor, teardown); + /* AddServer has the same behaviour */ + g_test_add ("/uid-permissions/containers/root", Fixture, &root_ok_config, + setup, test_containers, teardown); + g_test_add ("/uid-permissions/containers/messagebus", Fixture, + &messagebus_ok_config, + setup, test_containers, teardown); + g_test_add ("/uid-permissions/containers/other", Fixture, &other_fail_config, + setup, test_containers, teardown); + return g_test_run (); } From 3d2028dfe0d94247af67edcf680d38a99bcbc71b Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 20 Jun 2017 17:37:33 +0100 Subject: [PATCH 05/44] test/containers: New test So far it only exercises SupportedArguments. Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- test/Makefile.am | 7 +++ test/containers.c | 157 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 164 insertions(+) create mode 100644 test/containers.c diff --git a/test/Makefile.am b/test/Makefile.am index 57342034..130f5600 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -181,6 +181,7 @@ installable_helpers += \ $(NULL) installable_tests += \ + test-containers \ test-sd-activation \ $(NULL) @@ -286,6 +287,12 @@ test_apparmor_activation_LDADD = \ $(NULL) endif +test_containers_SOURCES = containers.c +test_containers_LDADD = \ + libdbus-testutils.la \ + $(GLIB_LIBS) \ + $(NULL) + test_corrupt_SOURCES = corrupt.c test_corrupt_LDADD = \ libdbus-testutils.la \ diff --git a/test/containers.c b/test/containers.c new file mode 100644 index 00000000..5e2387e5 --- /dev/null +++ b/test/containers.c @@ -0,0 +1,157 @@ +/* Integration tests for restricted sockets for containers + * + * Copyright © 2017 Collabora Ltd. + * + * Permission is hereby granted, free of charge, to any person + * obtaining a copy of this software and associated documentation files + * (the "Software"), to deal in the Software without restriction, + * including without limitation the rights to use, copy, modify, merge, + * publish, distribute, sublicense, and/or sell copies of the Software, + * and to permit persons to whom the Software is furnished to do so, + * subject to the following conditions: + * + * The above copyright notice and this permission notice shall be + * included in all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +#include + +#include + +#include +#include +#include + +#if defined(DBUS_ENABLE_CONTAINERS) && defined(HAVE_GIO_UNIX) +#define HAVE_CONTAINERS_TEST +#include +#include +#endif + +#include "test-utils-glib.h" + +typedef struct { + gboolean skip; + gchar *bus_address; + GPid daemon_pid; + GError *error; + + GDBusProxy *proxy; + + GDBusConnection *unconfined_conn; +} Fixture; + +static void +setup (Fixture *f, + gconstpointer context) +{ + f->bus_address = test_get_dbus_daemon (NULL, TEST_USER_ME, NULL, + &f->daemon_pid); + + if (f->bus_address == NULL) + { + f->skip = TRUE; + return; + } + + f->unconfined_conn = g_dbus_connection_new_for_address_sync (f->bus_address, + (G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION | + G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT), + NULL, NULL, &f->error); + g_assert_no_error (f->error); +} + +/* + * Assert that Get(SupportedArguments) contains what we expect it to. + */ +static void +test_get_supported_arguments (Fixture *f, + gconstpointer context) +{ + GVariant *v; +#ifdef DBUS_ENABLE_CONTAINERS + const gchar **args; + gsize len; +#endif + + if (f->skip) + return; + + f->proxy = g_dbus_proxy_new_sync (f->unconfined_conn, G_DBUS_PROXY_FLAGS_NONE, + NULL, DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, DBUS_INTERFACE_CONTAINERS1, + NULL, &f->error); + + /* This one is DBUS_ENABLE_CONTAINERS rather than HAVE_CONTAINERS_TEST + * because we can still test whether the interface appears or not, even + * if we were not able to detect gio-unix-2.0 */ +#ifdef DBUS_ENABLE_CONTAINERS + g_assert_no_error (f->error); + + v = g_dbus_proxy_get_cached_property (f->proxy, "SupportedArguments"); + g_assert_cmpstr (g_variant_get_type_string (v), ==, "as"); + args = g_variant_get_strv (v, &len); + + /* No arguments are defined yet */ + g_assert_cmpuint (len, ==, 0); + + g_free (args); + g_variant_unref (v); +#else /* !DBUS_ENABLE_CONTAINERS */ + g_assert_no_error (f->error); + v = g_dbus_proxy_get_cached_property (f->proxy, "SupportedArguments"); + g_assert_null (v); +#endif /* !DBUS_ENABLE_CONTAINERS */ +} + +static void +teardown (Fixture *f, + gconstpointer context G_GNUC_UNUSED) +{ + g_clear_object (&f->proxy); + + if (f->unconfined_conn != NULL) + { + GError *error = NULL; + + g_dbus_connection_close_sync (f->unconfined_conn, NULL, &error); + + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CLOSED)) + g_clear_error (&error); + else + g_assert_no_error (error); + } + + g_clear_object (&f->unconfined_conn); + + if (f->daemon_pid != 0) + { + test_kill_pid (f->daemon_pid); + g_spawn_close_pid (f->daemon_pid); + f->daemon_pid = 0; + } + + g_free (f->bus_address); + g_clear_error (&f->error); +} + +int +main (int argc, + char **argv) +{ + test_init (&argc, &argv); + + g_test_add ("/containers/get-supported-arguments", Fixture, NULL, + setup, test_get_supported_arguments, teardown); + + return g_test_run (); +} From fd3f00364fb5ab1829e7ed35589c7421ff8f039e Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 2 Jun 2017 19:32:24 +0100 Subject: [PATCH 06/44] bus/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). Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/containers.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/bus/containers.c b/bus/containers.c index e8935490..cf9156f4 100644 --- a/bus/containers.c +++ b/bus/containers.c @@ -29,13 +29,86 @@ # error DBUS_ENABLE_CONTAINERS requires DBUS_UNIX #endif +#include "dbus/dbus-internals.h" +#include "dbus/dbus-sysdeps-unix.h" + dbus_bool_t bus_containers_handle_add_server (DBusConnection *connection, BusTransaction *transaction, DBusMessage *message, DBusError *error) { + DBusMessageIter iter; + DBusMessageIter dict_iter; + const char *type; + const char *name; + + /* We already checked this in bus_driver_handle_message() */ + _dbus_assert (dbus_message_has_signature (message, "ssa{sv}a{sv}")); + + /* Argument 0: Container type */ + if (!dbus_message_iter_init (message, &iter)) + _dbus_assert_not_reached ("Message type was already checked"); + + _dbus_assert (dbus_message_iter_get_arg_type (&iter) == DBUS_TYPE_STRING); + dbus_message_iter_get_basic (&iter, &type); + + if (!dbus_validate_interface (type, NULL)) + { + dbus_set_error (error, DBUS_ERROR_INVALID_ARGS, + "The container type identifier must have the " + "syntax of an interface name"); + goto fail; + } + + /* Argument 1: Name as defined by container manager */ + if (!dbus_message_iter_next (&iter)) + _dbus_assert_not_reached ("Message type was already checked"); + + _dbus_assert (dbus_message_iter_get_arg_type (&iter) == DBUS_TYPE_STRING); + dbus_message_iter_get_basic (&iter, &name); + + /* Argument 2: Metadata as defined by container manager */ + if (!dbus_message_iter_next (&iter)) + _dbus_assert_not_reached ("Message type was already checked"); + + _dbus_assert (dbus_message_iter_get_arg_type (&iter) == DBUS_TYPE_ARRAY); + /* TODO: Copy the metadata */ + + /* Argument 3: Named parameters */ + if (!dbus_message_iter_next (&iter)) + _dbus_assert_not_reached ("Message type was already checked"); + + _dbus_assert (dbus_message_iter_get_arg_type (&iter) == DBUS_TYPE_ARRAY); + dbus_message_iter_recurse (&iter, &dict_iter); + + while (dbus_message_iter_get_arg_type (&dict_iter) != DBUS_TYPE_INVALID) + { + DBusMessageIter pair_iter; + const char *param_name; + + _dbus_assert (dbus_message_iter_get_arg_type (&dict_iter) == + DBUS_TYPE_DICT_ENTRY); + + dbus_message_iter_recurse (&dict_iter, &pair_iter); + _dbus_assert (dbus_message_iter_get_arg_type (&pair_iter) == + DBUS_TYPE_STRING); + dbus_message_iter_get_basic (&pair_iter, ¶m_name); + + /* If we supported any named parameters, we'd copy them into a data + * structure here; but we don't, so fail instead. */ + dbus_set_error (error, DBUS_ERROR_INVALID_ARGS, + "Named parameter %s is not understood", param_name); + goto fail; + } + + /* End of arguments */ + _dbus_assert (!dbus_message_iter_has_next (&iter)); + + /* TODO: Actually implement the method */ dbus_set_error (error, DBUS_ERROR_NOT_SUPPORTED, "Not yet implemented"); + +fail: return FALSE; } From e65d6cf1ef74969b72ec3a4363aba1545109f409 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 2 Jun 2017 19:35:56 +0100 Subject: [PATCH 07/44] test/containers: Exercise the new parameter checking Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- test/containers.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/test/containers.c b/test/containers.c index 5e2387e5..fef6972c 100644 --- a/test/containers.c +++ b/test/containers.c @@ -113,6 +113,91 @@ test_get_supported_arguments (Fixture *f, #endif /* !DBUS_ENABLE_CONTAINERS */ } +/* + * Assert that named arguments are validated: passing an unsupported + * named argument causes an error. + */ +static void +test_unsupported_parameter (Fixture *f, + gconstpointer context) +{ +#ifdef HAVE_CONTAINERS_TEST + GVariant *tuple; + GVariant *parameters; + GVariantDict named_argument_builder; + + if (f->skip) + return; + + f->proxy = g_dbus_proxy_new_sync (f->unconfined_conn, + G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES, + NULL, DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, DBUS_INTERFACE_CONTAINERS1, + NULL, &f->error); + g_assert_no_error (f->error); + + g_variant_dict_init (&named_argument_builder, NULL); + g_variant_dict_insert (&named_argument_builder, + "ThisArgumentIsntImplemented", + "b", FALSE); + + parameters = g_variant_new ("(ssa{sv}@a{sv})", + "com.example.NotFlatpak", + "sample-app", + NULL, /* no metadata */ + g_variant_dict_end (&named_argument_builder)); + tuple = g_dbus_proxy_call_sync (f->proxy, "AddServer", + g_steal_pointer (¶meters), + G_DBUS_CALL_FLAGS_NONE, -1, NULL, &f->error); + + g_assert_error (f->error, G_DBUS_ERROR, G_DBUS_ERROR_INVALID_ARGS); + g_assert_null (tuple); + g_clear_error (&f->error); +#else /* !HAVE_CONTAINERS_TEST */ + g_test_skip ("Containers or gio-unix-2.0 not supported"); +#endif /* !HAVE_CONTAINERS_TEST */ +} + +/* + * Assert that container types are validated: a container type (container + * technology) that is not a syntactically valid D-Bus interface name + * causes an error. + */ +static void +test_invalid_type_name (Fixture *f, + gconstpointer context) +{ +#ifdef HAVE_CONTAINERS_TEST + GVariant *tuple; + GVariant *parameters; + + if (f->skip) + return; + + f->proxy = g_dbus_proxy_new_sync (f->unconfined_conn, + G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES, + NULL, DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, DBUS_INTERFACE_CONTAINERS1, + NULL, &f->error); + g_assert_no_error (f->error); + + parameters = g_variant_new ("(ssa{sv}a{sv})", + "this is not a valid container type name", + "sample-app", + NULL, /* no metadata */ + NULL); /* no named arguments */ + tuple = g_dbus_proxy_call_sync (f->proxy, "AddServer", + g_steal_pointer (¶meters), + G_DBUS_CALL_FLAGS_NONE, -1, NULL, &f->error); + + g_assert_error (f->error, G_DBUS_ERROR, G_DBUS_ERROR_INVALID_ARGS); + g_assert_null (tuple); + g_clear_error (&f->error); +#else /* !HAVE_CONTAINERS_TEST */ + g_test_skip ("Containers or gio-unix-2.0 not supported"); +#endif /* !HAVE_CONTAINERS_TEST */ +} + static void teardown (Fixture *f, gconstpointer context G_GNUC_UNUSED) @@ -152,6 +237,10 @@ main (int argc, g_test_add ("/containers/get-supported-arguments", Fixture, NULL, setup, test_get_supported_arguments, teardown); + g_test_add ("/containers/unsupported-parameter", Fixture, NULL, + setup, test_unsupported_parameter, teardown); + g_test_add ("/containers/invalid-type-name", Fixture, NULL, + setup, test_invalid_type_name, teardown); return g_test_run (); } From 7cd2354b993366f0b45b19cced9d220d9f336c39 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 20 Jun 2017 12:31:18 +0100 Subject: [PATCH 08/44] 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. Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/bus.c | 17 ++++ bus/bus.h | 2 + bus/containers.c | 240 ++++++++++++++++++++++++++++++++++++++++++++++- bus/containers.h | 12 +++ 4 files changed, 268 insertions(+), 3 deletions(-) diff --git a/bus/bus.c b/bus/bus.c index 295dc7b5..8b08b8ea 100644 --- a/bus/bus.c +++ b/bus/bus.c @@ -29,6 +29,7 @@ #include "activation.h" #include "connection.h" +#include "containers.h" #include "services.h" #include "utils.h" #include "policy.h" @@ -69,6 +70,7 @@ struct BusContext BusMatchmaker *matchmaker; BusLimits limits; DBusRLimit *initial_fd_limit; + BusContainers *containers; unsigned int fork : 1; unsigned int syslog : 1; unsigned int keep_umask : 1; @@ -887,6 +889,14 @@ bus_context_new (const DBusString *config_file, goto failed; } + context->containers = bus_containers_new (); + + if (context->containers == NULL) + { + BUS_SET_OOM (error); + goto failed; + } + /* check user before we fork */ if (context->user != NULL) { @@ -1172,6 +1182,7 @@ bus_context_unref (BusContext *context) context->matchmaker = NULL; } + bus_clear_containers (&context->containers); dbus_free (context->config_file); dbus_free (context->log_prefix); dbus_free (context->type); @@ -1282,6 +1293,12 @@ bus_context_get_policy (BusContext *context) return context->policy; } +BusContainers * +bus_context_get_containers (BusContext *context) +{ + return context->containers; +} + BusClientPolicy* bus_context_create_client_policy (BusContext *context, DBusConnection *connection, diff --git a/bus/bus.h b/bus/bus.h index 647f9988..edc95773 100644 --- a/bus/bus.h +++ b/bus/bus.h @@ -45,6 +45,7 @@ typedef struct BusTransaction BusTransaction; typedef struct BusMatchmaker BusMatchmaker; typedef struct BusMatchRule BusMatchRule; typedef struct BusActivationEntry BusActivationEntry; +typedef struct BusContainers BusContainers; typedef struct { @@ -106,6 +107,7 @@ dbus_bool_t bus_context_allow_unix_user (BusContext dbus_bool_t bus_context_allow_windows_user (BusContext *context, const char *windows_sid); BusPolicy* bus_context_get_policy (BusContext *context); +BusContainers *bus_context_get_containers (BusContext *context); BusClientPolicy* bus_context_create_client_policy (BusContext *context, DBusConnection *connection, diff --git a/bus/containers.c b/bus/containers.c index cf9156f4..1c922952 100644 --- a/bus/containers.c +++ b/bus/containers.c @@ -23,15 +23,185 @@ #include #include "containers.h" +#include "dbus/dbus-internals.h" + #ifdef DBUS_ENABLE_CONTAINERS #ifndef DBUS_UNIX # error DBUS_ENABLE_CONTAINERS requires DBUS_UNIX #endif -#include "dbus/dbus-internals.h" +#include "dbus/dbus-hash.h" +#include "dbus/dbus-message-internal.h" #include "dbus/dbus-sysdeps-unix.h" +#include "connection.h" +#include "utils.h" + +/* + * A container instance groups together a per-app-container server with + * all the connections for which it is responsible. + */ +typedef struct +{ + int refcount; + char *path; + char *type; + char *name; + DBusVariant *metadata; + BusContext *context; + BusContainers *containers; +} BusContainerInstance; + +/* + * Singleton data structure encapsulating the container-related parts of + * a BusContext. + */ +struct BusContainers +{ + int refcount; + /* path borrowed from BusContainerInstance => unowned BusContainerInstance + * The BusContainerInstance removes itself from here on destruction. */ + DBusHashTable *instances_by_path; + dbus_uint64_t next_container_id; +}; + +BusContainers * +bus_containers_new (void) +{ + /* We allocate the hash table lazily, expecting that the common case will + * be a connection where this feature is never used */ + BusContainers *self = dbus_new0 (BusContainers, 1); + + if (self == NULL) + return NULL; + + self->refcount = 1; + self->instances_by_path = NULL; + self->next_container_id = DBUS_UINT64_CONSTANT (0); + return self; +} + +BusContainers * +bus_containers_ref (BusContainers *self) +{ + _dbus_assert (self->refcount > 0); + _dbus_assert (self->refcount < _DBUS_INT_MAX); + + self->refcount++; + return self; +} + +void +bus_containers_unref (BusContainers *self) +{ + _dbus_assert (self != NULL); + _dbus_assert (self->refcount > 0); + + if (--self->refcount == 0) + { + _dbus_clear_hash_table (&self->instances_by_path); + dbus_free (self); + } +} + +static void +bus_container_instance_unref (BusContainerInstance *self) +{ + _dbus_assert (self->refcount > 0); + + if (--self->refcount == 0) + { + /* It's OK to do this even if we were never added to instances_by_path, + * because the paths are globally unique. */ + if (self->path != NULL && self->containers->instances_by_path != NULL) + _dbus_hash_table_remove_string (self->containers->instances_by_path, + self->path); + + _dbus_clear_variant (&self->metadata); + bus_context_unref (self->context); + bus_containers_unref (self->containers); + dbus_free (self->path); + dbus_free (self->type); + dbus_free (self->name); + dbus_free (self); + } +} + +static inline void +bus_clear_container_instance (BusContainerInstance **instance_p) +{ + _dbus_clear_pointer_impl (BusContainerInstance, instance_p, + bus_container_instance_unref); +} + +static BusContainerInstance * +bus_container_instance_new (BusContext *context, + BusContainers *containers, + DBusError *error) +{ + BusContainerInstance *self = NULL; + DBusString path = _DBUS_STRING_INIT_INVALID; + + _dbus_assert (context != NULL); + _dbus_assert (containers != NULL); + _DBUS_ASSERT_ERROR_IS_CLEAR (error); + + if (!_dbus_string_init (&path)) + { + BUS_SET_OOM (error); + goto fail; + } + + self = dbus_new0 (BusContainerInstance, 1); + + if (self == NULL) + { + BUS_SET_OOM (error); + goto fail; + } + + self->refcount = 1; + self->type = NULL; + self->name = NULL; + self->metadata = NULL; + self->context = bus_context_ref (context); + self->containers = bus_containers_ref (containers); + + if (containers->next_container_id >= + DBUS_UINT64_CONSTANT (0xFFFFFFFFFFFFFFFF)) + { + /* We can't increment it any further without wrapping around */ + dbus_set_error (error, DBUS_ERROR_LIMITS_EXCEEDED, + "Too many containers created during the lifetime of " + "this bus"); + goto fail; + } + + /* We assume PRIu64 exists on all Unix platforms: it's ISO C99, and the + * only non-C99 platform we support is MSVC on Windows. */ + if (!_dbus_string_append_printf (&path, + "/org/freedesktop/DBus/Containers1/c%" PRIu64, + containers->next_container_id++)) + { + BUS_SET_OOM (error); + goto fail; + } + + if (!_dbus_string_steal_data (&path, &self->path)) + goto fail; + + return self; + +fail: + _dbus_string_free (&path); + + if (self != NULL) + bus_container_instance_unref (self); + + return NULL; +} + dbus_bool_t bus_containers_handle_add_server (DBusConnection *connection, BusTransaction *transaction, @@ -42,6 +212,17 @@ bus_containers_handle_add_server (DBusConnection *connection, DBusMessageIter dict_iter; const char *type; const char *name; + BusContainerInstance *instance = NULL; + BusContext *context; + BusContainers *containers; + + context = bus_transaction_get_context (transaction); + containers = bus_context_get_containers (context); + + instance = bus_container_instance_new (context, containers, error); + + if (instance == NULL) + goto fail; /* We already checked this in bus_driver_handle_message() */ _dbus_assert (dbus_message_has_signature (message, "ssa{sv}a{sv}")); @@ -52,6 +233,10 @@ bus_containers_handle_add_server (DBusConnection *connection, _dbus_assert (dbus_message_iter_get_arg_type (&iter) == DBUS_TYPE_STRING); dbus_message_iter_get_basic (&iter, &type); + instance->type = _dbus_strdup (type); + + if (instance->type == NULL) + goto oom; if (!dbus_validate_interface (type, NULL)) { @@ -67,13 +252,19 @@ bus_containers_handle_add_server (DBusConnection *connection, _dbus_assert (dbus_message_iter_get_arg_type (&iter) == DBUS_TYPE_STRING); dbus_message_iter_get_basic (&iter, &name); + instance->name = _dbus_strdup (name); + + if (instance->name == NULL) + goto oom; /* Argument 2: Metadata as defined by container manager */ if (!dbus_message_iter_next (&iter)) _dbus_assert_not_reached ("Message type was already checked"); _dbus_assert (dbus_message_iter_get_arg_type (&iter) == DBUS_TYPE_ARRAY); - /* TODO: Copy the metadata */ + instance->metadata = _dbus_variant_read (&iter); + _dbus_assert (strcmp (_dbus_variant_get_signature (instance->metadata), + "a{sv}") == 0); /* Argument 3: Named parameters */ if (!dbus_message_iter_next (&iter)) @@ -95,7 +286,7 @@ bus_containers_handle_add_server (DBusConnection *connection, DBUS_TYPE_STRING); dbus_message_iter_get_basic (&pair_iter, ¶m_name); - /* If we supported any named parameters, we'd copy them into a data + /* If we supported any named parameters, we'd copy them into the data * structure here; but we don't, so fail instead. */ dbus_set_error (error, DBUS_ERROR_INVALID_ARGS, "Named parameter %s is not understood", param_name); @@ -105,10 +296,29 @@ bus_containers_handle_add_server (DBusConnection *connection, /* End of arguments */ _dbus_assert (!dbus_message_iter_has_next (&iter)); + if (containers->instances_by_path == NULL) + { + containers->instances_by_path = _dbus_hash_table_new (DBUS_HASH_STRING, + NULL, NULL); + + if (containers->instances_by_path == NULL) + goto oom; + } + + if (!_dbus_hash_table_insert_string (containers->instances_by_path, + instance->path, instance)) + goto oom; + /* TODO: Actually implement the method */ dbus_set_error (error, DBUS_ERROR_NOT_SUPPORTED, "Not yet implemented"); + goto fail; +oom: + BUS_SET_OOM (error); + /* fall through */ fail: + + bus_clear_container_instance (&instance); return FALSE; } @@ -125,4 +335,28 @@ bus_containers_supported_arguments_getter (BusContext *context, dbus_message_iter_close_container (var_iter, &arr_iter); } +#else + +BusContainers * +bus_containers_new (void) +{ + /* Return an arbitrary non-NULL pointer just to indicate that we didn't + * fail. There is no valid operation to do with it on this platform, + * other than unreffing it, which does nothing. */ + return (BusContainers *) 1; +} + +BusContainers * +bus_containers_ref (BusContainers *self) +{ + _dbus_assert (self == (BusContainers *) 1); + return self; +} + +void +bus_containers_unref (BusContainers *self) +{ + _dbus_assert (self == (BusContainers *) 1); +} + #endif /* DBUS_ENABLE_CONTAINERS */ diff --git a/bus/containers.h b/bus/containers.h index 3564bbd2..bda0a879 100644 --- a/bus/containers.h +++ b/bus/containers.h @@ -25,6 +25,12 @@ #include "bus.h" +#include + +BusContainers *bus_containers_new (void); +BusContainers *bus_containers_ref (BusContainers *self); +void bus_containers_unref (BusContainers *self); + dbus_bool_t bus_containers_handle_add_server (DBusConnection *connection, BusTransaction *transaction, DBusMessage *message, @@ -32,4 +38,10 @@ dbus_bool_t bus_containers_handle_add_server (DBusConnection *connecti dbus_bool_t bus_containers_supported_arguments_getter (BusContext *context, DBusMessageIter *var_iter); +static inline void +bus_clear_containers (BusContainers **containers_p) +{ + _dbus_clear_pointer_impl (BusContainers, containers_p, bus_containers_unref); +} + #endif /* multiple-inclusion guard */ From a7babbf10f162c938e464247a964811ed7d03fef Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 21 Jul 2017 20:09:15 +0100 Subject: [PATCH 09/44] 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. For the system bus (or root's session bus, where the difference is harmless but makes automated testing easier), rely on system-wide infrastructure to create /run/dbus/containers. The upstream dbus distribution no longer contains integration glue for non-systemd boot systems, but downstreams that maintain a non-systemd boot system and are interested in the Containers interface should create /run/dbus/containers during boot. Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/Makefile.am | 1 + bus/bus.c | 35 +++-- bus/bus.h | 3 + bus/containers.c | 294 +++++++++++++++++++++++++++++++++++- bus/containers.h | 1 + bus/tmpfiles.d/dbus.conf.in | 4 + dbus/dbus-internals.h | 1 + 7 files changed, 324 insertions(+), 15 deletions(-) diff --git a/bus/Makefile.am b/bus/Makefile.am index 33751412..d7408049 100644 --- a/bus/Makefile.am +++ b/bus/Makefile.am @@ -29,6 +29,7 @@ AM_CPPFLAGS = \ $(EXPAT_CFLAGS) \ $(APPARMOR_CFLAGS) \ -DDBUS_SYSTEM_CONFIG_FILE=\""$(dbusdatadir)/system.conf"\" \ + -DDBUS_RUNSTATEDIR=\""$(runstatedir)"\" \ -DDBUS_COMPILATION \ $(NULL) diff --git a/bus/bus.c b/bus/bus.c index 8b08b8ea..c4b71600 100644 --- a/bus/bus.c +++ b/bus/bus.c @@ -102,7 +102,8 @@ server_get_context (DBusServer *server) bd = BUS_SERVER_DATA (server); - /* every DBusServer in the dbus-daemon has gone through setup_server() */ + /* every DBusServer in the dbus-daemon's main loop has gone through + * bus_context_setup_server() */ _dbus_assert (bd != NULL); context = bd->context; @@ -219,6 +220,25 @@ setup_server (BusContext *context, DBusServer *server, char **auth_mechanisms, DBusError *error) +{ + if (!bus_context_setup_server (context, server, error)) + return FALSE; + + if (!dbus_server_set_auth_mechanisms (server, (const char**) auth_mechanisms)) + { + BUS_SET_OOM (error); + return FALSE; + } + + dbus_server_set_new_connection_function (server, new_connection_callback, + context, NULL); + return TRUE; +} + +dbus_bool_t +bus_context_setup_server (BusContext *context, + DBusServer *server, + DBusError *error) { BusServerData *bd; @@ -234,16 +254,6 @@ setup_server (BusContext *context, bd->context = context; - if (!dbus_server_set_auth_mechanisms (server, (const char**) auth_mechanisms)) - { - BUS_SET_OOM (error); - return FALSE; - } - - dbus_server_set_new_connection_function (server, - new_connection_callback, - context, NULL); - if (!dbus_server_set_watch_functions (server, add_server_watch, remove_server_watch, @@ -1112,6 +1122,9 @@ bus_context_shutdown (BusContext *context) link = _dbus_list_get_next_link (&context->servers, link); } + + if (context->containers != NULL) + bus_containers_stop_listening (context->containers); } BusContext * diff --git a/bus/bus.h b/bus/bus.h index edc95773..27d9502d 100644 --- a/bus/bus.h +++ b/bus/bus.h @@ -147,6 +147,9 @@ dbus_bool_t bus_context_check_security_policy (BusContext BusActivationEntry *activation_entry, DBusError *error); void bus_context_check_all_watches (BusContext *context); +dbus_bool_t bus_context_setup_server (BusContext *context, + DBusServer *server, + DBusError *error); #ifdef DBUS_ENABLE_EMBEDDED_TESTS void bus_context_quiet_log_begin (BusContext *context); diff --git a/bus/containers.c b/bus/containers.c index 1c922952..a59226b7 100644 --- a/bus/containers.c +++ b/bus/containers.c @@ -51,6 +51,7 @@ typedef struct DBusVariant *metadata; BusContext *context; BusContainers *containers; + DBusServer *server; } BusContainerInstance; /* @@ -63,6 +64,7 @@ struct BusContainers /* path borrowed from BusContainerInstance => unowned BusContainerInstance * The BusContainerInstance removes itself from here on destruction. */ DBusHashTable *instances_by_path; + DBusString address_template; dbus_uint64_t next_container_id; }; @@ -72,14 +74,53 @@ bus_containers_new (void) /* We allocate the hash table lazily, expecting that the common case will * be a connection where this feature is never used */ BusContainers *self = dbus_new0 (BusContainers, 1); + DBusString invalid = _DBUS_STRING_INIT_INVALID; if (self == NULL) - return NULL; + goto oom; self->refcount = 1; self->instances_by_path = NULL; self->next_container_id = DBUS_UINT64_CONSTANT (0); + self->address_template = invalid; + + /* Make it mutable */ + if (!_dbus_string_init (&self->address_template)) + goto oom; + + if (_dbus_getuid () == 0) + { + DBusString dir; + + /* System bus (we haven't dropped privileges at this point), or + * root's session bus. Use random socket paths resembling + * /run/dbus/containers/dbus-abcdef, which is next to /run/dbus/pid + * (if not using the Red Hat init scripts, which use a different + * pid file for historical reasons). + * + * We rely on the tmpfiles.d snippet or an OS-specific init script to + * have created this directory with the appropriate owner; if it hasn't, + * creating container sockets will just fail. */ + _dbus_string_init_const (&dir, DBUS_RUNSTATEDIR "/dbus/containers"); + + /* We specifically use paths, because an abstract socket that you can't + * bind-mount is not particularly useful. */ + if (!_dbus_string_append (&self->address_template, "unix:dir=") || + !_dbus_address_append_escaped (&self->address_template, &dir)) + goto oom; + } + else + { + /* Otherwise defer creating the directory for sockets until we need it, + * so that we can have better error behaviour */ + } + return self; + +oom: + bus_clear_containers (&self); + + return NULL; } BusContainers * @@ -101,10 +142,21 @@ bus_containers_unref (BusContainers *self) if (--self->refcount == 0) { _dbus_clear_hash_table (&self->instances_by_path); + _dbus_string_free (&self->address_template); dbus_free (self); } } +static BusContainerInstance * +bus_container_instance_ref (BusContainerInstance *self) +{ + _dbus_assert (self->refcount > 0); + _dbus_assert (self->refcount < _DBUS_INT_MAX); + + self->refcount++; + return self; +} + static void bus_container_instance_unref (BusContainerInstance *self) { @@ -112,6 +164,11 @@ bus_container_instance_unref (BusContainerInstance *self) if (--self->refcount == 0) { + /* As long as the server is listening, the BusContainerInstance can't + * be freed, because the DBusServer holds a reference to the + * BusContainerInstance */ + _dbus_assert (self->server == NULL); + /* It's OK to do this even if we were never added to instances_by_path, * because the paths are globally unique. */ if (self->path != NULL && self->containers->instances_by_path != NULL) @@ -135,6 +192,22 @@ bus_clear_container_instance (BusContainerInstance **instance_p) bus_container_instance_unref); } +static void +bus_container_instance_stop_listening (BusContainerInstance *self) +{ + /* In case the DBusServer holds the last reference to self */ + bus_container_instance_ref (self); + + if (self->server != NULL) + { + dbus_server_set_new_connection_function (self->server, NULL, NULL, NULL); + dbus_server_disconnect (self->server); + dbus_clear_server (&self->server); + } + + bus_container_instance_unref (self); +} + static BusContainerInstance * bus_container_instance_new (BusContext *context, BusContainers *containers, @@ -167,6 +240,7 @@ bus_container_instance_new (BusContext *context, self->metadata = NULL; self->context = bus_context_ref (context); self->containers = bus_containers_ref (containers); + self->server = NULL; if (containers->next_container_id >= DBUS_UINT64_CONSTANT (0xFFFFFFFFFFFFFFFF)) @@ -202,6 +276,129 @@ fail: return NULL; } +/* We only accept EXTERNAL authentication, because Unix platforms that are + * sufficiently capable to have app-containers ought to have it. */ +static const char * const auth_mechanisms[] = +{ + "EXTERNAL", + NULL +}; + +static void +new_connection_cb (DBusServer *server, + DBusConnection *new_connection, + void *data) +{ + /* TODO: handle new connection */ +} + +static const char * +bus_containers_ensure_address_template (BusContainers *self, + DBusError *error) +{ + DBusString dir = _DBUS_STRING_INIT_INVALID; + const char *ret = NULL; + const char *runtime_dir; + + /* Early-return if we already did this */ + if (_dbus_string_get_length (&self->address_template) > 0) + return _dbus_string_get_const_data (&self->address_template); + + runtime_dir = _dbus_getenv ("XDG_RUNTIME_DIR"); + + if (runtime_dir != NULL) + { + if (!_dbus_string_init (&dir)) + { + BUS_SET_OOM (error); + goto out; + } + + /* We listen on a random socket path resembling + * /run/user/1000/dbus-1/containers/dbus-abcdef, chosen to share + * the dbus-1 directory with the dbus-1/services used for transient + * session services. */ + if (!_dbus_string_append (&dir, runtime_dir) || + !_dbus_string_append (&dir, "/dbus-1")) + { + BUS_SET_OOM (error); + goto out; + } + + if (!_dbus_ensure_directory (&dir, error)) + goto out; + + if (!_dbus_string_append (&dir, "/containers")) + { + BUS_SET_OOM (error); + goto out; + } + + if (!_dbus_ensure_directory (&dir, error)) + goto out; + } + else + { + /* No XDG_RUNTIME_DIR, so don't do anything special or clever: just + * use a random socket like /tmp/dbus-abcdef. */ + const char *tmpdir; + + tmpdir = _dbus_get_tmpdir (); + _dbus_string_init_const (&dir, tmpdir); + } + + /* We specifically use paths, even on Linux (unix:dir= not unix:tmpdir=), + * because an abstract socket that you can't bind-mount is not useful + * when you want something you can bind-mount into a container. */ + if (!_dbus_string_append (&self->address_template, "unix:dir=") || + !_dbus_address_append_escaped (&self->address_template, &dir)) + { + _dbus_string_set_length (&self->address_template, 0); + BUS_SET_OOM (error); + goto out; + } + + ret = _dbus_string_get_const_data (&self->address_template); + +out: + _dbus_string_free (&dir); + return ret; +} + +static dbus_bool_t +bus_container_instance_listen (BusContainerInstance *self, + DBusError *error) +{ + BusContainers *containers = bus_context_get_containers (self->context); + const char *address; + + address = bus_containers_ensure_address_template (containers, error); + + if (address == NULL) + return FALSE; + + self->server = dbus_server_listen (address, error); + + if (self->server == NULL) + return FALSE; + + if (!bus_context_setup_server (self->context, self->server, error)) + return FALSE; + + if (!dbus_server_set_auth_mechanisms (self->server, + (const char **) auth_mechanisms)) + { + BUS_SET_OOM (error); + return FALSE; + } + + /* Cannot fail because the memory it uses was already allocated */ + dbus_server_set_new_connection_function (self->server, new_connection_cb, + bus_container_instance_ref (self), + (DBusFreeFunction) bus_container_instance_unref); + return TRUE; +} + dbus_bool_t bus_containers_handle_add_server (DBusConnection *connection, BusTransaction *transaction, @@ -210,11 +407,18 @@ bus_containers_handle_add_server (DBusConnection *connection, { DBusMessageIter iter; DBusMessageIter dict_iter; + DBusMessageIter writer; + DBusMessageIter array_writer; const char *type; const char *name; + const char *path; BusContainerInstance *instance = NULL; BusContext *context; BusContainers *containers; + char *address = NULL; + DBusAddressEntry **entries = NULL; + int n_entries; + DBusMessage *reply = NULL; context = bus_transaction_get_context (transaction); containers = bus_context_get_containers (context); @@ -309,16 +513,74 @@ bus_containers_handle_add_server (DBusConnection *connection, instance->path, instance)) goto oom; - /* TODO: Actually implement the method */ - dbus_set_error (error, DBUS_ERROR_NOT_SUPPORTED, "Not yet implemented"); - goto fail; + /* This part is separated out because we eventually want to be able to + * accept a fd-passed server socket in the named parameters, instead of + * creating our own server, and defer listening on it until later */ + if (!bus_container_instance_listen (instance, error)) + goto fail; + + address = dbus_server_get_address (instance->server); + + if (!dbus_parse_address (address, &entries, &n_entries, error)) + _dbus_assert_not_reached ("listening on unix:dir= should yield a valid address"); + + _dbus_assert (n_entries == 1); + _dbus_assert (strcmp (dbus_address_entry_get_method (entries[0]), "unix") == 0); + + path = dbus_address_entry_get_value (entries[0], "path"); + _dbus_assert (path != NULL); + + reply = dbus_message_new_method_return (message); + + if (!dbus_message_append_args (reply, + DBUS_TYPE_OBJECT_PATH, &instance->path, + DBUS_TYPE_INVALID)) + goto oom; + + dbus_message_iter_init_append (reply, &writer); + + if (!dbus_message_iter_open_container (&writer, DBUS_TYPE_ARRAY, + DBUS_TYPE_BYTE_AS_STRING, + &array_writer)) + goto oom; + + if (!dbus_message_iter_append_fixed_array (&array_writer, DBUS_TYPE_BYTE, + &path, strlen (path) + 1)) + { + dbus_message_iter_abandon_container (&writer, &array_writer); + goto oom; + } + + if (!dbus_message_iter_close_container (&writer, &array_writer)) + goto oom; + + if (!dbus_message_append_args (reply, + DBUS_TYPE_STRING, &address, + DBUS_TYPE_INVALID)) + goto oom; + + _dbus_assert (dbus_message_has_signature (reply, "oays")); + + if (! bus_transaction_send_from_driver (transaction, connection, reply)) + goto oom; + + dbus_message_unref (reply); + bus_container_instance_unref (instance); + dbus_address_entries_free (entries); + dbus_free (address); + return TRUE; oom: BUS_SET_OOM (error); /* fall through */ fail: + if (instance != NULL) + bus_container_instance_stop_listening (instance); + dbus_clear_message (&reply); + dbus_clear_address_entries (&entries); bus_clear_container_instance (&instance); + dbus_free (address); return FALSE; } @@ -335,6 +597,24 @@ bus_containers_supported_arguments_getter (BusContext *context, dbus_message_iter_close_container (var_iter, &arr_iter); } +void +bus_containers_stop_listening (BusContainers *self) +{ + if (self->instances_by_path != NULL) + { + DBusHashIter iter; + + _dbus_hash_iter_init (self->instances_by_path, &iter); + + while (_dbus_hash_iter_next (&iter)) + { + BusContainerInstance *instance = _dbus_hash_iter_get_value (&iter); + + bus_container_instance_stop_listening (instance); + } + } +} + #else BusContainers * @@ -359,4 +639,10 @@ bus_containers_unref (BusContainers *self) _dbus_assert (self == (BusContainers *) 1); } +void +bus_containers_stop_listening (BusContainers *self) +{ + _dbus_assert (self == (BusContainers *) 1); +} + #endif /* DBUS_ENABLE_CONTAINERS */ diff --git a/bus/containers.h b/bus/containers.h index bda0a879..9a7ac0ba 100644 --- a/bus/containers.h +++ b/bus/containers.h @@ -30,6 +30,7 @@ BusContainers *bus_containers_new (void); BusContainers *bus_containers_ref (BusContainers *self); void bus_containers_unref (BusContainers *self); +void bus_containers_stop_listening (BusContainers *self); dbus_bool_t bus_containers_handle_add_server (DBusConnection *connection, BusTransaction *transaction, diff --git a/bus/tmpfiles.d/dbus.conf.in b/bus/tmpfiles.d/dbus.conf.in index 754f0220..6ebecb6d 100644 --- a/bus/tmpfiles.d/dbus.conf.in +++ b/bus/tmpfiles.d/dbus.conf.in @@ -3,3 +3,7 @@ # Make ${localstatedir}/lib/dbus/machine-id a symlink to /etc/machine-id # if it does not already exist L @EXPANDED_LOCALSTATEDIR@/lib/dbus/machine-id - - - - /etc/machine-id + +# Create ${runstatedir}/dbus/containers owned by the system bus user. +# org.freedesktop.DBus.Containers1 uses this to create sockets. +d @EXPANDED_RUNSTATEDIR@/dbus/containers 0755 @DBUS_USER@ - - - diff --git a/dbus/dbus-internals.h b/dbus/dbus-internals.h index b210d615..57a67d08 100644 --- a/dbus/dbus-internals.h +++ b/dbus/dbus-internals.h @@ -376,6 +376,7 @@ void _dbus_unlock (DBusGlobalLock lock); DBUS_PRIVATE_EXPORT dbus_bool_t _dbus_threads_init_debug (void); +DBUS_PRIVATE_EXPORT dbus_bool_t _dbus_address_append_escaped (DBusString *escaped, const DBusString *unescaped); From 8359321ea12a655349a619b4b64d8565003b4442 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 6 Nov 2017 16:28:11 +0000 Subject: [PATCH 10/44] bus_context_add_incoming_connection: factor out Reviewed-by: Philip Withnall [smcv: Fix minor conflict] Signed-off-by: Simon McVittie Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/bus.c | 12 ++++++++++-- bus/bus.h | 2 ++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/bus/bus.c b/bus/bus.c index c4b71600..b0a71f67 100644 --- a/bus/bus.c +++ b/bus/bus.c @@ -176,8 +176,14 @@ new_connection_callback (DBusServer *server, DBusConnection *new_connection, void *data) { - BusContext *context = data; + /* If this fails it logs a warning, so we don't need to do that */ + bus_context_add_incoming_connection (data, new_connection); +} +dbus_bool_t +bus_context_add_incoming_connection (BusContext *context, + DBusConnection *new_connection) +{ /* If this fails it logs a warning, so we don't need to do that */ if (!bus_connections_setup_connection (context->connections, new_connection)) { @@ -187,6 +193,8 @@ new_connection_callback (DBusServer *server, * in general. */ dbus_connection_close (new_connection); + /* on OOM, we won't have ref'd the connection so it will die. */ + return FALSE; } dbus_connection_set_max_received_size (new_connection, @@ -204,7 +212,7 @@ new_connection_callback (DBusServer *server, dbus_connection_set_allow_anonymous (new_connection, context->allow_anonymous); - /* on OOM, we won't have ref'd the connection so it will die. */ + return TRUE; } static void diff --git a/bus/bus.h b/bus/bus.h index 27d9502d..5492af24 100644 --- a/bus/bus.h +++ b/bus/bus.h @@ -150,6 +150,8 @@ void bus_context_check_all_watches (BusContext dbus_bool_t bus_context_setup_server (BusContext *context, DBusServer *server, DBusError *error); +dbus_bool_t bus_context_add_incoming_connection (BusContext *context, + DBusConnection *new_connection); #ifdef DBUS_ENABLE_EMBEDDED_TESTS void bus_context_quiet_log_begin (BusContext *context); From b0fbde54ab8f4a01cc760817cc0386c0006ec3a7 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 22 Jun 2017 17:58:42 +0100 Subject: [PATCH 11/44] bus/containers: Set up new connections to join the bus Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/containers.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/bus/containers.c b/bus/containers.c index a59226b7..5bb8fc6a 100644 --- a/bus/containers.c +++ b/bus/containers.c @@ -289,7 +289,11 @@ new_connection_cb (DBusServer *server, DBusConnection *new_connection, void *data) { - /* TODO: handle new connection */ + BusContainerInstance *instance = data; + + /* If this fails it logs a warning, so we don't need to do that */ + if (!bus_context_add_incoming_connection (instance->context, new_connection)) + return; } static const char * From db6ba2d799f9faa507eafef696f20b9002423dce Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 20 Jun 2017 15:55:53 +0100 Subject: [PATCH 12/44] test/containers: Exercise a successful call to AddServer Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- test/containers.c | 207 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 206 insertions(+), 1 deletion(-) diff --git a/test/containers.c b/test/containers.c index fef6972c..b3710123 100644 --- a/test/containers.c +++ b/test/containers.c @@ -25,6 +25,8 @@ #include +#include + #include #include @@ -32,9 +34,14 @@ #include #if defined(DBUS_ENABLE_CONTAINERS) && defined(HAVE_GIO_UNIX) + #define HAVE_CONTAINERS_TEST + #include #include + +#include "dbus/dbus-sysdeps-unix.h" + #endif #include "test-utils-glib.h" @@ -47,7 +54,11 @@ typedef struct { GDBusProxy *proxy; + gchar *instance_path; + gchar *socket_path; + gchar *socket_dbus_address; GDBusConnection *unconfined_conn; + GDBusConnection *confined_conn; } Fixture; static void @@ -113,6 +124,148 @@ test_get_supported_arguments (Fixture *f, #endif /* !DBUS_ENABLE_CONTAINERS */ } +#ifdef HAVE_CONTAINERS_TEST +/* + * Try to make an AddServer call that usually succeeds, but may fail and + * be skipped if we are running as root and this version of dbus has not + * been fully installed. Return TRUE if we can continue. + * + * parameters is sunk if it is a floating reference. + */ +static gboolean +add_container_server (Fixture *f, + GVariant *parameters) +{ + GVariant *tuple; + GStatBuf stat_buf; + + f->proxy = g_dbus_proxy_new_sync (f->unconfined_conn, + G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES, + NULL, DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, DBUS_INTERFACE_CONTAINERS1, + NULL, &f->error); + g_assert_no_error (f->error); + + g_test_message ("Calling AddServer..."); + tuple = g_dbus_proxy_call_sync (f->proxy, "AddServer", parameters, + G_DBUS_CALL_FLAGS_NONE, -1, NULL, &f->error); + + /* For root, the sockets go in /run/dbus/containers, which we rely on + * system infrastructure to create; so it's OK for AddServer to fail + * when uninstalled, although not OK if it fails as an installed-test. */ + if (f->error != NULL && + _dbus_getuid () == 0 && + _dbus_getenv ("DBUS_TEST_UNINSTALLED") != NULL) + { + g_test_message ("AddServer: %s", f->error->message); + g_assert_error (f->error, G_DBUS_ERROR, G_DBUS_ERROR_FILE_NOT_FOUND); + g_test_skip ("AddServer failed, probably because this dbus " + "version is not fully installed"); + return FALSE; + } + + g_assert_no_error (f->error); + g_assert_nonnull (tuple); + + g_assert_cmpstr (g_variant_get_type_string (tuple), ==, "(oays)"); + g_variant_get (tuple, "(o^ays)", &f->instance_path, &f->socket_path, + &f->socket_dbus_address); + g_assert_true (g_str_has_prefix (f->socket_dbus_address, "unix:")); + g_assert_null (strchr (f->socket_dbus_address, ';')); + g_assert_null (strchr (f->socket_dbus_address + strlen ("unix:"), ':')); + g_clear_pointer (&tuple, g_variant_unref); + + g_assert_nonnull (f->instance_path); + g_assert_true (g_variant_is_object_path (f->instance_path)); + g_assert_nonnull (f->socket_path); + g_assert_true (g_path_is_absolute (f->socket_path)); + g_assert_nonnull (f->socket_dbus_address); + g_assert_cmpstr (g_stat (f->socket_path, &stat_buf) == 0 ? NULL : + g_strerror (errno), ==, NULL); + g_assert_cmpuint ((stat_buf.st_mode & S_IFMT), ==, S_IFSOCK); + return TRUE; +} +#endif + +/* + * Assert that a simple AddServer() call succeeds and has the behaviour + * we expect (we can connect a confined connection to it, the confined + * connection can talk to the dbus-daemon and to an unconfined connection, + * and the socket gets cleaned up when the dbus-daemon terminates). + */ +static void +test_basic (Fixture *f, + gconstpointer context) +{ +#ifdef HAVE_CONTAINERS_TEST + GVariant *parameters; + const gchar *manager_unique_name; + const gchar *name_owner; + GStatBuf stat_buf; + GVariant *tuple; + + if (f->skip) + return; + + parameters = g_variant_new ("(ssa{sv}a{sv})", + "com.example.NotFlatpak", + "sample-app", + NULL, /* no metadata */ + NULL); /* no named arguments */ + if (!add_container_server (f, g_steal_pointer (¶meters))) + return; + + g_test_message ("Connecting to %s...", f->socket_dbus_address); + f->confined_conn = g_dbus_connection_new_for_address_sync ( + f->socket_dbus_address, + (G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION | + G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT), + NULL, NULL, &f->error); + g_assert_no_error (f->error); + + g_test_message ("Making a method call from confined app..."); + tuple = g_dbus_connection_call_sync (f->confined_conn, DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, + "GetNameOwner", + g_variant_new ("(s)", DBUS_SERVICE_DBUS), + G_VARIANT_TYPE ("(s)"), + G_DBUS_CALL_FLAGS_NONE, -1, NULL, + &f->error); + g_assert_no_error (f->error); + g_assert_nonnull (tuple); + g_assert_cmpstr (g_variant_get_type_string (tuple), ==, "(s)"); + g_variant_get (tuple, "(&s)", &name_owner); + g_assert_cmpstr (name_owner, ==, DBUS_SERVICE_DBUS); + g_clear_pointer (&tuple, g_variant_unref); + + g_test_message ("Making a method call from confined app to unconfined..."); + manager_unique_name = g_dbus_connection_get_unique_name (f->unconfined_conn); + tuple = g_dbus_connection_call_sync (f->confined_conn, manager_unique_name, + "/", DBUS_INTERFACE_PEER, + "Ping", + NULL, G_VARIANT_TYPE_UNIT, + G_DBUS_CALL_FLAGS_NONE, -1, NULL, + &f->error); + g_assert_no_error (f->error); + g_assert_nonnull (tuple); + g_assert_cmpstr (g_variant_get_type_string (tuple), ==, "()"); + g_clear_pointer (&tuple, g_variant_unref); + + /* Check that the socket is cleaned up when the dbus-daemon is terminated */ + test_kill_pid (f->daemon_pid); + g_spawn_close_pid (f->daemon_pid); + f->daemon_pid = 0; + + while (g_stat (f->socket_path, &stat_buf) == 0) + g_usleep (G_USEC_PER_SEC / 20); + + g_assert_cmpint (errno, ==, ENOENT); + +#else /* !HAVE_CONTAINERS_TEST */ + g_test_skip ("Containers or gio-unix-2.0 not supported"); +#endif /* !HAVE_CONTAINERS_TEST */ +} + /* * Assert that named arguments are validated: passing an unsupported * named argument causes an error. @@ -218,6 +371,20 @@ teardown (Fixture *f, g_clear_object (&f->unconfined_conn); + if (f->confined_conn != NULL) + { + GError *error = NULL; + + g_dbus_connection_close_sync (f->confined_conn, NULL, &error); + + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CLOSED)) + g_clear_error (&error); + else + g_assert_no_error (error); + } + + g_clear_object (&f->confined_conn); + if (f->daemon_pid != 0) { test_kill_pid (f->daemon_pid); @@ -225,6 +392,9 @@ teardown (Fixture *f, f->daemon_pid = 0; } + g_free (f->instance_path); + g_free (f->socket_path); + g_free (f->socket_dbus_address); g_free (f->bus_address); g_clear_error (&f->error); } @@ -233,14 +403,49 @@ int main (int argc, char **argv) { + GError *error = NULL; + gchar *runtime_dir; + gchar *runtime_dbus_dir; + gchar *runtime_containers_dir; + gchar *runtime_services_dir; + int ret; + + runtime_dir = g_dir_make_tmp ("dbus-test-containers.XXXXXX", &error); + + if (runtime_dir == NULL) + { + g_print ("Bail out! %s\n", error->message); + g_clear_error (&error); + return 1; + } + + g_setenv ("XDG_RUNTIME_DIR", runtime_dir, TRUE); + runtime_dbus_dir = g_build_filename (runtime_dir, "dbus-1", NULL); + runtime_containers_dir = g_build_filename (runtime_dir, "dbus-1", + "containers", NULL); + runtime_services_dir = g_build_filename (runtime_dir, "dbus-1", + "services", NULL); + test_init (&argc, &argv); g_test_add ("/containers/get-supported-arguments", Fixture, NULL, setup, test_get_supported_arguments, teardown); + g_test_add ("/containers/basic", Fixture, NULL, + setup, test_basic, teardown); g_test_add ("/containers/unsupported-parameter", Fixture, NULL, setup, test_unsupported_parameter, teardown); g_test_add ("/containers/invalid-type-name", Fixture, NULL, setup, test_invalid_type_name, teardown); - return g_test_run (); + ret = g_test_run (); + + test_rmdir_if_exists (runtime_containers_dir); + test_rmdir_if_exists (runtime_services_dir); + test_rmdir_if_exists (runtime_dbus_dir); + test_rmdir_must_exist (runtime_dir); + g_free (runtime_containers_dir); + g_free (runtime_services_dir); + g_free (runtime_dbus_dir); + g_free (runtime_dir); + return ret; } From 12eed8cd66538debd9451bfcedc96a2d47e3c6bc Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 22 Jun 2017 18:02:00 +0100 Subject: [PATCH 13/44] bus/containers: Require connecting uid to match caller of AddServer 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. Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/containers.c | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/bus/containers.c b/bus/containers.c index 5bb8fc6a..6d32368e 100644 --- a/bus/containers.c +++ b/bus/containers.c @@ -31,6 +31,8 @@ # error DBUS_ENABLE_CONTAINERS requires DBUS_UNIX #endif +#include + #include "dbus/dbus-hash.h" #include "dbus/dbus-message-internal.h" #include "dbus/dbus-sysdeps-unix.h" @@ -52,6 +54,7 @@ typedef struct BusContext *context; BusContainers *containers; DBusServer *server; + unsigned long uid; } BusContainerInstance; /* @@ -284,6 +287,22 @@ static const char * const auth_mechanisms[] = NULL }; +/* Statically assert that we can store a uid in a void *, losslessly. + * + * In practice this is always true on Unix. For now we don't support this + * feature on systems where it isn't. */ +_DBUS_STATIC_ASSERT (sizeof (uid_t) <= sizeof (uintptr_t)); +/* True by definition. */ +_DBUS_STATIC_ASSERT (sizeof (void *) == sizeof (uintptr_t)); + +static dbus_bool_t +allow_same_uid_only (DBusConnection *connection, + unsigned long uid, + void *data) +{ + return (uid == (uintptr_t) data); +} + static void new_connection_cb (DBusServer *server, DBusConnection *new_connection, @@ -294,6 +313,23 @@ new_connection_cb (DBusServer *server, /* If this fails it logs a warning, so we don't need to do that */ if (!bus_context_add_incoming_connection (instance->context, new_connection)) return; + + /* We'd like to check the uid here, but we can't yet. Instead clear the + * BusContext's unix_user_function, which results in us getting the + * default behaviour: only the user that owns the bus can connect. + * + * TODO: For the system bus we might want a way to opt-in to allowing + * other uids, in which case we would refrain from overwriting the + * BusContext's unix_user_function; but that isn't part of the + * lowest-common-denominator implementation. */ + dbus_connection_set_unix_user_function (new_connection, + allow_same_uid_only, + /* The static assertion above + * allow_same_uid_only ensures that + * this cast does not lose + * information */ + (void *) (uintptr_t) instance->uid, + NULL); } static const char * @@ -432,6 +468,13 @@ bus_containers_handle_add_server (DBusConnection *connection, if (instance == NULL) goto fail; + if (!dbus_connection_get_unix_user (connection, &instance->uid)) + { + dbus_set_error (error, DBUS_ERROR_FAILED, + "Unable to determine user ID of caller"); + goto fail; + } + /* We already checked this in bus_driver_handle_message() */ _dbus_assert (dbus_message_has_signature (message, "ssa{sv}a{sv}")); From c3851f28e9ec132dd340675d99f488ede7532778 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 6 Jul 2017 16:37:50 +0100 Subject: [PATCH 14/44] test/containers: Exercise connecting to the new socket as the wrong uid Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- test/containers.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/test/containers.c b/test/containers.c index b3710123..6d8ca067 100644 --- a/test/containers.c +++ b/test/containers.c @@ -266,6 +266,49 @@ test_basic (Fixture *f, #endif /* !HAVE_CONTAINERS_TEST */ } +/* + * If we are running as root, assert that when one uid (root) creates a + * container server, another uid (TEST_USER_OTHER) cannot connect to it + */ +static void +test_wrong_uid (Fixture *f, + gconstpointer context) +{ +#ifdef HAVE_CONTAINERS_TEST + GVariant *parameters; + + if (f->skip) + return; + + parameters = g_variant_new ("(ssa{sv}a{sv})", + "com.example.NotFlatpak", + "sample-app", + NULL, /* no metadata */ + NULL); /* no named arguments */ + if (!add_container_server (f, g_steal_pointer (¶meters))) + return; + + g_test_message ("Connecting to %s...", f->socket_dbus_address); + f->confined_conn = test_try_connect_gdbus_as_user (f->socket_dbus_address, + TEST_USER_OTHER, + &f->error); + + /* That might be skipped if we can't become TEST_USER_OTHER */ + if (f->error != NULL && + g_error_matches (f->error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED)) + { + g_test_skip (f->error->message); + return; + } + + /* The connection was unceremoniously closed */ + g_assert_error (f->error, G_IO_ERROR, G_IO_ERROR_CLOSED); + +#else /* !HAVE_CONTAINERS_TEST */ + g_test_skip ("Containers or gio-unix-2.0 not supported"); +#endif /* !HAVE_CONTAINERS_TEST */ +} + /* * Assert that named arguments are validated: passing an unsupported * named argument causes an error. @@ -432,6 +475,8 @@ main (int argc, setup, test_get_supported_arguments, teardown); g_test_add ("/containers/basic", Fixture, NULL, setup, test_basic, teardown); + g_test_add ("/containers/wrong-uid", Fixture, NULL, + setup, test_wrong_uid, teardown); g_test_add ("/containers/unsupported-parameter", Fixture, NULL, setup, test_unsupported_parameter, teardown); g_test_add ("/containers/invalid-type-name", Fixture, NULL, From b704c886dce2a9b32ff150e6039b3a70e00a41b9 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 6 Nov 2017 16:24:03 +0000 Subject: [PATCH 15/44] bus/containers: Each connection to a container holds a reference Reviewed-by: Philip Withnall [smcv: Fix minor conflicts] Signed-off-by: Simon McVittie Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/containers.c | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/bus/containers.c b/bus/containers.c index 6d32368e..81438187 100644 --- a/bus/containers.c +++ b/bus/containers.c @@ -71,14 +71,25 @@ struct BusContainers dbus_uint64_t next_container_id; }; +/* Data slot on DBusConnection, holding BusContainerInstance */ +static dbus_int32_t contained_data_slot = -1; + BusContainers * bus_containers_new (void) { /* We allocate the hash table lazily, expecting that the common case will * be a connection where this feature is never used */ - BusContainers *self = dbus_new0 (BusContainers, 1); + BusContainers *self = NULL; DBusString invalid = _DBUS_STRING_INIT_INVALID; + /* One reference per BusContainers, unless we ran out of memory the first + * time we tried to allocate it, in which case it will be -1 when we + * free the BusContainers */ + if (!dbus_connection_allocate_data_slot (&contained_data_slot)) + goto oom; + + self = dbus_new0 (BusContainers, 1); + if (self == NULL) goto oom; @@ -121,7 +132,16 @@ bus_containers_new (void) return self; oom: - bus_clear_containers (&self); + if (self != NULL) + { + /* This will free the data slot too */ + bus_containers_unref (self); + } + else + { + if (contained_data_slot != -1) + dbus_connection_free_data_slot (&contained_data_slot); + } return NULL; } @@ -147,6 +167,9 @@ bus_containers_unref (BusContainers *self) _dbus_clear_hash_table (&self->instances_by_path); _dbus_string_free (&self->address_template); dbus_free (self); + + if (contained_data_slot != -1) + dbus_connection_free_data_slot (&contained_data_slot); } } @@ -310,6 +333,14 @@ new_connection_cb (DBusServer *server, { BusContainerInstance *instance = data; + if (!dbus_connection_set_data (new_connection, contained_data_slot, + bus_container_instance_ref (instance), + (DBusFreeFunction) bus_container_instance_unref)) + { + bus_container_instance_unref (instance); + return; + } + /* If this fails it logs a warning, so we don't need to do that */ if (!bus_context_add_incoming_connection (instance->context, new_connection)) return; From ee029b83701b97c29f6385be2048b3d0a88d276b Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 6 Nov 2017 16:25:01 +0000 Subject: [PATCH 16/44] 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. Reviewed-by: Philip Withnall [smcv: Fix minor conflict] Signed-off-by: Simon McVittie Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/containers.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 91 insertions(+), 3 deletions(-) diff --git a/bus/containers.c b/bus/containers.c index 81438187..23193ac7 100644 --- a/bus/containers.c +++ b/bus/containers.c @@ -54,9 +54,21 @@ typedef struct BusContext *context; BusContainers *containers; DBusServer *server; + DBusConnection *creator; unsigned long uid; } BusContainerInstance; +/* Data attached to a DBusConnection that has created container instances. */ +typedef struct +{ + /* List of instances created by this connection; unowned. + * The BusContainerInstance removes itself from here on destruction. */ + DBusList *instances; +} BusContainerCreatorData; + +/* Data slot on DBusConnection, holding BusContainerCreatorData */ +static dbus_int32_t container_creator_data_slot = -1; + /* * Singleton data structure encapsulating the container-related parts of * a BusContext. @@ -88,6 +100,10 @@ bus_containers_new (void) if (!dbus_connection_allocate_data_slot (&contained_data_slot)) goto oom; + /* Ditto */ + if (!dbus_connection_allocate_data_slot (&container_creator_data_slot)) + goto oom; + self = dbus_new0 (BusContainers, 1); if (self == NULL) @@ -141,6 +157,9 @@ oom: { if (contained_data_slot != -1) dbus_connection_free_data_slot (&contained_data_slot); + + if (container_creator_data_slot != -1) + dbus_connection_free_data_slot (&container_creator_data_slot); } return NULL; @@ -170,6 +189,9 @@ bus_containers_unref (BusContainers *self) if (contained_data_slot != -1) dbus_connection_free_data_slot (&contained_data_slot); + + if (container_creator_data_slot != -1) + dbus_connection_free_data_slot (&container_creator_data_slot); } } @@ -190,11 +212,18 @@ bus_container_instance_unref (BusContainerInstance *self) if (--self->refcount == 0) { + BusContainerCreatorData *creator_data; + /* As long as the server is listening, the BusContainerInstance can't * be freed, because the DBusServer holds a reference to the * BusContainerInstance */ _dbus_assert (self->server == NULL); + creator_data = dbus_connection_get_data (self->creator, + container_creator_data_slot); + _dbus_assert (creator_data != NULL); + _dbus_list_remove (&creator_data->instances, self); + /* It's OK to do this even if we were never added to instances_by_path, * because the paths are globally unique. */ if (self->path != NULL && self->containers->instances_by_path != NULL) @@ -204,6 +233,7 @@ bus_container_instance_unref (BusContainerInstance *self) _dbus_clear_variant (&self->metadata); bus_context_unref (self->context); bus_containers_unref (self->containers); + dbus_connection_unref (self->creator); dbus_free (self->path); dbus_free (self->type); dbus_free (self->name); @@ -237,6 +267,7 @@ bus_container_instance_stop_listening (BusContainerInstance *self) static BusContainerInstance * bus_container_instance_new (BusContext *context, BusContainers *containers, + DBusConnection *creator, DBusError *error) { BusContainerInstance *self = NULL; @@ -244,6 +275,7 @@ bus_container_instance_new (BusContext *context, _dbus_assert (context != NULL); _dbus_assert (containers != NULL); + _dbus_assert (creator != NULL); _DBUS_ASSERT_ERROR_IS_CLEAR (error); if (!_dbus_string_init (&path)) @@ -267,6 +299,7 @@ bus_container_instance_new (BusContext *context, self->context = bus_context_ref (context); self->containers = bus_containers_ref (containers); self->server = NULL; + self->creator = dbus_connection_ref (creator); if (containers->next_container_id >= DBUS_UINT64_CONSTANT (0xFFFFFFFFFFFFFFFF)) @@ -302,6 +335,16 @@ fail: return NULL; } +static void +bus_container_creator_data_free (BusContainerCreatorData *self) +{ + /* Each instance holds a ref to the creator, so there should be + * nothing here */ + _dbus_assert (self->instances == NULL); + + dbus_free (self); +} + /* We only accept EXTERNAL authentication, because Unix platforms that are * sufficiently capable to have app-containers ought to have it. */ static const char * const auth_mechanisms[] = @@ -326,6 +369,19 @@ allow_same_uid_only (DBusConnection *connection, return (uid == (uintptr_t) data); } +static void +bus_container_instance_lost_connection (BusContainerInstance *instance, + DBusConnection *connection) +{ + bus_container_instance_ref (instance); + dbus_connection_ref (connection); + + dbus_connection_set_data (connection, contained_data_slot, NULL, NULL); + + dbus_connection_unref (connection); + bus_container_instance_unref (instance); +} + static void new_connection_cb (DBusServer *server, DBusConnection *new_connection, @@ -338,12 +394,18 @@ new_connection_cb (DBusServer *server, (DBusFreeFunction) bus_container_instance_unref)) { bus_container_instance_unref (instance); + bus_container_instance_lost_connection (instance, new_connection); return; } - /* If this fails it logs a warning, so we don't need to do that */ + /* If this fails it logs a warning, so we don't need to do that. + * We don't know how to undo this, so do it last (apart from things that + * cannot fail) */ if (!bus_context_add_incoming_connection (instance->context, new_connection)) - return; + { + bus_container_instance_lost_connection (instance, new_connection); + return; + } /* We'd like to check the uid here, but we can't yet. Instead clear the * BusContext's unix_user_function, which results in us getting the @@ -476,6 +538,7 @@ bus_containers_handle_add_server (DBusConnection *connection, DBusMessage *message, DBusError *error) { + BusContainerCreatorData *creator_data; DBusMessageIter iter; DBusMessageIter dict_iter; DBusMessageIter writer; @@ -494,7 +557,29 @@ bus_containers_handle_add_server (DBusConnection *connection, context = bus_transaction_get_context (transaction); containers = bus_context_get_containers (context); - instance = bus_container_instance_new (context, containers, error); + creator_data = dbus_connection_get_data (connection, + container_creator_data_slot); + + if (creator_data == NULL) + { + creator_data = dbus_new0 (BusContainerCreatorData, 1); + + if (creator_data == NULL) + goto oom; + + creator_data->instances = NULL; + + if (!dbus_connection_set_data (connection, container_creator_data_slot, + creator_data, + (DBusFreeFunction) bus_container_creator_data_free)) + { + bus_container_creator_data_free (creator_data); + goto oom; + } + } + + instance = bus_container_instance_new (context, containers, connection, + error); if (instance == NULL) goto fail; @@ -591,6 +676,9 @@ bus_containers_handle_add_server (DBusConnection *connection, instance->path, instance)) goto oom; + if (!_dbus_list_append (&creator_data->instances, instance)) + goto oom; + /* This part is separated out because we eventually want to be able to * accept a fd-passed server socket in the named parameters, instead of * creating our own server, and defer listening on it until later */ From 3048c90ccbeab6ed807ac80662e7196520ba1ab6 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 22 Jun 2017 18:47:03 +0100 Subject: [PATCH 17/44] 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. Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/connection.c | 5 +++++ bus/containers.c | 38 ++++++++++++++++++++++++++++++++++++++ bus/containers.h | 3 +++ 3 files changed, 46 insertions(+) diff --git a/bus/connection.c b/bus/connection.c index 53605fa3..d53522f5 100644 --- a/bus/connection.c +++ b/bus/connection.c @@ -23,6 +23,8 @@ #include #include "connection.h" + +#include "containers.h" #include "dispatch.h" #include "policy.h" #include "services.h" @@ -306,6 +308,9 @@ bus_connection_disconnected (DBusConnection *connection) d->link_in_monitors = NULL; } + bus_containers_remove_connection (bus_context_get_containers (d->connections->context), + connection); + if (d->link_in_connection_list != NULL) { if (d->name != NULL) diff --git a/bus/containers.c b/bus/containers.c index 23193ac7..e30feb37 100644 --- a/bus/containers.c +++ b/bus/containers.c @@ -812,3 +812,41 @@ bus_containers_stop_listening (BusContainers *self) } #endif /* DBUS_ENABLE_CONTAINERS */ + +void +bus_containers_remove_connection (BusContainers *self, + DBusConnection *connection) +{ +#ifdef DBUS_ENABLE_CONTAINERS + BusContainerCreatorData *creator_data; + + dbus_connection_ref (connection); + creator_data = dbus_connection_get_data (connection, + container_creator_data_slot); + + if (creator_data != NULL) + { + DBusList *iter; + DBusList *next; + + for (iter = _dbus_list_get_first_link (&creator_data->instances); + iter != NULL; + iter = next) + { + BusContainerInstance *instance = iter->data; + + /* Remember where we got to before we do something that might free + * iter and instance */ + next = _dbus_list_get_next_link (&creator_data->instances, iter); + + _dbus_assert (instance->creator == connection); + + /* This will invalidate iter and instance if there are no open + * connections to this instance */ + bus_container_instance_stop_listening (instance); + } + } + + dbus_connection_unref (connection); +#endif /* DBUS_ENABLE_CONTAINERS */ +} diff --git a/bus/containers.h b/bus/containers.h index 9a7ac0ba..e51d6ba1 100644 --- a/bus/containers.h +++ b/bus/containers.h @@ -39,6 +39,9 @@ dbus_bool_t bus_containers_handle_add_server (DBusConnection *connecti dbus_bool_t bus_containers_supported_arguments_getter (BusContext *context, DBusMessageIter *var_iter); +void bus_containers_remove_connection (BusContainers *self, + DBusConnection *connection); + static inline void bus_clear_containers (BusContainers **containers_p) { From d5ff91ff9a3d0615f69b8b7664804a8c89bb0515 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 6 Nov 2017 16:25:42 +0000 Subject: [PATCH 18/44] bus/containers: Give each instance a list of all its connections Reviewed-by: Philip Withnall [smcv: Fix minor conflict] Signed-off-by: Simon McVittie Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/containers.c | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/bus/containers.c b/bus/containers.c index e30feb37..91806dbe 100644 --- a/bus/containers.c +++ b/bus/containers.c @@ -55,6 +55,9 @@ typedef struct BusContainers *containers; DBusServer *server; DBusConnection *creator; + /* List of owned DBusConnection, removed when the DBusConnection is + * removed from the bus */ + DBusList *connections; unsigned long uid; } BusContainerInstance; @@ -219,6 +222,11 @@ bus_container_instance_unref (BusContainerInstance *self) * BusContainerInstance */ _dbus_assert (self->server == NULL); + /* Similarly, as long as there are connections, the BusContainerInstance + * can't be freed, because each connection holds a reference to the + * BusContainerInstance */ + _dbus_assert (self->connections == NULL); + creator_data = dbus_connection_get_data (self->creator, container_creator_data_slot); _dbus_assert (creator_data != NULL); @@ -376,6 +384,11 @@ bus_container_instance_lost_connection (BusContainerInstance *instance, bus_container_instance_ref (instance); dbus_connection_ref (connection); + /* This is O(n), but we don't expect to have many connections per + * container instance. */ + if (_dbus_list_remove (&instance->connections, connection)) + dbus_connection_unref (connection); + dbus_connection_set_data (connection, contained_data_slot, NULL, NULL); dbus_connection_unref (connection); @@ -398,6 +411,16 @@ new_connection_cb (DBusServer *server, return; } + if (_dbus_list_append (&instance->connections, new_connection)) + { + dbus_connection_ref (new_connection); + } + else + { + bus_container_instance_lost_connection (instance, new_connection); + return; + } + /* If this fails it logs a warning, so we don't need to do that. * We don't know how to undo this, so do it last (apart from things that * cannot fail) */ @@ -819,6 +842,7 @@ bus_containers_remove_connection (BusContainers *self, { #ifdef DBUS_ENABLE_CONTAINERS BusContainerCreatorData *creator_data; + BusContainerInstance *instance; dbus_connection_ref (connection); creator_data = dbus_connection_get_data (connection, @@ -833,7 +857,7 @@ bus_containers_remove_connection (BusContainers *self, iter != NULL; iter = next) { - BusContainerInstance *instance = iter->data; + instance = iter->data; /* Remember where we got to before we do something that might free * iter and instance */ @@ -847,6 +871,18 @@ bus_containers_remove_connection (BusContainers *self, } } + instance = dbus_connection_get_data (connection, contained_data_slot); + + if (instance != NULL) + { + bus_container_instance_ref (instance); + + if (_dbus_list_remove (&instance->connections, connection)) + dbus_connection_unref (connection); + + bus_container_instance_unref (instance); + } + dbus_connection_unref (connection); #endif /* DBUS_ENABLE_CONTAINERS */ } From 69d164cbd38043359b9ee91255434939792ee4f6 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 22 Jun 2017 22:18:34 +0100 Subject: [PATCH 19/44] bus/containers: Implement methods to stop containers explicitly Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/containers.c | 100 +++++++++++++++++++++++++++++++++++++++++++ bus/containers.h | 8 ++++ bus/driver.c | 4 ++ dbus/dbus-protocol.h | 3 ++ 4 files changed, 115 insertions(+) diff --git a/bus/containers.c b/bus/containers.c index 91806dbe..9e81eb08 100644 --- a/bus/containers.c +++ b/bus/containers.c @@ -38,6 +38,7 @@ #include "dbus/dbus-sysdeps-unix.h" #include "connection.h" +#include "driver.h" #include "utils.h" /* @@ -786,6 +787,105 @@ bus_containers_supported_arguments_getter (BusContext *context, dbus_message_iter_close_container (var_iter, &arr_iter); } +dbus_bool_t +bus_containers_handle_stop_instance (DBusConnection *connection, + BusTransaction *transaction, + DBusMessage *message, + DBusError *error) +{ + BusContext *context; + BusContainers *containers; + BusContainerInstance *instance = NULL; + DBusList *iter; + const char *path; + + if (!dbus_message_get_args (message, error, + DBUS_TYPE_OBJECT_PATH, &path, + DBUS_TYPE_INVALID)) + goto failed; + + context = bus_transaction_get_context (transaction); + containers = bus_context_get_containers (context); + + if (containers->instances_by_path != NULL) + { + instance = _dbus_hash_table_lookup_string (containers->instances_by_path, + path); + } + + if (instance == NULL) + { + dbus_set_error (error, DBUS_ERROR_NOT_CONTAINER, + "There is no container with path '%s'", path); + goto failed; + } + + bus_container_instance_ref (instance); + bus_container_instance_stop_listening (instance); + + for (iter = _dbus_list_get_first_link (&instance->connections); + iter != NULL; + iter = _dbus_list_get_next_link (&instance->connections, iter)) + dbus_connection_close (iter->data); + + bus_container_instance_unref (instance); + + if (!bus_driver_send_ack_reply (connection, transaction, message, error)) + goto failed; + + return TRUE; + +failed: + _DBUS_ASSERT_ERROR_IS_SET (error); + return FALSE; +} + +dbus_bool_t +bus_containers_handle_stop_listening (DBusConnection *connection, + BusTransaction *transaction, + DBusMessage *message, + DBusError *error) +{ + BusContext *context; + BusContainers *containers; + BusContainerInstance *instance = NULL; + const char *path; + + if (!dbus_message_get_args (message, error, + DBUS_TYPE_OBJECT_PATH, &path, + DBUS_TYPE_INVALID)) + goto failed; + + context = bus_transaction_get_context (transaction); + containers = bus_context_get_containers (context); + + if (containers->instances_by_path != NULL) + { + instance = _dbus_hash_table_lookup_string (containers->instances_by_path, + path); + } + + if (instance == NULL) + { + dbus_set_error (error, DBUS_ERROR_NOT_CONTAINER, + "There is no container with path '%s'", path); + goto failed; + } + + bus_container_instance_ref (instance); + bus_container_instance_stop_listening (instance); + bus_container_instance_unref (instance); + + if (!bus_driver_send_ack_reply (connection, transaction, message, error)) + goto failed; + + return TRUE; + +failed: + _DBUS_ASSERT_ERROR_IS_SET (error); + return FALSE; +} + void bus_containers_stop_listening (BusContainers *self) { diff --git a/bus/containers.h b/bus/containers.h index e51d6ba1..f3ecf1d4 100644 --- a/bus/containers.h +++ b/bus/containers.h @@ -36,6 +36,14 @@ dbus_bool_t bus_containers_handle_add_server (DBusConnection *connecti BusTransaction *transaction, DBusMessage *message, DBusError *error); +dbus_bool_t bus_containers_handle_stop_instance (DBusConnection *connection, + BusTransaction *transaction, + DBusMessage *message, + DBusError *error); +dbus_bool_t bus_containers_handle_stop_listening (DBusConnection *connection, + BusTransaction *transaction, + DBusMessage *message, + DBusError *error); dbus_bool_t bus_containers_supported_arguments_getter (BusContext *context, DBusMessageIter *var_iter); diff --git a/bus/driver.c b/bus/driver.c index 9529b07c..bcf38d05 100644 --- a/bus/driver.c +++ b/bus/driver.c @@ -2522,6 +2522,10 @@ static const MessageHandler introspectable_message_handlers[] = { static const MessageHandler containers_message_handlers[] = { { "AddServer", "ssa{sv}a{sv}", "oays", bus_containers_handle_add_server, METHOD_FLAG_PRIVILEGED }, + { "StopInstance", "o", "", bus_containers_handle_stop_instance, + METHOD_FLAG_PRIVILEGED }, + { "StopListening", "o", "", bus_containers_handle_stop_listening, + METHOD_FLAG_PRIVILEGED }, { NULL, NULL, NULL, NULL } }; static const PropertyHandler containers_property_handlers[] = { diff --git a/dbus/dbus-protocol.h b/dbus/dbus-protocol.h index 933c3658..56be9832 100644 --- a/dbus/dbus-protocol.h +++ b/dbus/dbus-protocol.h @@ -455,6 +455,9 @@ extern "C" { * but could have succeeded if an interactive authorization step was * allowed. */ #define DBUS_ERROR_INTERACTIVE_AUTHORIZATION_REQUIRED "org.freedesktop.DBus.Error.InteractiveAuthorizationRequired" +/** The connection is not from a container, or the specified container instance + * does not exist. */ +#define DBUS_ERROR_NOT_CONTAINER "org.freedesktop.DBus.Error.NotContainer" /* XML introspection format */ From 0dc09f29ee47be207dd0754839eb5a07de576935 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 23 Jun 2017 15:20:04 +0100 Subject: [PATCH 20/44] 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. Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/containers.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/bus/containers.c b/bus/containers.c index 9e81eb08..eb2b89c6 100644 --- a/bus/containers.c +++ b/bus/containers.c @@ -798,6 +798,7 @@ bus_containers_handle_stop_instance (DBusConnection *connection, BusContainerInstance *instance = NULL; DBusList *iter; const char *path; + unsigned long uid; if (!dbus_message_get_args (message, error, DBUS_TYPE_OBJECT_PATH, &path, @@ -820,6 +821,21 @@ bus_containers_handle_stop_instance (DBusConnection *connection, goto failed; } + if (!dbus_connection_get_unix_user (connection, &uid)) + { + dbus_set_error (error, DBUS_ERROR_FAILED, + "Unable to determine user ID of caller"); + goto failed; + } + + if (uid != instance->uid) + { + dbus_set_error (error, DBUS_ERROR_ACCESS_DENIED, + "User %lu cannot stop a container server started by " + "user %lu", uid, instance->uid); + goto failed; + } + bus_container_instance_ref (instance); bus_container_instance_stop_listening (instance); @@ -850,6 +866,7 @@ bus_containers_handle_stop_listening (DBusConnection *connection, BusContainers *containers; BusContainerInstance *instance = NULL; const char *path; + unsigned long uid; if (!dbus_message_get_args (message, error, DBUS_TYPE_OBJECT_PATH, &path, @@ -872,6 +889,21 @@ bus_containers_handle_stop_listening (DBusConnection *connection, goto failed; } + if (!dbus_connection_get_unix_user (connection, &uid)) + { + dbus_set_error (error, DBUS_ERROR_FAILED, + "Unable to determine user ID of caller"); + goto failed; + } + + if (uid != instance->uid) + { + dbus_set_error (error, DBUS_ERROR_ACCESS_DENIED, + "User %lu cannot stop a container server started by " + "user %lu", uid, instance->uid); + goto failed; + } + bus_container_instance_ref (instance); bus_container_instance_stop_listening (instance); bus_container_instance_unref (instance); From 4208e47f38f8322251a7308259e84b81370d436c Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Tue, 20 Jun 2017 19:48:45 +0100 Subject: [PATCH 21/44] test/containers: Exercise the various ways to stop a container Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- test/containers.c | 349 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 347 insertions(+), 2 deletions(-) diff --git a/test/containers.c b/test/containers.c index 6d8ca067..4705243e 100644 --- a/test/containers.c +++ b/test/containers.c @@ -61,12 +61,52 @@ typedef struct { GDBusConnection *confined_conn; } Fixture; +typedef struct +{ + const gchar *config_file; + enum + { + STOP_SERVER_EXPLICITLY, + STOP_SERVER_DISCONNECT_FIRST, + STOP_SERVER_NEVER_CONNECTED, + STOP_SERVER_FORCE, + STOP_SERVER_WITH_MANAGER + } + stop_server; +} Config; + +static const Config default_config = +{ + NULL, + 0 /* not used, the stop-server test always uses non-default config */ +}; + +#ifdef DBUS_ENABLE_CONTAINERS +/* A GDBusNameVanishedCallback that sets a boolean flag. */ +static void +name_gone_set_boolean_cb (GDBusConnection *conn, + const gchar *name, + gpointer user_data) +{ + gboolean *gone_p = user_data; + + g_assert_nonnull (gone_p); + g_assert_false (*gone_p); + *gone_p = TRUE; +} +#endif + static void setup (Fixture *f, gconstpointer context) { - f->bus_address = test_get_dbus_daemon (NULL, TEST_USER_ME, NULL, - &f->daemon_pid); + const Config *config = context; + + if (config == NULL) + config = &default_config; + + f->bus_address = test_get_dbus_daemon (config->config_file, TEST_USER_ME, + NULL, &f->daemon_pid); if (f->bus_address == NULL) { @@ -309,6 +349,275 @@ test_wrong_uid (Fixture *f, #endif /* !HAVE_CONTAINERS_TEST */ } +/* + * With config->stop_server == STOP_SERVER_WITH_MANAGER: + * Assert that without special parameters, when the container manager + * disappears from the bus, so does the confined server. + * + * With config->stop_server == STOP_SERVER_EXPLICITLY or + * config->stop_server == STOP_SERVER_DISCONNECT_FIRST: + * Test StopListening(), which just closes the listening socket. + * + * With config->stop_server == STOP_SERVER_FORCE: + * Test StopInstance(), which closes the listening socket and + * disconnects all its clients. + */ +static void +test_stop_server (Fixture *f, + gconstpointer context) +{ +#ifdef HAVE_CONTAINERS_TEST + const Config *config = context; + GDBusConnection *attacker; + GDBusConnection *second_confined_conn; + GDBusProxy *attacker_proxy; + GSocket *client_socket; + GSocketAddress *socket_address; + GVariant *tuple; + GVariant *parameters; + const gchar *confined_unique_name; + const gchar *manager_unique_name; + const gchar *name_owner; + gboolean gone = FALSE; + guint name_watch; + guint i; + + g_assert_nonnull (config); + + if (f->skip) + return; + + parameters = g_variant_new ("(ssa{sv}a{sv})", + "com.example.NotFlatpak", + "sample-app", + NULL, /* no metadata */ + NULL); /* no named arguments */ + if (!add_container_server (f, g_steal_pointer (¶meters))) + return; + + socket_address = g_unix_socket_address_new (f->socket_path); + + if (config->stop_server != STOP_SERVER_NEVER_CONNECTED) + { + g_test_message ("Connecting to %s...", f->socket_dbus_address); + f->confined_conn = g_dbus_connection_new_for_address_sync ( + f->socket_dbus_address, + (G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION | + G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT), + NULL, NULL, &f->error); + g_assert_no_error (f->error); + + if (config->stop_server == STOP_SERVER_DISCONNECT_FIRST) + { + g_test_message ("Disconnecting confined connection..."); + gone = FALSE; + confined_unique_name = g_dbus_connection_get_unique_name ( + f->confined_conn); + name_watch = g_bus_watch_name_on_connection (f->unconfined_conn, + confined_unique_name, + G_BUS_NAME_WATCHER_FLAGS_NONE, + NULL, + name_gone_set_boolean_cb, + &gone, NULL); + g_dbus_connection_close_sync (f->confined_conn, NULL, &f->error); + g_assert_no_error (f->error); + + g_test_message ("Waiting for confined app bus name to disappear..."); + + while (!gone) + g_main_context_iteration (NULL, TRUE); + + g_bus_unwatch_name (name_watch); + } + } + + /* If we are able to switch uid (i.e. we are root), check that a local + * attacker with a different uid cannot close our container instances. */ + attacker = test_try_connect_gdbus_as_user (f->bus_address, TEST_USER_OTHER, + &f->error); + + if (attacker != NULL) + { + attacker_proxy = g_dbus_proxy_new_sync (attacker, + G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES, + NULL, DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, + DBUS_INTERFACE_CONTAINERS1, NULL, + &f->error); + g_assert_no_error (f->error); + + tuple = g_dbus_proxy_call_sync (attacker_proxy, "StopListening", + g_variant_new ("(o)", f->instance_path), + G_DBUS_CALL_FLAGS_NONE, -1, NULL, + &f->error); + g_assert_error (f->error, G_DBUS_ERROR, G_DBUS_ERROR_ACCESS_DENIED); + g_assert_null (tuple); + g_clear_error (&f->error); + + tuple = g_dbus_proxy_call_sync (attacker_proxy, "StopInstance", + g_variant_new ("(o)", f->instance_path), + G_DBUS_CALL_FLAGS_NONE, -1, NULL, + &f->error); + g_assert_error (f->error, G_DBUS_ERROR, G_DBUS_ERROR_ACCESS_DENIED); + g_assert_null (tuple); + g_clear_error (&f->error); + + g_clear_object (&attacker_proxy); + g_dbus_connection_close_sync (attacker, NULL, &f->error); + g_assert_no_error (f->error); + g_clear_object (&attacker); + } + else + { + /* If we aren't running as root, it's OK to not be able to connect again + * as some other user (usually 'nobody'). We don't g_test_skip() here + * because this is just extra coverage */ + g_assert_error (f->error, G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED); + g_clear_error (&f->error); + } + + switch (config->stop_server) + { + case STOP_SERVER_WITH_MANAGER: + /* Close the unconfined connection (the container manager) and wait + * for it to go away */ + g_test_message ("Closing container manager..."); + manager_unique_name = g_dbus_connection_get_unique_name (f->unconfined_conn); + name_watch = g_bus_watch_name_on_connection (f->confined_conn, + manager_unique_name, + G_BUS_NAME_WATCHER_FLAGS_NONE, + NULL, + name_gone_set_boolean_cb, + &gone, NULL); + g_dbus_connection_close_sync (f->unconfined_conn, NULL, &f->error); + g_assert_no_error (f->error); + + g_test_message ("Waiting for container manager bus name to disappear..."); + + while (!gone) + g_main_context_iteration (NULL, TRUE); + + g_bus_unwatch_name (name_watch); + break; + + case STOP_SERVER_EXPLICITLY: + case STOP_SERVER_DISCONNECT_FIRST: + case STOP_SERVER_NEVER_CONNECTED: + g_test_message ("Stopping server (but not confined connection)..."); + tuple = g_dbus_proxy_call_sync (f->proxy, "StopListening", + g_variant_new ("(o)", f->instance_path), + G_DBUS_CALL_FLAGS_NONE, -1, NULL, + &f->error); + g_assert_no_error (f->error); + g_variant_unref (tuple); + break; + + case STOP_SERVER_FORCE: + g_test_message ("Stopping server and all confined connections..."); + tuple = g_dbus_proxy_call_sync (f->proxy, "StopInstance", + g_variant_new ("(o)", f->instance_path), + G_DBUS_CALL_FLAGS_NONE, -1, NULL, + &f->error); + g_assert_no_error (f->error); + g_variant_unref (tuple); + break; + + default: + g_assert_not_reached (); + } + + /* Now if we try to connect to the server again, it will fail (eventually - + * closing the socket is not synchronous with respect to the name owner + * change, so try a few times) */ + for (i = 0; i < 50; i++) + { + g_test_message ("Trying to connect to %s again...", f->socket_path); + client_socket = g_socket_new (G_SOCKET_FAMILY_UNIX, G_SOCKET_TYPE_STREAM, + G_SOCKET_PROTOCOL_DEFAULT, &f->error); + g_assert_no_error (f->error); + + if (!g_socket_connect (client_socket, socket_address, NULL, &f->error)) + { + g_assert_cmpstr (g_quark_to_string (f->error->domain), ==, + g_quark_to_string (G_IO_ERROR)); + + if (f->error->code != G_IO_ERROR_CONNECTION_REFUSED && + f->error->code != G_IO_ERROR_NOT_FOUND) + g_error ("Unexpected error code %d", f->error->code); + + g_clear_error (&f->error); + g_clear_object (&client_socket); + break; + } + + g_clear_object (&client_socket); + g_usleep (G_USEC_PER_SEC / 10); + } + + /* The same thing happens for a D-Bus connection */ + g_test_message ("Trying to connect to %s again...", f->socket_dbus_address); + second_confined_conn = g_dbus_connection_new_for_address_sync ( + f->socket_dbus_address, + (G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION | + G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT), + NULL, NULL, &f->error); + g_assert_cmpstr (g_quark_to_string (f->error->domain), ==, + g_quark_to_string (G_IO_ERROR)); + + if (f->error->code != G_IO_ERROR_CONNECTION_REFUSED && + f->error->code != G_IO_ERROR_NOT_FOUND) + g_error ("Unexpected error code %d", f->error->code); + + g_clear_error (&f->error); + g_assert_null (second_confined_conn); + + /* The socket has been deleted */ + g_assert_false (g_file_test (f->socket_path, G_FILE_TEST_EXISTS)); + + switch (config->stop_server) + { + case STOP_SERVER_FORCE: + g_test_message ("Checking that the confined app gets disconnected..."); + + while (!g_dbus_connection_is_closed (f->confined_conn)) + g_main_context_iteration (NULL, TRUE); + break; + + case STOP_SERVER_DISCONNECT_FIRST: + case STOP_SERVER_NEVER_CONNECTED: + /* Nothing to be done here, no confined app is connected */ + break; + + case STOP_SERVER_EXPLICITLY: + case STOP_SERVER_WITH_MANAGER: + g_test_message ("Checking that the confined app still works..."); + tuple = g_dbus_connection_call_sync (f->confined_conn, + DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, + DBUS_INTERFACE_DBUS, + "GetNameOwner", + g_variant_new ("(s)", + DBUS_SERVICE_DBUS), + G_VARIANT_TYPE ("(s)"), + G_DBUS_CALL_FLAGS_NONE, -1, + NULL, &f->error); + g_assert_no_error (f->error); + g_assert_nonnull (tuple); + g_assert_cmpstr (g_variant_get_type_string (tuple), ==, "(s)"); + g_variant_get (tuple, "(&s)", &name_owner); + g_assert_cmpstr (name_owner, ==, DBUS_SERVICE_DBUS); + g_clear_pointer (&tuple, g_variant_unref); + break; + + default: + g_assert_not_reached (); + } + +#else /* !HAVE_CONTAINERS_TEST */ + g_test_skip ("Containers or gio-unix-2.0 not supported"); +#endif /* !HAVE_CONTAINERS_TEST */ +} + /* * Assert that named arguments are validated: passing an unsupported * named argument causes an error. @@ -442,6 +751,32 @@ teardown (Fixture *f, g_clear_error (&f->error); } +static const Config stop_server_explicitly = +{ + "valid-config-files/multi-user.conf", + STOP_SERVER_EXPLICITLY +}; +static const Config stop_server_disconnect_first = +{ + "valid-config-files/multi-user.conf", + STOP_SERVER_DISCONNECT_FIRST +}; +static const Config stop_server_never_connected = +{ + "valid-config-files/multi-user.conf", + STOP_SERVER_NEVER_CONNECTED +}; +static const Config stop_server_force = +{ + "valid-config-files/multi-user.conf", + STOP_SERVER_FORCE +}; +static const Config stop_server_with_manager = +{ + "valid-config-files/multi-user.conf", + STOP_SERVER_WITH_MANAGER +}; + int main (int argc, char **argv) @@ -477,6 +812,16 @@ main (int argc, setup, test_basic, teardown); g_test_add ("/containers/wrong-uid", Fixture, NULL, setup, test_wrong_uid, teardown); + g_test_add ("/containers/stop-server/explicitly", Fixture, + &stop_server_explicitly, setup, test_stop_server, teardown); + g_test_add ("/containers/stop-server/disconnect-first", Fixture, + &stop_server_disconnect_first, setup, test_stop_server, teardown); + g_test_add ("/containers/stop-server/never-connected", Fixture, + &stop_server_never_connected, setup, test_stop_server, teardown); + g_test_add ("/containers/stop-server/force", Fixture, + &stop_server_force, setup, test_stop_server, teardown); + g_test_add ("/containers/stop-server/with-manager", Fixture, + &stop_server_with_manager, setup, test_stop_server, teardown); g_test_add ("/containers/unsupported-parameter", Fixture, NULL, setup, test_unsupported_parameter, teardown); g_test_add ("/containers/invalid-type-name", Fixture, NULL, From 7bf06575c40b130bb0795dc04b731b7cccc1aa7d Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 23 Jun 2017 12:54:34 +0100 Subject: [PATCH 22/44] bus/containers: Emit InstanceRemoved signal Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/containers.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++ bus/driver.c | 5 +++- 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/bus/containers.c b/bus/containers.c index eb2b89c6..aea9f78b 100644 --- a/bus/containers.c +++ b/bus/containers.c @@ -38,6 +38,7 @@ #include "dbus/dbus-sysdeps-unix.h" #include "connection.h" +#include "dispatch.h" #include "driver.h" #include "utils.h" @@ -60,6 +61,7 @@ typedef struct * removed from the bus */ DBusList *connections; unsigned long uid; + unsigned announced:1; } BusContainerInstance; /* Data attached to a DBusConnection that has created container instances. */ @@ -209,6 +211,61 @@ bus_container_instance_ref (BusContainerInstance *self) return self; } +static dbus_bool_t +bus_container_instance_emit_removed (BusContainerInstance *self) +{ + BusTransaction *transaction = NULL; + DBusMessage *message = NULL; + DBusError error = DBUS_ERROR_INIT; + + transaction = bus_transaction_new (self->context); + + if (transaction == NULL) + goto oom; + + message = dbus_message_new_signal (DBUS_PATH_DBUS, + DBUS_INTERFACE_CONTAINERS1, + "InstanceRemoved"); + + if (message == NULL || + !dbus_message_set_sender (message, DBUS_SERVICE_DBUS) || + !dbus_message_append_args (message, + DBUS_TYPE_OBJECT_PATH, &self->path, + DBUS_TYPE_INVALID) || + !bus_transaction_capture (transaction, NULL, NULL, message)) + goto oom; + + if (!bus_dispatch_matches (transaction, NULL, NULL, message, &error)) + { + if (dbus_error_has_name (&error, DBUS_ERROR_NO_MEMORY)) + goto oom; + + /* This can't actually happen, because all of the error cases in + * bus_dispatch_matches() are only if there is an addressed recipient + * (a unicast message), which there is not in this case. But if it + * somehow does happen, we don't want to stay in the OOM-retry loop, + * because waiting for more memory will not help; so continue to + * execute the transaction anyway. */ + _dbus_warn ("Failed to send InstanceRemoved for a reason " + "other than OOM: %s: %s", error.name, error.message); + dbus_error_free (&error); + } + + bus_transaction_execute_and_free (transaction); + dbus_message_unref (message); + _DBUS_ASSERT_ERROR_IS_CLEAR (&error); + return TRUE; + +oom: + dbus_error_free (&error); + dbus_clear_message (&message); + + if (transaction != NULL) + bus_transaction_cancel_and_free (transaction); + + return FALSE; +} + static void bus_container_instance_unref (BusContainerInstance *self) { @@ -218,6 +275,21 @@ bus_container_instance_unref (BusContainerInstance *self) { BusContainerCreatorData *creator_data; + /* If we announced the container instance in a reply from + * AddServer() (which is also the time at which it becomes + * available for the querying methods), then we have to emit + * InstanceRemoved for it. + * + * Similar to bus/connection.c dropping well-known name ownership, + * this isn't really a situation where we can "fail", because + * this last-unref is likely to be happening as a result of a + * connection disconnecting; so we use a retry loop on OOM. */ + for (; self->announced; _dbus_wait_for_memory ()) + { + if (bus_container_instance_emit_removed (self)) + self->announced = FALSE; + } + /* As long as the server is listening, the BusContainerInstance can't * be freed, because the DBusServer holds a reference to the * BusContainerInstance */ @@ -754,6 +826,7 @@ bus_containers_handle_add_server (DBusConnection *connection, if (! bus_transaction_send_from_driver (transaction, connection, reply)) goto oom; + instance->announced = TRUE; dbus_message_unref (reply); bus_container_instance_unref (instance); dbus_address_entries_free (entries); diff --git a/bus/driver.c b/bus/driver.c index bcf38d05..feac80ad 100644 --- a/bus/driver.c +++ b/bus/driver.c @@ -2639,7 +2639,10 @@ static InterfaceHandler interface_handlers[] = { INTERFACE_FLAG_NONE }, #endif #ifdef DBUS_ENABLE_CONTAINERS - { DBUS_INTERFACE_CONTAINERS1, containers_message_handlers, NULL, + { DBUS_INTERFACE_CONTAINERS1, containers_message_handlers, + " \n" + " \n" + " \n", INTERFACE_FLAG_NONE, containers_property_handlers }, #endif { DBUS_INTERFACE_PEER, peer_message_handlers, NULL, From 1de35ba8eea111723ccb0d8c996c8abe7fa5a00d Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 23 Jun 2017 14:12:03 +0100 Subject: [PATCH 23/44] test/containers: Assert that InstanceRemoved is emitted Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- test/containers.c | 112 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 109 insertions(+), 3 deletions(-) diff --git a/test/containers.c b/test/containers.c index 4705243e..9964dd1b 100644 --- a/test/containers.c +++ b/test/containers.c @@ -59,6 +59,10 @@ typedef struct { gchar *socket_dbus_address; GDBusConnection *unconfined_conn; GDBusConnection *confined_conn; + + GDBusConnection *observer_conn; + GHashTable *containers_removed; + guint removed_sub; } Fixture; typedef struct @@ -96,6 +100,28 @@ name_gone_set_boolean_cb (GDBusConnection *conn, } #endif +static void +instance_removed_cb (GDBusConnection *observer, + const gchar *sender, + const gchar *path, + const gchar *iface, + const gchar *member, + GVariant *parameters, + gpointer user_data) +{ + Fixture *f = user_data; + const gchar *container; + + g_assert_cmpstr (sender, ==, DBUS_SERVICE_DBUS); + g_assert_cmpstr (path, ==, DBUS_PATH_DBUS); + g_assert_cmpstr (iface, ==, DBUS_INTERFACE_CONTAINERS1); + g_assert_cmpstr (member, ==, "InstanceRemoved"); + g_assert_cmpstr (g_variant_get_type_string (parameters), ==, "(o)"); + g_variant_get (parameters, "(&o)", &container); + g_assert (!g_hash_table_contains (f->containers_removed, container)); + g_hash_table_add (f->containers_removed, g_strdup (container)); +} + static void setup (Fixture *f, gconstpointer context) @@ -119,6 +145,22 @@ setup (Fixture *f, G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT), NULL, NULL, &f->error); g_assert_no_error (f->error); + + f->observer_conn = g_dbus_connection_new_for_address_sync (f->bus_address, + (G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION | + G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT), + NULL, NULL, &f->error); + g_assert_no_error (f->error); + f->containers_removed = g_hash_table_new_full (g_str_hash, g_str_equal, + g_free, NULL); + f->removed_sub = g_dbus_connection_signal_subscribe (f->observer_conn, + DBUS_SERVICE_DBUS, + DBUS_INTERFACE_CONTAINERS1, + "InstanceRemoved", + DBUS_PATH_DBUS, NULL, + G_DBUS_SIGNAL_FLAGS_NONE, + instance_removed_cb, + f, NULL); } /* @@ -413,7 +455,7 @@ test_stop_server (Fixture *f, gone = FALSE; confined_unique_name = g_dbus_connection_get_unique_name ( f->confined_conn); - name_watch = g_bus_watch_name_on_connection (f->unconfined_conn, + name_watch = g_bus_watch_name_on_connection (f->observer_conn, confined_unique_name, G_BUS_NAME_WATCHER_FLAGS_NONE, NULL, @@ -476,6 +518,9 @@ test_stop_server (Fixture *f, g_clear_error (&f->error); } + g_assert_false (g_hash_table_contains (f->containers_removed, + f->instance_path)); + switch (config->stop_server) { case STOP_SERVER_WITH_MANAGER: @@ -501,8 +546,6 @@ test_stop_server (Fixture *f, break; case STOP_SERVER_EXPLICITLY: - case STOP_SERVER_DISCONNECT_FIRST: - case STOP_SERVER_NEVER_CONNECTED: g_test_message ("Stopping server (but not confined connection)..."); tuple = g_dbus_proxy_call_sync (f->proxy, "StopListening", g_variant_new ("(o)", f->instance_path), @@ -510,6 +553,35 @@ test_stop_server (Fixture *f, &f->error); g_assert_no_error (f->error); g_variant_unref (tuple); + + /* The container instance remains open, because the connection has + * not gone away yet. Do another method call: if we were going to + * get the signal, it would arrive before the reply to this second + * method call. Any method will do here, even one that doesn't + * exist. */ + g_test_message ("Checking we do not get InstanceRemoved..."); + tuple = g_dbus_proxy_call_sync (f->proxy, "NoSuchMethod", NULL, + G_DBUS_CALL_FLAGS_NONE, -1, NULL, + &f->error); + g_assert_error (f->error, G_DBUS_ERROR, G_DBUS_ERROR_UNKNOWN_METHOD); + g_assert_null (tuple); + g_clear_error (&f->error); + break; + + case STOP_SERVER_DISCONNECT_FIRST: + case STOP_SERVER_NEVER_CONNECTED: + g_test_message ("Stopping server (with no confined connections)..."); + tuple = g_dbus_proxy_call_sync (f->proxy, "StopListening", + g_variant_new ("(o)", f->instance_path), + G_DBUS_CALL_FLAGS_NONE, -1, NULL, + &f->error); + g_assert_no_error (f->error); + g_variant_unref (tuple); + + g_test_message ("Waiting for InstanceRemoved..."); + while (!g_hash_table_contains (f->containers_removed, f->instance_path)) + g_main_context_iteration (NULL, TRUE); + break; case STOP_SERVER_FORCE: @@ -520,6 +592,11 @@ test_stop_server (Fixture *f, &f->error); g_assert_no_error (f->error); g_variant_unref (tuple); + + g_test_message ("Waiting for InstanceRemoved..."); + while (!g_hash_table_contains (f->containers_removed, f->instance_path)) + g_main_context_iteration (NULL, TRUE); + break; default: @@ -607,12 +684,24 @@ test_stop_server (Fixture *f, g_variant_get (tuple, "(&s)", &name_owner); g_assert_cmpstr (name_owner, ==, DBUS_SERVICE_DBUS); g_clear_pointer (&tuple, g_variant_unref); + + /* Now disconnect the last confined connection, which will make the + * container instance go away */ + g_test_message ("Closing confined connection..."); + g_dbus_connection_close_sync (f->confined_conn, NULL, &f->error); + g_assert_no_error (f->error); break; default: g_assert_not_reached (); } + /* Whatever happened above, by now it has gone away */ + + g_test_message ("Waiting for InstanceRemoved..."); + while (!g_hash_table_contains (f->containers_removed, f->instance_path)) + g_main_context_iteration (NULL, TRUE); + #else /* !HAVE_CONTAINERS_TEST */ g_test_skip ("Containers or gio-unix-2.0 not supported"); #endif /* !HAVE_CONTAINERS_TEST */ @@ -709,6 +798,23 @@ teardown (Fixture *f, { g_clear_object (&f->proxy); + if (f->observer_conn != NULL) + { + GError *error = NULL; + + g_dbus_connection_signal_unsubscribe (f->observer_conn, + f->removed_sub); + g_dbus_connection_close_sync (f->observer_conn, NULL, &error); + + if (g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CLOSED)) + g_clear_error (&error); + else + g_assert_no_error (error); + } + + g_clear_pointer (&f->containers_removed, g_hash_table_unref); + g_clear_object (&f->observer_conn); + if (f->unconfined_conn != NULL) { GError *error = NULL; From 7743c98b4b00d478849700e3b01d42a617b2f161 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 21 Jul 2017 20:51:53 +0100 Subject: [PATCH 24/44] bus/containers: Indicate in loginfo whether connection is contained Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/connection.c | 26 ++++++++++++++++++++++++++ bus/containers.c | 29 +++++++++++++++++++++++++++++ bus/containers.h | 4 ++++ 3 files changed, 59 insertions(+) diff --git a/bus/connection.c b/bus/connection.c index d53522f5..91b1966e 100644 --- a/bus/connection.c +++ b/bus/connection.c @@ -588,6 +588,9 @@ cache_peer_loginfo_string (BusConnectionData *d, unsigned long pid; char *windows_sid = NULL, *security_label = NULL; dbus_bool_t prev_added; + const char *container = NULL; + const char *container_type = NULL; + const char *container_name = NULL; if (!_dbus_string_init (&loginfo_buf)) return FALSE; @@ -659,6 +662,29 @@ cache_peer_loginfo_string (BusConnectionData *d, prev_added = TRUE; } + if (bus_containers_connection_is_contained (connection, &container, + &container_type, + &container_name)) + { + dbus_bool_t did_append; + + if (prev_added) + { + if (!_dbus_string_append_byte (&loginfo_buf, ' ')) + goto oom; + } + + did_append = _dbus_string_append_printf (&loginfo_buf, + "container=%s %s=\"%s\")", + container, + container_type, + container_name); + if (!did_append) + goto oom; + else + prev_added = TRUE; + } + if (!_dbus_string_steal_data (&loginfo_buf, &(d->cached_loginfo_string))) goto oom; diff --git a/bus/containers.c b/bus/containers.c index aea9f78b..8f1f71ff 100644 --- a/bus/containers.c +++ b/bus/containers.c @@ -1091,3 +1091,32 @@ bus_containers_remove_connection (BusContainers *self, dbus_connection_unref (connection); #endif /* DBUS_ENABLE_CONTAINERS */ } + +dbus_bool_t +bus_containers_connection_is_contained (DBusConnection *connection, + const char **path, + const char **type, + const char **name) +{ +#ifdef DBUS_ENABLE_CONTAINERS + BusContainerInstance *instance; + + instance = dbus_connection_get_data (connection, contained_data_slot); + + if (instance != NULL) + { + if (path != NULL) + *path = instance->path; + + if (type != NULL) + *type = instance->type; + + if (name != NULL) + *name = instance->name; + + return TRUE; + } +#endif /* DBUS_ENABLE_CONTAINERS */ + + return FALSE; +} diff --git a/bus/containers.h b/bus/containers.h index f3ecf1d4..2fb4b3aa 100644 --- a/bus/containers.h +++ b/bus/containers.h @@ -49,6 +49,10 @@ dbus_bool_t bus_containers_supported_arguments_getter (BusContext *context, void bus_containers_remove_connection (BusContainers *self, DBusConnection *connection); +dbus_bool_t bus_containers_connection_is_contained (DBusConnection *connection, + const char **path, + const char **type, + const char **name); static inline void bus_clear_containers (BusContainers **containers_p) From 6b8ee7a68c33400f0243a2389d3601689d4eb5e6 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 8 Jun 2017 14:43:18 +0100 Subject: [PATCH 25/44] 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. Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/driver.c | 34 +++++++++++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/bus/driver.c b/bus/driver.c index feac80ad..104a0b6f 100644 --- a/bus/driver.c +++ b/bus/driver.c @@ -110,6 +110,28 @@ bus_driver_get_conn_helper (DBusConnection *connection, return BUS_DRIVER_FOUND_PEER; } +static dbus_bool_t +bus_driver_check_caller_is_not_container (DBusConnection *connection, + BusTransaction *transaction, + DBusMessage *message, + DBusError *error) +{ + if (bus_containers_connection_is_contained (connection, NULL, NULL, NULL)) + { + const char *method = dbus_message_get_member (message); + + bus_context_log_and_set_error (bus_transaction_get_context (transaction), + DBUS_SYSTEM_LOG_SECURITY, error, DBUS_ERROR_ACCESS_DENIED, + "rejected attempt to call %s by connection %s (%s) in " + "container", method, + nonnull (bus_connection_get_name (connection), "(inactive)"), + bus_connection_get_loginfo (connection)); + return FALSE; + } + + return TRUE; +} + /* * Log a security warning and set error unless the uid of the connection * is either the uid of this process, or on Unix, uid 0 (root). @@ -129,7 +151,16 @@ bus_driver_check_caller_is_privileged (DBusConnection *connection, { #ifdef DBUS_UNIX unsigned long uid; +#elif defined(DBUS_WIN) + char *windows_sid = NULL; + dbus_bool_t ret = FALSE; +#endif + if (!bus_driver_check_caller_is_not_container (connection, transaction, + message, error)) + return FALSE; + +#ifdef DBUS_UNIX if (!dbus_connection_get_unix_user (connection, &uid)) { const char *method = dbus_message_get_member (message); @@ -169,9 +200,6 @@ bus_driver_check_caller_is_privileged (DBusConnection *connection, return TRUE; #elif defined(DBUS_WIN) - char *windows_sid = NULL; - dbus_bool_t ret = FALSE; - if (!dbus_connection_get_windows_user (connection, &windows_sid)) { const char *method = dbus_message_get_member (message); From 6537b583f64f294f9a9a54660c1ce4a64b4b2aa6 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 8 Jun 2017 14:44:05 +0100 Subject: [PATCH 26/44] test/containers: Check that containers can't make new containers We should prevent containers from trying to put a container in our container so we can sandbox while we sandbox. 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. When we remove that flag (after we've introduced better resource limits), we can specifically restrict this method to not be called by containers instead. This test will make sure we do. Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- test/containers.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/test/containers.c b/test/containers.c index 9964dd1b..4b4a944e 100644 --- a/test/containers.c +++ b/test/containers.c @@ -792,6 +792,69 @@ test_invalid_type_name (Fixture *f, #endif /* !HAVE_CONTAINERS_TEST */ } +/* + * Assert that a request to create a container server cannot come from a + * connection to an existing container server. + * (You cannot put containers in your container so you can sandbox while + * you sandbox.) + */ +static void +test_invalid_nesting (Fixture *f, + gconstpointer context) +{ +#ifdef HAVE_CONTAINERS_TEST + GDBusProxy *nested_proxy; + GVariant *tuple; + GVariant *parameters; + + if (f->skip) + return; + + parameters = g_variant_new ("(ssa{sv}a{sv})", + "com.example.NotFlatpak", + "sample-app", + NULL, /* no metadata */ + NULL); /* no named arguments */ + if (!add_container_server (f, g_steal_pointer (¶meters))) + return; + + g_test_message ("Connecting to %s...", f->socket_dbus_address); + f->confined_conn = g_dbus_connection_new_for_address_sync ( + f->socket_dbus_address, + (G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION | + G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT), + NULL, NULL, &f->error); + g_assert_no_error (f->error); + + g_test_message ("Checking that confined app cannot nest containers..."); + nested_proxy = g_dbus_proxy_new_sync (f->confined_conn, + G_DBUS_PROXY_FLAGS_NONE, NULL, + DBUS_SERVICE_DBUS, DBUS_PATH_DBUS, + DBUS_INTERFACE_CONTAINERS1, NULL, + &f->error); + g_assert_no_error (f->error); + + parameters = g_variant_new ("(ssa{sv}a{sv})", + "com.example.NotFlatpak", + "inner-app", + NULL, /* no metadata */ + NULL); /* no named arguments */ + tuple = g_dbus_proxy_call_sync (nested_proxy, "AddServer", + g_steal_pointer (¶meters), + G_DBUS_CALL_FLAGS_NONE, + -1, NULL, &f->error); + + g_assert_error (f->error, G_DBUS_ERROR, G_DBUS_ERROR_ACCESS_DENIED); + g_assert_null (tuple); + g_clear_error (&f->error); + + g_clear_object (&nested_proxy); + +#else /* !HAVE_CONTAINERS_TEST */ + g_test_skip ("Containers or gio-unix-2.0 not supported"); +#endif /* !HAVE_CONTAINERS_TEST */ +} + static void teardown (Fixture *f, gconstpointer context G_GNUC_UNUSED) @@ -932,6 +995,8 @@ main (int argc, setup, test_unsupported_parameter, teardown); g_test_add ("/containers/invalid-type-name", Fixture, NULL, setup, test_invalid_type_name, teardown); + g_test_add ("/containers/invalid-nesting", Fixture, NULL, + setup, test_invalid_nesting, teardown); ret = g_test_run (); From 2c492620573647141679704e659fd64a5783dece Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 8 Jun 2017 14:43:43 +0100 Subject: [PATCH 27/44] test/containers: Check that connections from containers are unprivileged Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- test/containers.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/containers.c b/test/containers.c index 4b4a944e..dd5797cb 100644 --- a/test/containers.c +++ b/test/containers.c @@ -333,6 +333,19 @@ test_basic (Fixture *f, g_assert_cmpstr (g_variant_get_type_string (tuple), ==, "()"); g_clear_pointer (&tuple, g_variant_unref); + g_test_message ("Checking that confined app is not considered privileged..."); + tuple = g_dbus_connection_call_sync (f->confined_conn, DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, + "UpdateActivationEnvironment", + g_variant_new ("(a{ss})", NULL), + G_VARIANT_TYPE_UNIT, + G_DBUS_CALL_FLAGS_NONE, -1, NULL, + &f->error); + g_assert_error (f->error, G_DBUS_ERROR, G_DBUS_ERROR_ACCESS_DENIED); + g_test_message ("Access denied as expected: %s", f->error->message); + g_clear_error (&f->error); + g_assert_null (tuple); + /* Check that the socket is cleaned up when the dbus-daemon is terminated */ test_kill_pid (f->daemon_pid); g_spawn_close_pid (f->daemon_pid); From 85c428d937e330c15132ef1899af72d33f60f369 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 9 Jun 2017 13:43:25 +0100 Subject: [PATCH 28/44] bus/driver: Add a flag for methods that can't be invoked by containers We can relax AddServer() from PRIVILEGED to NOT_CONTAINERS when we've put resource limits in place, although for now it must remain PRIVILEGED because it uses up resources. Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/driver.c | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/bus/driver.c b/bus/driver.c index 104a0b6f..e943ea0e 100644 --- a/bus/driver.c +++ b/bus/driver.c @@ -2397,9 +2397,15 @@ typedef enum /* If set, callers must be privileged. On Unix, the uid of the connection * must either be the uid of this process, or 0 (root). On Windows, - * the SID of the connection must be the SID of this process. */ + * the SID of the connection must be the SID of this process. + * + * This flag effectively implies METHOD_FLAG_NO_CONTAINERS, because + * containers are never privileged. */ METHOD_FLAG_PRIVILEGED = (1 << 1), + /* If set, callers must not be associated with a container instance. */ + METHOD_FLAG_NO_CONTAINERS = (1 << 2), + METHOD_FLAG_NONE = 0 } MethodFlags; @@ -2965,12 +2971,25 @@ bus_driver_handle_message (DBusConnection *connection, _dbus_verbose ("Found driver handler for %s\n", name); - if ((mh->flags & METHOD_FLAG_PRIVILEGED) && - !bus_driver_check_caller_is_privileged (connection, transaction, - message, error)) + if (mh->flags & METHOD_FLAG_PRIVILEGED) { - _DBUS_ASSERT_ERROR_IS_SET (error); - return FALSE; + if (!bus_driver_check_caller_is_privileged (connection, + transaction, message, + error)) + { + _DBUS_ASSERT_ERROR_IS_SET (error); + return FALSE; + } + } + else if (mh->flags & METHOD_FLAG_NO_CONTAINERS) + { + if (!bus_driver_check_caller_is_not_container (connection, + transaction, + message, error)) + { + _DBUS_ASSERT_ERROR_IS_SET (error); + return FALSE; + } } if (!(is_canonical_path || (mh->flags & METHOD_FLAG_ANY_PATH))) From fa139de7111604242b2327b91832f69e29635185 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 8 Jun 2017 18:17:31 +0100 Subject: [PATCH 29/44] 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. Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/driver.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/bus/driver.c b/bus/driver.c index e943ea0e..cef1e862 100644 --- a/bus/driver.c +++ b/bus/driver.c @@ -2577,9 +2577,9 @@ static const MessageHandler monitoring_message_handlers[] = { #ifdef DBUS_ENABLE_VERBOSE_MODE static const MessageHandler verbose_message_handlers[] = { { "EnableVerbose", "", "", bus_driver_handle_enable_verbose, - METHOD_FLAG_NONE }, + METHOD_FLAG_NO_CONTAINERS }, { "DisableVerbose", "", "", bus_driver_handle_disable_verbose, - METHOD_FLAG_NONE }, + METHOD_FLAG_NO_CONTAINERS }, { NULL, NULL, NULL, NULL } }; #endif @@ -2587,11 +2587,11 @@ static const MessageHandler verbose_message_handlers[] = { #ifdef DBUS_ENABLE_STATS static const MessageHandler stats_message_handlers[] = { { "GetStats", "", "a{sv}", bus_stats_handle_get_stats, - METHOD_FLAG_NONE }, + METHOD_FLAG_NO_CONTAINERS }, { "GetConnectionStats", "s", "a{sv}", bus_stats_handle_get_connection_stats, - METHOD_FLAG_NONE }, + METHOD_FLAG_NO_CONTAINERS }, { "GetAllMatchRules", "", "a{sas}", bus_stats_handle_get_all_match_rules, - METHOD_FLAG_NONE }, + METHOD_FLAG_NO_CONTAINERS }, { NULL, NULL, NULL, NULL } }; #endif From 843acf3c59735ceaf006e8d80139a7932759c381 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 9 Jun 2017 15:58:56 +0100 Subject: [PATCH 30/44] bus/driver: Add basic container info to GetConnectionCredentials() Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/driver.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/bus/driver.c b/bus/driver.c index cef1e862..ff14ccab 100644 --- a/bus/driver.c +++ b/bus/driver.c @@ -1915,7 +1915,10 @@ bus_driver_handle_get_connection_credentials (DBusConnection *connection, DBusMessageIter array_iter; unsigned long ulong_uid, ulong_pid; char *s; + const char *name; + const char *path; const char *service; + const char *type; BusDriverFound found; _DBUS_ASSERT_ERROR_IS_CLEAR (error); @@ -2003,6 +2006,19 @@ bus_driver_handle_get_connection_credentials (DBusConnection *connection, dbus_free (s); } + if (found == BUS_DRIVER_FOUND_PEER && + bus_containers_connection_is_contained (conn, &path, &type, &name)) + { + if (!_dbus_asv_add_object_path (&array_iter, + DBUS_INTERFACE_CONTAINERS1 ".Instance", + path) || + !_dbus_asv_add_string (&array_iter, + DBUS_INTERFACE_CONTAINERS1 ".Type", type) || + !_dbus_asv_add_string (&array_iter, + DBUS_INTERFACE_CONTAINERS1 ".Name", name)) + goto oom; + } + if (!_dbus_asv_close (&reply_iter, &array_iter)) goto oom; From 59100558bdf6fc16d82152ac4d647b87870f1911 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 9 Jun 2017 15:59:44 +0100 Subject: [PATCH 31/44] test/dbus-daemon: Assert absence of Containers1 credentials These connections are not to a container server. Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- test/dbus-daemon.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/dbus-daemon.c b/test/dbus-daemon.c index 8cada03e..9305866b 100644 --- a/test/dbus-daemon.c +++ b/test/dbus-daemon.c @@ -507,6 +507,10 @@ test_creds (Fixture *f, g_assert_not_reached (); #endif } + else if (g_str_has_prefix (name, DBUS_INTERFACE_CONTAINERS1 ".")) + { + g_assert_not_reached (); + } dbus_message_iter_next (&arr_iter); } From 232615f3717df30b5377f54b583d0e0e45e5ddbb Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 21 Jul 2017 18:08:40 +0100 Subject: [PATCH 32/44] bus/driver: Add GetConnectionInstance(), GetInstanceInfo() Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/containers.c | 142 +++++++++++++++++++++++++++++++++++++++++++++++ bus/containers.h | 8 +++ bus/driver.c | 5 ++ 3 files changed, 155 insertions(+) diff --git a/bus/containers.c b/bus/containers.c index 8f1f71ff..2818a3be 100644 --- a/bus/containers.c +++ b/bus/containers.c @@ -991,6 +991,148 @@ failed: return FALSE; } +dbus_bool_t +bus_containers_handle_get_connection_instance (DBusConnection *caller, + BusTransaction *transaction, + DBusMessage *message, + DBusError *error) +{ + BusContainerInstance *instance; + BusDriverFound found; + DBusConnection *subject; + DBusMessage *reply = NULL; + DBusMessageIter writer; + const char *bus_name; + + _DBUS_ASSERT_ERROR_IS_CLEAR (error); + + found = bus_driver_get_conn_helper (caller, message, "container instance", + &bus_name, &subject, error); + + switch (found) + { + case BUS_DRIVER_FOUND_SELF: + dbus_set_error (error, DBUS_ERROR_NOT_CONTAINER, + "The message bus is not in a container"); + goto failed; + + case BUS_DRIVER_FOUND_PEER: + break; + + case BUS_DRIVER_FOUND_ERROR: + /* fall through */ + default: + goto failed; + } + + instance = dbus_connection_get_data (subject, contained_data_slot); + + if (instance == NULL) + { + dbus_set_error (error, DBUS_ERROR_NOT_CONTAINER, + "Connection '%s' is not in a container", bus_name); + goto failed; + } + + reply = dbus_message_new_method_return (message); + + if (reply == NULL) + goto oom; + + if (!dbus_message_append_args (reply, + DBUS_TYPE_OBJECT_PATH, &instance->path, + DBUS_TYPE_STRING, &instance->type, + DBUS_TYPE_STRING, &instance->name, + DBUS_TYPE_INVALID)) + goto oom; + + dbus_message_iter_init_append (reply, &writer); + + if (!_dbus_variant_write (instance->metadata, &writer)) + goto oom; + + if (!bus_transaction_send_from_driver (transaction, caller, reply)) + goto oom; + + dbus_message_unref (reply); + return TRUE; + +oom: + BUS_SET_OOM (error); + /* fall through */ +failed: + _DBUS_ASSERT_ERROR_IS_SET (error); + + dbus_clear_message (&reply); + return FALSE; +} + +dbus_bool_t +bus_containers_handle_get_instance_info (DBusConnection *connection, + BusTransaction *transaction, + DBusMessage *message, + DBusError *error) +{ + BusContext *context; + BusContainers *containers; + BusContainerInstance *instance = NULL; + DBusMessage *reply = NULL; + DBusMessageIter writer; + const char *path; + + if (!dbus_message_get_args (message, error, + DBUS_TYPE_OBJECT_PATH, &path, + DBUS_TYPE_INVALID)) + goto failed; + + context = bus_transaction_get_context (transaction); + containers = bus_context_get_containers (context); + + if (containers->instances_by_path != NULL) + { + instance = _dbus_hash_table_lookup_string (containers->instances_by_path, + path); + } + + if (instance == NULL) + { + dbus_set_error (error, DBUS_ERROR_NOT_CONTAINER, + "There is no container with path '%s'", path); + goto failed; + } + + reply = dbus_message_new_method_return (message); + + if (reply == NULL) + goto oom; + + if (!dbus_message_append_args (reply, + DBUS_TYPE_STRING, &instance->type, + DBUS_TYPE_STRING, &instance->name, + DBUS_TYPE_INVALID)) + goto oom; + + dbus_message_iter_init_append (reply, &writer); + + if (!_dbus_variant_write (instance->metadata, &writer)) + goto oom; + + if (!bus_transaction_send_from_driver (transaction, connection, reply)) + goto oom; + + dbus_message_unref (reply); + return TRUE; + +oom: + BUS_SET_OOM (error); + /* fall through */ +failed: + _DBUS_ASSERT_ERROR_IS_SET (error); + + dbus_clear_message (&reply); + return FALSE; +} + void bus_containers_stop_listening (BusContainers *self) { diff --git a/bus/containers.h b/bus/containers.h index 2fb4b3aa..05ed0e75 100644 --- a/bus/containers.h +++ b/bus/containers.h @@ -44,6 +44,14 @@ dbus_bool_t bus_containers_handle_stop_listening (DBusConnection *connecti BusTransaction *transaction, DBusMessage *message, DBusError *error); +dbus_bool_t bus_containers_handle_get_instance_info (DBusConnection *connection, + BusTransaction *transaction, + DBusMessage *message, + DBusError *error); +dbus_bool_t bus_containers_handle_get_connection_instance (DBusConnection *connection, + BusTransaction *transaction, + DBusMessage *message, + DBusError *error); dbus_bool_t bus_containers_supported_arguments_getter (BusContext *context, DBusMessageIter *var_iter); diff --git a/bus/driver.c b/bus/driver.c index ff14ccab..3e42fabc 100644 --- a/bus/driver.c +++ b/bus/driver.c @@ -2576,6 +2576,11 @@ static const MessageHandler containers_message_handlers[] = { METHOD_FLAG_PRIVILEGED }, { "StopListening", "o", "", bus_containers_handle_stop_listening, METHOD_FLAG_PRIVILEGED }, + { "GetConnectionInstance", "s", "ossa{sv}", + bus_containers_handle_get_connection_instance, + METHOD_FLAG_NONE }, + { "GetInstanceInfo", "o", "ssa{sv}", bus_containers_handle_get_instance_info, + METHOD_FLAG_NONE }, { NULL, NULL, NULL, NULL } }; static const PropertyHandler containers_property_handlers[] = { From df2913a598849ff3d795366205519effb40d718f Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 6 Jul 2017 17:29:26 +0100 Subject: [PATCH 33/44] t/containers: Exercise trivial and non-trivial container metadata Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- test/containers.c | 242 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 242 insertions(+) diff --git a/test/containers.c b/test/containers.c index dd5797cb..4cd9fd90 100644 --- a/test/containers.c +++ b/test/containers.c @@ -274,15 +274,22 @@ add_container_server (Fixture *f, * we expect (we can connect a confined connection to it, the confined * connection can talk to the dbus-daemon and to an unconfined connection, * and the socket gets cleaned up when the dbus-daemon terminates). + * + * This also tests simple cases for metadata. */ static void test_basic (Fixture *f, gconstpointer context) { #ifdef HAVE_CONTAINERS_TEST + GVariant *asv; GVariant *parameters; + const gchar *confined_unique_name; + const gchar *path_from_query; const gchar *manager_unique_name; + const gchar *name; const gchar *name_owner; + const gchar *type; GStatBuf stat_buf; GVariant *tuple; @@ -346,6 +353,38 @@ test_basic (Fixture *f, g_clear_error (&f->error); g_assert_null (tuple); + g_test_message ("Inspecting connection container info"); + confined_unique_name = g_dbus_connection_get_unique_name (f->confined_conn); + tuple = g_dbus_proxy_call_sync (f->proxy, "GetConnectionInstance", + g_variant_new ("(s)", confined_unique_name), + G_DBUS_CALL_FLAGS_NONE, -1, NULL, &f->error); + g_assert_no_error (f->error); + g_assert_nonnull (tuple); + g_assert_cmpstr (g_variant_get_type_string (tuple), ==, "(ossa{sv})"); + g_variant_get (tuple, "(&o&s&s@a{sv})", &path_from_query, &type, &name, &asv); + g_assert_cmpstr (path_from_query, ==, f->instance_path); + g_assert_cmpstr (type, ==, "com.example.NotFlatpak"); + g_assert_cmpstr (name, ==, "sample-app"); + /* Trivial case: the metadata a{sv} is empty */ + g_assert_cmpuint (g_variant_n_children (asv), ==, 0); + g_clear_pointer (&asv, g_variant_unref); + g_clear_pointer (&tuple, g_variant_unref); + + g_test_message ("Inspecting container instance info"); + tuple = g_dbus_proxy_call_sync (f->proxy, "GetInstanceInfo", + g_variant_new ("(o)", f->instance_path), + G_DBUS_CALL_FLAGS_NONE, -1, NULL, &f->error); + g_assert_no_error (f->error); + g_assert_nonnull (tuple); + g_assert_cmpstr (g_variant_get_type_string (tuple), ==, "(ssa{sv})"); + g_variant_get (tuple, "(&s&s@a{sv})", &type, &name, &asv); + g_assert_cmpstr (type, ==, "com.example.NotFlatpak"); + g_assert_cmpstr (name, ==, "sample-app"); + /* Trivial case: the metadata a{sv} is empty */ + g_assert_cmpuint (g_variant_n_children (asv), ==, 0); + g_clear_pointer (&asv, g_variant_unref); + g_clear_pointer (&tuple, g_variant_unref); + /* Check that the socket is cleaned up when the dbus-daemon is terminated */ test_kill_pid (f->daemon_pid); g_spawn_close_pid (f->daemon_pid); @@ -404,6 +443,133 @@ test_wrong_uid (Fixture *f, #endif /* !HAVE_CONTAINERS_TEST */ } +/* + * Test for non-trivial metadata: assert that the metadata a{sv} is + * carried through correctly, and that the app name is allowed to be empty. + */ +static void +test_metadata (Fixture *f, + gconstpointer context) +{ +#ifdef HAVE_CONTAINERS_TEST + GVariant *asv; + GVariant *tuple; + GVariant *parameters; + GVariantDict dict; + const gchar *confined_unique_name; + const gchar *path_from_query; + const gchar *name; + const gchar *type; + guint u; + gboolean b; + const gchar *s; + + if (f->skip) + return; + + g_variant_dict_init (&dict, NULL); + g_variant_dict_insert (&dict, "Species", "s", "Martes martes"); + g_variant_dict_insert (&dict, "IsCrepuscular", "b", TRUE); + g_variant_dict_insert (&dict, "NChildren", "u", 2); + + parameters = g_variant_new ("(ss@a{sv}a{sv})", + "org.example.Springwatch", + /* Verify that empty app names are OK */ + "", + g_variant_dict_end (&dict), + NULL); /* no named arguments */ + if (!add_container_server (f, g_steal_pointer (¶meters))) + return; + + g_test_message ("Connecting to %s...", f->socket_dbus_address); + f->confined_conn = g_dbus_connection_new_for_address_sync ( + f->socket_dbus_address, + (G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION | + G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT), + NULL, NULL, &f->error); + g_assert_no_error (f->error); + + g_test_message ("Inspecting connection credentials..."); + confined_unique_name = g_dbus_connection_get_unique_name (f->confined_conn); + tuple = g_dbus_connection_call_sync (f->confined_conn, DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, DBUS_INTERFACE_DBUS, + "GetConnectionCredentials", + g_variant_new ("(s)", + confined_unique_name), + G_VARIANT_TYPE ("(a{sv})"), + G_DBUS_CALL_FLAGS_NONE, -1, NULL, + &f->error); + g_assert_no_error (f->error); + g_assert_nonnull (tuple); + g_assert_cmpstr (g_variant_get_type_string (tuple), ==, "(a{sv})"); + asv = g_variant_get_child_value (tuple, 0); + g_variant_dict_init (&dict, asv); + g_assert_true (g_variant_dict_lookup (&dict, + DBUS_INTERFACE_CONTAINERS1 ".Instance", + "&o", &path_from_query)); + g_assert_cmpstr (path_from_query, ==, f->instance_path); + g_assert_true (g_variant_dict_lookup (&dict, + DBUS_INTERFACE_CONTAINERS1 ".Type", + "&s", &type)); + g_assert_cmpstr (type, ==, "org.example.Springwatch"); + g_assert_true (g_variant_dict_lookup (&dict, + DBUS_INTERFACE_CONTAINERS1 ".Name", + "&s", &name)); + g_assert_cmpstr (name, ==, ""); + g_variant_dict_clear (&dict); + g_clear_pointer (&asv, g_variant_unref); + g_clear_pointer (&tuple, g_variant_unref); + + g_test_message ("Inspecting connection container info"); + tuple = g_dbus_proxy_call_sync (f->proxy, "GetConnectionInstance", + g_variant_new ("(s)", confined_unique_name), + G_DBUS_CALL_FLAGS_NONE, -1, NULL, &f->error); + g_assert_no_error (f->error); + g_assert_nonnull (tuple); + g_assert_cmpstr (g_variant_get_type_string (tuple), ==, "(ossa{sv})"); + g_variant_get (tuple, "(&o&s&s@a{sv})", &path_from_query, &type, &name, &asv); + g_assert_cmpstr (path_from_query, ==, f->instance_path); + g_assert_cmpstr (type, ==, "org.example.Springwatch"); + g_assert_cmpstr (name, ==, ""); + g_variant_dict_init (&dict, asv); + g_assert_true (g_variant_dict_lookup (&dict, "NChildren", "u", &u)); + g_assert_cmpuint (u, ==, 2); + g_assert_true (g_variant_dict_lookup (&dict, "IsCrepuscular", "b", &b)); + g_assert_cmpint (b, ==, TRUE); + g_assert_true (g_variant_dict_lookup (&dict, "Species", "&s", &s)); + g_assert_cmpstr (s, ==, "Martes martes"); + g_variant_dict_clear (&dict); + g_assert_cmpuint (g_variant_n_children (asv), ==, 3); + g_clear_pointer (&asv, g_variant_unref); + g_clear_pointer (&tuple, g_variant_unref); + + g_test_message ("Inspecting container instance info"); + tuple = g_dbus_proxy_call_sync (f->proxy, "GetInstanceInfo", + g_variant_new ("(o)", f->instance_path), + G_DBUS_CALL_FLAGS_NONE, -1, NULL, &f->error); + g_assert_no_error (f->error); + g_assert_nonnull (tuple); + g_assert_cmpstr (g_variant_get_type_string (tuple), ==, "(ssa{sv})"); + g_variant_get (tuple, "(&s&s@a{sv})", &type, &name, &asv); + g_assert_cmpstr (type, ==, "org.example.Springwatch"); + g_assert_cmpstr (name, ==, ""); + g_variant_dict_init (&dict, asv); + g_assert_true (g_variant_dict_lookup (&dict, "NChildren", "u", &u)); + g_assert_cmpuint (u, ==, 2); + g_assert_true (g_variant_dict_lookup (&dict, "IsCrepuscular", "b", &b)); + g_assert_cmpint (b, ==, TRUE); + g_assert_true (g_variant_dict_lookup (&dict, "Species", "&s", &s)); + g_assert_cmpstr (s, ==, "Martes martes"); + g_variant_dict_clear (&dict); + g_assert_cmpuint (g_variant_n_children (asv), ==, 3); + g_clear_pointer (&asv, g_variant_unref); + g_clear_pointer (&tuple, g_variant_unref); + +#else /* !HAVE_CONTAINERS_TEST */ + g_test_skip ("Containers or gio-unix-2.0 not supported"); +#endif /* !HAVE_CONTAINERS_TEST */ +} + /* * With config->stop_server == STOP_SERVER_WITH_MANAGER: * Assert that without special parameters, when the container manager @@ -720,6 +886,78 @@ test_stop_server (Fixture *f, #endif /* !HAVE_CONTAINERS_TEST */ } +/* + * Assert that we cannot get the container metadata for a path that + * isn't a container instance, or a bus name that isn't in a container + * or doesn't exist at all. + */ +static void +test_invalid_metadata_getters (Fixture *f, + gconstpointer context) +{ + const gchar *unique_name; + GVariant *tuple; + gchar *error_name; + + f->proxy = g_dbus_proxy_new_sync (f->unconfined_conn, + G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES, + NULL, DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, DBUS_INTERFACE_CONTAINERS1, + NULL, &f->error); + g_assert_no_error (f->error); + + g_test_message ("Inspecting unconfined connection"); + unique_name = g_dbus_connection_get_unique_name (f->unconfined_conn); + tuple = g_dbus_proxy_call_sync (f->proxy, "GetConnectionInstance", + g_variant_new ("(s)", unique_name), + G_DBUS_CALL_FLAGS_NONE, -1, NULL, &f->error); + g_assert_nonnull (f->error); + g_assert_null (tuple); + error_name = g_dbus_error_get_remote_error (f->error); +#ifdef DBUS_ENABLE_CONTAINERS + g_assert_cmpstr (error_name, ==, DBUS_ERROR_NOT_CONTAINER); +#else + /* TODO: We can use g_assert_error for this when we depend on GLib 2.42 */ + g_assert_cmpstr (error_name, ==, DBUS_ERROR_UNKNOWN_INTERFACE); +#endif + g_free (error_name); + g_clear_error (&f->error); + + g_test_message ("Inspecting a non-connection"); + unique_name = g_dbus_connection_get_unique_name (f->unconfined_conn); + tuple = g_dbus_proxy_call_sync (f->proxy, "GetConnectionInstance", + g_variant_new ("(s)", "com.example.Nope"), + G_DBUS_CALL_FLAGS_NONE, -1, NULL, &f->error); + g_assert_nonnull (f->error); + g_assert_null (tuple); +#ifdef DBUS_ENABLE_CONTAINERS + g_assert_error (f->error, G_DBUS_ERROR, G_DBUS_ERROR_NAME_HAS_NO_OWNER); +#else + /* TODO: We can use g_assert_error for this when we depend on GLib 2.42 */ + error_name = g_dbus_error_get_remote_error (f->error); + g_assert_cmpstr (error_name, ==, DBUS_ERROR_UNKNOWN_INTERFACE); + g_free (error_name); +#endif + g_clear_error (&f->error); + + + g_test_message ("Inspecting container instance info"); + tuple = g_dbus_proxy_call_sync (f->proxy, "GetInstanceInfo", + g_variant_new ("(o)", "/nope"), + G_DBUS_CALL_FLAGS_NONE, -1, NULL, &f->error); + g_assert_nonnull (f->error); + g_assert_null (tuple); + error_name = g_dbus_error_get_remote_error (f->error); +#ifdef DBUS_ENABLE_CONTAINERS + g_assert_cmpstr (error_name, ==, DBUS_ERROR_NOT_CONTAINER); +#else + /* TODO: We can use g_assert_error for this when we depend on GLib 2.42 */ + g_assert_cmpstr (error_name, ==, DBUS_ERROR_UNKNOWN_INTERFACE); +#endif + g_free (error_name); + g_clear_error (&f->error); +} + /* * Assert that named arguments are validated: passing an unsupported * named argument causes an error. @@ -1004,6 +1242,10 @@ main (int argc, &stop_server_force, setup, test_stop_server, teardown); g_test_add ("/containers/stop-server/with-manager", Fixture, &stop_server_with_manager, setup, test_stop_server, teardown); + g_test_add ("/containers/metadata", Fixture, NULL, + setup, test_metadata, teardown); + g_test_add ("/containers/invalid-metadata-getters", Fixture, NULL, + setup, test_invalid_metadata_getters, teardown); g_test_add ("/containers/unsupported-parameter", Fixture, NULL, setup, test_unsupported_parameter, teardown); g_test_add ("/containers/invalid-type-name", Fixture, NULL, From 5be6ca4163ae82d2cd1dadb9476a2df7e4185bc1 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 23 Jun 2017 15:11:47 +0100 Subject: [PATCH 34/44] test/containers: Check that GetInstanceInfo stops working After the container instance is removed, the method should not work. Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- test/containers.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/test/containers.c b/test/containers.c index 4cd9fd90..4cf9edf2 100644 --- a/test/containers.c +++ b/test/containers.c @@ -61,6 +61,7 @@ typedef struct { GDBusConnection *confined_conn; GDBusConnection *observer_conn; + GDBusProxy *observer_proxy; GHashTable *containers_removed; guint removed_sub; } Fixture; @@ -596,6 +597,7 @@ test_stop_server (Fixture *f, GSocketAddress *socket_address; GVariant *tuple; GVariant *parameters; + gchar *error_name; const gchar *confined_unique_name; const gchar *manager_unique_name; const gchar *name_owner; @@ -608,6 +610,14 @@ test_stop_server (Fixture *f, if (f->skip) return; + f->observer_proxy = g_dbus_proxy_new_sync (f->observer_conn, + G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES, + NULL, DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, + DBUS_INTERFACE_CONTAINERS1, NULL, + &f->error); + g_assert_no_error (f->error); + parameters = g_variant_new ("(ssa{sv}a{sv})", "com.example.NotFlatpak", "sample-app", @@ -864,6 +874,16 @@ test_stop_server (Fixture *f, g_assert_cmpstr (name_owner, ==, DBUS_SERVICE_DBUS); g_clear_pointer (&tuple, g_variant_unref); + /* The container instance will not disappear from the bus + * until the confined connection goes away */ + tuple = g_dbus_proxy_call_sync (f->observer_proxy, "GetInstanceInfo", + g_variant_new ("(o)", f->instance_path), + G_DBUS_CALL_FLAGS_NONE, -1, NULL, + &f->error); + g_assert_no_error (f->error); + g_assert_nonnull (tuple); + g_clear_pointer (&tuple, g_variant_unref); + /* Now disconnect the last confined connection, which will make the * container instance go away */ g_test_message ("Closing confined connection..."); @@ -881,6 +901,17 @@ test_stop_server (Fixture *f, while (!g_hash_table_contains (f->containers_removed, f->instance_path)) g_main_context_iteration (NULL, TRUE); + tuple = g_dbus_proxy_call_sync (f->observer_proxy, "GetInstanceInfo", + g_variant_new ("(o)", f->instance_path), + G_DBUS_CALL_FLAGS_NONE, -1, NULL, + &f->error); + g_assert_nonnull (f->error); + error_name = g_dbus_error_get_remote_error (f->error); + g_assert_cmpstr (error_name, ==, DBUS_ERROR_NOT_CONTAINER); + g_free (error_name); + g_assert_null (tuple); + g_clear_error (&f->error); + #else /* !HAVE_CONTAINERS_TEST */ g_test_skip ("Containers or gio-unix-2.0 not supported"); #endif /* !HAVE_CONTAINERS_TEST */ From 913ea94c2211523fa5917e50a42935dbd424e5d8 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 21 Jun 2017 16:35:34 +0100 Subject: [PATCH 35/44] bus: Add (unused) settings for resource limits for containers These will be enforced in subsequent commits. Reviewed-by: Philip Withnall [smcv: Fix whitespace] Signed-off-by: Simon McVittie Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/bus.c | 20 ++++++++++++++++++++ bus/bus.h | 8 ++++++++ bus/config-parser.c | 40 ++++++++++++++++++++++++++++++++++++++-- bus/session.conf.in | 6 ++++++ bus/system.conf.in | 4 ++++ doc/dbus-daemon.1.xml.in | 8 ++++++++ 6 files changed, 84 insertions(+), 2 deletions(-) diff --git a/bus/bus.c b/bus/bus.c index b0a71f67..a6f8db47 100644 --- a/bus/bus.c +++ b/bus/bus.c @@ -1397,6 +1397,26 @@ bus_context_get_reply_timeout (BusContext *context) return context->limits.reply_timeout; } +int bus_context_get_max_containers (BusContext *context) +{ + return context->limits.max_containers; +} + +int bus_context_get_max_containers_per_user (BusContext *context) +{ + return context->limits.max_containers_per_user; +} + +int bus_context_get_max_container_metadata_bytes (BusContext *context) +{ + return context->limits.max_container_metadata_bytes; +} + +int bus_context_get_max_connections_per_container (BusContext *context) +{ + return context->limits.max_connections_per_container; +} + DBusRLimit * bus_context_get_initial_fd_limit (BusContext *context) { diff --git a/bus/bus.h b/bus/bus.h index 5492af24..8f96222f 100644 --- a/bus/bus.h +++ b/bus/bus.h @@ -66,6 +66,10 @@ typedef struct int max_match_rules_per_connection; /**< Max number of match rules for a single connection */ int max_replies_per_connection; /**< Max number of replies that can be pending for each connection */ int reply_timeout; /**< How long to wait before timing out a reply */ + int max_containers; /**< Max number of restricted servers for app-containers */ + int max_containers_per_user; /**< Max number of restricted servers for app-containers, per user */ + int max_connections_per_container; /**< Max number of connections per restricted server */ + int max_container_metadata_bytes; /**< Max number of bytes of metadata per restricted server */ } BusLimits; typedef enum @@ -123,6 +127,10 @@ int bus_context_get_max_services_per_connection (BusContext int bus_context_get_max_match_rules_per_connection (BusContext *context); int bus_context_get_max_replies_per_connection (BusContext *context); int bus_context_get_reply_timeout (BusContext *context); +int bus_context_get_max_containers (BusContext *context); +int bus_context_get_max_containers_per_user (BusContext *context); +int bus_context_get_max_container_metadata_bytes (BusContext *context); +int bus_context_get_max_connections_per_container (BusContext *context); DBusRLimit * bus_context_get_initial_fd_limit (BusContext *context); dbus_bool_t bus_context_get_using_syslog (BusContext *context); void bus_context_log (BusContext *context, diff --git a/bus/config-parser.c b/bus/config-parser.c index c99a7170..f49ab1dc 100644 --- a/bus/config-parser.c +++ b/bus/config-parser.c @@ -481,7 +481,10 @@ bus_config_parser_new (const DBusString *basedir, else { - /* Make up some numbers! woot! */ + /* Make up some numbers! woot! + * Please keep these hard-coded values in sync with the comments + * in bus/system.conf.in. */ + parser->limits.max_incoming_bytes = _DBUS_ONE_MEGABYTE * 127; parser->limits.max_outgoing_bytes = _DBUS_ONE_MEGABYTE * 127; parser->limits.max_message_size = _DBUS_ONE_MEGABYTE * 32; @@ -514,12 +517,21 @@ bus_config_parser_new (const DBusString *basedir, parser->limits.max_incomplete_connections = 64; parser->limits.max_connections_per_user = 256; + parser->limits.max_containers_per_user = 16; /* Note that max_completed_connections / max_connections_per_user * is the number of users that would have to work together to - * DOS all the other users. + * DOS all the other users. The same applies to containers. */ parser->limits.max_completed_connections = 2048; + parser->limits.max_containers = 512; + /* Similarly max_connections_per_user / max_connections_per_container + * is the number of app-containers per user that would have to work + * together to DoS all the other processes of that user */ + parser->limits.max_connections_per_container = 8; + /* Someone trying to do a denial of service attack can make us store + * this much data per app-container */ + parser->limits.max_container_metadata_bytes = 4096; parser->limits.max_pending_activations = 512; parser->limits.max_services_per_connection = 512; @@ -2177,6 +2189,30 @@ set_limit (BusConfigParser *parser, must_be_int = TRUE; parser->limits.max_replies_per_connection = value; } + else if (strcmp (name, "max_containers") == 0) + { + must_be_positive = TRUE; + must_be_int = TRUE; + parser->limits.max_containers = value; + } + else if (strcmp (name, "max_containers_per_user") == 0) + { + must_be_positive = TRUE; + must_be_int = TRUE; + parser->limits.max_containers_per_user = value; + } + else if (strcmp (name, "max_container_metadata_bytes") == 0) + { + must_be_positive = TRUE; + must_be_int = TRUE; + parser->limits.max_container_metadata_bytes = value; + } + else if (strcmp (name, "max_connections_per_container") == 0) + { + must_be_positive = TRUE; + must_be_int = TRUE; + parser->limits.max_connections_per_container = value; + } else { dbus_set_error (error, DBUS_ERROR_FAILED, diff --git a/bus/session.conf.in b/bus/session.conf.in index affa7f1d..ace073c9 100644 --- a/bus/session.conf.in +++ b/bus/session.conf.in @@ -76,5 +76,11 @@ 50000 50000 50000 + 10000 + 10000 + 1000000000 + + 16 diff --git a/bus/system.conf.in b/bus/system.conf.in index f139b557..2ca4ae58 100644 --- a/bus/system.conf.in +++ b/bus/system.conf.in @@ -124,6 +124,10 @@ + + + + diff --git a/doc/dbus-daemon.1.xml.in b/doc/dbus-daemon.1.xml.in index b029232d..6368464f 100644 --- a/doc/dbus-daemon.1.xml.in +++ b/doc/dbus-daemon.1.xml.in @@ -749,6 +749,14 @@ Available limit names are: (number of calls-in-progress) "reply_timeout" : milliseconds (thousandths) until a method call times out + "max_containers" : max number of restricted servers for use + in app-containers, in total + "max_containers_per_user" : max number of app-containers per Unix uid + "max_container_metadata_bytes": max number of bytes of metadata to store + for each app-container + "max_connections_per_container": max number of (authenticated or + unauthenticated) connections to each + app-container From e4b5ec2b07aa329d5b76f7728eb25c00392da671 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 22 Jun 2017 12:19:51 +0100 Subject: [PATCH 36/44] bus/containers: Limit the size of metadata we will store Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/containers.c | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/bus/containers.c b/bus/containers.c index 2818a3be..6095e671 100644 --- a/bus/containers.c +++ b/bus/containers.c @@ -649,6 +649,8 @@ bus_containers_handle_add_server (DBusConnection *connection, DBusAddressEntry **entries = NULL; int n_entries; DBusMessage *reply = NULL; + int metadata_size; + int limit; context = bus_transaction_get_context (transaction); containers = bus_context_get_containers (context); @@ -729,6 +731,34 @@ bus_containers_handle_add_server (DBusConnection *connection, _dbus_assert (strcmp (_dbus_variant_get_signature (instance->metadata), "a{sv}") == 0); + /* For simplicity we don't count the size of the BusContainerInstance + * itself, the object path, lengths, the non-payload parts of the DBusString, + * NUL terminators and so on. That overhead is O(1) and relatively small. + * This cannot overflow because all parts came from a message, and messages + * are constrained to be orders of magnitude smaller than the maximum + * int value. */ + metadata_size = _dbus_variant_get_length (instance->metadata) + + (int) strlen (type) + + (int) strlen (name); + limit = bus_context_get_max_container_metadata_bytes (context); + + if (metadata_size > limit) + { + DBusError local_error = DBUS_ERROR_INIT; + + dbus_set_error (&local_error, DBUS_ERROR_LIMITS_EXCEEDED, + "Connection \"%s\" (%s) is not allowed to set " + "%d bytes of container metadata " + "(max_container_metadata_bytes=%d)", + bus_connection_get_name (connection), + bus_connection_get_loginfo (connection), + metadata_size, limit); + bus_context_log_literal (context, DBUS_SYSTEM_LOG_WARNING, + local_error.message); + dbus_move_error (&local_error, error); + goto fail; + } + /* Argument 3: Named parameters */ if (!dbus_message_iter_next (&iter)) _dbus_assert_not_reached ("Message type was already checked"); From 7b5c76b65581843b7d3399c68bd98d89853b614e Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 22 Jun 2017 22:49:06 +0100 Subject: [PATCH 37/44] bus/containers: Enforce max_containers limit Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/containers.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/bus/containers.c b/bus/containers.c index 6095e671..7aa3e2b7 100644 --- a/bus/containers.c +++ b/bus/containers.c @@ -798,6 +798,24 @@ bus_containers_handle_add_server (DBusConnection *connection, goto oom; } + limit = bus_context_get_max_containers (context); + + if (_dbus_hash_table_get_n_entries (containers->instances_by_path) >= limit) + { + DBusError local_error = DBUS_ERROR_INIT; + + dbus_set_error (&local_error, DBUS_ERROR_LIMITS_EXCEEDED, + "Connection \"%s\" (%s) is not allowed to create more " + "container servers (max_containers=%d)", + bus_connection_get_name (connection), + bus_connection_get_loginfo (connection), + limit); + bus_context_log_literal (context, DBUS_SYSTEM_LOG_WARNING, + local_error.message); + dbus_move_error (&local_error, error); + goto fail; + } + if (!_dbus_hash_table_insert_string (containers->instances_by_path, instance->path, instance)) goto oom; From 62e8fd7a9c873f230a51a4e1c9b356f1de7b8867 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Thu, 22 Jun 2017 12:50:48 +0100 Subject: [PATCH 38/44] bus/containers: Enforce max_connections_per_container Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/containers.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/bus/containers.c b/bus/containers.c index 7aa3e2b7..994f89b1 100644 --- a/bus/containers.c +++ b/bus/containers.c @@ -474,6 +474,19 @@ new_connection_cb (DBusServer *server, void *data) { BusContainerInstance *instance = data; + int limit = bus_context_get_max_connections_per_container (instance->context); + + /* This is O(n), but we assume n is small in practice. */ + if (_dbus_list_get_length (&instance->connections) >= limit) + { + /* We can't send this error to the new connection, so just log it */ + bus_context_log (instance->context, DBUS_SYSTEM_LOG_WARNING, + "Closing connection to container server " + "%s (%s \"%s\") because it would exceed resource limit " + "(max_connections_per_container=%d)", + instance->path, instance->type, instance->name, limit); + return; + } if (!dbus_connection_set_data (new_connection, contained_data_slot, bus_container_instance_ref (instance), From 4b4f2a89be0c31c2ab68f248de55606f2cb2ef59 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 24 Jul 2017 12:36:32 +0100 Subject: [PATCH 39/44] containers: Enforce max_containers_per_user Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/containers.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++-- dbus/dbus-hash.h | 2 ++ 2 files changed, 72 insertions(+), 3 deletions(-) diff --git a/bus/containers.c b/bus/containers.c index 994f89b1..f9865706 100644 --- a/bus/containers.c +++ b/bus/containers.c @@ -85,6 +85,8 @@ struct BusContainers /* path borrowed from BusContainerInstance => unowned BusContainerInstance * The BusContainerInstance removes itself from here on destruction. */ DBusHashTable *instances_by_path; + /* uid => (void *) (uintptr_t) number of containers */ + DBusHashTable *n_containers_by_user; DBusString address_template; dbus_uint64_t next_container_id; }; @@ -190,6 +192,7 @@ bus_containers_unref (BusContainers *self) if (--self->refcount == 0) { _dbus_clear_hash_table (&self->instances_by_path); + _dbus_clear_hash_table (&self->n_containers_by_user); _dbus_string_free (&self->address_template); dbus_free (self); @@ -307,9 +310,26 @@ bus_container_instance_unref (BusContainerInstance *self) /* It's OK to do this even if we were never added to instances_by_path, * because the paths are globally unique. */ - if (self->path != NULL && self->containers->instances_by_path != NULL) - _dbus_hash_table_remove_string (self->containers->instances_by_path, - self->path); + if (self->path != NULL && self->containers->instances_by_path != NULL && + _dbus_hash_table_remove_string (self->containers->instances_by_path, + self->path)) + { + DBusHashIter entry; + uintptr_t n = 0; + + if (!_dbus_hash_iter_lookup (self->containers->n_containers_by_user, + (void *) (uintptr_t) self->uid, + FALSE, &entry)) + _dbus_assert_not_reached ("Container should not be placed in " + "instances_by_path until its " + "n_containers_by_user entry has " + "been allocated"); + + n = (uintptr_t) _dbus_hash_iter_get_value (&entry); + _dbus_assert (n > 0); + n -= 1; + _dbus_hash_iter_set_value (&entry, (void *) n); + } _dbus_clear_variant (&self->metadata); bus_context_unref (self->context); @@ -664,6 +684,8 @@ bus_containers_handle_add_server (DBusConnection *connection, DBusMessage *reply = NULL; int metadata_size; int limit; + DBusHashIter n_containers_by_user_entry; + uintptr_t this_user_containers = 0; context = bus_transaction_get_context (transaction); containers = bus_context_get_containers (context); @@ -811,6 +833,15 @@ bus_containers_handle_add_server (DBusConnection *connection, goto oom; } + if (containers->n_containers_by_user == NULL) + { + containers->n_containers_by_user = _dbus_hash_table_new (DBUS_HASH_UINTPTR, + NULL, NULL); + + if (containers->n_containers_by_user == NULL) + goto oom; + } + limit = bus_context_get_max_containers (context); if (_dbus_hash_table_get_n_entries (containers->instances_by_path) >= limit) @@ -829,10 +860,46 @@ bus_containers_handle_add_server (DBusConnection *connection, goto fail; } + if (!_dbus_hash_iter_lookup (containers->n_containers_by_user, + /* We statically assert that a uid fits in a + * uintptr_t, so this can't lose information */ + (void *) (uintptr_t) instance->uid, TRUE, + &n_containers_by_user_entry)) + goto oom; + + this_user_containers = (uintptr_t) _dbus_hash_iter_get_value (&n_containers_by_user_entry); + limit = bus_context_get_max_containers_per_user (context); + + /* We need to be careful with types here because this_user_containers is + * unsigned. */ + if (limit <= 0 || this_user_containers >= (unsigned) limit) + { + DBusError local_error = DBUS_ERROR_INIT; + + dbus_set_error (&local_error, DBUS_ERROR_LIMITS_EXCEEDED, + "Connection \"%s\" (%s) is not allowed to create more " + "container servers for uid %lu " + "(max_containers_per_user=%d)", + bus_connection_get_name (connection), + bus_connection_get_loginfo (connection), + instance->uid, limit); + bus_context_log_literal (context, DBUS_SYSTEM_LOG_WARNING, + local_error.message); + dbus_move_error (&local_error, error); + goto fail; + } + if (!_dbus_hash_table_insert_string (containers->instances_by_path, instance->path, instance)) goto oom; + /* This cannot fail (we already allocated the memory) so we can do it after + * we already succeeded in adding it to instances_by_path. The matching + * decrement is done whenever we remove it from instances_by_path. */ + this_user_containers += 1; + _dbus_hash_iter_set_value (&n_containers_by_user_entry, + (void *) this_user_containers); + if (!_dbus_list_append (&creator_data->instances, instance)) goto oom; diff --git a/dbus/dbus-hash.h b/dbus/dbus-hash.h index fadab91f..42704f47 100644 --- a/dbus/dbus-hash.h +++ b/dbus/dbus-hash.h @@ -88,6 +88,7 @@ DBUS_PRIVATE_EXPORT void _dbus_hash_iter_remove_entry (DBusHashIter *iter); DBUS_PRIVATE_EXPORT void* _dbus_hash_iter_get_value (DBusHashIter *iter); +DBUS_PRIVATE_EXPORT void _dbus_hash_iter_set_value (DBusHashIter *iter, void *value); DBUS_PRIVATE_EXPORT @@ -96,6 +97,7 @@ DBUS_PRIVATE_EXPORT const char* _dbus_hash_iter_get_string_key (DBusHashIter *iter); DBUS_PRIVATE_EXPORT uintptr_t _dbus_hash_iter_get_uintptr_key (DBusHashIter *iter); +DBUS_PRIVATE_EXPORT dbus_bool_t _dbus_hash_iter_lookup (DBusHashTable *table, void *key, dbus_bool_t create_if_not_found, From c04e52cd615020507d5a1f2bbf5dc22eec556ed1 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 23 Jun 2017 16:24:22 +0100 Subject: [PATCH 40/44] test/containers: Exercise the resource limits Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- test/Makefile.am | 2 + test/containers.c | 324 +++++++++++++++++- .../limit-containers.conf.in | 21 ++ .../valid-config-files/max-containers.conf.in | 18 + 4 files changed, 364 insertions(+), 1 deletion(-) create mode 100644 test/data/valid-config-files/limit-containers.conf.in create mode 100644 test/data/valid-config-files/max-containers.conf.in diff --git a/test/Makefile.am b/test/Makefile.am index 130f5600..893b7387 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -429,8 +429,10 @@ in_data = \ data/valid-config-files/finite-timeout.conf.in \ data/valid-config-files/forbidding.conf.in \ data/valid-config-files/incoming-limit.conf.in \ + data/valid-config-files/limit-containers.conf.in \ data/valid-config-files/max-completed-connections.conf.in \ data/valid-config-files/max-connections-per-user.conf.in \ + data/valid-config-files/max-containers.conf.in \ data/valid-config-files/max-match-rules-per-connection.conf.in \ data/valid-config-files/max-names-per-connection.conf.in \ data/valid-config-files/max-replies-per-connection.conf.in \ diff --git a/test/containers.c b/test/containers.c index 4cf9edf2..b706bdd1 100644 --- a/test/containers.c +++ b/test/containers.c @@ -1137,6 +1137,255 @@ test_invalid_nesting (Fixture *f, #endif /* !HAVE_CONTAINERS_TEST */ } +/* + * Assert that we can have up to 3 containers, but no more than that, + * either because max-containers.conf imposes max_containers=3 + * or because limit-containers.conf imposes max_containers_per_user=3 + * (and we only have one uid). + */ +static void +test_max_containers (Fixture *f, + gconstpointer context) +{ +#ifdef HAVE_CONTAINERS_TEST + GVariant *parameters; + GVariant *tuple; + /* Length must match max_containers in max-containers.conf, and also + * max_containers_per_user in limit-containers.conf */ + gchar *placeholders[3] = { NULL }; + guint i; + + if (f->skip) + return; + + f->proxy = g_dbus_proxy_new_sync (f->unconfined_conn, + G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES, + NULL, DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, DBUS_INTERFACE_CONTAINERS1, + NULL, &f->error); + g_assert_no_error (f->error); + + parameters = g_variant_new ("(ssa{sv}a{sv})", + "com.example.NotFlatpak", + "sample-app", + NULL, /* no metadata */ + NULL); /* no named arguments */ + /* We will reuse this variant several times, so don't use floating refs */ + g_variant_ref_sink (parameters); + + /* We can go up to the limit without exceeding it */ + for (i = 0; i < G_N_ELEMENTS (placeholders); i++) + { + tuple = g_dbus_proxy_call_sync (f->proxy, "AddServer", + parameters, G_DBUS_CALL_FLAGS_NONE, -1, + NULL, &f->error); + g_assert_no_error (f->error); + g_assert_nonnull (tuple); + g_variant_get (tuple, "(o^ays)", &placeholders[i], NULL, NULL); + g_variant_unref (tuple); + g_test_message ("Placeholder server at %s", placeholders[i]); + } + + /* We cannot exceed the limit */ + tuple = g_dbus_proxy_call_sync (f->proxy, "AddServer", + parameters, G_DBUS_CALL_FLAGS_NONE, -1, + NULL, &f->error); + g_assert_error (f->error, G_DBUS_ERROR, G_DBUS_ERROR_LIMITS_EXCEEDED); + g_clear_error (&f->error); + g_assert_null (tuple); + + /* Stop one of the placeholders */ + tuple = g_dbus_proxy_call_sync (f->proxy, "StopListening", + g_variant_new ("(o)", placeholders[1]), + G_DBUS_CALL_FLAGS_NONE, -1, NULL, + &f->error); + g_assert_no_error (f->error); + g_assert_nonnull (tuple); + g_variant_unref (tuple); + + /* We can have another container server now that we are back below the + * limit */ + tuple = g_dbus_proxy_call_sync (f->proxy, "AddServer", + parameters, G_DBUS_CALL_FLAGS_NONE, -1, + NULL, &f->error); + g_assert_no_error (f->error); + g_assert_nonnull (tuple); + g_variant_unref (tuple); + + g_variant_unref (parameters); + + for (i = 0; i < G_N_ELEMENTS (placeholders); i++) + g_free (placeholders[i]); + +#else /* !HAVE_CONTAINERS_TEST */ + g_test_skip ("Containers or gio-unix-2.0 not supported"); +#endif /* !HAVE_CONTAINERS_TEST */ +} + +#ifdef HAVE_CONTAINERS_TEST +static void +assert_connection_closed (GError *error) +{ + /* "before 2.44 some "connection closed" errors returned + * G_IO_ERROR_BROKEN_PIPE, but others returned G_IO_ERROR_FAILED" + * —GIO documentation */ + if (error->code == G_IO_ERROR_BROKEN_PIPE) + { + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_BROKEN_PIPE); + } + else + { + g_assert_error (error, G_IO_ERROR, G_IO_ERROR_FAILED); + g_test_message ("Old GLib: %s", error->message); + /* This is wrong and bad, but it's the only way to detect this, and + * the older GLib versions that raised FAILED are no longer a moving + * target */ + g_assert_true (strstr (error->message, g_strerror (ECONNRESET)) != NULL); + } +} +#endif + +/* + * Test that if we have multiple app-containers, + * max_connections_per_container applies to each one individually. + */ +static void +test_max_connections_per_container (Fixture *f, + gconstpointer context) +{ +#ifdef HAVE_CONTAINERS_TEST + /* Length is arbitrary */ + gchar *socket_paths[2] = { NULL }; + gchar *dbus_addresses[G_N_ELEMENTS (socket_paths)] = { NULL }; + GSocketAddress *socket_addresses[G_N_ELEMENTS (socket_paths)] = { NULL }; + /* Length must be length of socket_paths * max_connections_per_container in + * limit-containers.conf */ + GSocket *placeholders[G_N_ELEMENTS (socket_paths) * 3] = { NULL }; + GVariant *parameters; + GVariant *tuple; + guint i; + + if (f->skip) + return; + + f->proxy = g_dbus_proxy_new_sync (f->unconfined_conn, + G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES, + NULL, DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, DBUS_INTERFACE_CONTAINERS1, + NULL, &f->error); + g_assert_no_error (f->error); + + parameters = g_variant_new ("(ssa{sv}a{sv})", + "com.example.NotFlatpak", + "sample-app", + NULL, /* no metadata */ + NULL); /* no named arguments */ + /* We will reuse this variant several times, so don't use floating refs */ + g_variant_ref_sink (parameters); + + for (i = 0; i < G_N_ELEMENTS (socket_paths); i++) + { + tuple = g_dbus_proxy_call_sync (f->proxy, "AddServer", + parameters, G_DBUS_CALL_FLAGS_NONE, -1, + NULL, &f->error); + g_assert_no_error (f->error); + g_assert_nonnull (tuple); + g_variant_get (tuple, "(o^ays)", NULL, &socket_paths[i], + &dbus_addresses[i]); + g_variant_unref (tuple); + socket_addresses[i] = g_unix_socket_address_new (socket_paths[i]); + g_test_message ("Server #%u at %s", i, socket_paths[i]); + } + + for (i = 0; i < G_N_ELEMENTS (placeholders); i++) + { + /* We enforce the resource limit for any connection to the socket, + * not just D-Bus connections that have done the handshake */ + placeholders[i] = g_socket_new (G_SOCKET_FAMILY_UNIX, + G_SOCKET_TYPE_STREAM, + G_SOCKET_PROTOCOL_DEFAULT, &f->error); + g_assert_no_error (f->error); + + g_socket_connect (placeholders[i], + socket_addresses[i % G_N_ELEMENTS (socket_paths)], + NULL, &f->error); + g_assert_no_error (f->error); + g_test_message ("Placeholder connection #%u to %s", i, + socket_paths[i % G_N_ELEMENTS (socket_paths)]); + } + + /* An extra connection to either of the sockets fails: they are both at + * capacity now */ + for (i = 0; i < G_N_ELEMENTS (socket_paths); i++) + { + f->confined_conn = g_dbus_connection_new_for_address_sync ( + dbus_addresses[i], + (G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION | + G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT), + NULL, NULL, &f->error); + assert_connection_closed (f->error); + + g_clear_error (&f->error); + g_assert_null (f->confined_conn); + } + + /* Free up one slot (this happens to be connected to socket_paths[0]) */ + g_socket_close (placeholders[2], &f->error); + g_assert_no_error (f->error); + + /* Now we can connect, but only once. Use a retry loop since the dbus-daemon + * won't necessarily notice our socket closing synchronously. */ + while (f->confined_conn == NULL) + { + g_test_message ("Trying to use the slot we just freed up..."); + f->confined_conn = g_dbus_connection_new_for_address_sync ( + dbus_addresses[0], + (G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION | + G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT), + NULL, NULL, &f->error); + + if (f->confined_conn == NULL) + { + assert_connection_closed (f->error); + g_clear_error (&f->error); + g_assert_nonnull (f->confined_conn); + } + else + { + g_assert_no_error (f->error); + } + } + + /* An extra connection to either of the sockets fails: they are both at + * capacity again */ + for (i = 0; i < G_N_ELEMENTS (socket_paths); i++) + { + GDBusConnection *another = g_dbus_connection_new_for_address_sync ( + dbus_addresses[i], + (G_DBUS_CONNECTION_FLAGS_MESSAGE_BUS_CONNECTION | + G_DBUS_CONNECTION_FLAGS_AUTHENTICATION_CLIENT), + NULL, NULL, &f->error); + + assert_connection_closed (f->error); + g_clear_error (&f->error); + g_assert_null (another); + } + + g_variant_unref (parameters); + + for (i = 0; i < G_N_ELEMENTS (socket_paths); i++) + { + g_free (socket_paths[i]); + g_free (dbus_addresses[i]); + g_clear_object (&socket_addresses[i]); + } + +#undef LIMIT +#else /* !HAVE_CONTAINERS_TEST */ + g_test_skip ("Containers or gio-unix-2.0 not supported"); +#endif /* !HAVE_CONTAINERS_TEST */ +} + static void teardown (Fixture *f, gconstpointer context G_GNUC_UNUSED) @@ -1202,6 +1451,59 @@ teardown (Fixture *f, g_clear_error (&f->error); } +/* + * Test what happens when we exceed max_container_metadata_bytes. + * test_metadata() exercises the non-excessive case with the same + * configuration. + */ +static void +test_max_container_metadata_bytes (Fixture *f, + gconstpointer context) +{ +#ifdef HAVE_CONTAINERS_TEST + /* Must be >= max_container_metadata_bytes in limit-containers.conf, so that + * when the serialization overhead, app-container type and app name are + * added, it is too much for the limit */ + guchar waste_of_space[4096] = { 0 }; + GVariant *tuple; + GVariant *parameters; + GVariantDict dict; + + if (f->skip) + return; + + f->proxy = g_dbus_proxy_new_sync (f->unconfined_conn, + G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES, + NULL, DBUS_SERVICE_DBUS, + DBUS_PATH_DBUS, DBUS_INTERFACE_CONTAINERS1, + NULL, &f->error); + g_assert_no_error (f->error); + + g_variant_dict_init (&dict, NULL); + g_variant_dict_insert (&dict, "waste of space", "@ay", + g_variant_new_fixed_array (G_VARIANT_TYPE_BYTE, + waste_of_space, + sizeof (waste_of_space), + 1)); + + /* Floating reference, call_..._sync takes ownership */ + parameters = g_variant_new ("(ss@a{sv}a{sv})", + "com.wasteheadquarters", + "Packt Like Sardines in a Crushd Tin Box", + g_variant_dict_end (&dict), + NULL); /* no named arguments */ + + tuple = g_dbus_proxy_call_sync (f->proxy, "AddServer", parameters, + G_DBUS_CALL_FLAGS_NONE, -1, NULL, &f->error); + g_assert_error (f->error, G_DBUS_ERROR, G_DBUS_ERROR_LIMITS_EXCEEDED); + g_assert_null (tuple); + g_clear_error (&f->error); + +#else /* !HAVE_CONTAINERS_TEST */ + g_test_skip ("Containers or gio-unix-2.0 not supported"); +#endif /* !HAVE_CONTAINERS_TEST */ +} + static const Config stop_server_explicitly = { "valid-config-files/multi-user.conf", @@ -1227,6 +1529,16 @@ static const Config stop_server_with_manager = "valid-config-files/multi-user.conf", STOP_SERVER_WITH_MANAGER }; +static const Config limit_containers = +{ + "valid-config-files/limit-containers.conf", + 0 /* not relevant for this test */ +}; +static const Config max_containers = +{ + "valid-config-files/max-containers.conf", + 0 /* not relevant for this test */ +}; int main (int argc, @@ -1273,7 +1585,7 @@ main (int argc, &stop_server_force, setup, test_stop_server, teardown); g_test_add ("/containers/stop-server/with-manager", Fixture, &stop_server_with_manager, setup, test_stop_server, teardown); - g_test_add ("/containers/metadata", Fixture, NULL, + g_test_add ("/containers/metadata", Fixture, &limit_containers, setup, test_metadata, teardown); g_test_add ("/containers/invalid-metadata-getters", Fixture, NULL, setup, test_invalid_metadata_getters, teardown); @@ -1283,6 +1595,16 @@ main (int argc, setup, test_invalid_type_name, teardown); g_test_add ("/containers/invalid-nesting", Fixture, NULL, setup, test_invalid_nesting, teardown); + g_test_add ("/containers/max-containers", Fixture, &max_containers, + setup, test_max_containers, teardown); + g_test_add ("/containers/max-containers-per-user", Fixture, &limit_containers, + setup, test_max_containers, teardown); + g_test_add ("/containers/max-connections-per-container", Fixture, + &limit_containers, + setup, test_max_connections_per_container, teardown); + g_test_add ("/containers/max-container-metadata-bytes", Fixture, + &limit_containers, + setup, test_max_container_metadata_bytes, teardown); ret = g_test_run (); diff --git a/test/data/valid-config-files/limit-containers.conf.in b/test/data/valid-config-files/limit-containers.conf.in new file mode 100644 index 00000000..3cc3bb05 --- /dev/null +++ b/test/data/valid-config-files/limit-containers.conf.in @@ -0,0 +1,21 @@ + + + + session + @TEST_LISTEN@ + + + + + + + + + + + 5 + 3 + 4096 + 3 + diff --git a/test/data/valid-config-files/max-containers.conf.in b/test/data/valid-config-files/max-containers.conf.in new file mode 100644 index 00000000..8ca09b21 --- /dev/null +++ b/test/data/valid-config-files/max-containers.conf.in @@ -0,0 +1,18 @@ + + + + session + @TEST_LISTEN@ + + + + + + + + + + + 3 + From d972c11ab77dc53f58d9c6672fc5f11e33105ad9 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Mon, 24 Jul 2017 12:37:12 +0100 Subject: [PATCH 41/44] Revert "test/uid-permissions: Assert that AddServer is privileged" I'm about to make that not be true. Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- test/uid-permissions.c | 76 ------------------------------------------ 1 file changed, 76 deletions(-) diff --git a/test/uid-permissions.c b/test/uid-permissions.c index 061333f5..4642ff37 100644 --- a/test/uid-permissions.c +++ b/test/uid-permissions.c @@ -192,73 +192,6 @@ test_monitor (Fixture *f, * (TEST_USER_MESSAGEBUS) or by root, but cannot be called by other * users for now. */ -static void -test_containers (Fixture *f, - gconstpointer context) -{ -#ifdef DBUS_ENABLE_CONTAINERS - const Config *config = context; -#endif - DBusMessage *m; - DBusPendingCall *pc; - - if (f->skip) - return; - - /* We cheat and pass the wrong arguments, because passing an a{sv} with - * the libdbus API is really long-winded. The bus driver code checks - * for privileged or unprivileged access before it checks the arguments - * anyway. */ - m = dbus_message_new_method_call (DBUS_SERVICE_DBUS, - DBUS_PATH_DBUS, DBUS_INTERFACE_CONTAINERS1, "AddServer"); - - if (m == NULL) - g_error ("OOM"); - - if (!dbus_connection_send_with_reply (f->conn, m, &pc, - DBUS_TIMEOUT_USE_DEFAULT) || - pc == NULL) - g_error ("OOM"); - - dbus_clear_message (&m); - - if (dbus_pending_call_get_completed (pc)) - test_pending_call_store_reply (pc, &m); - else if (!dbus_pending_call_set_notify (pc, test_pending_call_store_reply, - &m, NULL)) - g_error ("OOM"); - - while (m == NULL) - test_main_context_iterate (f->ctx, TRUE); - -#ifdef DBUS_ENABLE_CONTAINERS - if (config->expect_success) - { - /* It would have succeeded if we'd passed the right arguments! */ - g_assert_cmpint (dbus_message_get_type (m), ==, DBUS_MESSAGE_TYPE_ERROR); - g_assert_cmpstr (dbus_message_get_error_name (m), ==, - DBUS_ERROR_INVALID_ARGS); - g_assert_cmpstr (dbus_message_get_signature (m), ==, "s"); - } - else - { - /* It fails, yielding an error message with one string argument */ - g_assert_cmpint (dbus_message_get_type (m), ==, DBUS_MESSAGE_TYPE_ERROR); - g_assert_cmpstr (dbus_message_get_error_name (m), ==, - DBUS_ERROR_ACCESS_DENIED); - g_assert_cmpstr (dbus_message_get_signature (m), ==, "s"); - } -#else - g_assert_cmpint (dbus_message_get_type (m), ==, DBUS_MESSAGE_TYPE_ERROR); - g_assert_cmpstr (dbus_message_get_error_name (m), ==, - DBUS_ERROR_UNKNOWN_INTERFACE); - g_assert_cmpstr (dbus_message_get_signature (m), ==, "s"); -#endif - - dbus_clear_pending_call (&pc); - dbus_clear_message (&m); -} - static void teardown (Fixture *f, gconstpointer context G_GNUC_UNUSED) @@ -324,14 +257,5 @@ main (int argc, g_test_add ("/uid-permissions/monitor/other", Fixture, &other_fail_config, setup, test_monitor, teardown); - /* AddServer has the same behaviour */ - g_test_add ("/uid-permissions/containers/root", Fixture, &root_ok_config, - setup, test_containers, teardown); - g_test_add ("/uid-permissions/containers/messagebus", Fixture, - &messagebus_ok_config, - setup, test_containers, teardown); - g_test_add ("/uid-permissions/containers/other", Fixture, &other_fail_config, - setup, test_containers, teardown); - return g_test_run (); } From a28b8c864004ffad13d7453ffd7d39baf254712a Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 23 Jun 2017 16:45:13 +0100 Subject: [PATCH 42/44] bus/driver: Allow unprivileged connections to create app-containers This lets ordinary users create a limited number of app-containers on the system bus. Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bus/driver.c b/bus/driver.c index 3e42fabc..ea6724d3 100644 --- a/bus/driver.c +++ b/bus/driver.c @@ -2571,11 +2571,11 @@ static const MessageHandler introspectable_message_handlers[] = { #ifdef DBUS_ENABLE_CONTAINERS static const MessageHandler containers_message_handlers[] = { { "AddServer", "ssa{sv}a{sv}", "oays", bus_containers_handle_add_server, - METHOD_FLAG_PRIVILEGED }, + METHOD_FLAG_NO_CONTAINERS }, { "StopInstance", "o", "", bus_containers_handle_stop_instance, - METHOD_FLAG_PRIVILEGED }, + METHOD_FLAG_NO_CONTAINERS }, { "StopListening", "o", "", bus_containers_handle_stop_listening, - METHOD_FLAG_PRIVILEGED }, + METHOD_FLAG_NO_CONTAINERS }, { "GetConnectionInstance", "s", "ossa{sv}", bus_containers_handle_get_connection_instance, METHOD_FLAG_NONE }, From 485e81969314487d35823fb6d45515cb1754835e Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Fri, 30 Jun 2017 15:50:56 +0100 Subject: [PATCH 43/44] system.conf: Allow creating containers on the system bus Signed-off-by: Simon McVittie Reviewed-by: Philip Withnall Bug: https://bugs.freedesktop.org/show_bug.cgi?id=101354 --- bus/system.conf.in | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bus/system.conf.in b/bus/system.conf.in index 2ca4ae58..7c79a1ae 100644 --- a/bus/system.conf.in +++ b/bus/system.conf.in @@ -69,6 +69,8 @@ send_interface="org.freedesktop.DBus.Introspectable"/> + Date: Tue, 12 Dec 2017 17:41:01 +0000 Subject: [PATCH 44/44] Update NEWS Signed-off-by: Simon McVittie --- NEWS | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/NEWS b/NEWS index f28527d6..926b9b11 100644 --- a/NEWS +++ b/NEWS @@ -8,6 +8,11 @@ a 1.14.0 stable release at an unspecified point in the future. Enhancements: +• Add experimental support for creating extra servers at runtime, to + be used by app containers like Flatpak or Snap. This API is still + subject to change and is not compiled in by default. + (fd.o #101354, Simon McVittie) + • Improve automated test logging (fd.o #103601, Simon McVittie) Fixes: @@ -32,9 +37,6 @@ Fixes: Internal changes: -• Preparations for future support for restricted sockets used by - app containers like Flatpak (fd.o #101354, Simon McVittie) - • Harden the nonce-tcp: transport against resource leaks and use-after-free (fd.o #103597, Simon McVittie)