diff --git a/CHANGES b/CHANGES index 2594c525..1366fab8 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,9 @@ v3.x.y - YYYY-MMM-DD (to be released) ------------------------------------- + - Added the basics for supporting better error/warning handling while + loading configurations. + [@zimmerle] - IMPORTANT: SecDefaultAction behaves changing: SecDefaultAction specified on a child configuration will overwrite the ones specified on the parent; Previously it was concatenating. diff --git a/headers/modsecurity/modsecurity.h b/headers/modsecurity/modsecurity.h index 54a3c3ea..9739c5bc 100644 --- a/headers/modsecurity/modsecurity.h +++ b/headers/modsecurity/modsecurity.h @@ -78,6 +78,7 @@ #include #include #include +#include #endif @@ -95,6 +96,10 @@ namespace modsecurity { */ using ModSecString = std::string; + using RulesErrors = std::vector>; + using RulesWarnings = std::vector>; + + using RuleId = int64_t; /** diff --git a/headers/modsecurity/rules.h b/headers/modsecurity/rules.h index df122e88..eeebd29d 100644 --- a/headers/modsecurity/rules.h +++ b/headers/modsecurity/rules.h @@ -40,33 +40,35 @@ class Transformation; } } + class Rules { public: - void dump() const; + using container=std::vector>; + using iterator=typename container::iterator; + using const_iterator=typename container::const_iterator; - int append(Rules *from, - const std::vector &ids, - std::ostringstream *err); + int append(Rules *from); bool insert(const std::shared_ptr &rule); - bool insert(std::shared_ptr rule, - const std::vector *ids, - std::ostringstream *err); - size_t size() const; std::shared_ptr operator[](int index) const; std::shared_ptr at(int index) const; - void fixDefaultActions(); + void fixDefaultActions(RulesWarnings *warnings, RulesErrors *errors); std::vector > m_defaultActions; std::vector > m_defaultTransformations; - std::vector > m_rules; void dump(); + inline iterator begin() noexcept { return m_rules.begin(); } + inline const_iterator cbegin() const noexcept { return m_rules.cbegin(); } + inline iterator end() noexcept { return m_rules.end(); } + inline const_iterator cend() const noexcept { return m_rules.cend(); } + private: + container m_rules; }; diff --git a/headers/modsecurity/rules_set.h b/headers/modsecurity/rules_set.h index e5870333..77324155 100644 --- a/headers/modsecurity/rules_set.h +++ b/headers/modsecurity/rules_set.h @@ -22,6 +22,7 @@ #include #include #include +#include #endif @@ -42,8 +43,6 @@ namespace Parser { class Driver; } - - /** @ingroup ModSecurity_CPP_API */ class RulesSet : public RulesSetProperties { public: @@ -68,12 +67,13 @@ class RulesSet : public RulesSetProperties { int load(const char *rules); int load(const char *rules, const std::string &ref); - void dump() const; + void dump(); int merge(Parser::Driver *driver); int merge(RulesSet *rules); int evaluate(int phase, Transaction *transaction); + std::string getParserError(); void debug(int level, const std::string &id, const std::string &uri, @@ -81,6 +81,7 @@ class RulesSet : public RulesSetProperties { RulesSetPhases m_rulesSetPhases; private: + bool containsDuplicatedIds(RulesWarnings *warnings, RulesErrors *errors); #ifndef NO_LOGS uint8_t m_secmarker_skipped; #endif diff --git a/headers/modsecurity/rules_set_phases.h b/headers/modsecurity/rules_set_phases.h index 17844ce0..2b9b67c9 100644 --- a/headers/modsecurity/rules_set_phases.h +++ b/headers/modsecurity/rules_set_phases.h @@ -23,6 +23,7 @@ #include #include #include +#include #endif @@ -42,18 +43,33 @@ class Driver; /** @ingroup ModSecurity_CPP_API */ class RulesSetPhases { public: + using container = std::array; + using iterator = typename container::iterator; + using const_iterator = typename container::const_iterator; - bool insert(std::shared_ptr rule); + void insert(std::shared_ptr rule); + void append(RulesSetPhases *from); int append(RulesSetPhases *from, std::ostringstream *err); - void dump() const; + void dump(); Rules *operator[](int index); Rules *at(int index); + static size_t size() { return modsecurity::Phases::NUMBER_OF_PHASES; } + + void fixDefaultActions(RulesWarnings *warnings, RulesErrors *errors) { + for (auto &phase : m_rulesAtPhase) { + phase.fixDefaultActions(warnings, errors); + } + } + + inline iterator begin() noexcept { return m_rulesAtPhase.begin(); } + inline const_iterator cbegin() const noexcept { return m_rulesAtPhase.cbegin(); } + inline iterator end() noexcept { return m_rulesAtPhase.end(); } + inline const_iterator cend() const noexcept { return m_rulesAtPhase.cend(); } private: - Rules m_rulesAtPhase[8]; - + container m_rulesAtPhase; }; diff --git a/headers/modsecurity/rules_set_properties.h b/headers/modsecurity/rules_set_properties.h index 7fc40178..e90b1ec8 100644 --- a/headers/modsecurity/rules_set_properties.h +++ b/headers/modsecurity/rules_set_properties.h @@ -330,7 +330,9 @@ class RulesSetProperties { static int mergeProperties(RulesSetProperties *from, - RulesSetProperties *to, std::ostringstream *err) { + RulesSetProperties *to, + RulesWarnings *warning, + RulesErrors *error) { merge_ruleengine_value(to->m_secRuleEngine, from->m_secRuleEngine, PropertyNotSetRuleEngine); @@ -401,10 +403,10 @@ class RulesSetProperties { } if (to->m_auditLog) { - std::string error; - to->m_auditLog->merge(from->m_auditLog, &error); - if (error.size() > 0) { - *err << error; + std::string error_; + to->m_auditLog->merge(from->m_auditLog, &error_); + if (error_.size() > 0) { + error->push_back(std::unique_ptr(new std::string(error_))); return -1; } } @@ -412,12 +414,12 @@ class RulesSetProperties { if (from->m_debugLog && to->m_debugLog && from->m_debugLog->isLogFileSet()) { if (to->m_debugLog->isLogFileSet() == false) { - std::string error; + std::string error_; to->m_debugLog->setDebugLogFile( from->m_debugLog->getDebugLogFile(), - &error); - if (error.size() > 0) { - *err << error; + &error_); + if (error_.size() > 0) { + error->push_back(std::unique_ptr(new std::string(error_))); return -1; } } diff --git a/src/parser/driver.cc b/src/parser/driver.cc index 66764cd7..66d44b4b 100644 --- a/src/parser/driver.cc +++ b/src/parser/driver.cc @@ -176,18 +176,6 @@ int Driver::addSecRule(std::unique_ptr r) { return false; } - for (int i = 0; i < modsecurity::Phases::NUMBER_OF_PHASES; i++) { - Rules *rules = m_rulesSetPhases[i]; - for (int j = 0; j < rules->size(); j++) { - RuleWithOperator *lr = dynamic_cast(rules->at(j).get()); - if (lr && lr->getId() == rule->getId()) { - m_parserError << "Rule id: " << std::to_string(rule->getId()) \ - << " is duplicated" << std::endl; - return false; - } - } - } - m_lastRule = rule.get(); m_rulesSetPhases.insert(rule); diff --git a/src/rules.cc b/src/rules.cc index 44f98106..0665231a 100644 --- a/src/rules.cc +++ b/src/rules.cc @@ -20,38 +20,23 @@ namespace modsecurity { -int Rules::append(Rules *from, const std::vector &ids, std::ostringstream *err) { - size_t j = 0; - for (; j < from->size(); j++) { - RuleWithActions *rule = dynamic_cast(from->at(j).get()); - if (rule && std::binary_search(ids.begin(), ids.end(), rule->getId())) { - if (err != NULL) { - *err << "Rule id: " << std::to_string(rule->getId()) \ - << " is duplicated" << std::endl; - } - return -1; +int Rules::append(Rules *from) { + m_rules.insert(m_rules.end(), from->m_rules.begin(), from->m_rules.end()); + if (!from->m_defaultActions.empty() || !from->m_defaultTransformations.empty()) { + m_defaultActions.clear(); + m_defaultTransformations.clear(); + for (auto &a : from->m_defaultActions) { + m_defaultActions.push_back(a); + } + for (auto &a : from->m_defaultTransformations) { + m_defaultTransformations.push_back(a); } } - m_rules.insert(m_rules.end(), from->m_rules.begin(), from->m_rules.end()); - return j; + return from->size(); } bool Rules::insert(const std::shared_ptr &rule) { - return insert(rule, nullptr, nullptr); -} - - -bool Rules::insert(std::shared_ptr rule, const std::vector *ids, std::ostringstream *err) { - RuleWithActions*r = dynamic_cast(rule.get()); - if (r && ids != nullptr - && std::binary_search(ids->begin(), ids->end(), r->getId())) { - if (err != NULL) { - *err << "Rule id: " << std::to_string(r->getId()) \ - << " is duplicated" << std::endl; - } - return false; - } m_rules.push_back(rule); return true; } @@ -72,7 +57,7 @@ std::shared_ptr Rules::at(int index) const { } -void Rules::dump() const { +void Rules::dump() { for (int j = 0; j < m_rules.size(); j++) { std::cout << " Rule ID: " << m_rules.at(j)->getReference(); std::cout << "--" << m_rules.at(j) << std::endl; diff --git a/src/rules_set.cc b/src/rules_set.cc index 4295e1b2..8ec964ce 100644 --- a/src/rules_set.cc +++ b/src/rules_set.cc @@ -18,6 +18,7 @@ #include #include #include +#include #include "modsecurity/rules_set.h" #include "src/rule_marker.h" @@ -32,7 +33,7 @@ using modsecurity::Utils::HttpsClient; namespace modsecurity { - void Rules::fixDefaultActions() { + void Rules::fixDefaultActions(RulesWarnings *warnings, RulesErrors *errors) { for (size_t i = 0; i < m_rules.size(); i++) { auto &rule = m_rules[i]; @@ -142,6 +143,52 @@ int RulesSet::load(const char *plainRules) { } +bool RulesSet::containsDuplicatedIds(RulesWarnings *warning, RulesErrors *error) { + std::multimap allIds; + std::set duplicatedIds; + for (auto &rules : m_rulesSetPhases) { + for (auto &j : rules) { + RuleWithActions *rule = dynamic_cast(j.get()); + if (rule) { + allIds.insert(std::pair(rule->getId(), rule)); + } + } + } + + auto id = allIds.begin(); + auto next = id; + if (id != allIds.end()) { + next++; + } + while (id != allIds.end() && next != allIds.end()) { + if (id->first == next->first) { + duplicatedIds.insert(id->first); + } + id++; + next++; + } + + for (auto i : duplicatedIds) { + auto ret = allIds.equal_range(i); + std::stringstream ss; + + ss << "There are multiple rules defined with "; + ss << "same id. The ID " << i << " is defined at: " << std::endl; + for (auto it = ret.first; it != ret.second; ++it) { + auto rule = it->second; + ss << " " << *rule->getFileName() << ":"; + ss << rule->getLineNumber() << std::endl; + } + + std::unique_ptr e(new std::string(ss.str())); + error->push_back(std::move(e)); + } + + return !duplicatedIds.empty(); +} + + + std::string RulesSet::getParserError() { return this->m_parserError.str(); } @@ -265,13 +312,24 @@ int RulesSet::evaluate(int phase, Transaction *t) { int RulesSet::merge(Driver *from) { int amount_of_rules = 0; + RulesErrors errors; + RulesWarnings warnings; - amount_of_rules = m_rulesSetPhases.append(&from->m_rulesSetPhases, - &m_parserError); + m_rulesSetPhases.append(&from->m_rulesSetPhases); mergeProperties( dynamic_cast(from), dynamic_cast(this), - &m_parserError); + &warnings, &errors); + + m_rulesSetPhases.fixDefaultActions(&warnings, &errors); + containsDuplicatedIds(&warnings, &errors); + + if (!errors.empty()) { + for (auto &i : errors) { + m_parserError << "*** Error: " << *i << std::endl; + } + return -1; + } return amount_of_rules; } @@ -279,13 +337,24 @@ int RulesSet::merge(Driver *from) { int RulesSet::merge(RulesSet *from) { int amount_of_rules = 0; + RulesErrors errors; + RulesWarnings warnings; - amount_of_rules = m_rulesSetPhases.append(&from->m_rulesSetPhases, - &m_parserError); + m_rulesSetPhases.append(&from->m_rulesSetPhases); mergeProperties( dynamic_cast(from), dynamic_cast(this), - &m_parserError); + &warnings, &errors); + + m_rulesSetPhases.fixDefaultActions(&warnings, &errors); + containsDuplicatedIds(&warnings, &errors); + + if (!errors.empty()) { + for (auto &i : errors) { + m_parserError << "*** Error: " << *i << std::endl; + } + return -1; + } return amount_of_rules; } @@ -299,7 +368,7 @@ void RulesSet::debug(int level, const std::string &id, } -void RulesSet::dump() const { +void RulesSet::dump() { m_rulesSetPhases.dump(); } diff --git a/src/rules_set_phases.cc b/src/rules_set_phases.cc index 6f3acd19..ed9dfe7c 100644 --- a/src/rules_set_phases.cc +++ b/src/rules_set_phases.cc @@ -15,78 +15,34 @@ #include "modsecurity/rules_set_phases.h" #include "src/rule_with_operator.h" - +#include namespace modsecurity { -bool RulesSetPhases::insert(std::shared_ptr rule) { - if (rule->getPhase() >= modsecurity::Phases::NUMBER_OF_PHASES) { - return false; +void RulesSetPhases::insert(std::shared_ptr rule) { + if (rule->getPhase() >= size()) { + return; } m_rulesAtPhase[rule->getPhase()].insert(rule); - - return true; } -int RulesSetPhases::append(RulesSetPhases *from, std::ostringstream *err) { - int amount_of_rules = 0; - std::vector v; - - for (int i = 0; i < modsecurity::Phases::NUMBER_OF_PHASES; i++) { - v.reserve(m_rulesAtPhase[i].size()); - for (size_t z = 0; z < m_rulesAtPhase[i].size(); z++) { - RuleWithOperator *rule_ckc = dynamic_cast(m_rulesAtPhase[i].at(z).get()); - //RuleWithOperator *rule_ckc = dynamic_cast(m_rulesAtPhase->at(i).get()); - if (!rule_ckc) { - continue; - } - v.push_back(rule_ckc->getId()); - } +void RulesSetPhases::append(RulesSetPhases *from) { + int phase = 0; + for (auto &a : *from) { + m_rulesAtPhase[phase++].append(&a); } - std::sort (v.begin(), v.end()); - - for (int phase = 0; phase < modsecurity::Phases::NUMBER_OF_PHASES; phase++) { - int res = m_rulesAtPhase[phase].append(from->at(phase), v, err); - if (res < 0) { - return res; - } - amount_of_rules = amount_of_rules + res; - - /** - * An action set in a child will overwrite an action set on a parent. - * - */ - std::vector > *actions_to = &at(phase)->m_defaultActions; - std::vector > *actions_t_to = &at(phase)->m_defaultTransformations; - if (actions_to->size() == 0 || actions_t_to->size() == 0) { - std::vector > *actions_from = &from->at(phase)->m_defaultActions; - - actions_to->clear(); - for (size_t j = 0; j < actions_from->size(); j++) { - actions_to->push_back(actions_from->at(j)); - } - - std::vector > *actions_t_from = &from->at(phase)->m_defaultTransformations; - actions_t_to->clear(); - for (size_t j = 0; j < actions_t_from->size(); j++) { - actions_t_to->push_back(actions_t_from->at(j)); - } - at(phase)->fixDefaultActions(); - } - } - - return amount_of_rules; } -void RulesSetPhases::dump() const { - for (int i = 0; i <= modsecurity::Phases::NUMBER_OF_PHASES; i++) { - const Rules *rules = &m_rulesAtPhase[i]; - std::cout << "Phase: " << std::to_string(i); - std::cout << " (" << std::to_string(m_rulesAtPhase[i].size()); + +void RulesSetPhases::dump() { + int phase = 0; + for (auto &rules : m_rulesAtPhase) { + std::cout << "Phase: " << std::to_string(phase++); + std::cout << " (" << std::to_string(rules.size()); std::cout << " rules)" << std::endl; - m_rulesAtPhase[i].dump(); + rules.dump(); } } diff --git a/test/test-cases/regression/config-include-bad.json b/test/test-cases/regression/config-include-bad.json index 76797552..ed8b0156 100644 --- a/test/test-cases/regression/config-include-bad.json +++ b/test/test-cases/regression/config-include-bad.json @@ -43,7 +43,7 @@ "version_min":300000, "title":"Include - duplicate id", "expected":{ - "parser_error": "Rule id: 40 is duplicated" + "parser_error": "There are multiple rules defined with same id. The ID 40 is defined at" }, "rules":[ "SecRuleEngine On",