From 2562df496ae8e2c8277dd8a86b6632adbdf358d1 Mon Sep 17 00:00:00 2001 From: Adrian Szyndela Date: Fri, 17 Apr 2020 10:25:25 +0200 Subject: [PATCH 1/9] bus/policy: make local functions static These two functions are used only locally in bus/policy.c: - bus_client_policy_optimize() - bus_client_policy_append_rule() Make them static, remove declarations from the header file, add forward declarations in the C file. Change-Id: Ideba1fea470bc0d38c04f428b23270fe6176ac95 --- bus/policy.c | 8 ++++++-- bus/policy.h | 3 --- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/bus/policy.c b/bus/policy.c index 74cb41bd..7a581581 100644 --- a/bus/policy.c +++ b/bus/policy.c @@ -31,6 +31,10 @@ #include #include +static void bus_client_policy_optimize (BusClientPolicy *policy); +static dbus_bool_t bus_client_policy_append_rule (BusClientPolicy *policy, + BusPolicyRule *rule); + BusPolicyRule* bus_policy_rule_new (BusPolicyRuleType type, dbus_bool_t allow) @@ -778,7 +782,7 @@ remove_rules_by_type_up_to (BusClientPolicy *policy, } } -void +static void bus_client_policy_optimize (BusClientPolicy *policy) { DBusList *link; @@ -858,7 +862,7 @@ bus_client_policy_optimize (BusClientPolicy *policy) _dbus_list_get_length (&policy->rules)); } -dbus_bool_t +static dbus_bool_t bus_client_policy_append_rule (BusClientPolicy *policy, BusPolicyRule *rule) { diff --git a/bus/policy.h b/bus/policy.h index df59d090..82212ebe 100644 --- a/bus/policy.h +++ b/bus/policy.h @@ -170,9 +170,6 @@ dbus_bool_t bus_client_policy_check_can_receive (BusClientPolicy *policy, dbus_int32_t *toggles); dbus_bool_t bus_client_policy_check_can_own (BusClientPolicy *policy, const DBusString *service_name); -dbus_bool_t bus_client_policy_append_rule (BusClientPolicy *policy, - BusPolicyRule *rule); -void bus_client_policy_optimize (BusClientPolicy *policy); #ifdef DBUS_ENABLE_EMBEDDED_TESTS dbus_bool_t bus_policy_check_can_own (BusPolicy *policy, From 9cba634305faf604d2ab5cd429009a982551e8b3 Mon Sep 17 00:00:00 2001 From: Adrian Szyndela Date: Fri, 17 Apr 2020 10:37:06 +0200 Subject: [PATCH 2/9] bus/policy: remove optimization This removes optimization, because in subsequent commits we're going to apply: * direct checking of policy * hash tables instead of lists. Change-Id: I6de32c4a29bac1d185f76eb88b22198c9ea22413 --- bus/policy.c | 106 --------------------------------------------------- 1 file changed, 106 deletions(-) diff --git a/bus/policy.c b/bus/policy.c index 7a581581..ec21d985 100644 --- a/bus/policy.c +++ b/bus/policy.c @@ -31,7 +31,6 @@ #include #include -static void bus_client_policy_optimize (BusClientPolicy *policy); static dbus_bool_t bus_client_policy_append_rule (BusClientPolicy *policy, BusPolicyRule *rule); @@ -372,8 +371,6 @@ bus_policy_create_client_policy (BusPolicy *policy, client)) goto nomem; - bus_client_policy_optimize (client); - return client; nomem: @@ -759,109 +756,6 @@ bus_client_policy_unref (BusClientPolicy *policy) } } -static void -remove_rules_by_type_up_to (BusClientPolicy *policy, - BusPolicyRuleType type, - DBusList *up_to) -{ - DBusList *link; - - link = _dbus_list_get_first_link (&policy->rules); - while (link != up_to) - { - BusPolicyRule *rule = link->data; - DBusList *next = _dbus_list_get_next_link (&policy->rules, link); - - if (rule->type == type) - { - _dbus_list_remove_link (&policy->rules, link); - bus_policy_rule_unref (rule); - } - - link = next; - } -} - -static void -bus_client_policy_optimize (BusClientPolicy *policy) -{ - DBusList *link; - - /* The idea here is that if we have: - * - * - * - * - * (for example) the deny will always override the allow. So we - * delete the allow. Ditto for deny followed by allow, etc. This is - * a dumb thing to put in a config file, but the feature - * of files allows for an "inheritance and override" pattern where - * it could make sense. If an included file wants to "start over" - * with a blanket deny, no point keeping the rules from the parent - * file. - */ - - _dbus_verbose ("Optimizing policy with %d rules\n", - _dbus_list_get_length (&policy->rules)); - - link = _dbus_list_get_first_link (&policy->rules); - while (link != NULL) - { - BusPolicyRule *rule; - DBusList *next; - dbus_bool_t remove_preceding; - - next = _dbus_list_get_next_link (&policy->rules, link); - rule = link->data; - - remove_preceding = FALSE; - - _dbus_assert (rule != NULL); - - switch (rule->type) - { - case BUS_POLICY_RULE_SEND: - remove_preceding = - rule->d.send.message_type == DBUS_MESSAGE_TYPE_INVALID && - rule->d.send.path == NULL && - rule->d.send.interface == NULL && - rule->d.send.member == NULL && - rule->d.send.error == NULL && - rule->d.send.destination == NULL; - break; - case BUS_POLICY_RULE_RECEIVE: - remove_preceding = - rule->d.receive.message_type == DBUS_MESSAGE_TYPE_INVALID && - rule->d.receive.path == NULL && - rule->d.receive.interface == NULL && - rule->d.receive.member == NULL && - rule->d.receive.error == NULL && - rule->d.receive.origin == NULL; - break; - case BUS_POLICY_RULE_OWN: - remove_preceding = - rule->d.own.service_name == NULL; - break; - - /* The other rule types don't appear in this list */ - case BUS_POLICY_RULE_USER: - case BUS_POLICY_RULE_GROUP: - default: - _dbus_assert_not_reached ("invalid rule"); - break; - } - - if (remove_preceding) - remove_rules_by_type_up_to (policy, rule->type, - link); - - link = next; - } - - _dbus_verbose ("After optimization, policy has %d rules\n", - _dbus_list_get_length (&policy->rules)); -} - static dbus_bool_t bus_client_policy_append_rule (BusClientPolicy *policy, BusPolicyRule *rule) From af92d0d2aab4ac4d875c0e0a2f7bf61d3ae9bc56 Mon Sep 17 00:00:00 2001 From: Adrian Szyndela Date: Wed, 20 May 2020 14:03:10 +0200 Subject: [PATCH 3/9] bus/policy: extract check_* functions No functional changes, just moving code around. This extracts check_send_rule, check_receive_rule, and check_own_rule from their own respective bus_client_policy_can_check_* functions. Change-Id: Ice4b2b96054b33a376bc3f48df29447747e7980e --- bus/policy.c | 882 +++++++++++++++++++++++++++------------------------ 1 file changed, 459 insertions(+), 423 deletions(-) diff --git a/bus/policy.c b/bus/policy.c index ec21d985..5e2c0c90 100644 --- a/bus/policy.c +++ b/bus/policy.c @@ -771,6 +771,440 @@ bus_client_policy_append_rule (BusClientPolicy *policy, return TRUE; } +static dbus_bool_t +check_send_rule (const BusPolicyRule *rule, + BusRegistry *registry, + dbus_bool_t requested_reply, + DBusConnection *receiver, + DBusMessage *message) +{ + /* Rule is skipped if it specifies a different + * message name from the message, or a different + * destination from the message + */ + if (rule->type != BUS_POLICY_RULE_SEND) + { + _dbus_verbose (" (policy) skipping non-send rule\n"); + return FALSE; + } + + if (rule->d.send.message_type != DBUS_MESSAGE_TYPE_INVALID) + { + if (dbus_message_get_type (message) != rule->d.send.message_type) + { + _dbus_verbose (" (policy) skipping rule for different message type\n"); + return FALSE; + } + } + + /* If it's a reply, the requested_reply flag kicks in */ + if (dbus_message_get_reply_serial (message) != 0) + { + /* for allow, requested_reply=true means the rule applies + * only when reply was requested. requested_reply=false means + * always allow. + */ + if (!requested_reply && rule->allow && rule->d.send.requested_reply && !rule->d.send.eavesdrop) + { + _dbus_verbose (" (policy) skipping allow rule since it only applies to requested replies and does not allow eavesdropping\n"); + return FALSE; + } + + /* for deny, requested_reply=false means the rule applies only + * when the reply was not requested. requested_reply=true means the + * rule always applies. + */ + if (requested_reply && !rule->allow && !rule->d.send.requested_reply) + { + _dbus_verbose (" (policy) skipping deny rule since it only applies to unrequested replies\n"); + return FALSE; + } + } + + if (rule->d.send.path != NULL) + { + if (dbus_message_get_path (message) != NULL && + strcmp (dbus_message_get_path (message), + rule->d.send.path) != 0) + { + _dbus_verbose (" (policy) skipping rule for different path\n"); + return FALSE; + } + } + + if (rule->d.send.interface != NULL) + { + /* The interface is optional in messages. For allow rules, if the message + * has no interface we want to skip the rule (and thus not allow); + * for deny rules, if the message has no interface we want to use the + * rule (and thus deny). + */ + dbus_bool_t no_interface; + + no_interface = dbus_message_get_interface (message) == NULL; + + if ((no_interface && rule->allow) || + (!no_interface && + strcmp (dbus_message_get_interface (message), + rule->d.send.interface) != 0)) + { + _dbus_verbose (" (policy) skipping rule for different interface\n"); + return FALSE; + } + } + + if (rule->d.send.member != NULL) + { + if (dbus_message_get_member (message) != NULL && + strcmp (dbus_message_get_member (message), + rule->d.send.member) != 0) + { + _dbus_verbose (" (policy) skipping rule for different member\n"); + return FALSE; + } + } + + if (rule->d.send.error != NULL) + { + if (dbus_message_get_error_name (message) != NULL && + strcmp (dbus_message_get_error_name (message), + rule->d.send.error) != 0) + { + _dbus_verbose (" (policy) skipping rule for different error name\n"); + return FALSE; + } + } + + if (rule->d.send.broadcast != BUS_POLICY_TRISTATE_ANY) + { + if (dbus_message_get_destination (message) == NULL && + dbus_message_get_type (message) == DBUS_MESSAGE_TYPE_SIGNAL) + { + /* it's a broadcast */ + if (rule->d.send.broadcast == BUS_POLICY_TRISTATE_FALSE) + { + _dbus_verbose (" (policy) skipping rule because message is a broadcast\n"); + return FALSE; + } + } + /* else it isn't a broadcast: there is some destination */ + else if (rule->d.send.broadcast == BUS_POLICY_TRISTATE_TRUE) + { + _dbus_verbose (" (policy) skipping rule because message is not a broadcast\n"); + return FALSE; + } + } + + if (rule->d.send.destination != NULL && !rule->d.send.destination_is_prefix) + { + /* receiver can be NULL for messages that are sent to the + * message bus itself, we check the strings in that case as + * built-in services don't have a DBusConnection but messages + * to them have a destination service name. + * + * Similarly, receiver can be NULL when we're deciding whether + * activation should be allowed; we make the authorization decision + * on the assumption that the activated service will have the + * requested name and no others. + */ + if (receiver == NULL) + { + if (!dbus_message_has_destination (message, + rule->d.send.destination)) + { + _dbus_verbose (" (policy) skipping rule because message dest is not %s\n", + rule->d.send.destination); + return FALSE; + } + } + else + { + DBusString str; + BusService *service; + + _dbus_string_init_const (&str, rule->d.send.destination); + + service = bus_registry_lookup (registry, &str); + if (service == NULL) + { + _dbus_verbose (" (policy) skipping rule because dest %s doesn't exist\n", + rule->d.send.destination); + return FALSE; + } + + if (!bus_service_owner_in_queue (service, receiver)) + { + _dbus_verbose (" (policy) skipping rule because receiver isn't primary or queued owner of name %s\n", + rule->d.send.destination); + return FALSE; + } + } + } + + if (rule->d.send.destination != NULL && rule->d.send.destination_is_prefix) + { + /* receiver can be NULL - the same as in !send.destination_is_prefix */ + if (receiver == NULL) + { + const char *destination = dbus_message_get_destination (message); + DBusString dest_name; + + if (destination == NULL) + { + _dbus_verbose (" (policy) skipping rule because message has no dest\n"); + return FALSE; + } + + _dbus_string_init_const (&dest_name, destination); + + if (!_dbus_string_starts_with_words_c_str (&dest_name, + rule->d.send.destination, + '.')) + { + _dbus_verbose (" (policy) skipping rule because message dest doesn't have prefix %s\n", + rule->d.send.destination); + return FALSE; + } + } + else + { + if (!bus_connection_is_queued_owner_by_prefix (receiver, + rule->d.send.destination)) + { + _dbus_verbose (" (policy) skipping rule because recipient isn't primary or queued owner of any name below %s\n", + rule->d.send.destination); + return FALSE; + } + } + } + + if (rule->d.send.min_fds > 0 || + rule->d.send.max_fds < DBUS_MAXIMUM_MESSAGE_UNIX_FDS) + { + unsigned int n_fds = _dbus_message_get_n_unix_fds (message); + + if (n_fds < rule->d.send.min_fds || n_fds > rule->d.send.max_fds) + { + _dbus_verbose (" (policy) skipping rule because message has %u fds " + "and that is outside range [%u,%u]", + n_fds, rule->d.send.min_fds, rule->d.send.max_fds); + return FALSE; + } + } + + /* Use this rule */ + return TRUE; +} + +static dbus_bool_t +check_receive_rule (const BusPolicyRule *rule, + BusRegistry *registry, + dbus_bool_t requested_reply, + DBusConnection *sender, + DBusMessage *message, + dbus_bool_t eavesdropping) +{ + if (rule->type != BUS_POLICY_RULE_RECEIVE) + { + _dbus_verbose (" (policy) skipping non-receive rule\n"); + return FALSE; + } + + if (rule->d.receive.message_type != DBUS_MESSAGE_TYPE_INVALID) + { + if (dbus_message_get_type (message) != rule->d.receive.message_type) + { + _dbus_verbose (" (policy) skipping rule for different message type\n"); + return FALSE; + } + } + + /* for allow, eavesdrop=false means the rule doesn't apply when + * eavesdropping. eavesdrop=true means always allow. + */ + if (eavesdropping && rule->allow && !rule->d.receive.eavesdrop) + { + _dbus_verbose (" (policy) skipping allow rule since it doesn't apply to eavesdropping\n"); + return FALSE; + } + + /* for deny, eavesdrop=true means the rule applies only when + * eavesdropping; eavesdrop=false means always deny. + */ + if (!eavesdropping && !rule->allow && rule->d.receive.eavesdrop) + { + _dbus_verbose (" (policy) skipping deny rule since it only applies to eavesdropping\n"); + return FALSE; + } + + /* If it's a reply, the requested_reply flag kicks in */ + if (dbus_message_get_reply_serial (message) != 0) + { + /* for allow, requested_reply=true means the rule applies + * only when reply was requested. requested_reply=false means + * always allow. + */ + if (!requested_reply && rule->allow && rule->d.receive.requested_reply && !rule->d.receive.eavesdrop) + { + _dbus_verbose (" (policy) skipping allow rule since it only applies to requested replies and does not allow eavesdropping\n"); + return FALSE; + } + + /* for deny, requested_reply=false means the rule applies only + * when the reply was not requested. requested_reply=true means the + * rule always applies. + */ + if (requested_reply && !rule->allow && !rule->d.receive.requested_reply) + { + _dbus_verbose (" (policy) skipping deny rule since it only applies to unrequested replies\n"); + return FALSE; + } + } + + if (rule->d.receive.path != NULL) + { + if (dbus_message_get_path (message) != NULL && + strcmp (dbus_message_get_path (message), + rule->d.receive.path) != 0) + { + _dbus_verbose (" (policy) skipping rule for different path\n"); + return FALSE; + } + } + + if (rule->d.receive.interface != NULL) + { + /* The interface is optional in messages. For allow rules, if the message + * has no interface we want to skip the rule (and thus not allow); + * for deny rules, if the message has no interface we want to use the + * rule (and thus deny). + */ + dbus_bool_t no_interface; + + no_interface = dbus_message_get_interface (message) == NULL; + + if ((no_interface && rule->allow) || + (!no_interface && + strcmp (dbus_message_get_interface (message), + rule->d.receive.interface) != 0)) + { + _dbus_verbose (" (policy) skipping rule for different interface\n"); + return FALSE; + } + } + + if (rule->d.receive.member != NULL) + { + if (dbus_message_get_member (message) != NULL && + strcmp (dbus_message_get_member (message), + rule->d.receive.member) != 0) + { + _dbus_verbose (" (policy) skipping rule for different member\n"); + return FALSE; + } + } + + if (rule->d.receive.error != NULL) + { + if (dbus_message_get_error_name (message) != NULL && + strcmp (dbus_message_get_error_name (message), + rule->d.receive.error) != 0) + { + _dbus_verbose (" (policy) skipping rule for different error name\n"); + return FALSE; + } + } + + if (rule->d.receive.origin != NULL) + { + /* sender can be NULL for messages that originate from the + * message bus itself, we check the strings in that case as + * built-in services don't have a DBusConnection but will + * still set the sender on their messages. + */ + if (sender == NULL) + { + if (!dbus_message_has_sender (message, + rule->d.receive.origin)) + { + _dbus_verbose (" (policy) skipping rule because message sender is not %s\n", + rule->d.receive.origin); + return FALSE; + } + } + else + { + BusService *service; + DBusString str; + + _dbus_string_init_const (&str, rule->d.receive.origin); + + service = bus_registry_lookup (registry, &str); + + if (service == NULL) + { + _dbus_verbose (" (policy) skipping rule because origin %s doesn't exist\n", + rule->d.receive.origin); + return FALSE; + } + + if (!bus_service_owner_in_queue (service, sender)) + { + _dbus_verbose (" (policy) skipping rule because sender isn't primary or queued owner of %s\n", + rule->d.receive.origin); + return FALSE; + } + } + } + + if (rule->d.receive.min_fds > 0 || + rule->d.receive.max_fds < DBUS_MAXIMUM_MESSAGE_UNIX_FDS) + { + unsigned int n_fds = _dbus_message_get_n_unix_fds (message); + + if (n_fds < rule->d.receive.min_fds || n_fds > rule->d.receive.max_fds) + { + _dbus_verbose (" (policy) skipping rule because message has %u fds " + "and that is outside range [%u,%u]", + n_fds, rule->d.receive.min_fds, + rule->d.receive.max_fds); + return FALSE; + } + } + + /* Use this rule */ + return TRUE; +} + +static dbus_bool_t +check_own_rule (const BusPolicyRule *rule, + const DBusString *service_name) +{ + /* Rule is skipped if it specifies a different service name from + * the desired one. + */ + + if (rule->type != BUS_POLICY_RULE_OWN) + return FALSE; + + if (!rule->d.own.prefix && rule->d.own.service_name != NULL) + { + if (!_dbus_string_equal_c_str (service_name, + rule->d.own.service_name)) + return FALSE; + } + else if (rule->d.own.prefix) + { + if (!_dbus_string_starts_with_words_c_str (service_name, + rule->d.own.service_name, + '.')) + return FALSE; + } + + /* Use this rule */ + return TRUE; +} + dbus_bool_t bus_client_policy_check_can_send (BusClientPolicy *policy, BusRegistry *registry, @@ -782,244 +1216,31 @@ bus_client_policy_check_can_send (BusClientPolicy *policy, { DBusList *link; dbus_bool_t allowed; - + /* policy->rules is in the order the rules appeared * in the config file, i.e. last rule that applies wins */ _dbus_verbose (" (policy) checking send rules\n"); *toggles = 0; - + allowed = FALSE; link = _dbus_list_get_first_link (&policy->rules); while (link != NULL) { - BusPolicyRule *rule = link->data; + const BusPolicyRule *rule = link->data; link = _dbus_list_get_next_link (&policy->rules, link); - - /* Rule is skipped if it specifies a different - * message name from the message, or a different - * destination from the message - */ - - if (rule->type != BUS_POLICY_RULE_SEND) + + if (check_send_rule (rule, registry, requested_reply, receiver, message)) { - _dbus_verbose (" (policy) skipping non-send rule\n"); - continue; + allowed = rule->allow; + *log = rule->d.send.log; + (*toggles)++; + + _dbus_verbose (" (policy) used rule, allow now = %d\n", + allowed); } - - if (rule->d.send.message_type != DBUS_MESSAGE_TYPE_INVALID) - { - if (dbus_message_get_type (message) != rule->d.send.message_type) - { - _dbus_verbose (" (policy) skipping rule for different message type\n"); - continue; - } - } - - /* If it's a reply, the requested_reply flag kicks in */ - if (dbus_message_get_reply_serial (message) != 0) - { - /* for allow, requested_reply=true means the rule applies - * only when reply was requested. requested_reply=false means - * always allow. - */ - if (!requested_reply && rule->allow && rule->d.send.requested_reply && !rule->d.send.eavesdrop) - { - _dbus_verbose (" (policy) skipping allow rule since it only applies to requested replies and does not allow eavesdropping\n"); - continue; - } - - /* for deny, requested_reply=false means the rule applies only - * when the reply was not requested. requested_reply=true means the - * rule always applies. - */ - if (requested_reply && !rule->allow && !rule->d.send.requested_reply) - { - _dbus_verbose (" (policy) skipping deny rule since it only applies to unrequested replies\n"); - continue; - } - } - - if (rule->d.send.path != NULL) - { - if (dbus_message_get_path (message) != NULL && - strcmp (dbus_message_get_path (message), - rule->d.send.path) != 0) - { - _dbus_verbose (" (policy) skipping rule for different path\n"); - continue; - } - } - - if (rule->d.send.interface != NULL) - { - /* The interface is optional in messages. For allow rules, if the message - * has no interface we want to skip the rule (and thus not allow); - * for deny rules, if the message has no interface we want to use the - * rule (and thus deny). - */ - dbus_bool_t no_interface; - - no_interface = dbus_message_get_interface (message) == NULL; - - if ((no_interface && rule->allow) || - (!no_interface && - strcmp (dbus_message_get_interface (message), - rule->d.send.interface) != 0)) - { - _dbus_verbose (" (policy) skipping rule for different interface\n"); - continue; - } - } - - if (rule->d.send.member != NULL) - { - if (dbus_message_get_member (message) != NULL && - strcmp (dbus_message_get_member (message), - rule->d.send.member) != 0) - { - _dbus_verbose (" (policy) skipping rule for different member\n"); - continue; - } - } - - if (rule->d.send.error != NULL) - { - if (dbus_message_get_error_name (message) != NULL && - strcmp (dbus_message_get_error_name (message), - rule->d.send.error) != 0) - { - _dbus_verbose (" (policy) skipping rule for different error name\n"); - continue; - } - } - - if (rule->d.send.broadcast != BUS_POLICY_TRISTATE_ANY) - { - if (dbus_message_get_destination (message) == NULL && - dbus_message_get_type (message) == DBUS_MESSAGE_TYPE_SIGNAL) - { - /* it's a broadcast */ - if (rule->d.send.broadcast == BUS_POLICY_TRISTATE_FALSE) - { - _dbus_verbose (" (policy) skipping rule because message is a broadcast\n"); - continue; - } - } - /* else it isn't a broadcast: there is some destination */ - else if (rule->d.send.broadcast == BUS_POLICY_TRISTATE_TRUE) - { - _dbus_verbose (" (policy) skipping rule because message is not a broadcast\n"); - continue; - } - } - - if (rule->d.send.destination != NULL && !rule->d.send.destination_is_prefix) - { - /* receiver can be NULL for messages that are sent to the - * message bus itself, we check the strings in that case as - * built-in services don't have a DBusConnection but messages - * to them have a destination service name. - * - * Similarly, receiver can be NULL when we're deciding whether - * activation should be allowed; we make the authorization decision - * on the assumption that the activated service will have the - * requested name and no others. - */ - if (receiver == NULL) - { - if (!dbus_message_has_destination (message, - rule->d.send.destination)) - { - _dbus_verbose (" (policy) skipping rule because message dest is not %s\n", - rule->d.send.destination); - continue; - } - } - else - { - DBusString str; - BusService *service; - - _dbus_string_init_const (&str, rule->d.send.destination); - - service = bus_registry_lookup (registry, &str); - if (service == NULL) - { - _dbus_verbose (" (policy) skipping rule because dest %s doesn't exist\n", - rule->d.send.destination); - continue; - } - - if (!bus_service_owner_in_queue (service, receiver)) - { - _dbus_verbose (" (policy) skipping rule because receiver isn't primary or queued owner of name %s\n", - rule->d.send.destination); - continue; - } - } - } - - if (rule->d.send.destination != NULL && rule->d.send.destination_is_prefix) - { - /* receiver can be NULL - the same as in !send.destination_is_prefix */ - if (receiver == NULL) - { - const char *destination = dbus_message_get_destination (message); - DBusString dest_name; - - if (destination == NULL) - { - _dbus_verbose (" (policy) skipping rule because message has no dest\n"); - continue; - } - - _dbus_string_init_const (&dest_name, destination); - - if (!_dbus_string_starts_with_words_c_str (&dest_name, - rule->d.send.destination, - '.')) - { - _dbus_verbose (" (policy) skipping rule because message dest doesn't have prefix %s\n", - rule->d.send.destination); - continue; - } - } - else - { - if (!bus_connection_is_queued_owner_by_prefix (receiver, - rule->d.send.destination)) - { - _dbus_verbose (" (policy) skipping rule because recipient isn't primary or queued owner of any name below %s\n", - rule->d.send.destination); - continue; - } - } - } - - if (rule->d.send.min_fds > 0 || - rule->d.send.max_fds < DBUS_MAXIMUM_MESSAGE_UNIX_FDS) - { - unsigned int n_fds = _dbus_message_get_n_unix_fds (message); - - if (n_fds < rule->d.send.min_fds || n_fds > rule->d.send.max_fds) - { - _dbus_verbose (" (policy) skipping rule because message has %u fds " - "and that is outside range [%u,%u]", - n_fds, rule->d.send.min_fds, rule->d.send.max_fds); - continue; - } - } - - /* Use this rule */ - allowed = rule->allow; - *log = rule->d.send.log; - (*toggles)++; - - _dbus_verbose (" (policy) used rule, allow now = %d\n", - allowed); } return allowed; @@ -1045,7 +1266,7 @@ bus_client_policy_check_can_receive (BusClientPolicy *policy, eavesdropping = addressed_recipient != proposed_recipient && dbus_message_get_destination (message) != NULL; - + /* policy->rules is in the order the rules appeared * in the config file, i.e. last rule that applies wins */ @@ -1057,191 +1278,25 @@ bus_client_policy_check_can_receive (BusClientPolicy *policy, link = _dbus_list_get_first_link (&policy->rules); while (link != NULL) { - BusPolicyRule *rule = link->data; + const BusPolicyRule *rule = link->data; - link = _dbus_list_get_next_link (&policy->rules, link); - - if (rule->type != BUS_POLICY_RULE_RECEIVE) + link = _dbus_list_get_next_link (&policy->rules, link); + + if (check_receive_rule (rule, registry, requested_reply, sender, + message, eavesdropping)) { - _dbus_verbose (" (policy) skipping non-receive rule\n"); - continue; + allowed = rule->allow; + (*toggles)++; + + _dbus_verbose (" (policy) used rule, allow now = %d\n", + allowed); } - if (rule->d.receive.message_type != DBUS_MESSAGE_TYPE_INVALID) - { - if (dbus_message_get_type (message) != rule->d.receive.message_type) - { - _dbus_verbose (" (policy) skipping rule for different message type\n"); - continue; - } - } - - /* for allow, eavesdrop=false means the rule doesn't apply when - * eavesdropping. eavesdrop=true means always allow. - */ - if (eavesdropping && rule->allow && !rule->d.receive.eavesdrop) - { - _dbus_verbose (" (policy) skipping allow rule since it doesn't apply to eavesdropping\n"); - continue; - } - - /* for deny, eavesdrop=true means the rule applies only when - * eavesdropping; eavesdrop=false means always deny. - */ - if (!eavesdropping && !rule->allow && rule->d.receive.eavesdrop) - { - _dbus_verbose (" (policy) skipping deny rule since it only applies to eavesdropping\n"); - continue; - } - - /* If it's a reply, the requested_reply flag kicks in */ - if (dbus_message_get_reply_serial (message) != 0) - { - /* for allow, requested_reply=true means the rule applies - * only when reply was requested. requested_reply=false means - * always allow. - */ - if (!requested_reply && rule->allow && rule->d.receive.requested_reply && !rule->d.receive.eavesdrop) - { - _dbus_verbose (" (policy) skipping allow rule since it only applies to requested replies and does not allow eavesdropping\n"); - continue; - } - - /* for deny, requested_reply=false means the rule applies only - * when the reply was not requested. requested_reply=true means the - * rule always applies. - */ - if (requested_reply && !rule->allow && !rule->d.receive.requested_reply) - { - _dbus_verbose (" (policy) skipping deny rule since it only applies to unrequested replies\n"); - continue; - } - } - - if (rule->d.receive.path != NULL) - { - if (dbus_message_get_path (message) != NULL && - strcmp (dbus_message_get_path (message), - rule->d.receive.path) != 0) - { - _dbus_verbose (" (policy) skipping rule for different path\n"); - continue; - } - } - - if (rule->d.receive.interface != NULL) - { - /* The interface is optional in messages. For allow rules, if the message - * has no interface we want to skip the rule (and thus not allow); - * for deny rules, if the message has no interface we want to use the - * rule (and thus deny). - */ - dbus_bool_t no_interface; - - no_interface = dbus_message_get_interface (message) == NULL; - - if ((no_interface && rule->allow) || - (!no_interface && - strcmp (dbus_message_get_interface (message), - rule->d.receive.interface) != 0)) - { - _dbus_verbose (" (policy) skipping rule for different interface\n"); - continue; - } - } - - if (rule->d.receive.member != NULL) - { - if (dbus_message_get_member (message) != NULL && - strcmp (dbus_message_get_member (message), - rule->d.receive.member) != 0) - { - _dbus_verbose (" (policy) skipping rule for different member\n"); - continue; - } - } - - if (rule->d.receive.error != NULL) - { - if (dbus_message_get_error_name (message) != NULL && - strcmp (dbus_message_get_error_name (message), - rule->d.receive.error) != 0) - { - _dbus_verbose (" (policy) skipping rule for different error name\n"); - continue; - } - } - - if (rule->d.receive.origin != NULL) - { - /* sender can be NULL for messages that originate from the - * message bus itself, we check the strings in that case as - * built-in services don't have a DBusConnection but will - * still set the sender on their messages. - */ - if (sender == NULL) - { - if (!dbus_message_has_sender (message, - rule->d.receive.origin)) - { - _dbus_verbose (" (policy) skipping rule because message sender is not %s\n", - rule->d.receive.origin); - continue; - } - } - else - { - BusService *service; - DBusString str; - - _dbus_string_init_const (&str, rule->d.receive.origin); - - service = bus_registry_lookup (registry, &str); - - if (service == NULL) - { - _dbus_verbose (" (policy) skipping rule because origin %s doesn't exist\n", - rule->d.receive.origin); - continue; - } - - if (!bus_service_owner_in_queue (service, sender)) - { - _dbus_verbose (" (policy) skipping rule because sender isn't primary or queued owner of %s\n", - rule->d.receive.origin); - continue; - } - } - } - - if (rule->d.receive.min_fds > 0 || - rule->d.receive.max_fds < DBUS_MAXIMUM_MESSAGE_UNIX_FDS) - { - unsigned int n_fds = _dbus_message_get_n_unix_fds (message); - - if (n_fds < rule->d.receive.min_fds || n_fds > rule->d.receive.max_fds) - { - _dbus_verbose (" (policy) skipping rule because message has %u fds " - "and that is outside range [%u,%u]", - n_fds, rule->d.receive.min_fds, - rule->d.receive.max_fds); - continue; - } - } - - /* Use this rule */ - allowed = rule->allow; - (*toggles)++; - - _dbus_verbose (" (policy) used rule, allow now = %d\n", - allowed); } return allowed; } - - static dbus_bool_t bus_rules_check_can_own (DBusList *rules, const DBusString *service_name) @@ -1257,33 +1312,14 @@ bus_rules_check_can_own (DBusList *rules, link = _dbus_list_get_first_link (&rules); while (link != NULL) { - BusPolicyRule *rule = link->data; + const BusPolicyRule *rule = link->data; link = _dbus_list_get_next_link (&rules, link); - /* Rule is skipped if it specifies a different service name from - * the desired one. - */ - - if (rule->type != BUS_POLICY_RULE_OWN) - continue; - - if (!rule->d.own.prefix && rule->d.own.service_name != NULL) + if (check_own_rule (rule, service_name)) { - if (!_dbus_string_equal_c_str (service_name, - rule->d.own.service_name)) - continue; + allowed = rule->allow; } - else if (rule->d.own.prefix) - { - if (!_dbus_string_starts_with_words_c_str (service_name, - rule->d.own.service_name, - '.')) - continue; - } - - /* Use this rule */ - allowed = rule->allow; } return allowed; From d98ab276ca8e13ea8dbcd8d0c2bb121a9141edb8 Mon Sep 17 00:00:00 2001 From: Adrian Szyndela Date: Wed, 20 May 2020 14:17:33 +0200 Subject: [PATCH 4/9] bus/policy: reduce number of params in check_ functions No functional changes, just packed arguments to structs. Change-Id: I0e5a22a208ba7085727e617c52cd061c39524967 --- bus/policy.c | 132 +++++++++++++++++++++++++++++---------------------- 1 file changed, 75 insertions(+), 57 deletions(-) diff --git a/bus/policy.c b/bus/policy.c index 5e2c0c90..b43d45e5 100644 --- a/bus/policy.c +++ b/bus/policy.c @@ -771,12 +771,24 @@ bus_client_policy_append_rule (BusClientPolicy *policy, return TRUE; } +struct MatchSendRuleParams { + BusRegistry *registry; + dbus_bool_t requested_reply; + DBusConnection *receiver; + DBusMessage *message; +}; + +struct MatchReceiveRuleParams { + BusRegistry *registry; + dbus_bool_t requested_reply; + DBusConnection *sender; + DBusMessage *message; + dbus_bool_t eavesdropping; +}; + static dbus_bool_t check_send_rule (const BusPolicyRule *rule, - BusRegistry *registry, - dbus_bool_t requested_reply, - DBusConnection *receiver, - DBusMessage *message) + const struct MatchSendRuleParams *match_params) { /* Rule is skipped if it specifies a different * message name from the message, or a different @@ -790,7 +802,7 @@ check_send_rule (const BusPolicyRule *rule, if (rule->d.send.message_type != DBUS_MESSAGE_TYPE_INVALID) { - if (dbus_message_get_type (message) != rule->d.send.message_type) + if (dbus_message_get_type (match_params->message) != rule->d.send.message_type) { _dbus_verbose (" (policy) skipping rule for different message type\n"); return FALSE; @@ -798,13 +810,13 @@ check_send_rule (const BusPolicyRule *rule, } /* If it's a reply, the requested_reply flag kicks in */ - if (dbus_message_get_reply_serial (message) != 0) + if (dbus_message_get_reply_serial (match_params->message) != 0) { /* for allow, requested_reply=true means the rule applies * only when reply was requested. requested_reply=false means * always allow. */ - if (!requested_reply && rule->allow && rule->d.send.requested_reply && !rule->d.send.eavesdrop) + if (!match_params->requested_reply && rule->allow && rule->d.send.requested_reply && !rule->d.send.eavesdrop) { _dbus_verbose (" (policy) skipping allow rule since it only applies to requested replies and does not allow eavesdropping\n"); return FALSE; @@ -814,7 +826,7 @@ check_send_rule (const BusPolicyRule *rule, * when the reply was not requested. requested_reply=true means the * rule always applies. */ - if (requested_reply && !rule->allow && !rule->d.send.requested_reply) + if (match_params->requested_reply && !rule->allow && !rule->d.send.requested_reply) { _dbus_verbose (" (policy) skipping deny rule since it only applies to unrequested replies\n"); return FALSE; @@ -823,8 +835,8 @@ check_send_rule (const BusPolicyRule *rule, if (rule->d.send.path != NULL) { - if (dbus_message_get_path (message) != NULL && - strcmp (dbus_message_get_path (message), + if (dbus_message_get_path (match_params->message) != NULL && + strcmp (dbus_message_get_path (match_params->message), rule->d.send.path) != 0) { _dbus_verbose (" (policy) skipping rule for different path\n"); @@ -841,11 +853,11 @@ check_send_rule (const BusPolicyRule *rule, */ dbus_bool_t no_interface; - no_interface = dbus_message_get_interface (message) == NULL; + no_interface = dbus_message_get_interface (match_params->message) == NULL; if ((no_interface && rule->allow) || (!no_interface && - strcmp (dbus_message_get_interface (message), + strcmp (dbus_message_get_interface (match_params->message), rule->d.send.interface) != 0)) { _dbus_verbose (" (policy) skipping rule for different interface\n"); @@ -855,8 +867,8 @@ check_send_rule (const BusPolicyRule *rule, if (rule->d.send.member != NULL) { - if (dbus_message_get_member (message) != NULL && - strcmp (dbus_message_get_member (message), + if (dbus_message_get_member (match_params->message) != NULL && + strcmp (dbus_message_get_member (match_params->message), rule->d.send.member) != 0) { _dbus_verbose (" (policy) skipping rule for different member\n"); @@ -866,8 +878,8 @@ check_send_rule (const BusPolicyRule *rule, if (rule->d.send.error != NULL) { - if (dbus_message_get_error_name (message) != NULL && - strcmp (dbus_message_get_error_name (message), + if (dbus_message_get_error_name (match_params->message) != NULL && + strcmp (dbus_message_get_error_name (match_params->message), rule->d.send.error) != 0) { _dbus_verbose (" (policy) skipping rule for different error name\n"); @@ -877,8 +889,8 @@ check_send_rule (const BusPolicyRule *rule, if (rule->d.send.broadcast != BUS_POLICY_TRISTATE_ANY) { - if (dbus_message_get_destination (message) == NULL && - dbus_message_get_type (message) == DBUS_MESSAGE_TYPE_SIGNAL) + if (dbus_message_get_destination (match_params->message) == NULL && + dbus_message_get_type (match_params->message) == DBUS_MESSAGE_TYPE_SIGNAL) { /* it's a broadcast */ if (rule->d.send.broadcast == BUS_POLICY_TRISTATE_FALSE) @@ -907,9 +919,9 @@ check_send_rule (const BusPolicyRule *rule, * on the assumption that the activated service will have the * requested name and no others. */ - if (receiver == NULL) + if (match_params->receiver == NULL) { - if (!dbus_message_has_destination (message, + if (!dbus_message_has_destination (match_params->message, rule->d.send.destination)) { _dbus_verbose (" (policy) skipping rule because message dest is not %s\n", @@ -924,7 +936,7 @@ check_send_rule (const BusPolicyRule *rule, _dbus_string_init_const (&str, rule->d.send.destination); - service = bus_registry_lookup (registry, &str); + service = bus_registry_lookup (match_params->registry, &str); if (service == NULL) { _dbus_verbose (" (policy) skipping rule because dest %s doesn't exist\n", @@ -932,7 +944,7 @@ check_send_rule (const BusPolicyRule *rule, return FALSE; } - if (!bus_service_owner_in_queue (service, receiver)) + if (!bus_service_owner_in_queue (service, match_params->receiver)) { _dbus_verbose (" (policy) skipping rule because receiver isn't primary or queued owner of name %s\n", rule->d.send.destination); @@ -944,9 +956,9 @@ check_send_rule (const BusPolicyRule *rule, if (rule->d.send.destination != NULL && rule->d.send.destination_is_prefix) { /* receiver can be NULL - the same as in !send.destination_is_prefix */ - if (receiver == NULL) + if (match_params->receiver == NULL) { - const char *destination = dbus_message_get_destination (message); + const char *destination = dbus_message_get_destination (match_params->message); DBusString dest_name; if (destination == NULL) @@ -968,7 +980,7 @@ check_send_rule (const BusPolicyRule *rule, } else { - if (!bus_connection_is_queued_owner_by_prefix (receiver, + if (!bus_connection_is_queued_owner_by_prefix (match_params->receiver, rule->d.send.destination)) { _dbus_verbose (" (policy) skipping rule because recipient isn't primary or queued owner of any name below %s\n", @@ -981,7 +993,7 @@ check_send_rule (const BusPolicyRule *rule, if (rule->d.send.min_fds > 0 || rule->d.send.max_fds < DBUS_MAXIMUM_MESSAGE_UNIX_FDS) { - unsigned int n_fds = _dbus_message_get_n_unix_fds (message); + unsigned int n_fds = _dbus_message_get_n_unix_fds (match_params->message); if (n_fds < rule->d.send.min_fds || n_fds > rule->d.send.max_fds) { @@ -998,11 +1010,7 @@ check_send_rule (const BusPolicyRule *rule, static dbus_bool_t check_receive_rule (const BusPolicyRule *rule, - BusRegistry *registry, - dbus_bool_t requested_reply, - DBusConnection *sender, - DBusMessage *message, - dbus_bool_t eavesdropping) + const struct MatchReceiveRuleParams *match_params) { if (rule->type != BUS_POLICY_RULE_RECEIVE) { @@ -1012,7 +1020,7 @@ check_receive_rule (const BusPolicyRule *rule, if (rule->d.receive.message_type != DBUS_MESSAGE_TYPE_INVALID) { - if (dbus_message_get_type (message) != rule->d.receive.message_type) + if (dbus_message_get_type (match_params->message) != rule->d.receive.message_type) { _dbus_verbose (" (policy) skipping rule for different message type\n"); return FALSE; @@ -1022,7 +1030,7 @@ check_receive_rule (const BusPolicyRule *rule, /* for allow, eavesdrop=false means the rule doesn't apply when * eavesdropping. eavesdrop=true means always allow. */ - if (eavesdropping && rule->allow && !rule->d.receive.eavesdrop) + if (match_params->eavesdropping && rule->allow && !rule->d.receive.eavesdrop) { _dbus_verbose (" (policy) skipping allow rule since it doesn't apply to eavesdropping\n"); return FALSE; @@ -1031,20 +1039,20 @@ check_receive_rule (const BusPolicyRule *rule, /* for deny, eavesdrop=true means the rule applies only when * eavesdropping; eavesdrop=false means always deny. */ - if (!eavesdropping && !rule->allow && rule->d.receive.eavesdrop) + if (!match_params->eavesdropping && !rule->allow && rule->d.receive.eavesdrop) { _dbus_verbose (" (policy) skipping deny rule since it only applies to eavesdropping\n"); return FALSE; } /* If it's a reply, the requested_reply flag kicks in */ - if (dbus_message_get_reply_serial (message) != 0) + if (dbus_message_get_reply_serial (match_params->message) != 0) { /* for allow, requested_reply=true means the rule applies * only when reply was requested. requested_reply=false means * always allow. */ - if (!requested_reply && rule->allow && rule->d.receive.requested_reply && !rule->d.receive.eavesdrop) + if (!match_params->requested_reply && rule->allow && rule->d.receive.requested_reply && !rule->d.receive.eavesdrop) { _dbus_verbose (" (policy) skipping allow rule since it only applies to requested replies and does not allow eavesdropping\n"); return FALSE; @@ -1054,7 +1062,7 @@ check_receive_rule (const BusPolicyRule *rule, * when the reply was not requested. requested_reply=true means the * rule always applies. */ - if (requested_reply && !rule->allow && !rule->d.receive.requested_reply) + if (match_params->requested_reply && !rule->allow && !rule->d.receive.requested_reply) { _dbus_verbose (" (policy) skipping deny rule since it only applies to unrequested replies\n"); return FALSE; @@ -1063,8 +1071,8 @@ check_receive_rule (const BusPolicyRule *rule, if (rule->d.receive.path != NULL) { - if (dbus_message_get_path (message) != NULL && - strcmp (dbus_message_get_path (message), + if (dbus_message_get_path (match_params->message) != NULL && + strcmp (dbus_message_get_path (match_params->message), rule->d.receive.path) != 0) { _dbus_verbose (" (policy) skipping rule for different path\n"); @@ -1081,11 +1089,11 @@ check_receive_rule (const BusPolicyRule *rule, */ dbus_bool_t no_interface; - no_interface = dbus_message_get_interface (message) == NULL; + no_interface = dbus_message_get_interface (match_params->message) == NULL; if ((no_interface && rule->allow) || (!no_interface && - strcmp (dbus_message_get_interface (message), + strcmp (dbus_message_get_interface (match_params->message), rule->d.receive.interface) != 0)) { _dbus_verbose (" (policy) skipping rule for different interface\n"); @@ -1095,8 +1103,8 @@ check_receive_rule (const BusPolicyRule *rule, if (rule->d.receive.member != NULL) { - if (dbus_message_get_member (message) != NULL && - strcmp (dbus_message_get_member (message), + if (dbus_message_get_member (match_params->message) != NULL && + strcmp (dbus_message_get_member (match_params->message), rule->d.receive.member) != 0) { _dbus_verbose (" (policy) skipping rule for different member\n"); @@ -1106,8 +1114,8 @@ check_receive_rule (const BusPolicyRule *rule, if (rule->d.receive.error != NULL) { - if (dbus_message_get_error_name (message) != NULL && - strcmp (dbus_message_get_error_name (message), + if (dbus_message_get_error_name (match_params->message) != NULL && + strcmp (dbus_message_get_error_name (match_params->message), rule->d.receive.error) != 0) { _dbus_verbose (" (policy) skipping rule for different error name\n"); @@ -1122,9 +1130,9 @@ check_receive_rule (const BusPolicyRule *rule, * built-in services don't have a DBusConnection but will * still set the sender on their messages. */ - if (sender == NULL) + if (match_params->sender == NULL) { - if (!dbus_message_has_sender (message, + if (!dbus_message_has_sender (match_params->message, rule->d.receive.origin)) { _dbus_verbose (" (policy) skipping rule because message sender is not %s\n", @@ -1139,7 +1147,7 @@ check_receive_rule (const BusPolicyRule *rule, _dbus_string_init_const (&str, rule->d.receive.origin); - service = bus_registry_lookup (registry, &str); + service = bus_registry_lookup (match_params->registry, &str); if (service == NULL) { @@ -1148,7 +1156,7 @@ check_receive_rule (const BusPolicyRule *rule, return FALSE; } - if (!bus_service_owner_in_queue (service, sender)) + if (!bus_service_owner_in_queue (service, match_params->sender)) { _dbus_verbose (" (policy) skipping rule because sender isn't primary or queued owner of %s\n", rule->d.receive.origin); @@ -1160,7 +1168,7 @@ check_receive_rule (const BusPolicyRule *rule, if (rule->d.receive.min_fds > 0 || rule->d.receive.max_fds < DBUS_MAXIMUM_MESSAGE_UNIX_FDS) { - unsigned int n_fds = _dbus_message_get_n_unix_fds (message); + unsigned int n_fds = _dbus_message_get_n_unix_fds (match_params->message); if (n_fds < rule->d.receive.min_fds || n_fds > rule->d.receive.max_fds) { @@ -1216,6 +1224,13 @@ bus_client_policy_check_can_send (BusClientPolicy *policy, { DBusList *link; dbus_bool_t allowed; + + struct MatchSendRuleParams params; + + params.registry = registry; + params.requested_reply = requested_reply; + params.receiver = receiver; + params.message = message; /* policy->rules is in the order the rules appeared * in the config file, i.e. last rule that applies wins @@ -1223,7 +1238,7 @@ bus_client_policy_check_can_send (BusClientPolicy *policy, _dbus_verbose (" (policy) checking send rules\n"); *toggles = 0; - + allowed = FALSE; link = _dbus_list_get_first_link (&policy->rules); while (link != NULL) @@ -1232,7 +1247,7 @@ bus_client_policy_check_can_send (BusClientPolicy *policy, link = _dbus_list_get_next_link (&policy->rules, link); - if (check_send_rule (rule, registry, requested_reply, receiver, message)) + if (check_send_rule (rule, ¶ms)) { allowed = rule->allow; *log = rule->d.send.log; @@ -1261,17 +1276,21 @@ bus_client_policy_check_can_receive (BusClientPolicy *policy, { DBusList *link; dbus_bool_t allowed; - dbus_bool_t eavesdropping; + struct MatchReceiveRuleParams params; - eavesdropping = + params.eavesdropping = addressed_recipient != proposed_recipient && dbus_message_get_destination (message) != NULL; + params.registry = registry; + params.requested_reply = requested_reply; + params.sender = sender; + params.message = message; /* policy->rules is in the order the rules appeared * in the config file, i.e. last rule that applies wins */ - _dbus_verbose (" (policy) checking receive rules, eavesdropping = %d\n", eavesdropping); + _dbus_verbose (" (policy) checking receive rules, eavesdropping = %d\n", params.eavesdropping); *toggles = 0; allowed = FALSE; @@ -1282,8 +1301,7 @@ bus_client_policy_check_can_receive (BusClientPolicy *policy, link = _dbus_list_get_next_link (&policy->rules, link); - if (check_receive_rule (rule, registry, requested_reply, sender, - message, eavesdropping)) + if (check_receive_rule (rule, ¶ms)) { allowed = rule->allow; (*toggles)++; From cfaa325283ac6e2e3ce3818db5e965b4946e58c2 Mon Sep 17 00:00:00 2001 From: Adrian Szyndela Date: Wed, 20 May 2020 15:01:22 +0200 Subject: [PATCH 5/9] bus/policy: generalized policy checking process The process of checking policy is the same for all policy types: - walk through the rules list, - match each rule against parameters, - last matching rule wins. This commit introduces generalized checking function, that is fed with "matching parameters". Change-Id: I573ddbc7e64bef38ed7517644bd842728e14679b --- bus/policy.c | 201 ++++++++++++++++++++++++++------------------------- 1 file changed, 101 insertions(+), 100 deletions(-) diff --git a/bus/policy.c b/bus/policy.c index b43d45e5..91943f13 100644 --- a/bus/policy.c +++ b/bus/policy.c @@ -771,24 +771,29 @@ bus_client_policy_append_rule (BusClientPolicy *policy, return TRUE; } -struct MatchSendRuleParams { - BusRegistry *registry; - dbus_bool_t requested_reply; - DBusConnection *receiver; - DBusMessage *message; -}; +typedef struct SendReceiveParams { + BusRegistry *registry; + dbus_bool_t requested_reply; + DBusConnection *peer; + DBusMessage *message; + dbus_bool_t eavesdropping; +} SendReceiveParams; -struct MatchReceiveRuleParams { - BusRegistry *registry; - dbus_bool_t requested_reply; - DBusConnection *sender; - DBusMessage *message; - dbus_bool_t eavesdropping; -}; +typedef struct OwnParams { + const DBusString *name; +} OwnParams; + +typedef struct RuleParams { + enum {PARAMS_OWN, PARAMS_SEND, PARAMS_RECEIVE} type; + union { + SendReceiveParams sr; + OwnParams own; + } u; +} RuleParams; static dbus_bool_t -check_send_rule (const BusPolicyRule *rule, - const struct MatchSendRuleParams *match_params) +check_send_rule (const BusPolicyRule *rule, + const SendReceiveParams *match_params) { /* Rule is skipped if it specifies a different * message name from the message, or a different @@ -919,7 +924,7 @@ check_send_rule (const BusPolicyRule *rule, * on the assumption that the activated service will have the * requested name and no others. */ - if (match_params->receiver == NULL) + if (match_params->peer == NULL) { if (!dbus_message_has_destination (match_params->message, rule->d.send.destination)) @@ -944,7 +949,7 @@ check_send_rule (const BusPolicyRule *rule, return FALSE; } - if (!bus_service_owner_in_queue (service, match_params->receiver)) + if (!bus_service_owner_in_queue (service, match_params->peer)) { _dbus_verbose (" (policy) skipping rule because receiver isn't primary or queued owner of name %s\n", rule->d.send.destination); @@ -956,7 +961,7 @@ check_send_rule (const BusPolicyRule *rule, if (rule->d.send.destination != NULL && rule->d.send.destination_is_prefix) { /* receiver can be NULL - the same as in !send.destination_is_prefix */ - if (match_params->receiver == NULL) + if (match_params->peer == NULL) { const char *destination = dbus_message_get_destination (match_params->message); DBusString dest_name; @@ -980,7 +985,7 @@ check_send_rule (const BusPolicyRule *rule, } else { - if (!bus_connection_is_queued_owner_by_prefix (match_params->receiver, + if (!bus_connection_is_queued_owner_by_prefix (match_params->peer, rule->d.send.destination)) { _dbus_verbose (" (policy) skipping rule because recipient isn't primary or queued owner of any name below %s\n", @@ -1009,8 +1014,8 @@ check_send_rule (const BusPolicyRule *rule, } static dbus_bool_t -check_receive_rule (const BusPolicyRule *rule, - const struct MatchReceiveRuleParams *match_params) +check_receive_rule (const BusPolicyRule *rule, + const SendReceiveParams *match_params) { if (rule->type != BUS_POLICY_RULE_RECEIVE) { @@ -1130,7 +1135,7 @@ check_receive_rule (const BusPolicyRule *rule, * built-in services don't have a DBusConnection but will * still set the sender on their messages. */ - if (match_params->sender == NULL) + if (match_params->peer == NULL) { if (!dbus_message_has_sender (match_params->message, rule->d.receive.origin)) @@ -1156,7 +1161,7 @@ check_receive_rule (const BusPolicyRule *rule, return FALSE; } - if (!bus_service_owner_in_queue (service, match_params->sender)) + if (!bus_service_owner_in_queue (service, match_params->peer)) { _dbus_verbose (" (policy) skipping rule because sender isn't primary or queued owner of %s\n", rule->d.receive.origin); @@ -1186,8 +1191,10 @@ check_receive_rule (const BusPolicyRule *rule, static dbus_bool_t check_own_rule (const BusPolicyRule *rule, - const DBusString *service_name) + const OwnParams *params) { + const DBusString *service_name = params->name; + /* Rule is skipped if it specifies a different service name from * the desired one. */ @@ -1213,6 +1220,53 @@ check_own_rule (const BusPolicyRule *rule, return TRUE; } +static dbus_bool_t +check_rules_list (const DBusList *rules, + const RuleParams *params, + dbus_int32_t *toggles, + dbus_bool_t *log) +{ + const DBusList *link; + dbus_bool_t allowed; + + link = _dbus_list_get_first_link ((DBusList **)&rules); + while (link != NULL) + { + const BusPolicyRule *rule = link->data; + dbus_bool_t matches; + + link = _dbus_list_get_next_link ((DBusList **)&rules, link); + + switch (params->type) + { + case PARAMS_OWN: + matches = check_own_rule (rule, ¶ms->u.own); + break; + case PARAMS_SEND: + matches = check_send_rule (rule, ¶ms->u.sr); + break; + case PARAMS_RECEIVE: + matches = check_receive_rule (rule, ¶ms->u.sr); + break; + default: + _dbus_assert_not_reached ("wrong type of policy"); + } + + if (matches) + { + if (log) + *log = rule->d.send.log; + if (toggles) + (*toggles)++; + allowed = rule->allow; + + _dbus_verbose (" (policy) used rule, allow now = %d\n", + allowed); + } + } + return allowed; +} + dbus_bool_t bus_client_policy_check_can_send (BusClientPolicy *policy, BusRegistry *registry, @@ -1222,15 +1276,13 @@ bus_client_policy_check_can_send (BusClientPolicy *policy, dbus_int32_t *toggles, dbus_bool_t *log) { - DBusList *link; - dbus_bool_t allowed; - - struct MatchSendRuleParams params; + struct RuleParams params; - params.registry = registry; - params.requested_reply = requested_reply; - params.receiver = receiver; - params.message = message; + params.type = PARAMS_SEND; + params.u.sr.registry = registry; + params.u.sr.requested_reply = requested_reply; + params.u.sr.peer = receiver; + params.u.sr.message = message; /* policy->rules is in the order the rules appeared * in the config file, i.e. last rule that applies wins @@ -1238,27 +1290,8 @@ bus_client_policy_check_can_send (BusClientPolicy *policy, _dbus_verbose (" (policy) checking send rules\n"); *toggles = 0; - - allowed = FALSE; - link = _dbus_list_get_first_link (&policy->rules); - while (link != NULL) - { - const BusPolicyRule *rule = link->data; - link = _dbus_list_get_next_link (&policy->rules, link); - - if (check_send_rule (rule, ¶ms)) - { - allowed = rule->allow; - *log = rule->d.send.log; - (*toggles)++; - - _dbus_verbose (" (policy) used rule, allow now = %d\n", - allowed); - } - } - - return allowed; + return check_rules_list (policy->rules, ¶ms, toggles, log); } /* See docs on what the args mean on bus_context_check_security_policy() @@ -1274,73 +1307,41 @@ bus_client_policy_check_can_receive (BusClientPolicy *policy, DBusMessage *message, dbus_int32_t *toggles) { - DBusList *link; - dbus_bool_t allowed; - struct MatchReceiveRuleParams params; + struct RuleParams params; - params.eavesdropping = + params.type = PARAMS_RECEIVE; + params.u.sr.registry = registry; + params.u.sr.requested_reply = requested_reply; + params.u.sr.peer = sender; + params.u.sr.message = message; + params.u.sr.eavesdropping = addressed_recipient != proposed_recipient && dbus_message_get_destination (message) != NULL; - params.registry = registry; - params.requested_reply = requested_reply; - params.sender = sender; - params.message = message; /* policy->rules is in the order the rules appeared * in the config file, i.e. last rule that applies wins */ - _dbus_verbose (" (policy) checking receive rules, eavesdropping = %d\n", params.eavesdropping); + _dbus_verbose (" (policy) checking receive rules, eavesdropping = %d\n", params.u.sr.eavesdropping); *toggles = 0; - - allowed = FALSE; - link = _dbus_list_get_first_link (&policy->rules); - while (link != NULL) - { - const BusPolicyRule *rule = link->data; - link = _dbus_list_get_next_link (&policy->rules, link); - - if (check_receive_rule (rule, ¶ms)) - { - allowed = rule->allow; - (*toggles)++; - - _dbus_verbose (" (policy) used rule, allow now = %d\n", - allowed); - } - - } - - return allowed; + return check_rules_list (policy->rules, ¶ms, toggles, NULL); } static dbus_bool_t -bus_rules_check_can_own (DBusList *rules, +bus_rules_check_can_own (DBusList *rules, const DBusString *service_name) { - DBusList *link; - dbus_bool_t allowed; + RuleParams params; + + params.type = PARAMS_OWN; + params.u.own.name = service_name; - /* rules is in the order the rules appeared + /* policy->rules is in the order the rules appeared * in the config file, i.e. last rule that applies wins */ - allowed = FALSE; - link = _dbus_list_get_first_link (&rules); - while (link != NULL) - { - const BusPolicyRule *rule = link->data; - - link = _dbus_list_get_next_link (&rules, link); - - if (check_own_rule (rule, service_name)) - { - allowed = rule->allow; - } - } - - return allowed; + return check_rules_list (rules, ¶ms, NULL, NULL); } dbus_bool_t From 3aba9979e3c9854cbd151c775350d80a231ff98b Mon Sep 17 00:00:00 2001 From: Adrian Szyndela Date: Wed, 20 May 2020 15:09:21 +0200 Subject: [PATCH 6/9] bus/policy: direct checking of policy, without copies Instead of building a copy of policy for each client, we can check it using the main copy. Client copies are rebuilt when the main copy is changed, anyway. This removes copying of the main policy to client copies. Instead, copy data that was used to select rules, and use it for selection of rules from the main copy. Change-Id: I42926c107aae0be1a1247a61f3558122b07f9914 --- bus/policy.c | 274 +++++++++++++++++++-------------------------------- 1 file changed, 104 insertions(+), 170 deletions(-) diff --git a/bus/policy.c b/bus/policy.c index 91943f13..14389fe5 100644 --- a/bus/policy.c +++ b/bus/policy.c @@ -31,8 +31,17 @@ #include #include -static dbus_bool_t bus_client_policy_append_rule (BusClientPolicy *policy, - BusPolicyRule *rule); +struct BusClientPolicy +{ + int refcount; + + BusPolicy *policy; + unsigned long *groups; + int n_groups; + dbus_uid_t uid; + dbus_bool_t uid_set; + dbus_bool_t at_console; +}; BusPolicyRule* bus_policy_rule_new (BusPolicyRuleType type, @@ -243,49 +252,12 @@ bus_policy_unref (BusPolicy *policy) } } -static dbus_bool_t -add_list_to_client (DBusList **list, - BusClientPolicy *client) -{ - DBusList *link; - - link = _dbus_list_get_first_link (list); - while (link != NULL) - { - BusPolicyRule *rule = link->data; - link = _dbus_list_get_next_link (list, link); - - switch (rule->type) - { - case BUS_POLICY_RULE_USER: - case BUS_POLICY_RULE_GROUP: - /* These aren't per-connection policies */ - break; - - case BUS_POLICY_RULE_OWN: - case BUS_POLICY_RULE_SEND: - case BUS_POLICY_RULE_RECEIVE: - /* These are per-connection */ - if (!bus_client_policy_append_rule (client, rule)) - return FALSE; - break; - - default: - _dbus_assert_not_reached ("invalid rule"); - } - } - - return TRUE; -} - BusClientPolicy* bus_policy_create_client_policy (BusPolicy *policy, DBusConnection *connection, DBusError *error) { BusClientPolicy *client; - dbus_uid_t uid; - dbus_bool_t at_console; _dbus_assert (dbus_connection_get_is_authenticated (connection)); _DBUS_ASSERT_ERROR_IS_CLEAR (error); @@ -294,82 +266,22 @@ bus_policy_create_client_policy (BusPolicy *policy, if (client == NULL) goto nomem; - if (!add_list_to_client (&policy->default_rules, - client)) - goto nomem; - - /* we avoid the overhead of looking up user's groups - * if we don't have any group rules anyway - */ if (_dbus_hash_table_get_n_entries (policy->rules_by_gid) > 0) { - unsigned long *groups; - int n_groups; - int i; - - if (!bus_connection_get_unix_groups (connection, &groups, &n_groups, error)) + if (!bus_connection_get_unix_groups (connection, &client->groups, &client->n_groups, error)) goto failed; - - i = 0; - while (i < n_groups) - { - DBusList **list; - - list = _dbus_hash_table_lookup_uintptr (policy->rules_by_gid, - groups[i]); - - if (list != NULL) - { - if (!add_list_to_client (list, client)) - { - dbus_free (groups); - goto nomem; - } - } - - ++i; - } - - dbus_free (groups); } - if (dbus_connection_get_unix_user (connection, &uid)) + if (dbus_connection_get_unix_user (connection, &client->uid)) { - if (_dbus_hash_table_get_n_entries (policy->rules_by_uid) > 0) - { - DBusList **list; - - list = _dbus_hash_table_lookup_uintptr (policy->rules_by_uid, - uid); - - if (list != NULL) - { - if (!add_list_to_client (list, client)) - goto nomem; - } - } - - /* Add console rules */ - at_console = _dbus_unix_user_is_at_console (uid, error); + client->uid_set = TRUE; + client->at_console = _dbus_unix_user_is_at_console (client->uid, error); - if (at_console) - { - if (!add_list_to_client (&policy->at_console_true_rules, client)) - goto nomem; - } - else if (dbus_error_is_set (error) == TRUE) - { + if (dbus_error_is_set (error) == TRUE) goto failed; - } - else if (!add_list_to_client (&policy->at_console_false_rules, client)) - { - goto nomem; - } } - if (!add_list_to_client (&policy->mandatory_rules, - client)) - goto nomem; + client->policy = bus_policy_ref (policy); return client; @@ -697,13 +609,6 @@ bus_policy_merge (BusPolicy *policy, return TRUE; } -struct BusClientPolicy -{ - int refcount; - - DBusList *rules; -}; - BusClientPolicy* bus_client_policy_new (void) { @@ -728,15 +633,6 @@ bus_client_policy_ref (BusClientPolicy *policy) return policy; } -static void -rule_unref_foreach (void *data, - void *user_data) -{ - BusPolicyRule *rule = data; - - bus_policy_rule_unref (rule); -} - void bus_client_policy_unref (BusClientPolicy *policy) { @@ -746,31 +642,15 @@ bus_client_policy_unref (BusClientPolicy *policy) if (policy->refcount == 0) { - _dbus_list_foreach (&policy->rules, - rule_unref_foreach, - NULL); + if (policy->policy) + bus_policy_unref (policy->policy); - _dbus_list_clear (&policy->rules); + dbus_free (policy->groups); dbus_free (policy); } } -static dbus_bool_t -bus_client_policy_append_rule (BusClientPolicy *policy, - BusPolicyRule *rule) -{ - _dbus_verbose ("Appending rule %p with type %d to policy %p\n", - rule, rule->type, policy); - - if (!_dbus_list_append (&policy->rules, rule)) - return FALSE; - - bus_policy_rule_ref (rule); - - return TRUE; -} - typedef struct SendReceiveParams { BusRegistry *registry; dbus_bool_t requested_reply; @@ -1267,6 +1147,78 @@ check_rules_list (const DBusList *rules, return allowed; } +static dbus_bool_t +check_policy (BusClientPolicy *policy, + const RuleParams *params, + dbus_int32_t *toggles, + dbus_bool_t *log) +{ + dbus_bool_t allowed; + + if (toggles) + *toggles = 0; + + /* policy->rules is in the order the rules appeared + * in the config file, i.e. last rule that applies wins + */ + + allowed = check_rules_list (policy->policy->default_rules, + params, + toggles, + log); + + /* we avoid the overhead of looking up user's groups + * if we don't have any group rules anyway + */ + if (_dbus_hash_table_get_n_entries (policy->policy->rules_by_gid) > 0) + { + int i; + + for (i = 0; i < policy->n_groups; ++i) + { + const DBusList **list; + + list = _dbus_hash_table_lookup_uintptr (policy->policy->rules_by_gid, + policy->groups[i]); + + if (list != NULL) + allowed = check_rules_list (*list, params, toggles, log); + } + } + + if (policy->uid_set) + { + if (_dbus_hash_table_get_n_entries (policy->policy->rules_by_uid) > 0) + { + const DBusList **list; + + list = _dbus_hash_table_lookup_uintptr (policy->policy->rules_by_uid, + policy->uid); + + if (list != NULL) + allowed = check_rules_list (*list, params, toggles, log); + + if (policy->at_console) + allowed = check_rules_list (policy->policy->at_console_true_rules, + params, + toggles, + log); + else + allowed = check_rules_list (policy->policy->at_console_false_rules, + params, + toggles, + log); + } + } + + allowed = check_rules_list (policy->policy->mandatory_rules, + params, + toggles, + log); + + return allowed; +} + dbus_bool_t bus_client_policy_check_can_send (BusClientPolicy *policy, BusRegistry *registry, @@ -1284,14 +1236,9 @@ bus_client_policy_check_can_send (BusClientPolicy *policy, params.u.sr.peer = receiver; params.u.sr.message = message; - /* policy->rules is in the order the rules appeared - * in the config file, i.e. last rule that applies wins - */ - _dbus_verbose (" (policy) checking send rules\n"); - *toggles = 0; - return check_rules_list (policy->rules, ¶ms, toggles, log); + return check_policy (policy, ¶ms, toggles, log); } /* See docs on what the args mean on bus_context_check_security_policy() @@ -1318,37 +1265,20 @@ bus_client_policy_check_can_receive (BusClientPolicy *policy, addressed_recipient != proposed_recipient && dbus_message_get_destination (message) != NULL; - /* policy->rules is in the order the rules appeared - * in the config file, i.e. last rule that applies wins - */ - _dbus_verbose (" (policy) checking receive rules, eavesdropping = %d\n", params.u.sr.eavesdropping); - *toggles = 0; - return check_rules_list (policy->rules, ¶ms, toggles, NULL); -} - -static dbus_bool_t -bus_rules_check_can_own (DBusList *rules, - const DBusString *service_name) -{ - RuleParams params; - - params.type = PARAMS_OWN; - params.u.own.name = service_name; - - /* policy->rules is in the order the rules appeared - * in the config file, i.e. last rule that applies wins - */ - - return check_rules_list (rules, ¶ms, NULL, NULL); + return check_policy (policy, ¶ms, toggles, NULL); } dbus_bool_t bus_client_policy_check_can_own (BusClientPolicy *policy, const DBusString *service_name) { - return bus_rules_check_can_own (policy->rules, service_name); + RuleParams params; + params.type = PARAMS_OWN; + params.u.own.name = service_name; + + return check_policy (policy, ¶ms, NULL, NULL); } #ifdef DBUS_ENABLE_EMBEDDED_TESTS @@ -1356,6 +1286,10 @@ dbus_bool_t bus_policy_check_can_own (BusPolicy *policy, const DBusString *service_name) { - return bus_rules_check_can_own (policy->default_rules, service_name); + RuleParams params; + params.type = PARAMS_OWN; + params.u.own.name = service_name; + + return check_rules_list (policy->default_rules, ¶ms, NULL, NULL); } #endif /* DBUS_ENABLE_EMBEDDED_TESTS */ From 29409366ad2dc82f90c173a4bcc7cf10971aae6c Mon Sep 17 00:00:00 2001 From: Adrian Szyndela Date: Wed, 13 May 2020 15:21:53 +0200 Subject: [PATCH 7/9] bus/connection: API for getting list of names from connections The new function is going to be useful for checking policy in non-linear way. Now, each rule is matched against parameters, which include a sender or a receiver. Thus, each name appearing in a rule can be used in a query to the sender or the receiver. In non-linear way, we need to start with sender's or receiver's list of owned names and match each of the names to a selection of rules. Change-Id: I2854f6cc51b26ff04a9984778f899ba9656ba290 --- bus/connection.c | 9 +++++++++ bus/connection.h | 1 + 2 files changed, 10 insertions(+) diff --git a/bus/connection.c b/bus/connection.c index add1c8b8..d5cbc1c5 100644 --- a/bus/connection.c +++ b/bus/connection.c @@ -1487,6 +1487,15 @@ bus_connection_is_queued_owner_by_prefix (DBusConnection *connection, return FALSE; } +const DBusList * +bus_connection_get_owned_services_list (DBusConnection *connection) +{ + BusConnectionData *d; + + d = BUS_CONNECTION_DATA (connection); + return d->services_owned; +} + void bus_connection_add_owned_service_link (DBusConnection *connection, DBusList *link) diff --git a/bus/connection.h b/bus/connection.h index a48ab46b..acbaa8a6 100644 --- a/bus/connection.h +++ b/bus/connection.h @@ -85,6 +85,7 @@ void bus_connection_request_headers (DBusConnection *connection, /* called by policy.c */ dbus_bool_t bus_connection_is_queued_owner_by_prefix (DBusConnection *connection, const char *name_prefix); +const DBusList *bus_connection_get_owned_services_list (DBusConnection *connection); /* called by signals.c */ dbus_bool_t bus_connection_add_match_rule (DBusConnection *connection, From dc74021f5a222b1aff9525e8af47f053fcad0db3 Mon Sep 17 00:00:00 2001 From: Adrian Szyndela Date: Thu, 21 May 2020 10:32:19 +0200 Subject: [PATCH 8/9] bus/policy: use hash tables for checking policy Only for send/receive/own rules in default context. Change-Id: Iabbbfa5d582f9993b832f49193da93225c645014 --- bus/policy.c | 377 ++++++++++++++++++++++++++++++++++++++++++++++++--- bus/policy.h | 3 +- 2 files changed, 359 insertions(+), 21 deletions(-) diff --git a/bus/policy.c b/bus/policy.c index 14389fe5..3c8cd7c7 100644 --- a/bus/policy.c +++ b/bus/policy.c @@ -30,6 +30,7 @@ #include #include #include +#include struct BusClientPolicy { @@ -149,8 +150,17 @@ struct BusPolicy DBusHashTable *rules_by_gid; /**< per-GID policy rules */ DBusList *at_console_true_rules; /**< console user policy rules where at_console="true"*/ DBusList *at_console_false_rules; /**< console user policy rules where at_console="false"*/ + + DBusHashTable *default_rules_by_name; + unsigned int n_default_rules; }; +typedef struct BusPolicyRulesWithScore +{ + DBusList *rules; + int score; +} BusPolicyRulesWithScore; + static void free_rule_func (void *data, void *user_data) @@ -175,6 +185,21 @@ free_rule_list_func (void *data) dbus_free (list); } +static void +free_rule_list_with_score_func (void *data) +{ + BusPolicyRulesWithScore *rules = data; + + if (rules == NULL) + return; + + _dbus_list_foreach (&rules->rules, free_rule_func, NULL); + + _dbus_list_clear (&rules->rules); + + dbus_free (rules); +} + BusPolicy* bus_policy_new (void) { @@ -198,6 +223,12 @@ bus_policy_new (void) if (policy->rules_by_gid == NULL) goto failed; + policy->default_rules_by_name = _dbus_hash_table_new (DBUS_HASH_STRING, + NULL, + free_rule_list_with_score_func); + if (policy->default_rules_by_name == NULL) + goto failed; + return policy; failed: @@ -247,7 +278,13 @@ bus_policy_unref (BusPolicy *policy) _dbus_hash_table_unref (policy->rules_by_gid); policy->rules_by_gid = NULL; } - + + if (policy->default_rules_by_name) + { + _dbus_hash_table_unref (policy->default_rules_by_name); + policy->default_rules_by_name = NULL; + } + dbus_free (policy); } } @@ -407,12 +444,74 @@ bus_policy_allow_windows_user (BusPolicy *policy, return _dbus_windows_user_is_process_owner (windows_sid); } +static BusPolicyRulesWithScore * +get_rules_by_string (DBusHashTable *hash, + const char *key) +{ + BusPolicyRulesWithScore *rules; + + rules = _dbus_hash_table_lookup_string (hash, key); + if (rules == NULL) + { + rules = dbus_new0 (BusPolicyRulesWithScore, 1); + if (rules == NULL) + return NULL; + + if (!_dbus_hash_table_insert_string (hash, (char *)key, rules)) + { + dbus_free (rules); + return NULL; + } + } + + return rules; +} + +static const char * +get_name_from_rule (BusPolicyRule *rule) +{ + const char *name = NULL; + if (rule->type == BUS_POLICY_RULE_SEND) + name = rule->d.send.destination; + else if (rule->type == BUS_POLICY_RULE_RECEIVE) + name = rule->d.receive.origin; + else if (rule->type == BUS_POLICY_RULE_OWN) + name = rule->d.own.service_name; + + if (name == NULL) + name = ""; + + return name; +} + dbus_bool_t bus_policy_append_default_rule (BusPolicy *policy, BusPolicyRule *rule) { - if (!_dbus_list_append (&policy->default_rules, rule)) - return FALSE; + if (rule->type == BUS_POLICY_RULE_USER || rule->type == BUS_POLICY_RULE_GROUP) + { + if (!_dbus_list_append (&policy->default_rules, rule)) + return FALSE; + } + else + { + DBusList **list; + BusPolicyRulesWithScore *rules; + + rules = get_rules_by_string (policy->default_rules_by_name, + get_name_from_rule (rule)); + + if (rules == NULL) + return FALSE; + + list = &rules->rules; + + if (!_dbus_list_prepend (list, rule)) + return FALSE; + + rule->score = ++policy->n_default_rules; + rules->score = rule->score; + } bus_policy_rule_ref (rule); @@ -574,6 +673,64 @@ merge_id_hash (DBusHashTable *dest, return TRUE; } +static dbus_bool_t +merge_string_hash (unsigned int *n_rules, + unsigned int n_rules_to_absorb, + DBusHashTable *dest, + DBusHashTable *to_absorb) +{ + DBusHashIter iter; +#ifndef DBUS_DISABLE_ASSERT + unsigned cnt_rules = 0; +#endif + + _dbus_hash_iter_init (to_absorb, &iter); + while (_dbus_hash_iter_next (&iter)) + { + const char *id = _dbus_hash_iter_get_string_key (&iter); + BusPolicyRulesWithScore *to_absorb_rules =_dbus_hash_iter_get_value (&iter); + DBusList **list = &to_absorb_rules->rules; + BusPolicyRulesWithScore *target_rules = get_rules_by_string (dest, id); + DBusList **target; + DBusList *list_iter; + DBusList *target_first_link; + + if (target_rules == NULL) + return FALSE; + + target = &target_rules->rules; + target_first_link = _dbus_list_get_first_link (target); + + list_iter = _dbus_list_get_first_link (list); + while (list_iter != NULL) + { + DBusList *new_link; + BusPolicyRule *rule = list_iter->data; + + rule->score += *n_rules; + list_iter = _dbus_list_get_next_link (list, list_iter); +#ifndef DBUS_DISABLE_ASSERT + cnt_rules++; +#endif + new_link = _dbus_list_alloc_link (rule); + if (new_link == NULL) + return FALSE; + + bus_policy_rule_ref (rule); + + _dbus_list_insert_before_link (target, target_first_link, new_link); + } + + target_rules->score = to_absorb_rules->score + *n_rules; + } + + _dbus_assert (n_rules_to_absorb == cnt_rules); + + *n_rules += n_rules_to_absorb; + + return TRUE; +} + dbus_bool_t bus_policy_merge (BusPolicy *policy, BusPolicy *to_absorb) @@ -606,6 +763,12 @@ bus_policy_merge (BusPolicy *policy, to_absorb->rules_by_gid)) return FALSE; + if (!merge_string_hash (&policy->n_default_rules, + to_absorb->n_default_rules, + policy->default_rules_by_name, + to_absorb->default_rules_by_name)) + return FALSE; + return TRUE; } @@ -1101,14 +1264,19 @@ check_own_rule (const BusPolicyRule *rule, } static dbus_bool_t -check_rules_list (const DBusList *rules, - const RuleParams *params, - dbus_int32_t *toggles, - dbus_bool_t *log) +check_rules_list (const DBusList *rules, + dbus_bool_t allowed_current, + const RuleParams *params, + dbus_int32_t *toggles, + dbus_bool_t *log, + const BusPolicyRule **matched_rule, + dbus_bool_t break_on_first_match) { const DBusList *link; dbus_bool_t allowed; + allowed = allowed_current; + link = _dbus_list_get_first_link ((DBusList **)&rules); while (link != NULL) { @@ -1138,15 +1306,174 @@ check_rules_list (const DBusList *rules, *log = rule->d.send.log; if (toggles) (*toggles)++; + if (matched_rule) + *matched_rule = rule; allowed = rule->allow; _dbus_verbose (" (policy) used rule, allow now = %d\n", allowed); + + if (break_on_first_match) + break; } } return allowed; } +static int +check_rules_for_name (DBusHashTable *rules, + const char *name, + int score, + const RuleParams *params, + dbus_int32_t *toggles, + dbus_bool_t *log, + const BusPolicyRule **matched_rule) +{ + dbus_int32_t local_toggles; + dbus_bool_t local_log; + const BusPolicyRule *local_matched_rule; + const BusPolicyRulesWithScore *rules_list; + + rules_list = _dbus_hash_table_lookup_string (rules, name); + + if (rules_list == NULL || rules_list->score <= score) + return score; + + local_toggles = 0; + + check_rules_list (rules_list->rules, + FALSE, + params, + &local_toggles, + &local_log, + &local_matched_rule, + TRUE); + + if (local_toggles > 0) + { + _dbus_assert (local_matched_rule != NULL); + + if (local_matched_rule->score > score) + { + if (toggles) + *toggles += local_toggles; + if (log) + *log = local_log; + if (matched_rule) + *matched_rule = local_matched_rule; + return local_matched_rule->score; + } + } + + return score; +} + +static int +find_and_check_rules_for_name (DBusHashTable *rules, + const char *c_str, + int score, + const RuleParams *params, + dbus_int32_t *toggles, + dbus_bool_t *log, + const BusPolicyRule **matched_rule) +{ + char name[DBUS_MAXIMUM_NAME_LENGTH+2]; + int pos = strlen(c_str); + + _dbus_assert (pos <= DBUS_MAXIMUM_NAME_LENGTH); + + strncpy (name, c_str, sizeof(name)-1); + + /* + * To check 'prefix' rules we not only need to check a name, + * but also every prefix of the name. For example, + * if name is 'foo.bar.baz.qux' we need to check rules for: + * - foo.bar.baz.qux + * - foo.bar.baz + * - foo.bar + * - foo + */ + while (pos > 0) + { + score = check_rules_for_name (rules, name, + score, params, + toggles, log, + matched_rule); + + /* strip the last component for next iteration */ + while (pos > 0 && name[pos] != '.') + pos--; + + name[pos] = 0; + } + + return score; +} + +static dbus_bool_t +find_and_check_rules (DBusHashTable *rules, + const RuleParams *params, + dbus_int32_t *toggles, + dbus_bool_t *log) +{ + dbus_bool_t allowed; + const DBusList *services = NULL; + const BusPolicyRule *matched_rule = NULL; + int score = 0; + + allowed = FALSE; + + if (params->type == PARAMS_SEND || params->type == PARAMS_RECEIVE) + { + if (params->u.sr.peer != NULL) + { + DBusList *link; + + services = bus_connection_get_owned_services_list (params->u.sr.peer); + + link = _dbus_list_get_first_link ((DBusList **)&services); + while (link != NULL) + { + const char *name = bus_service_get_name (link->data); + + link = _dbus_list_get_next_link ((DBusList **)&services, link); + + /* skip unique id names */ + if (name[0] == ':') + continue; + + score = find_and_check_rules_for_name (rules, name, score, + params, toggles, log, &matched_rule); + } + } + else + { + /* NULL peer means dbus-daemon or activation */ + const char *rule_target_name; + + if (params->type == PARAMS_SEND) + rule_target_name = dbus_message_get_destination (params->u.sr.message); + else if (params->type == PARAMS_RECEIVE) + rule_target_name = dbus_message_get_sender (params->u.sr.message); + + if (rule_target_name != NULL) + score = find_and_check_rules_for_name (rules, rule_target_name, score, + params, toggles, log, &matched_rule); + } + } + else + score = find_and_check_rules_for_name (rules, _dbus_string_get_const_data(params->u.own.name), + score, params, toggles, log, &matched_rule); + + /* check also wildcard rules */ + check_rules_for_name (rules, "", score, params, toggles, log, &matched_rule); + + if (matched_rule) + allowed = matched_rule->allow; + + return allowed; +} + static dbus_bool_t check_policy (BusClientPolicy *policy, const RuleParams *params, @@ -1158,14 +1485,12 @@ check_policy (BusClientPolicy *policy, if (toggles) *toggles = 0; - /* policy->rules is in the order the rules appeared - * in the config file, i.e. last rule that applies wins - */ + allowed = find_and_check_rules (policy->policy->default_rules_by_name, + params, + toggles, + log); - allowed = check_rules_list (policy->policy->default_rules, - params, - toggles, - log); + _dbus_verbose("checked, allow now = %d\n", allowed); /* we avoid the overhead of looking up user's groups * if we don't have any group rules anyway @@ -1182,7 +1507,7 @@ check_policy (BusClientPolicy *policy, policy->groups[i]); if (list != NULL) - allowed = check_rules_list (*list, params, toggles, log); + allowed = check_rules_list (*list, allowed, params, toggles, log, NULL, FALSE); } } @@ -1196,25 +1521,34 @@ check_policy (BusClientPolicy *policy, policy->uid); if (list != NULL) - allowed = check_rules_list (*list, params, toggles, log); + allowed = check_rules_list (*list, allowed, params, toggles, log, NULL, FALSE); if (policy->at_console) allowed = check_rules_list (policy->policy->at_console_true_rules, + allowed, params, toggles, - log); + log, + NULL, + FALSE); else allowed = check_rules_list (policy->policy->at_console_false_rules, + allowed, params, toggles, - log); + log, + NULL, + FALSE); } } allowed = check_rules_list (policy->policy->mandatory_rules, + allowed, params, toggles, - log); + log, + NULL, + FALSE); return allowed; } @@ -1290,6 +1624,9 @@ bus_policy_check_can_own (BusPolicy *policy, params.type = PARAMS_OWN; params.u.own.name = service_name; - return check_rules_list (policy->default_rules, ¶ms, NULL, NULL); + return find_and_check_rules (policy->default_rules_by_name, + ¶ms, + NULL, + NULL); } #endif /* DBUS_ENABLE_EMBEDDED_TESTS */ diff --git a/bus/policy.h b/bus/policy.h index 82212ebe..91c2bc6c 100644 --- a/bus/policy.h +++ b/bus/policy.h @@ -57,7 +57,8 @@ struct BusPolicyRule BusPolicyRuleType type; unsigned int allow : 1; /**< #TRUE if this allows, #FALSE if it denies */ - + unsigned int score : 20; /**< for keeping the importance of the rule */ + union { struct From 626cb5cf4e944aa0710f1ccc6e950d76cbaafa29 Mon Sep 17 00:00:00 2001 From: Adrian Szyndela Date: Wed, 27 Apr 2022 14:10:28 +0200 Subject: [PATCH 9/9] bus/policy: separate prefix rules in default context To handle prefix rules stored with all other rules in the default context we need to match each prefix of each name against policy rules. That's because names are looked up in the hash tables, so we can miss a prefix rule for a prefix of the name. However, if prefix rules are separated from non-prefix rules, we can simply check them all once for each name, and also check hash tables once for each name. This is what this commit changes. It separates prefix rules from non-prefix rules, and handles them in sequence. This gives a little boost, especially if there are no prefix rules. Change-Id: Ifade906d35af96a973920ce9c2f6065f5b9b549e --- bus/policy.c | 147 ++++++++++++++++++++++++++++++--------------------- 1 file changed, 86 insertions(+), 61 deletions(-) diff --git a/bus/policy.c b/bus/policy.c index 3c8cd7c7..15532b90 100644 --- a/bus/policy.c +++ b/bus/policy.c @@ -152,6 +152,7 @@ struct BusPolicy DBusList *at_console_false_rules; /**< console user policy rules where at_console="false"*/ DBusHashTable *default_rules_by_name; + DBusList *default_prefix_rules; unsigned int n_default_rules; }; @@ -285,6 +286,9 @@ bus_policy_unref (BusPolicy *policy) policy->default_rules_by_name = NULL; } + _dbus_list_foreach (&policy->default_prefix_rules, free_rule_func, NULL); + _dbus_list_clear (&policy->default_prefix_rules); + dbus_free (policy); } } @@ -484,6 +488,17 @@ get_name_from_rule (BusPolicyRule *rule) return name; } +static dbus_bool_t +is_prefix_rule (BusPolicyRule *rule) +{ + if (rule->type == BUS_POLICY_RULE_SEND && rule->d.send.destination_is_prefix) + return TRUE; + if (rule->type == BUS_POLICY_RULE_OWN && rule->d.own.prefix) + return TRUE; + + return FALSE; +} + dbus_bool_t bus_policy_append_default_rule (BusPolicy *policy, BusPolicyRule *rule) @@ -495,22 +510,31 @@ bus_policy_append_default_rule (BusPolicy *policy, } else { - DBusList **list; - BusPolicyRulesWithScore *rules; - - rules = get_rules_by_string (policy->default_rules_by_name, - get_name_from_rule (rule)); - - if (rules == NULL) - return FALSE; - - list = &rules->rules; - - if (!_dbus_list_prepend (list, rule)) - return FALSE; - rule->score = ++policy->n_default_rules; - rules->score = rule->score; + + if (is_prefix_rule (rule)) + { + if (!_dbus_list_append (&policy->default_prefix_rules, rule)) + return FALSE; + } + else + { + DBusList **list; + BusPolicyRulesWithScore *rules; + + rules = get_rules_by_string (policy->default_rules_by_name, + get_name_from_rule (rule)); + + if (rules == NULL) + return FALSE; + + list = &rules->rules; + + if (!_dbus_list_prepend (list, rule)) + return FALSE; + + rules->score = rule->score; + } } bus_policy_rule_ref (rule); @@ -769,6 +793,10 @@ bus_policy_merge (BusPolicy *policy, to_absorb->default_rules_by_name)) return FALSE; + if (!append_copy_of_policy_list (&policy->default_prefix_rules, + &to_absorb->default_prefix_rules)) + return FALSE; + return TRUE; } @@ -1321,33 +1349,27 @@ check_rules_list (const DBusList *rules, } static int -check_rules_for_name (DBusHashTable *rules, - const char *name, - int score, - const RuleParams *params, - dbus_int32_t *toggles, - dbus_bool_t *log, - const BusPolicyRule **matched_rule) +check_rules_list_with_score (DBusList *rules, + int score, + const RuleParams *params, + dbus_int32_t *toggles, + dbus_bool_t *log, + const BusPolicyRule **matched_rule, + dbus_bool_t break_on_first_match) { dbus_int32_t local_toggles; dbus_bool_t local_log; const BusPolicyRule *local_matched_rule; - const BusPolicyRulesWithScore *rules_list; - - rules_list = _dbus_hash_table_lookup_string (rules, name); - - if (rules_list == NULL || rules_list->score <= score) - return score; local_toggles = 0; - check_rules_list (rules_list->rules, + check_rules_list (rules, FALSE, params, &local_toggles, &local_log, &local_matched_rule, - TRUE); + break_on_first_match); if (local_toggles > 0) { @@ -1368,8 +1390,28 @@ check_rules_for_name (DBusHashTable *rules, return score; } +static int +check_rules_for_name (DBusHashTable *rules, + const char *name, + int score, + const RuleParams *params, + dbus_int32_t *toggles, + dbus_bool_t *log, + const BusPolicyRule **matched_rule) +{ + const BusPolicyRulesWithScore *rules_list; + + rules_list = _dbus_hash_table_lookup_string (rules, name); + + if (rules_list == NULL || rules_list->score <= score) + return score; + + return check_rules_list_with_score (rules_list->rules, score, params, toggles, log, matched_rule, TRUE); +} + static int find_and_check_rules_for_name (DBusHashTable *rules, + DBusList *prefix_rules, const char *c_str, int score, const RuleParams *params, @@ -1377,41 +1419,22 @@ find_and_check_rules_for_name (DBusHashTable *rules, dbus_bool_t *log, const BusPolicyRule **matched_rule) { - char name[DBUS_MAXIMUM_NAME_LENGTH+2]; - int pos = strlen(c_str); + score = check_rules_for_name (rules, c_str, + score, params, + toggles, log, + matched_rule); - _dbus_assert (pos <= DBUS_MAXIMUM_NAME_LENGTH); - - strncpy (name, c_str, sizeof(name)-1); - - /* - * To check 'prefix' rules we not only need to check a name, - * but also every prefix of the name. For example, - * if name is 'foo.bar.baz.qux' we need to check rules for: - * - foo.bar.baz.qux - * - foo.bar.baz - * - foo.bar - * - foo - */ - while (pos > 0) - { - score = check_rules_for_name (rules, name, - score, params, - toggles, log, - matched_rule); - - /* strip the last component for next iteration */ - while (pos > 0 && name[pos] != '.') - pos--; - - name[pos] = 0; - } + score = check_rules_list_with_score (prefix_rules, + score, params, + toggles, log, + matched_rule, FALSE); return score; } static dbus_bool_t find_and_check_rules (DBusHashTable *rules, + DBusList *prefix_rules, const RuleParams *params, dbus_int32_t *toggles, dbus_bool_t *log) @@ -1442,7 +1465,7 @@ find_and_check_rules (DBusHashTable *rules, if (name[0] == ':') continue; - score = find_and_check_rules_for_name (rules, name, score, + score = find_and_check_rules_for_name (rules, prefix_rules, name, score, params, toggles, log, &matched_rule); } } @@ -1457,12 +1480,12 @@ find_and_check_rules (DBusHashTable *rules, rule_target_name = dbus_message_get_sender (params->u.sr.message); if (rule_target_name != NULL) - score = find_and_check_rules_for_name (rules, rule_target_name, score, + score = find_and_check_rules_for_name (rules, prefix_rules, rule_target_name, score, params, toggles, log, &matched_rule); } } else - score = find_and_check_rules_for_name (rules, _dbus_string_get_const_data(params->u.own.name), + score = find_and_check_rules_for_name (rules, prefix_rules, _dbus_string_get_const_data(params->u.own.name), score, params, toggles, log, &matched_rule); /* check also wildcard rules */ @@ -1486,6 +1509,7 @@ check_policy (BusClientPolicy *policy, *toggles = 0; allowed = find_and_check_rules (policy->policy->default_rules_by_name, + policy->policy->default_prefix_rules, params, toggles, log); @@ -1625,6 +1649,7 @@ bus_policy_check_can_own (BusPolicy *policy, params.u.own.name = service_name; return find_and_check_rules (policy->default_rules_by_name, + policy->default_prefix_rules, ¶ms, NULL, NULL);