|
Description
Simon McVittie
2018-03-21 12:37:37 UTC
A sketch of acceptance criteria (these might change based on implementation experience and review, but they're close):
[ ] An AddServer call can include the Allow parameter
[ ] These *unconditionally allowed* operations are allowed for all container instances:
[ ] Receive a method call from a peer outside, as long as it has no Unix fds
[ ] Receive a unicast signal from a peer outside, as long as it has no Unix fds
[ ] Send a reply or error in response to a method call from outside that expects one
[ ] Receive NameOwnerChanged for another connection in the same instance
[ ] Send Hello()
[ ] Send o.fd.DBus.Peer method calls to dbus-daemon
[ ] Send Introspect() to the dbus-daemon
[ ] Send GetId() to the dbus-daemon
[ ] Send AddMatch() if not eavesdropping
[ ] Send RemoveMatch()
[ ] Send method call or unicast signal to another connection in the same instance
[ ] Request to own an invalid name, a unique name or o.fd.DBus (allowed, but fails with InvalidArgs)
[ ] These *conditionally allowed* operations are allowed for container instances that do not have an Allow parameter, but raise the given error for container instances that do have an Allow parameter:
[ ] Receive a broadcast (no error, message just does not arrive)
[ ] Send a reply or error to a spurious message serial number (AccessDenied)
[ ] Send a second reply or error to a method call from outside (AccessDenied)
[ ] Send a reply or error to a method call from outside that does not expect one (AccessDenied)
[ ] Send method calls to the outside (AccessDenied)
[ ] Send unicast signals to the outside (AccessDenied)
[ ] Receive NameOwnerChanged for connections outside the instance (no error, signal is silently dropped)
[ ] Request to own a well-known name (AccessDenied)
[ ] Release a well-known name (AccessDenied - this avoids using the result as an oracle to see whether the name has another owner)
[ ] List queued name owners of a well-known name (AccessDenied)
[ ] List (activatable) names (no error, but only names inside the container instance + o.fd.DBus are visible)
[ ] GetNameOwner (no error, but only names inside the container instance + o.fd.DBus are visible)
[ ] NameHasOwner (no error, but only names inside the container instance + o.fd.DBus are visible)
[ ] StartServiceByName (AccessDenied)
[ ] Receive a unicast with Unix fds (AccessDenied to sender? InvalidArgs to sender?)
[ ] These *restricted* operations raise the given error for container instances with an Allow parameter, and have unspecified results for container instances without:
[ ] Call BecomeMonitor (AccessDenied)
[ ] Try to eavesdrop (AccessDenied)
[ ] Call UpdateActivationEnvironment (AccessDenied)
[ ] Stats, Verbose method calls (AccessDenied)
[ ] Unit tests
[ ] Container instance without Allow can do all the *conditionally allowed* and *unconditionally allowed* things
[ ] Container instance with empty Allow can do all the *unconditionally allowed* things
[ ] Container instance with non-empty Allow cannot do any of the *conditionally allowed* things, and gets the specified error
To be designed
==============
[ ] Decide what should happen in these cases, and implement it:
[ ] Send a broadcast (no error, but broadcast can only be received within the instance and a synthetic error is sent to monitors? or AccessDenied error?)
Out of scope
============
* Actually useful filtering (rules to allow things)
* A non-empty Allow list is not required to do anything useful (it will either be treated as empty or cause an error, at this stage)
* Absence of an Allow list is not required to allow privileged stuff like BecomeMonitor (Bug #103458)
(In reply to Simon McVittie from comment #0) > As a starting point for T7327 That should say: for Bug #101902. (Sorry, this came from a non-public task tracking system where I was planning out what's needed.) Progress: I have a version of this that passes tests, with a couple of test-cases #if 0 because they depend on "If a contained peer receives a method call or unicast signal from outside the container, then it receives SEE access to the sender's unique name" which isn't implemented yet (Bug #105658). (In reply to Simon McVittie from comment #3) > Progress: I have a version of this that passes tests It's going to need some rebasing before it's reviewable, though, particularly the tests. Currently at: 26 files changed, 3919 insertions(+), 273 deletions(-) which is about 75% tests. Created attachment 139908 [details] [review] 01/39] driver: Remove references to an obsolete constant Created attachment 139909 [details] [review] 02/39] activation: Refuse to load activatable services with invalid names We aren't going to allow activating them anyway, so there's no point in having them in memory. (A secondary justification is that this makes my Containers1 work more straightforward, by making sure I don't have to handle the case where unique names, malformed names or org.freedesktop.DBus can be included in the entries hash table.) Created attachment 139911 [details] [review] 03/39] tests: Add a GAsyncReadyCallback that stores the GAsyncResult It seems I eventually introduce this into every project where I've added GLib-based unit tests. Today it's dbus' turn. Created attachment 139912 [details] [review] 04/39] bus_driver_get_owner_of_name: Clarify role of connection This connection is the one looking at the name, as opposed to the one that owns the name (if any). Created attachment 139914 [details] [review] 05/39] bus_service_list_queued_owners: Don't return a DBusError This makes it clearer that the only possible error is out-of-memory, so its use in ListQueuedOwners() is not leaking information to callers that might not be allowed to know the difference between "doesn't exist" and "exists but you are not allowed to know that". Created attachment 139915 [details] [review] 06/39] bus: Document arguments of bus_activation_activate_service It isn't completely obvious that connection is allowed to be NULL here. Created attachment 139916 [details] [review] 07/39] bus: Make bus_driver_send_name_owner_changed take DBusConnections All callers had access to the DBusConnection that is the old owner, and the DBusConnection that is the new owner. Pass those to the function instead of their names. This gives the function a bit more contextual information, which will be important when we filter NameOwnerChanged messages entering containers according to whether the container is allowed to see the connection. While I'm breaking its API anyway, rename the function to match the signal that it emits, which was renamed in early 2005. Created attachment 139917 [details] [review] 08/39] containers: Factor out connection_get_instance and make it NULL-safe When I introduce per-container message filtering, it'll be useful to be able to get the instance for a connection without worrying about whether that connection is NULL (representing the dbus-daemon itself, or an activatable service that has not yet been activated). Also make it robust against Containers having not been initialized, for completeness. Created attachment 139919 [details] [review] 09/39] bus_registry_list_services: Take the DBusConnection that wants to know This will be necessary when we give connections from containers a different answer that only lists a subset of the names. Created attachment 139920 [details] [review] 10/39] bus_activation_list_services: Take the DBusConnection that wants to know This will be necessary when we give connections from containers a different answer that only lists a subset of the names. Created attachment 139921 [details] [review] 11/39] driver: Reorganise GetNameOwner, ListQueuedOwners for filtering This converts them to a mostly early-return pattern (via a goto because some cleanup is needed). If the desired name is DBUS_SERVICE_DBUS, we can just early-return without doing the other work. Similarly, if the desired name is not syntactically valid, we can early-return without even asking the registry whether someone owns it, because nobody can own an invalid name. Add a specific code path for reporting that the name doesn't exist, so that when we filter, all code paths that should be indistinguishable (can't see because it doesn't exist, or because policy says we can't see it) will be using the same code. Created attachment 139923 [details] [review] 12/39] containers: Add stub functions to check against filtering --- This could maybe be broken down further, but life's too short. Created attachment 139925 [details] [review] 13/39] bus: If containers block a requested reply, substitute an error reply If some connection in a container has legitimately had one of its methods called by another connection (either unconfined or in a different container), but the reply is unsuitably for delivery to the caller (perhaps because it contains fds), we don't want the caller to be left waiting. Add a way for the security policy to request that we send an error reply to the original method call, as well as sending an error reply to the undesirable reply. Created attachment 139926 [details] [review] 14/39] containers test: Move teardown below all test-cases Created attachment 139927 [details] [review] 15/39] containers test: Factor out disconnecting the unconfined manager As this test's coverage expands, this function will have to do more (clear up name watches, filters, etc.) so it'll be helpful to keep it all in one place. Created attachment 139928 [details] [review] 16/39] containers test: Factor out fixture_disconnect_observer Created attachment 139929 [details] [review] 17/39] containers test: Record the unconfined manager connection's unique name This is a bit more convenient than fetching it as-needed. Created attachment 139930 [details] [review] 18/39] containers test: Maintain an array of confined connections Created attachment 139931 [details] [review] 19/39] containers test: Monitor observer name-ownership We can use this after disconnecting the observer, as a way to work out when a confined connection that might not be allowed to see the observer would have seen NameOwnerChanged for the observer if it was ever going to: if the unconfined connection has seen the observer disappear, and the confined connection has seen a message that the unconfined connection only sent after it saw the observer disappear, then causal ordering means the confined connection would also have seen NameOwnerChanged (if it was going to). --- Reading that paragraph again, I'm having trouble understanding it myself, but I promise this is a useful function :-) It will become useful after 30/39. Created attachment 139932 [details] [review] 20/39] test: Add a function to catch up with pending messages This is very loosely based on test infrastructure that used to exist in Telepathy. It arranges for all prior messages on both the connections to have been processed, by making use of total ordering: if caller sends method call M1 to callee, and callee sends back a reply M2, then all messages received by callee prior to M1, received by caller prior to M2, sent by callee prior to M2 or sent by caller prior to M1 are also guaranteed to have been processed. Created attachment 139933 [details] [review] 21/39] containers test: NameOwnerChanged is visible within a container At the moment this is trivially true because we don't filter messages at all, but this test-case adds infrastructure for checking less trivial situations under different rulesets. Created attachment 139934 [details] [review] 22/39] containers: Allow an empty array of Allow rules This is the least possible we can do: the existence of an Allow array sets a flag that says "this connection applies policies", but we only allow one policy, namely "reject almost everything". Created attachment 139935 [details] [review] 23/39] containers test: Exercise invalid Allow rules We don't actually accept any Allow rules yet, so the single Allow rule that we try to add is trivially invalid; but when we start adding possible Allow rules, it will become more useful to verify that obviously-invalid rules remain invalid. Created attachment 139936 [details] [review] 24/39] containers test: Actually add Allow rules --- ... or at least we would if the array was ever non-empty, so this is partially dead code right now. It will become directly useful as soon as I implement at least one rule that we can add... Created attachment 139937 [details] [review] 25/39] containers test: Optionally make second confined connection own a name This also adds convenience infrastructure for owning names. Created attachment 139938 [details] [review] 26/39] containers: Containers with an Allow policy cannot see well-known names There is one exception: they can see the dbus-daemon itself. Created attachment 139939 [details] [review] 27/39] containers: Containers with an Allow policy cannot see most unique names We unconditionally allow connections in the same container to see each other, and as a trivial case, allow a connection to see itself. Created attachment 139940 [details] [review] 28/39] containers test: Exercise name-listing methods These are currently controlled by the hard-coded baseline policy (containers with no Allow list can see all names, containers with an empty Allow list cannot see most names) but will eventually have finer granularity via non-empty Allow lists. Created attachment 139941 [details] [review] 29/39] containers test: Exercise visibility of a confined well-known name We can currently only test this for the case where we have no policy. When we add Allow rules that allow owning a name, we can test that the name is also correctly visible. Created attachment 139942 [details] [review] 30/39] containers test: Exercise visibility of observer's names This tests whether the contained connection can see the unique and well-known names of a connection that never communicated with it. Created attachment 139943 [details] [review] 31/39] containers: Don't allow containers to send unsolicited replies Containers are new functionality, so we don't need to preserve historical warts, like the fact that the behaviour for unsolicited replies is (inexplicably) sysadmin-configurable. Created attachment 139944 [details] [review] 32/39] containers test: Check that unsolicited replies can't be sent To do this, we need to send messages that contain a special message body, since there is no straightforward way to tell which message is which otherwise. Strictly speaking, this only needs the filter on the unconfined connection (that's where we're sending the unsolicited replies), but adding the same filter to the unconfined observer connection and to each confined connection will make life easier for subsequent tests. Created attachment 139945 [details] [review] 33/39] containers: Containers with an Allow policy cannot own names In future we will offer a way to allow them to own names, but this is not yet implemented. Created attachment 139946 [details] [review] 34/39] containers: Containers with an Allow policy cannot activate services Created attachment 139947 [details] [review] 35/39] containers: Containers with Allow policy can't inspect most connections By "inspect" I mean methods like GetConnectionCredentials. Again, we make an exception for connections that share a container. Created attachment 139948 [details] [review] 36/39] containers: Limit the messages sent by connections with an Allow policy The baseline rules for connections in containers with a policy are as follows: * Can send legitimate replies that don't contain Unix fds * Can't send Unix fds (as an easy way to mitigate the complexity and low arbitrary limits of Unix fd passing, which carries a risk of exploitable bugs) * Can send messages to the dbus-daemon that don't contain Unix fds * Can send messages to connections in the same container (including, as a trivial case, themselves) * Can't send anything not explicitly allowed Created attachment 139949 [details] [review] 37/39] containers: Limit messages received by containers with Allow policy The baseline for containers with an Allow policy is as follows: * Containers may not receive Unix fds, as an easy way to mitigate the complexity and risk of bugs that result from fd-passing * Unicast messages (that were allowed to be sent) are always allowed * NameOwnerChanged broadcast messages from the dbus-daemon are checked elsewhere and so are always allowed here * Connections within one container may broadcast among themselves * Other broadcasts are not allowed Future enhancements will add Allow rules that allow receiving Unix fds and allow receiving broadcasts. Created attachment 139950 [details] [review] 38/39] containers test: Add a test for sending and receiving method calls In addition to the messages themselves, this exercises name ownership. Created attachment 139951 [details] [review] 39/39] containers test: Exercise allowing/denying signals Comment on attachment 139940 [details] [review] 28/39] containers test: Exercise name-listing methods Review of attachment 139940 [details] [review]: ----------------------------------------------------------------- Travis-CI tells me we will need a compatibility definition of g_strv_contains() for ye olde GLib: ../../test/containers.c:546:3: error: nested extern declaration of ‘g_strv_contains’ [-Werror=nested-externs] Created attachment 140008 [details] [review] [27½] test: Add an implementation of g_strv_contains() for GLib < 2.44 We still use Ubuntu 14.04 on Travis-CI, and that doesn't have g_strv_contains(). Created attachment 140009 [details] [review] [38 v2] containers test: Add a test for sending and receiving method calls In addition to the messages themselves, this exercises name ownership. --- Don't assume that GetMachineId() will succeed, because minimal autobuilder environments that have neither dbus nor systemd installed will not always have a machine ID that we can use. Where we are testing both GetMachineId() and Ping(), use METHOD_ALLOWS_ACCESS instead of METHOD_SUCCEEDS for GetMachineId() - this will allow either success, or any failure other than ACCESS_DENIED. In particular FILE_NOT_FOUND is OK, and that's the error raised by GetMachineId() if /etc/machine-id and ${localstatedir}/dbus/machine-id don't exist. Where we are only testing GetMachineId(), use Ping() instead, because it's simpler: we can assume it will always succeed. Comment on attachment 139908 [details] [review] 01/39] driver: Remove references to an obsolete constant Review of attachment 139908 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139909 [details] [review] 02/39] activation: Refuse to load activatable services with invalid names Review of attachment 139909 [details] [review]: ----------------------------------------------------------------- Generally r+, but with a couple of suggestions which you might want to deal with or ignore. ::: bus/activation.c @@ +325,5 @@ > + dbus_set_error (error, DBUS_ERROR_INVALID_ARGS, > + "Bus name \"%s\" cannot be activatable because it " > + "is reserved for the message bus", name); > + goto out; > + } Would it make sense to factor out these three checks into a new dbus_validate_activatable_bus_name() or something? @@ +2790,5 @@ > return ret; > } > > +void > +bus_activation_check_services (BusActivation *self) Nitpick: A style I’ve been using recently in my code is to put a summary comment above each unit test function, briefly describing what the test does and what it’s trying to test. I don’t know if that’s something you want to adopt too. I find it’s helped when I go back to tests later. ::: test/data/invalid-service-files/org.freedesktop.DBus.TestSuiteNotValidName.service @@ +1,2 @@ > +[D-BUS Service] > +Name=?! What about a test for an empty `Name=`? Comment on attachment 139911 [details] [review] 03/39] tests: Add a GAsyncReadyCallback that stores the GAsyncResult Review of attachment 139911 [details] [review]: ----------------------------------------------------------------- Trivially r+ Comment on attachment 139912 [details] [review] 04/39] bus_driver_get_owner_of_name: Clarify role of connection Review of attachment 139912 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139914 [details] [review] 05/39] bus_service_list_queued_owners: Don't return a DBusError Review of attachment 139914 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139915 [details] [review] 06/39] bus: Document arguments of bus_activation_activate_service Review of attachment 139915 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139916 [details] [review] 07/39] bus: Make bus_driver_send_name_owner_changed take DBusConnections Review of attachment 139916 [details] [review]: ----------------------------------------------------------------- r- ::: bus/driver.c @@ +252,5 @@ > > _DBUS_ASSERT_ERROR_IS_CLEAR (error); > > + if (old_owner != NULL) > + old_owner_name = bus_connection_get_name (old_owner); Looks like it’s possible for bus_connection_get_name() to return NULL if the service is inactive (see bus_connection_is_active()), in which case you’ll have `old_owner_name == NULL` rather than `old_owner_name == ""`. Comment on attachment 139917 [details] [review] 08/39] containers: Factor out connection_get_instance and make it NULL-safe Review of attachment 139917 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139919 [details] [review] 09/39] bus_registry_list_services: Take the DBusConnection that wants to know Review of attachment 139919 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139920 [details] [review] 10/39] bus_activation_list_services: Take the DBusConnection that wants to know Review of attachment 139920 [details] [review]: ----------------------------------------------------------------- r+ (In reply to Philip Withnall from comment #48) > Comment on attachment 139909 [details] [review] > 02/39] activation: Refuse to load activatable services with invalid names > ::: bus/activation.c > @@ +325,5 @@ > > + dbus_set_error (error, DBUS_ERROR_INVALID_ARGS, > > + "Bus name \"%s\" cannot be activatable because it " > > + "is reserved for the message bus", name); > > + goto out; > > + } > > Would it make sense to factor out these three checks into a new > dbus_validate_activatable_bus_name() or something? IMO not unless/until we have another user for it. Are you thinking public API or internal? In principle I suppose we could have a variant of dbus_validate_bus_name() called dbus_validate_well_known_name(), because that's what this is really - org.freedesktop.DBus is special, and is neither well-known nor unique (it behaves more like a unique name but has the syntax of a well-known name). Prior art: GDBus has g_dbus_is_unique_name() and g_dbus_is_name(), but expects you to know that every bus name that is valid, not unique and not org.freedesktop.DBus is well-known. dbus-python has an internal function dbus_py_validate_bus_name(const char *, dbus_bool_t may_be_unique, dbus_bool_t may_be_not_unique) but doesn't special-case org.freedesktop.DBus there. telepathy-glib is perhaps the most pedantically correct, and has tp_dbus_check_valid_bus_name(const char *, TpDBusNameType, GError **) where TpDBusNameType is a bitfield { UNIQUE, WELL_KNOWN, DBUS_DAEMON } plus convenience aliases NOT_DBUS_DAEMON and ANY. If we want this as public API then I think that should be a separate feature request. > Nitpick: A style I’ve been using recently in my code is to put a summary > comment above each unit test function Good idea, added in v2 > What about a test for an empty `Name=`? Good idea, added in v2 (In reply to Philip Withnall from comment #53) > Comment on attachment 139916 [details] [review] > 07/39] bus: Make bus_driver_send_name_owner_changed take DBusConnections > > + if (old_owner != NULL) > > + old_owner_name = bus_connection_get_name (old_owner); > > Looks like it’s possible for bus_connection_get_name() to return NULL if the > service is inactive (see bus_connection_is_active()), in which case you’ll > have `old_owner_name == NULL` rather than `old_owner_name == ""`. Inactive connections can't own names. I've added assertions for v2. When a connection becomes active, it is given its unique name in its data structure before we announce that it has received it and emit NameOwnerChanged. The only method that inactive connections are allowed to call is o.fd.DBus.Hello (there is a hard-coded check), so they have no opportunity to receive a well-known name, which can only be done by calling o.fd.DBus.RequestName. When a connection disconnects, it never actually becomes inactive, it just goes away. Monitors lose ownership of their unique name, but it stays in their data structure and the monitor stays active forever. (In reply to Simon McVittie from comment #57) > (In reply to Philip Withnall from comment #48) > > Would it make sense to factor out these three checks into a new > > dbus_validate_activatable_bus_name() or something? > > IMO not unless/until we have another user for it. Are you thinking public > API or internal? I was thinking internal API. Comment on attachment 139921 [details] [review] 11/39] driver: Reorganise GetNameOwner, ListQueuedOwners for filtering Review of attachment 139921 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139923 [details] [review] 12/39] containers: Add stub functions to check against filtering Review of attachment 139923 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139925 [details] [review] 13/39] bus: If containers block a requested reply, substitute an error reply Review of attachment 139925 [details] [review]: ----------------------------------------------------------------- r+ apart from the commit message: Nitpick: s/unsuitably/unsuitable/ in the commit message. Comment on attachment 139926 [details] [review] 14/39] containers test: Move teardown below all test-cases Review of attachment 139926 [details] [review]: ----------------------------------------------------------------- Trivially r+ Comment on attachment 139927 [details] [review] 15/39] containers test: Factor out disconnecting the unconfined manager Review of attachment 139927 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139928 [details] [review] 16/39] containers test: Factor out fixture_disconnect_observer Review of attachment 139928 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139929 [details] [review] 17/39] containers test: Record the unconfined manager connection's unique name Review of attachment 139929 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139930 [details] [review] 18/39] containers test: Maintain an array of confined connections Review of attachment 139930 [details] [review]: ----------------------------------------------------------------- r-, some comments. ::: test/containers.c @@ +234,5 @@ > + else > + g_assert_no_error (error); > + } > + > + g_clear_object (&f->confined_conns[i]); Would it make sense to clear the corresponding f->confined_unique_names[i] here too? @@ +546,5 @@ > g_assert_null (tuple); > > g_test_message ("Inspecting connection container info"); > + f->confined_unique_names[0] = g_strdup ( > + g_dbus_connection_get_unique_name (f->confined_conns[0])); Does f->confined_unique_names[0] need to be set here again, after it’s set in fixture_connect_confined()? @@ +1625,4 @@ > teardown (Fixture *f, > gconstpointer context G_GNUC_UNUSED) > { > + guint i; Nitpick: I’d use gsize for array indexing, as a general rule. In this case, it’s completely irrelevant though. Comment on attachment 139931 [details] [review] 19/39] containers test: Monitor observer name-ownership Review of attachment 139931 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139932 [details] [review] 20/39] test: Add a function to catch up with pending messages Review of attachment 139932 [details] [review]: ----------------------------------------------------------------- r+ (In reply to Philip Withnall from comment #67) > Comment on attachment 139930 [details] [review] > 18/39] containers test: Maintain an array of confined connections > > + g_clear_object (&f->confined_conns[i]); > > Would it make sense to clear the corresponding f->confined_unique_names[i] > here too? No: we might still want to remember which unique name corresponded to which confined connection, so that we can make assertions about the NameOwnerChanged signals that result from the confined connection going away (and in fact I think a later patch does just that). > > + f->confined_unique_names[0] = g_strdup ( > > + g_dbus_connection_get_unique_name (f->confined_conns[0])); > > Does f->confined_unique_names[0] need to be set here again, after it’s set > in fixture_connect_confined()? No, and it's a memory leak. Good catch! > > + guint i; > > Nitpick: I’d use gsize for array indexing, as a general rule. In this case, > it’s completely irrelevant though. Fine, it's gsize now (and so is the parameter to fixture_connect_confined() and fixture_disconnect_confined()). (In reply to Philip Withnall from comment #59) > (In reply to Simon McVittie from comment #57) > > IMO not unless/until we have another user for it. Are you thinking public > > API or internal? > > I was thinking internal API. Then my answer is no, we're not going to need it, unless/until a second user for that function proves me wrong. > 01/39] driver: Remove references to an obsolete constant
> 03/39] tests: Add a GAsyncReadyCallback that stores the GAsyncResult
> 04/39] bus_driver_get_owner_of_name: Clarify role of connection
> 05/39] bus_service_list_queued_owners: Don't return a DBusError
> 06/39] bus: Document arguments of bus_activation_activate_service
> 08/39] containers: Factor out connection_get_instance and make it NULL-safe
> 14/39] containers test: Move teardown below all test-cases
> 15/39] containers test: Factor out disconnecting the unconfined manager
> 16/39] containers test: Factor out fixture_disconnect_observer
> 17/39] containers test: Record the unconfined manager connection's unique
> name
I rebased the branch to sort these reviewed/uncontroversial patches to the beginning, and pushed them to the `try` branch on my github. I'll push them to master, assuming CI succeeds.
Created attachment 140269 [details] [review] [02+] fixup! activation: Refuse to load activatable services with invalid names * Add TestSuiteNoName test case * Add more comments --- To be squashed in the obvious way into [02]. Created attachment 140270 [details] [review] [07+] fixup! bus: Make bus_driver_send_name_owner_changed take DBusConnections Justify why the names can't be NULL, and add assertions Created attachment 140271 [details] [review] [18+] fixup! containers test: Maintain an array of confined connections Remove duplicate set of confined_unique_names[0]. Use gsize to index the array. Created attachment 140272 [details] [review] [38+] fixup! containers test: Add a test for sending and receiving method calls Don't test EnableVerbose() or GetStats() if that functionality was not compiled in. For dbus-daemon (only), the check for whether we know the method comes before the filtering. r+ on the fixups 02+, 07+, 18+ I can’t review the fixup for 38 yet, since I haven’t yet reviewed 38 itself. Comment on attachment 139933 [details] [review] 21/39] containers test: NameOwnerChanged is visible within a container Review of attachment 139933 [details] [review]: ----------------------------------------------------------------- Solid r+ ::: test/containers.c @@ +2054,4 @@ > gchar *runtime_dbus_dir; > gchar *runtime_containers_dir; > gchar *runtime_services_dir; > + guint i; Nitpick: gsize here (but I really don’t expect a fixup for this patch, since this is the only issue I’ve spotted) Comment on attachment 139934 [details] [review] 22/39] containers: Allow an empty array of Allow rules Review of attachment 139934 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139935 [details] [review] 23/39] containers test: Exercise invalid Allow rules Review of attachment 139935 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139936 [details] [review] 24/39] containers test: Actually add Allow rules Review of attachment 139936 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139937 [details] [review] 25/39] containers test: Optionally make second confined connection own a name Review of attachment 139937 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139938 [details] [review] 26/39] containers: Containers with an Allow policy cannot see well-known names Review of attachment 139938 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139938 [details] [review] 26/39] containers: Containers with an Allow policy cannot see well-known names Review of attachment 139938 [details] [review]: ----------------------------------------------------------------- Actually, having looked at the following patch: Does it make sense for containers to be able to see their own well-known name (if they own one)? Comment on attachment 139939 [details] [review] 27/39] containers: Containers with an Allow policy cannot see most unique names Review of attachment 139939 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 140008 [details] [review] [27½] test: Add an implementation of g_strv_contains() for GLib < 2.44 Review of attachment 140008 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139940 [details] [review] 28/39] containers test: Exercise name-listing methods Review of attachment 139940 [details] [review]: ----------------------------------------------------------------- r+, although I have a question about one of the comments. ::: test/containers.c @@ +411,5 @@ > + * Helper for Allow tests: Assert that GetNameOwner(), NameHasOwner() > + * and the given result of ListNames() agree. > + * > + * @names is really a (const gchar * const *) but it's passed via a > + * gconstpointer to avoid a lot of very ugly casts. Sad, but reasonable, times. @@ +720,5 @@ > + "com.example.SendDeniedByNonexistentAppArmorLabel", > + "com.example.SystemdActivatable1", > + "com.example.SystemdActivatable2", > + "com.example.SystemdActivatable3", > + /* For some reason this counts as activatable too */ ‘For some reason’ does not fill me with confidence. Presumably this is just some historical quirk which we’re stuck with? Comment on attachment 139941 [details] [review] 29/39] containers test: Exercise visibility of a confined well-known name Review of attachment 139941 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139942 [details] [review] 30/39] containers test: Exercise visibility of observer's names Review of attachment 139942 [details] [review]: ----------------------------------------------------------------- r+, nice (In reply to Philip Withnall from comment #87) > Comment on attachment 139940 [details] [review] > 28/39] containers test: Exercise name-listing methods > > + /* For some reason this counts as activatable too */ > > ‘For some reason’ does not fill me with confidence. Presumably this is just > some historical quirk which we’re stuck with? It's a fact about ListActivatableNames(), not a fact about filtering. I'm not going to change what ListActivatableNames() does for unfiltered connections on this branch. ListActivatableNames() listing org.freedesktop.DBus appears to be deliberate (someone had to write a couple of lines of extra code to make it happen) but I don't know why. I'll open a separate bug for clarification. (In reply to Simon McVittie from comment #90) > It's a fact about ListActivatableNames(), not a fact about filtering. I'm > not going to change what ListActivatableNames() does for unfiltered > connections on this branch. > > ListActivatableNames() listing org.freedesktop.DBus appears to be deliberate > (someone had to write a couple of lines of extra code to make it happen) but > I don't know why. I'll open a separate bug for clarification. FTR: https://bugs.freedesktop.org/show_bug.cgi?id=107024 Comment on attachment 139943 [details] [review] 31/39] containers: Don't allow containers to send unsolicited replies Review of attachment 139943 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139944 [details] [review] 32/39] containers test: Check that unsolicited replies can't be sent Review of attachment 139944 [details] [review]: ----------------------------------------------------------------- r-, some leaks ::: test/containers.c @@ +2797,5 @@ > + f->confined_unique_names[0], "/", DBUS_INTERFACE_PEER, "Ping"); > + g_dbus_message_set_flags (message_without_reply, > + G_DBUS_MESSAGE_FLAGS_NO_REPLY_EXPECTED); > + message_with_reply = g_dbus_message_new_method_call ( > + f->confined_unique_names[0], "/", DBUS_INTERFACE_PEER, "Ping"); message_with_reply and message_without_reply are leaked. @@ +2822,5 @@ > + while (result == NULL) > + g_main_context_iteration (NULL, TRUE); > + > + reply_message = g_dbus_connection_send_message_with_reply_finish ( > + f->unconfined_conn, result, &f->error); result and reply_message are leaked. @@ +2832,5 @@ > + * won't arrive at the unconfined connection and trip the check > + * in allow_tests_message_filter(). */ > + for (use_error_reply = 0; use_error_reply < 2; use_error_reply++) > + { > + guint32 reply_serials[] = Nitpick: Could be const. @@ +2848,5 @@ > + { > + GDBusMessage *unsolicited_reply; > + guint32 reply_serial = reply_serials[i]; > + > + unsolicited_reply = g_dbus_message_new (); Is unsolicited_reply leaked at the end of each iteration? Comment on attachment 139945 [details] [review] 33/39] containers: Containers with an Allow policy cannot own names Review of attachment 139945 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139946 [details] [review] 34/39] containers: Containers with an Allow policy cannot activate services Review of attachment 139946 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139946 [details] [review] 34/39] containers: Containers with an Allow policy cannot activate services Review of attachment 139946 [details] [review]: ----------------------------------------------------------------- Hmm, although if o.fd.DBus is activatable (wrt ListActivatableNames), perhaps activating that should always be allowed? I’m not sure what that would mean, but it may be more consistent with the fact that o.fd.DBus is listed in ListActivatableNames than denying activating it. Comment on attachment 139947 [details] [review] 35/39] containers: Containers with Allow policy can't inspect most connections Review of attachment 139947 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139948 [details] [review] 36/39] containers: Limit the messages sent by connections with an Allow policy Review of attachment 139948 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139949 [details] [review] 37/39] containers: Limit messages received by containers with Allow policy Review of attachment 139949 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 140009 [details] [review] [38 v2] containers test: Add a test for sending and receiving method calls Review of attachment 140009 [details] [review]: ----------------------------------------------------------------- r+ with one nitpick. I haven’t reviewed the arrays of test cases in detail, but I assume they are reasonable. Have you been looking at the code coverage of the new code while writing all these tests? ::: test/containers.c @@ +644,5 @@ > + reply = g_dbus_message_new_method_error (message, > + DBUS_ERROR_UNKNOWN_METHOD, "No such method"); > + } > + else if (g_strcmp0 (g_dbus_message_get_interface (message), > + DBUS_INTERFACE_PROPERTIES) == 0 && Nitpick: Looks like an indentation problem here, but it might just be Bugzilla. Comment on attachment 140272 [details] [review] [38+] fixup! containers test: Add a test for sending and receiving method calls Review of attachment 140272 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 139951 [details] [review] 39/39] containers test: Exercise allowing/denying signals Review of attachment 139951 [details] [review]: ----------------------------------------------------------------- r+. Just wondering (not related to anything I saw/didn’t see in this patch): have you run all the new tests through valgrind? (In reply to Philip Withnall from comment #84) > 26/39] containers: Containers with an Allow policy cannot see well-known > names > > Actually, having looked at the following patch: Does it make sense for > containers to be able to see their own well-known name (if they own one)? There is no answer to this question, because they can't own well-known names. The ability to own well-known names is a later feature (which I now realise I haven't yet opened). When it's implemented, my intention is for a rule "may OWN com.example.Foo" to imply at least "may SEE com.example.Foo" (even if someone else currently owns com.example.Foo), but until that's possible, there's no need to have a special case for it. (In reply to Philip Withnall from comment #93) > 32/39] containers test: Check that unsolicited replies can't be sent > > Is unsolicited_reply leaked at the end of each iteration? Yes. Fixing that and all the other leaks you spotted. (In reply to Philip Withnall from comment #100) > [38 v2] containers test: Add a test for sending and receiving method calls > > Have you been looking at the code coverage > of the new code while writing all these tests? No, only at the specification I wrote. You're right that I should run this branch through a coverage check though. > Nitpick: Looks like an indentation problem here Correct, the "else if" is off by 1 space. (In reply to Philip Withnall from comment #102) > have you run all the new tests through valgrind? Not so far (I can't even remember whether we have infrastructure to do that). Created attachment 140452 [details] [review] [21+] fixup! containers test: NameOwnerChanged is visible within a container Use gsize for memory iteration Created attachment 140453 [details] [review] [32+] fixup! containers test: Check that unsolicited replies can't be sent * Use const for array of serial numbers * Fix several leaks Created attachment 140454 [details] [review] [38+] fixup! containers test: Add a test for sending and receiving method calls Fix off-by-one indentation (In reply to Simon McVittie from comment #105) > You're right that I should run this > branch through a coverage check though. Notable missing coverage in bus/containers.c: - bus_containers_check_can_activate() doesn't reject a message - bus_containers_check_can_receive() doesn't seem to reject a broadcast other than NameOwnerChanged, which needs investigation, because I thought I'd tested that; maybe the match rule isn't there? - bus_containers_check_can_receive() doesn't seem to accept a broadcast from inside the same container, likewise - bus_containers_check_can_see_well_known_name() doesn't hit the special case for DBUS_SERVICE_DBUS being visible and not specifically related to this feature: - bus_container_instance_lost_connection() doesn't seem to be hit (it should be getting called when the confined connection closes) (In reply to Simon McVittie from comment #110) > and not specifically related to this feature: > > - bus_container_instance_lost_connection() doesn't seem to be hit > (it should be getting called when the confined connection closes) That one at least is easy to explain: when the confined connection closes (in non-OOM code paths), we execute code that is almost equivalent to bus_container_instance_lost_connection(), although not quite (it explicitly frees the data slot on the connection that marks it as being a member of a container). Comment on attachment 140452 [details] [review] [21+] fixup! containers test: NameOwnerChanged is visible within a container Review of attachment 140452 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 140453 [details] [review] [32+] fixup! containers test: Check that unsolicited replies can't be sent Review of attachment 140453 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 140454 [details] [review] [38+] fixup! containers test: Add a test for sending and receiving method calls Review of attachment 140454 [details] [review]: ----------------------------------------------------------------- r+, most trivially (In reply to Simon McVittie from comment #110) > (In reply to Simon McVittie from comment #105) > > You're right that I should run this > > branch through a coverage check though. > > Notable missing coverage in bus/containers.c: > > - bus_containers_check_can_activate() doesn't reject a message > > - bus_containers_check_can_receive() doesn't seem to reject a broadcast > other than NameOwnerChanged, which needs investigation, because I thought > I'd tested that; maybe the match rule isn't there? > > - bus_containers_check_can_receive() doesn't seem to accept a broadcast > from inside the same container, likewise > > - bus_containers_check_can_see_well_known_name() doesn't hit the special > case for DBUS_SERVICE_DBUS being visible OK. I’d ideally like to see tests for these before this all gets merged (in case the tests find bugs), but that’s your call. I’d be almost as happy for these new tests to be a follow-up bug instead. (In reply to Simon McVittie from comment #111) > (In reply to Simon McVittie from comment #110) > > and not specifically related to this feature: > > > > - bus_container_instance_lost_connection() doesn't seem to be hit > > (it should be getting called when the confined connection closes) > > That one at least is easy to explain: when the confined connection closes > (in non-OOM code paths), we execute code that is almost equivalent to > bus_container_instance_lost_connection(), although not quite (it explicitly > frees the data slot on the connection that marks it as being a member of a > container). Would it make sense to factor out the common code? I haven’t checked. I can't see anything r- that I haven't already fixed. Is the remaining negative review for the fact that the coverage doesn't match what it ought to be, or have I missed a problem report? (In reply to Philip Withnall from comment #115) > I’d ideally like to see tests for these before this all gets merged (in > case the tests find bugs), but that’s your call. Step 1 is to get coverage stats for the parts of Containers that already exist on master, I think. > (In reply to Simon McVittie from comment #111) > > That one at least is easy to explain: when the confined connection closes > > (in non-OOM code paths), we execute code that is almost equivalent to > > bus_container_instance_lost_connection(), although not quite (it explicitly > > frees the data slot on the connection that marks it as being a member of a > > container). > > Would it make sense to factor out the common code? I haven’t checked. Probably, but I'd have to think carefully about what the common code ought to do, and what the worst-case scenario would be if a reference to a connection was leaked (as defensive programming, if we somehow process a message from a connection that should have been cleaned up already, we'll want to make sure it's still considered to be part of its container and doesn't get to escalate privileges to look like an uncontained connection). That code is not new in this bug anyway. (In reply to Simon McVittie from comment #116) > I can't see anything r- that I haven't already fixed. Is the remaining > negative review for the fact that the coverage doesn't match what it ought > to be, or have I missed a problem report? Er, I think it’s a mistake on my part. It’s review+ as far as I’m concerned; it’s up to you about what to do with the coverage/missing tests. (In reply to Simon McVittie from comment #106) > (In reply to Philip Withnall from comment #102) > > have you run all the new tests through valgrind? > > Not so far (I can't even remember whether we have infrastructure to do that). We didn't. Bug #107194 is a start towards this. (In reply to Simon McVittie from comment #116) > > (In reply to Simon McVittie from comment #111) > > > That one at least is easy to explain: when the confined connection closes > > > (in non-OOM code paths), we execute code that is almost equivalent to > > > bus_container_instance_lost_connection(), although not quite (it explicitly > > > frees the data slot on the connection that marks it as being a member of a > > > container). > > > > Would it make sense to factor out the common code? I haven’t checked. > > Probably, but I'd have to think carefully about what the common code ought > to do, and what the worst-case scenario would be if a reference to a > connection was leaked (as defensive programming, if we somehow process a > message from a connection that should have been cleaned up already, we'll > want to make sure it's still considered to be part of its container and > doesn't get to escalate privileges to look like an uncontained connection). See Bug #107739 for this. Created attachment 141363 [details] [review] [21+] fixup! containers test: NameOwnerChanged is visible within a container --- Avoid a g_queue_foreach() idiom that gcc 8 doesn't like, similar to Bug #107349 Created attachment 141364 [details] [review] [28+] fixup! containers test: Exercise name-listing methods --- Fix misuse of G_N_ELEMENTS on a GStrv (thanks, gcc 8; I take it all back, your new warnings *are* useful) (In reply to Simon McVittie from comment #110) > - bus_containers_check_can_activate() doesn't reject a message Fix in progress > - bus_containers_check_can_receive() doesn't seem to reject a broadcast > other than NameOwnerChanged, which needs investigation, because I thought > I'd tested that; maybe the match rule isn't there? TODO > - bus_containers_check_can_receive() doesn't seem to accept a broadcast > from inside the same container, likewise TODO > - bus_containers_check_can_see_well_known_name() doesn't hit the special > case for DBUS_SERVICE_DBUS being visible That special case is never hit, because all callers already special-cased DBUS_SERVICE_DBUS; I'll remove the special-case here. There is also missing coverage for a contained connection inspecting an uncontained connection ("inspecting" means GetConnectionCredentials etc.) but I can't test that until I implement SEE access, because at the moment the method call is rejected early for trying to learn whether a disallowed well-known or unique name exists, before we get as far as seeing whether we're allowed to inspect it. Comment on attachment 141363 [details] [review] [21+] fixup! containers test: NameOwnerChanged is visible within a container Review of attachment 141363 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 141364 [details] [review] [28+] fixup! containers test: Exercise name-listing methods Review of attachment 141364 [details] [review]: ----------------------------------------------------------------- r+ Created attachment 141378 [details] [review] [26+] fixup! containers: Containers with an Allow policy cannot see well-known names --- bus_containers_check_can_see_well_known_name: Don't handle DBUS_SERVICE_DBUS Callers of this function have a fast path for DBUS_SERVICE_DBUS anyway. This kills off some untested code. Created attachment 141379 [details] [review] [38+] fixup! containers test: Add a test for sending and receiving method calls Expand coverage for activation Created attachment 141380 [details] [review] [38+] fixup! containers test: Add a test for sending and receiving method calls Expand test coverage for inspecting other connections These all raise NameHasNoOwner without checking whether we are allowed to inspect the other connection, because we are not even allowed to see the other connection. After implementing fd.o#105658, in an equivalent test where we are allowed to see the unconfined connection but not inspect it, they should raise AccessDenied. Created attachment 141382 [details] [review] [39 v2] containers test: Exercise allowing/denying signals --- This is a rewrite rather than a fixup, because it turns out the signals test was completely broken: SIGNAL_SENT was numerically equal to SIGNAL_INVALID, the array terminator. So we didn't actually do any iterations, which is why we had no test coverage for broadcasts. /o\ I've taken the opportunity to redo it to be more obviously correct: instead of inferring in code who should receive which signals from whether the signal is a broadcast, I've made it explicit via SIGNAL_RECEIVED_OUTSIDE and/or SIGNAL_RECEIVED_INSIDE in the test-case struct. My apologies for the re-review, but I think it's clearer this way. Test logs for the "most privileged" case: # Unconfined connection: ":1.0" # Unconfined observer connection: ":1.1" # Calling AddServer... # Connecting to unix:path=/tmp/dbus-test-containers.VLTFOZ/dbus-1/containers/dbus-wb3lyMjBR5,guid=fa14aed9122e9c5477e4cf0b5b87ff88... # Confined connection 0: ":1.3" # Connecting to unix:path=/tmp/dbus-test-containers.VLTFOZ/dbus-1/containers/dbus-wb3lyMjBR5,guid=fa14aed9122e9c5477e4cf0b5b87ff88... # Confined connection 1: ":1.4" owns "com.example.Confined" # test_allow_signals omit-allow #0 # confined connection sending signal # ... unicast to :confined[1] (:1.4) # ... path / # ... com.example.Foo.UnicastSignal # Synchronizing caller 0x55a1ae474860 with callee 0x55a1ae474a60... # Unconfined connection saw unconfined observer connection ":1.1" appear, owned by ":1.1" # Unconfined connection saw unconfined observer connection "com.example.Observer" disappear # Confined connection saw NameOwnerChanged: ":1.4" owner "" -> ":1.4" # Confined connection saw NameOwnerChanged: "com.example.Confined" owner "" -> ":1.4" # Confined connection saw NameOwnerChanged: "com.example.Unconfined" owner "" -> ":1.0" # Confined connection saw NameOwnerChanged: "com.example.Observer" owner "" -> ":1.1" # Unconfined connection saw unconfined observer connection "com.example.Observer" appear, owned by ":1.1" # Connection 0x55a1ae474a60 received com.example.Foo.UnicastSignal from :1.3:/ # Synchronized caller 0x55a1ae474860 with callee 0x55a1ae474a60 # -> 1 signal(s) received by confined connection 1 <0x55a1ae474a60> # test_allow_signals omit-allow #1 # unconfined connection sending signal # ... unicast to :confined (:1.3) # ... path / # ... com.example.Foo.UnicastSignal # Synchronizing caller 0x55a1ae4a8210 with callee 0x55a1ae474e60... # Connection 0x55a1ae474860 received com.example.Foo.UnicastSignal from :1.0:/ # Synchronized caller 0x55a1ae4a8210 with callee 0x55a1ae474e60 # Synchronizing caller 0x55a1ae4a8210 with callee 0x55a1ae474860... # Synchronized caller 0x55a1ae4a8210 with callee 0x55a1ae474860 # -> 0 signal(s) received by observer <0x55a1ae474e60> # -> 1 signal(s) received by confined connection 0 <0x55a1ae474860> # test_allow_signals omit-allow #2 # confined connection sending signal # ... unicast to :unconfined (:1.0) # ... path / # ... com.example.Foo.UnicastSignal # ... with attached file descriptor # Synchronizing caller 0x55a1ae4a8210 with callee 0x55a1ae474860... # Connection 0x55a1ae4a8210 received com.example.Foo.UnicastSignal from :1.3:/ # Synchronized caller 0x55a1ae4a8210 with callee 0x55a1ae474860 # -> 1 signal(s) received by unconfined connection <0x55a1ae4a8210> # test_allow_signals omit-allow #3 # unconfined connection sending signal # ... unicast to :confined (:1.3) # ... path / # ... com.example.Foo.UnicastSignal # ... with attached file descriptor # Synchronizing caller 0x55a1ae4a8210 with callee 0x55a1ae474e60... # Connection 0x55a1ae474860 received com.example.Foo.UnicastSignal from :1.0:/ # Synchronized caller 0x55a1ae4a8210 with callee 0x55a1ae474e60 # Synchronizing caller 0x55a1ae4a8210 with callee 0x55a1ae474860... # Synchronized caller 0x55a1ae4a8210 with callee 0x55a1ae474860 # -> 0 signal(s) received by observer <0x55a1ae474e60> # -> 1 signal(s) received by confined connection 0 <0x55a1ae474860> # test_allow_signals omit-allow #4 # unconfined connection sending signal # ... broadcast # ... path / # ... com.example.Foo.BroadcastSignal # Synchronizing caller 0x55a1ae4a8210 with callee 0x55a1ae474e60... # Connection 0x55a1ae474860 received com.example.Foo.BroadcastSignal from :1.0:/ # Connection 0x55a1ae474e60 received com.example.Foo.BroadcastSignal from :1.0:/ # Synchronized caller 0x55a1ae4a8210 with callee 0x55a1ae474e60 # Synchronizing caller 0x55a1ae4a8210 with callee 0x55a1ae474860... # Synchronized caller 0x55a1ae4a8210 with callee 0x55a1ae474860 # -> 1 signal(s) received by observer <0x55a1ae474e60> # -> 1 signal(s) received by confined connection 0 <0x55a1ae474860> # test_allow_signals omit-allow #5 # confined connection sending signal # ... unicast to :unconfined (:1.0) # ... path / # ... com.example.Foo.UnicastSignal # Synchronizing caller 0x55a1ae4a8210 with callee 0x55a1ae474860... # Connection 0x55a1ae4a8210 received com.example.Foo.UnicastSignal from :1.3:/ # Synchronized caller 0x55a1ae4a8210 with callee 0x55a1ae474860 # -> 1 signal(s) received by unconfined connection <0x55a1ae4a8210> # test_allow_signals omit-allow #6 # confined connection sending signal # ... broadcast # ... path / # ... com.example.Foo.BroadcastSignal # Synchronizing caller 0x55a1ae474e60 with callee 0x55a1ae474860... # Connection 0x55a1ae474e60 received com.example.Foo.BroadcastSignal from :1.3:/ # Connection 0x55a1ae474a60 received com.example.Foo.BroadcastSignal from :1.3:/ # Synchronized caller 0x55a1ae474e60 with callee 0x55a1ae474860 # Synchronizing caller 0x55a1ae474860 with callee 0x55a1ae474a60... # Synchronized caller 0x55a1ae474860 with callee 0x55a1ae474a60 # -> 1 signal(s) received by observer <0x55a1ae474e60> # -> 1 signal(s) received by confined connection 1 <0x55a1ae474a60> ok 25 /containers/allow/omit-allow/signals PASS: test-containers 25 /containers/allow/omit-allow/signals and the "least privileged" case: # Unconfined connection: ":1.0" # Unconfined observer connection: ":1.1" # Calling AddServer... # Connecting to unix:path=/tmp/dbus-test-containers.VLTFOZ/dbus-1/containers/dbus-eyRRoneF0B,guid=790cf3a46ed83e5b8caf41905b87ff89... # Confined connection 0: ":1.3" # Connecting to unix:path=/tmp/dbus-test-containers.VLTFOZ/dbus-1/containers/dbus-eyRRoneF0B,guid=790cf3a46ed83e5b8caf41905b87ff89... # Confined connection 1: ":1.4" # test_allow_signals empty-allow #0 # confined connection sending signal # ... unicast to :confined[1] (:1.4) # ... path / # ... com.example.Foo.UnicastSignal # Synchronizing caller 0x55a1ae4a8610 with callee 0x55a1ae4a8510... # Unconfined connection saw unconfined observer connection ":1.1" appear, owned by ":1.1" # Unconfined connection saw unconfined observer connection "com.example.Observer" disappear # Confined connection saw NameOwnerChanged: ":1.4" owner "" -> ":1.4" # Unconfined connection saw unconfined observer connection "com.example.Observer" appear, owned by ":1.1" # Connection 0x55a1ae4a8510 received com.example.Foo.UnicastSignal from :1.3:/ # Synchronized caller 0x55a1ae4a8610 with callee 0x55a1ae4a8510 # -> 1 signal(s) received by confined connection 1 <0x55a1ae4a8510> # test_allow_signals empty-allow #1 # unconfined connection sending signal # ... unicast to :confined (:1.3) # ... path / # ... com.example.Foo.UnicastSignal # Synchronizing caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8b10... # Connection 0x55a1ae4a8610 received com.example.Foo.UnicastSignal from :1.0:/ # Synchronized caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8b10 # Synchronizing caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8610... # Synchronized caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8610 # -> 0 signal(s) received by observer <0x55a1ae4a8b10> # -> 1 signal(s) received by confined connection 0 <0x55a1ae4a8610> # test_allow_signals empty-allow #2 # confined connection sending signal # ... unicast to :unconfined (:1.0) # ... path / # ... com.example.Foo.UnicastSignal # ... with attached file descriptor # Synchronizing caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8610... # Synchronized caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8610 # -> 0 signal(s) received by unconfined connection <0x55a1ae4a8e10> # test_allow_signals empty-allow #3 # unconfined connection sending signal # ... unicast to :confined (:1.3) # ... path / # ... com.example.Foo.UnicastSignal # ... with attached file descriptor # Synchronizing caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8b10... # Synchronized caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8b10 # Synchronizing caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8610... # Synchronized caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8610 # -> 0 signal(s) received by observer <0x55a1ae4a8b10> # -> 0 signal(s) received by confined connection 0 <0x55a1ae4a8610> # test_allow_signals empty-allow #4 # unconfined connection sending signal # ... broadcast # ... path / # ... com.example.Foo.BroadcastSignal # Synchronizing caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8b10... # Connection 0x55a1ae4a8b10 received com.example.Foo.BroadcastSignal from :1.0:/ # Synchronized caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8b10 # Synchronizing caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8610... # Synchronized caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8610 # -> 1 signal(s) received by observer <0x55a1ae4a8b10> # -> 0 signal(s) received by confined connection 0 <0x55a1ae4a8610> # test_allow_signals empty-allow #5 # confined connection sending signal # ... unicast to :unconfined (:1.0) # ... path / # ... com.example.Foo.UnicastSignal # Synchronizing caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8610... # Synchronized caller 0x55a1ae4a8e10 with callee 0x55a1ae4a8610 # -> 0 signal(s) received by unconfined connection <0x55a1ae4a8e10> # test_allow_signals empty-allow #6 # confined connection sending signal # ... broadcast # ... path / # ... com.example.Foo.BroadcastSignal # Synchronizing caller 0x55a1ae4a8b10 with callee 0x55a1ae4a8610... # Connection 0x55a1ae4a8510 received com.example.Foo.BroadcastSignal from :1.3:/ # Synchronized caller 0x55a1ae4a8b10 with callee 0x55a1ae4a8610 # Synchronizing caller 0x55a1ae4a8610 with callee 0x55a1ae4a8510... # Synchronized caller 0x55a1ae4a8610 with callee 0x55a1ae4a8510 # -> 0 signal(s) received by observer <0x55a1ae4a8b10> # -> 1 signal(s) received by confined connection 1 <0x55a1ae4a8510> ok 31 /containers/allow/empty-allow/signals PASS: test-containers 31 /containers/allow/empty-allow/signals Comment on attachment 141378 [details] [review] [26+] fixup! containers: Containers with an Allow policy cannot see well-known names Review of attachment 141378 [details] [review]: ----------------------------------------------------------------- > Callers of this function have a fast path for DBUS_SERVICE_DBUS anyway. Really? bus_activation_list_services() doesn’t seem to. Neither does send_one_message() or bus_driver_get_owner_of_name() or bus_registry_list_services(). I’m erring more on the side of either keeping the check in this function, or explicitly asserting that `strcmp (name, DBUS_SERVICE_DBUS) != 0` and requiring the caller to handle DBUS_SERVICE_DBUS explicitly itself. I’m not so happy about the middle ground where DBUS_SERVICE_DBUS could get forgotten about on the caller and callee sides. Comment on attachment 141379 [details] [review] [38+] fixup! containers test: Add a test for sending and receiving method calls Review of attachment 141379 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 141380 [details] [review] [38+] fixup! containers test: Add a test for sending and receiving method calls Review of attachment 141380 [details] [review]: ----------------------------------------------------------------- r+ Comment on attachment 141382 [details] [review] [39 v2] containers test: Exercise allowing/denying signals Review of attachment 141382 [details] [review]: ----------------------------------------------------------------- r+ with one nitpick ::: test/containers.c @@ +4098,5 @@ > + else if (g_strcmp0 (signal->bus_name, > + REPLACE_WITH_CONFINED_1_UNIQUE_NAME) == 0) > + { > + /* It's unicast to the other confined connection */ > + g_assert ((signal->result & SIGNAL_RECEIVED_OUTSIDE) == 0); Nitpick: I’d use g_assert_cmpuint() for this, partially because it gives a more descriptive failure message; and partially because g_assert() can be compiled out when compiling with G_DISABLE_ASSERT, which wouldn’t be so good for the unit tests. g_assert_*() cannot. -- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/201. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.