From a9bef001b184f2eb6834e70def388ecf943c8668 Mon Sep 17 00:00:00 2001 From: Aleksy Barcz Date: Fri, 6 Apr 2018 11:34:03 +0200 Subject: [PATCH] refactored parsing policy Changed unnecessarily recursive code into code that reflects the actual configuration syntax, for better readability and maintainability. Removed unused __tag_state and __attr variables. Fixed potential future bug: ptree api doesn't guarantee that will be the first child, so we shouldn't assume it will be first. Fixed bug, when policy at_console was parsed as the last seen policy type (or as unitialized type, if it was the first policy read). Added test case to assure that at_console policies will always be ignored. Change-Id: I7e28262ee4837547e10ca1b33f7d5a90166f4667 --- src/internal/naive_policy_db.cpp | 3 + src/internal/policy.cpp | 209 +++++++++++------------ src/internal/policy.hpp | 28 +-- tests/default_deny/system.d/ownerships.test.conf | 5 + 4 files changed, 118 insertions(+), 127 deletions(-) diff --git a/src/internal/naive_policy_db.cpp b/src/internal/naive_policy_db.cpp index be88fa0..2437105 100755 --- a/src/internal/naive_policy_db.cpp +++ b/src/internal/naive_policy_db.cpp @@ -180,6 +180,9 @@ void NaivePolicyDb::addItem(NaivePolicyDb::PolicyTypeSetSR& set, case PolicyType::GROUP: set.group[policy_type_value.group].addItem(item); break; + case PolicyType::NONE: + assert(false); + break; } } diff --git a/src/internal/policy.cpp b/src/internal/policy.cpp index f3937df..f40407d 100755 --- a/src/internal/policy.cpp +++ b/src/internal/policy.cpp @@ -10,9 +10,11 @@ #include #include #include +#include #include using namespace ldp_xml_parser; +using boost::property_tree::ptree; static const char* message_type[] = { "ANY", "METHOD_CALL", "METHOD_RETURN", "ERROR", "SIGNAL"}; static const char* message_dir[] = { "ANY", "SEND", "RECEIVE"}; @@ -46,9 +48,6 @@ static inline const char* __decision_to_str(Decision dec) { return message_decision[static_cast(dec)]; } -DbAdapter::DbAdapter() : __attr(false), __tag_state(NONE) { -} - static uid_t convertToUid(const char* user) { long val = -1; errno = 0; @@ -85,125 +84,123 @@ static gid_t convertToGid(const char* group) { return gg->gr_gid; } -static bool field_has(const boost::property_tree::ptree::value_type& v, const std::string& substr) { +static bool field_has(const ptree::value_type& v, const std::string& substr) { return (v.first.find(substr) != std::string::npos); } -void DbAdapter::updateDecision(const boost::property_tree::ptree::value_type& v, - PolicyType& policy_type, - PolicyTypeValue& policy_type_value, - state& t, - bool& attr) { - const char* value = NULL; - if (v.first == "allow" && t == POLICY) { - __builder.reset(); - __builder.addDecision(Decision::ALLOW); - t = ALLOW_DENY_CHECK; - attr = false; - } else if (v.first == "deny" && t == POLICY) { - __builder.reset(); - __builder.addDecision(Decision::DENY); - t = ALLOW_DENY_CHECK; - attr = false; - } else if (v.first == "check" && t == POLICY) { - __builder.reset(); - __builder.addDecision(Decision::CHECK); - t = ALLOW_DENY_CHECK; - attr = false; - } else if (v.first == "") { - attr = true; - } else if (attr && t == POLICY) { - if (v.second.data() != "*") - value = v.second.data().c_str(); - - if (value) { - if (v.first == "context") { - if (std::strcmp(value, "mandatory") == 0 ) { - policy_type = PolicyType::CONTEXT; - policy_type_value.context = ContextType::MANDATORY; - } else if (std::strcmp(value, "default") == 0) { - policy_type = PolicyType::CONTEXT; - policy_type_value.context = ContextType::DEFAULT; - } - } else if (v.first == "user") { - policy_type = PolicyType::USER; - policy_type_value.user = convertToUid(value); - } else if (v.first == "group") { - policy_type = PolicyType::GROUP; - policy_type_value.group = convertToGid(value); - } - } - } else if (attr && t == ALLOW_DENY_CHECK) { - if (v.second.data() != "*") - value = v.second.data().c_str(); - - if (field_has(v, "send_")) { - __builder.addDirection(MessageDirection::SEND); - } else if (field_has(v, "receive_")) { - __builder.addDirection(MessageDirection::RECEIVE); - } else if (v.first == "own") { - __builder.addOwner(value); - __builder.setPrefix(false); - } else if (v.first == "own_prefix") { - __builder.addOwner(value); - __builder.setPrefix(true); - } else if (v.first == "privilege") { - __builder.addPrivilege(value); +void DbAdapter::updateDb(bool bus, const ptree& pt, std::vector& incl_dirs) { + const auto& busconfig = pt.get_child("busconfig"); + for (const auto& x : busconfig) { + if (x.first == "policy") { + parsePolicy(bus, x.second); + } else if (x.first == "includedir") { + incl_dirs.push_back(x.second.data()); } - - if (field_has(v, "_destination")) - __builder.addName(value); - else if (field_has(v, "_sender")) - __builder.addName(value); - else if (field_has(v, "_path")) - __builder.addPath(value); - else if (field_has(v, "_interface")) - __builder.addInterface(value); - else if (field_has(v, "_member")) - __builder.addMember(value); - else if (field_has(v, "_type")) - __builder.addMessageType(__str_to_message_type(value)); - } else { - attr = false; - t = NONE; } } -void DbAdapter::xmlTraversal(bool bus, - const boost::property_tree::ptree& pt, - DbAdapter::state tag, - PolicyType& policy_type, - PolicyTypeValue& policy_type_value, - bool attr, - int level) { - static const int Q_XML_MAX_LEVEL = 10; - if (level < Q_XML_MAX_LEVEL) { - for(const auto& v : pt) { - if (v.first == "") { continue; } - state t = tag; - updateDecision(v, policy_type, policy_type_value, t, attr); - xmlTraversal(bus, v.second, t, policy_type, policy_type_value, attr, level + 1); +static const std::map str2decision{ + {"allow", Decision::ALLOW}, + {"deny", Decision::DENY}, + {"check", Decision::CHECK}}; + +void DbAdapter::parsePolicy(bool bus, const ptree& pt) +{ + PolicyType policy_type = PolicyType::NONE; + PolicyTypeValue policy_type_value; + + // we must read policy attrs before parsing rules, as ptree API + // doesn't guarantee that will occur before other + // child elements + const auto xmlattr = pt.get_child_optional(""); + if (xmlattr) { + for (const auto& va : *xmlattr) { + parsePolicyAttribute(va, policy_type, policy_type_value); } - if (!pt.empty() && level > 1) + } + if (policy_type == PolicyType::NONE) { + return; + } + for (const auto& v : pt) { + if (str2decision.find(v.first) != str2decision.end()) { + parseRule(v); __builder.generateItem(policy_checker().db(bus), policy_type, policy_type_value); + } } } -void DbAdapter::updateDb(bool bus, boost::property_tree::ptree& xmlTree, std::vector& incl_dirs) { - const auto& children = xmlTree.get_child("busconfig"); - PolicyType policy_type; - PolicyTypeValue policy_type_value; - for (const auto& x : children) { - if (x.first == "policy") { - __tag_state = POLICY; - __attr = false; - xmlTraversal(bus, x.second, POLICY, policy_type, policy_type_value); - } else if (x.first == "includedir") { - incl_dirs.push_back(x.second.data()); +void DbAdapter::parsePolicyAttribute(const ptree::value_type& v, PolicyType& policy_type, PolicyTypeValue& policy_type_value) +{ + //possible values (from dbus-daemon man): + // context="(default|mandatory)" + // at_console="(true|false)" (deprecated and not used on targets, we ignore it) + // user="username or userid" + // group="group name or gid" + const char* value = v.second.data().c_str(); + if (v.first == "context") { + if (std::strcmp(value, "mandatory") == 0 ) { + policy_type = PolicyType::CONTEXT; + policy_type_value.context = ContextType::MANDATORY; + } else if (std::strcmp(value, "default") == 0) { + policy_type = PolicyType::CONTEXT; + policy_type_value.context = ContextType::DEFAULT; + } + } else if (v.first == "user") { + policy_type = PolicyType::USER; + policy_type_value.user = convertToUid(value); + } else if (v.first == "group") { + policy_type = PolicyType::GROUP; + policy_type_value.group = convertToGid(value); + } +} + +void DbAdapter::parseRule(const ptree::value_type& v) +{ + __builder.reset(); + __builder.addDecision(str2decision.at(v.first)); + + const auto xmlattr = v.second.get_child_optional(""); + if (xmlattr) { + for (const auto& va : *xmlattr) { + parseRuleAttribute(va); } } } +void DbAdapter::parseRuleAttribute(const ptree::value_type& v) +{ + const char* value = nullptr; + if (v.second.data() != "*") + value = v.second.data().c_str(); + + if (field_has(v, "send_")) { + __builder.addDirection(MessageDirection::SEND); + } else if (field_has(v, "receive_")) { + __builder.addDirection(MessageDirection::RECEIVE); + } else if (v.first == "own") { + __builder.addOwner(value); + __builder.setPrefix(false); + } else if (v.first == "own_prefix") { + __builder.addOwner(value); + __builder.setPrefix(true); + } else if (v.first == "privilege") { + __builder.addPrivilege(value); + } + + if (field_has(v, "_destination")) + __builder.addName(value); + else if (field_has(v, "_sender")) + __builder.addName(value); + else if (field_has(v, "_path")) + __builder.addPath(value); + else if (field_has(v, "_interface")) + __builder.addInterface(value); + else if (field_has(v, "_member")) + __builder.addMember(value); + else if (field_has(v, "_type")) + __builder.addMessageType(__str_to_message_type(value)); +} + void DbAdapter::updateGroupDb(bool bus) { policy_checker().db(bus).updateSupGroup(); diff --git a/src/internal/policy.hpp b/src/internal/policy.hpp index 2fb720e..428bb44 100755 --- a/src/internal/policy.hpp +++ b/src/internal/policy.hpp @@ -60,6 +60,7 @@ namespace ldp_xml_parser /** Policy type, either context, user or group */ enum class PolicyType : uint8_t { + NONE, CONTEXT, USER, GROUP @@ -217,29 +218,14 @@ namespace ldp_xml_parser /** Adapter which allows to access policy db. Contains references to system and session db. */ class DbAdapter { private: - enum state { - NONE, - POLICY, - ALLOW_DENY_CHECK - }; - bool __attr; - state __tag_state; ItemBuilder __builder; - void updateDecision(const boost::property_tree::ptree::value_type& v, - PolicyType& policy_type, - PolicyTypeValue& policy_type_value, - state& tag, - bool& attr); - void xmlTraversal(bool bus, - const boost::property_tree::ptree& pt, - state tag, - PolicyType& policy_type, - PolicyTypeValue& policy_type_value, - bool attr = false, - int level = 0); + void parsePolicy(bool bus, const boost::property_tree::ptree& pt); + void parsePolicyAttribute(const boost::property_tree::ptree::value_type& v, PolicyType&, PolicyTypeValue&); + void parseRule(const boost::property_tree::ptree::value_type& v); + void parseRuleAttribute(const boost::property_tree::ptree::value_type& v); + public: - DbAdapter(); - void updateDb(bool bus, boost::property_tree::ptree& xmlTree, std::vector& incl_dirs); + void updateDb(bool bus, const boost::property_tree::ptree& xmlTree, std::vector& incl_dirs); void updateGroupDb(bool bus); }; } diff --git a/tests/default_deny/system.d/ownerships.test.conf b/tests/default_deny/system.d/ownerships.test.conf index 8ebd47f..c3527a5 100644 --- a/tests/default_deny/system.d/ownerships.test.conf +++ b/tests/default_deny/system.d/ownerships.test.conf @@ -76,5 +76,10 @@ + + + + + -- 2.7.4