From e23bc0e7cac8ba79f4e14ab98ecd68c79cc87aab Mon Sep 17 00:00:00 2001 From: David Herrmann Date: Thu, 16 Jul 2015 15:14:43 +0200 Subject: [PATCH] bus-proxy: never pass on unmatched broadcasts The lovely libvirtd goes into crazy mode if it receives broadcasts that it didn't subscribe to. With bus-proxyd, this might happen in 2 cases: 1) The kernel passes us an unmatched signal due to a false-positive bloom-match. 2) We generate NameOwnerChanged/NameAcquired/NameLost locally even though the peer didn't subscribe to it. dbus-daemon is reliable in what signals it passes on. So make sure we follow that style. Never ever send a signal to a local peer if it doesn't match an installed filter of that peer. --- src/bus-proxyd/driver.c | 5 ++-- src/bus-proxyd/driver.h | 3 +- src/bus-proxyd/proxy.c | 55 ++++++++++++++++++++++++++++++++----- src/bus-proxyd/proxy.h | 3 ++ src/bus-proxyd/synthesize.c | 19 +++++++++++-- src/bus-proxyd/synthesize.h | 3 +- 6 files changed, 75 insertions(+), 13 deletions(-) diff --git a/src/bus-proxyd/driver.c b/src/bus-proxyd/driver.c index 4ac955da41b..1cb5ea50086 100644 --- a/src/bus-proxyd/driver.c +++ b/src/bus-proxyd/driver.c @@ -33,6 +33,7 @@ #include "strv.h" #include "set.h" #include "driver.h" +#include "proxy.h" #include "synthesize.h" static int get_creds_by_name(sd_bus *bus, const char *name, uint64_t mask, sd_bus_creds **_creds, sd_bus_error *error) { @@ -70,7 +71,7 @@ static int get_creds_by_message(sd_bus *bus, sd_bus_message *m, uint64_t mask, s return get_creds_by_name(bus, name, mask, _creds, error); } -int bus_proxy_process_driver(sd_bus *a, sd_bus *b, sd_bus_message *m, SharedPolicy *sp, const struct ucred *ucred, Set *owned_names) { +int bus_proxy_process_driver(Proxy *p, sd_bus *a, sd_bus *b, sd_bus_message *m, SharedPolicy *sp, const struct ucred *ucred, Set *owned_names) { int r; assert(a); @@ -189,7 +190,7 @@ int bus_proxy_process_driver(sd_bus *a, sd_bus *b, sd_bus_message *m, SharedPoli if (r < 0) return synthetic_reply_method_errno(m, r, NULL); - r = sd_bus_add_match(a, NULL, match, NULL, NULL); + r = sd_bus_add_match(a, NULL, match, proxy_match, p); if (r < 0) return synthetic_reply_method_errno(m, r, NULL); diff --git a/src/bus-proxyd/driver.h b/src/bus-proxyd/driver.h index b8cedf5ce5c..da3834f8b05 100644 --- a/src/bus-proxyd/driver.h +++ b/src/bus-proxyd/driver.h @@ -23,5 +23,6 @@ #include "sd-bus.h" #include "bus-xml-policy.h" +#include "proxy.h" -int bus_proxy_process_driver(sd_bus *a, sd_bus *b, sd_bus_message *m, SharedPolicy *sp, const struct ucred *ucred, Set *owned_names); +int bus_proxy_process_driver(Proxy *p, sd_bus *a, sd_bus *b, sd_bus_message *m, SharedPolicy *sp, const struct ucred *ucred, Set *owned_names); diff --git a/src/bus-proxyd/proxy.c b/src/bus-proxyd/proxy.c index 189ee969c76..89c68134fc2 100644 --- a/src/bus-proxyd/proxy.c +++ b/src/bus-proxyd/proxy.c @@ -144,9 +144,17 @@ static int proxy_create_local(Proxy *p, int in_fd, int out_fd, bool negotiate_fd return 0; } +static int proxy_match_synthetic(sd_bus_message *m, void *userdata, sd_bus_error *error) { + Proxy *p = userdata; + + p->synthetic_matched = true; + return 0; /* make sure to continue processing it in further handlers */ +} + /* - * dbus-1 clients receive NameOwnerChanged and directed signals without - * subscribing to them; install the matches to receive them on kdbus. + * We always need NameOwnerChanged so we can synthesize NameLost and + * NameAcquired. Furthermore, dbus-1 always passes unicast-signals through, so + * subscribe unconditionally. */ static int proxy_prepare_matches(Proxy *p) { _cleanup_free_ char *match = NULL; @@ -172,7 +180,7 @@ static int proxy_prepare_matches(Proxy *p) { if (!match) return log_oom(); - r = sd_bus_add_match(p->destination_bus, NULL, match, NULL, NULL); + r = sd_bus_add_match(p->destination_bus, NULL, match, proxy_match_synthetic, p); if (r < 0) return log_error_errno(r, "Failed to add match for NameLost: %m"); @@ -189,7 +197,7 @@ static int proxy_prepare_matches(Proxy *p) { if (!match) return log_oom(); - r = sd_bus_add_match(p->destination_bus, NULL, match, NULL, NULL); + r = sd_bus_add_match(p->destination_bus, NULL, match, proxy_match_synthetic, p); if (r < 0) return log_error_errno(r, "Failed to add match for NameAcquired: %m"); @@ -202,7 +210,7 @@ static int proxy_prepare_matches(Proxy *p) { if (!match) return log_oom(); - r = sd_bus_add_match(p->destination_bus, NULL, match, NULL, NULL); + r = sd_bus_add_match(p->destination_bus, NULL, match, proxy_match_synthetic, p); if (r < 0) log_error_errno(r, "Failed to add match for directed signals: %m"); /* FIXME: temporarily ignore error to support older kdbus versions */ @@ -679,11 +687,28 @@ static int patch_sender(sd_bus *a, sd_bus_message *m) { static int proxy_process_destination_to_local(Proxy *p) { _cleanup_bus_message_unref_ sd_bus_message *m = NULL; + bool matched, matched_synthetic; int r; assert(p); + /* + * Usually, we would just take any message that the bus passes to us + * and forward it to the local connection. However, there are actually + * applications that fail if they receive broadcasts that they didn't + * subscribe to. Therefore, we actually emulate a real broadcast + * matching here, and discard any broadcasts that weren't matched. Our + * match-handlers remembers whether a message was matched by any rule, + * by marking it in @p->message_matched. + */ + r = sd_bus_process(p->destination_bus, &m); + + matched = p->message_matched; + matched_synthetic = p->synthetic_matched; + p->message_matched = false; + p->synthetic_matched = false; + if (r == -ECONNRESET || r == -ENOTCONN) /* Treat 'connection reset by peer' as clean exit condition */ return r; if (r < 0) { @@ -699,12 +724,21 @@ static int proxy_process_destination_to_local(Proxy *p) { if (sd_bus_message_is_signal(m, "org.freedesktop.DBus.Local", "Disconnected")) return -ECONNRESET; - r = synthesize_name_acquired(p->destination_bus, p->local_bus, m); + r = synthesize_name_acquired(p, p->destination_bus, p->local_bus, m); if (r == -ECONNRESET || r == -ENOTCONN) return r; if (r < 0) return log_error_errno(r, "Failed to synthesize message: %m"); + /* discard broadcasts that were not matched by any MATCH rule */ + if (!matched && !sd_bus_message_get_destination(m)) { + if (!matched_synthetic) + log_warning("Dropped unmatched broadcast: uid=" UID_FMT " gid=" GID_FMT" message=%s path=%s interface=%s member=%s", + p->local_creds.uid, p->local_creds.gid, bus_message_type_to_string(m->header->type), + strna(m->path), strna(m->interface), strna(m->member)); + return 1; + } + patch_sender(p->destination_bus, m); if (p->policy) { @@ -788,7 +822,7 @@ static int proxy_process_local_to_destination(Proxy *p) { if (r > 0) return 1; - r = bus_proxy_process_driver(p->destination_bus, p->local_bus, m, p->policy, &p->local_creds, p->owned_names); + r = bus_proxy_process_driver(p, p->destination_bus, p->local_bus, m, p->policy, &p->local_creds, p->owned_names); if (r == -ECONNRESET || r == -ENOTCONN) return r; if (r < 0) @@ -834,6 +868,13 @@ static int proxy_process_local_to_destination(Proxy *p) { return 1; } +int proxy_match(sd_bus_message *m, void *userdata, sd_bus_error *error) { + Proxy *p = userdata; + + p->message_matched = true; + return 0; /* make sure to continue processing it in further handlers */ +} + int proxy_run(Proxy *p) { int r; diff --git a/src/bus-proxyd/proxy.h b/src/bus-proxyd/proxy.h index ff278a24652..ccb951c1092 100644 --- a/src/bus-proxyd/proxy.h +++ b/src/bus-proxyd/proxy.h @@ -39,6 +39,8 @@ struct Proxy { bool got_hello : 1; bool queue_overflow : 1; + bool message_matched : 1; + bool synthetic_matched : 1; }; int proxy_new(Proxy **out, int in_fd, int out_fd, const char *dest); @@ -46,6 +48,7 @@ Proxy *proxy_free(Proxy *p); int proxy_set_policy(Proxy *p, SharedPolicy *policy, char **configuration); int proxy_hello_policy(Proxy *p, uid_t original_uid); +int proxy_match(sd_bus_message *m, void *userdata, sd_bus_error *error); int proxy_run(Proxy *p); DEFINE_TRIVIAL_CLEANUP_FUNC(Proxy*, proxy_free); diff --git a/src/bus-proxyd/synthesize.c b/src/bus-proxyd/synthesize.c index 67bcc7a242e..3ecedfd5753 100644 --- a/src/bus-proxyd/synthesize.c +++ b/src/bus-proxyd/synthesize.c @@ -28,6 +28,7 @@ #include "bus-internal.h" #include "bus-message.h" #include "bus-util.h" +#include "bus-match.h" #include "synthesize.h" int synthetic_driver_send(sd_bus *b, sd_bus_message *m) { @@ -152,11 +153,12 @@ int synthetic_reply_method_return_strv(sd_bus_message *call, char **l) { return synthetic_driver_send(call->bus, m); } -int synthesize_name_acquired(sd_bus *a, sd_bus *b, sd_bus_message *m) { +int synthesize_name_acquired(Proxy *p, sd_bus *a, sd_bus *b, sd_bus_message *m) { _cleanup_bus_message_unref_ sd_bus_message *n = NULL; const char *name, *old_owner, *new_owner; int r; + assert(p); assert(a); assert(b); assert(m); @@ -216,5 +218,18 @@ int synthesize_name_acquired(sd_bus *a, sd_bus *b, sd_bus_message *m) { if (r < 0) return r; - return sd_bus_send(b, n, NULL); + /* + * Make sure to only forward NameLost/NameAcquired messages if they + * match an installed MATCH rule of the local client. We really must + * not send messages the client doesn't expect. + */ + + r = bus_match_run(b, &b->match_callbacks, n); + if (r >= 0 && p->message_matched) + r = sd_bus_send(b, n, NULL); + + p->message_matched = false; + p->synthetic_matched = false; + + return r; } diff --git a/src/bus-proxyd/synthesize.h b/src/bus-proxyd/synthesize.h index e850350bc5f..b596daddf21 100644 --- a/src/bus-proxyd/synthesize.h +++ b/src/bus-proxyd/synthesize.h @@ -22,6 +22,7 @@ ***/ #include "sd-bus.h" +#include "proxy.h" int synthetic_driver_send(sd_bus *b, sd_bus_message *m); @@ -33,4 +34,4 @@ int synthetic_reply_method_errorf(sd_bus_message *call, const char *name, const int synthetic_reply_method_errno(sd_bus_message *call, int error, const sd_bus_error *p); int synthetic_reply_method_errnof(sd_bus_message *call, int error, const char *format, ...) _sd_printf_(3, 4); -int synthesize_name_acquired(sd_bus *a, sd_bus *b, sd_bus_message *m); +int synthesize_name_acquired(Proxy *p, sd_bus *a, sd_bus *b, sd_bus_message *m);