diff --git a/Makefile.am b/Makefile.am index b57f23ba..85fefaae 100644 --- a/Makefile.am +++ b/Makefile.am @@ -125,6 +125,7 @@ TESTS+=test/test-cases/regression/config-body_limits.json TESTS+=test/test-cases/regression/config-calling_phases_by_name.json TESTS+=test/test-cases/regression/config-include-bad.json TESTS+=test/test-cases/regression/config-include.json +TESTS+=test/test-cases/regression/config-phases.json TESTS+=test/test-cases/regression/config-remove_by_id.json TESTS+=test/test-cases/regression/config-remove_by_msg.json TESTS+=test/test-cases/regression/config-remove_by_tag.json diff --git a/headers/modsecurity/rules_set.h b/headers/modsecurity/rules_set.h index 2ead9a81..a6a3c0ff 100644 --- a/headers/modsecurity/rules_set.h +++ b/headers/modsecurity/rules_set.h @@ -41,6 +41,87 @@ class Driver; } +class RulesSetPhases { + public: + + ~RulesSetPhases() { + /** Cleanup the rules */ + for (int i = 0; i < modsecurity::Phases::NUMBER_OF_PHASES; i++) { + std::vector rules = m_rules[i]; + while (rules.empty() == false) { + Rule *rule = rules.back(); + rules.pop_back(); + if (rule->refCountDecreaseAndCheck()) { + rule = NULL; + } + } + } + } + + bool insert(Rule *rule) { + if (rule->m_phase >= modsecurity::Phases::NUMBER_OF_PHASES) { + return false; + } + m_rules[rule->m_phase].push_back(rule); + + return true; + } + + int 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_rules[i].size()); + for (size_t z = 0; z < m_rules[i].size(); z++) { + Rule *rule_ckc = m_rules[i].at(z); + if (rule_ckc->m_secMarker == true) { + continue; + } + v.push_back(rule_ckc->m_ruleId); + } + } + std::sort (v.begin(), v.end()); + + for (int i = 0; i < modsecurity::Phases::NUMBER_OF_PHASES; i++) { + for (size_t j = 0; j < from->at(i).size(); j++) { + Rule *rule = from->at(i).at(j); + if (std::binary_search(v.begin(), v.end(), rule->m_ruleId)) { + if (err != NULL) { + *err << "Rule id: " << std::to_string(rule->m_ruleId) \ + << " is duplicated" << std::endl; + } + return -1; + } + amount_of_rules++; + rule->refCountIncrease(); + m_rules[i].push_back(rule); + } + } + + return amount_of_rules; + } + + void dump() { + for (int i = 0; i <= modsecurity::Phases::NUMBER_OF_PHASES; i++) { + std::vector rules = m_rules[i]; + std::cout << "Phase: " << std::to_string(i); + std::cout << " (" << std::to_string(rules.size()); + std::cout << " rules)" << std::endl; + for (int j = 0; j < rules.size(); j++) { + std::cout << " Rule ID: " << std::to_string(rules[j]->m_ruleId); + std::cout << "--" << rules[j] << std::endl; + } + } + } + + std::vector operator[](int index) const { return m_rules[index]; } + std::vector at(int index) const { return m_rules[index]; } + + std::vector m_rules[8]; +}; + + /** @ingroup ModSecurity_CPP_API */ class RulesSet : public RulesSetProperties { public: @@ -80,6 +161,7 @@ class RulesSet : public RulesSetProperties { int64_t unicode_codepage; + RulesSetPhases m_rulesSetPhases; private: #ifndef NO_LOGS uint8_t m_secmarker_skipped; diff --git a/headers/modsecurity/rules_set_properties.h b/headers/modsecurity/rules_set_properties.h index 252f51f3..66a09a25 100644 --- a/headers/modsecurity/rules_set_properties.h +++ b/headers/modsecurity/rules_set_properties.h @@ -202,17 +202,7 @@ class RulesSetProperties { ~RulesSetProperties() { int i = 0; - /** Cleanup the rules */ - for (i = 0; i < modsecurity::Phases::NUMBER_OF_PHASES; i++) { - std::vector rules = m_rules[i]; - while (rules.empty() == false) { - Rule *rule = rules.back(); - rules.pop_back(); - if (rule->refCountDecreaseAndCheck()) { - rule = NULL; - } - } - } + for (i = 0; i < modsecurity::Phases::NUMBER_OF_PHASES; i++) { std::vector *tmp = &m_defaultActions[i]; while (tmp->empty() == false) { @@ -354,12 +344,6 @@ class RulesSetProperties { static int mergeProperties(RulesSetProperties *from, RulesSetProperties *to, std::ostringstream *err) { - int amount_of_rules = 0; - - amount_of_rules = appendRules(from->m_rules, to->m_rules, err); - if (amount_of_rules < 0) { - return amount_of_rules; - } merge_ruleengine_value(to->m_secRuleEngine, from->m_secRuleEngine, PropertyNotSetRuleEngine); @@ -471,56 +455,7 @@ class RulesSetProperties { } } - return amount_of_rules; - } - - - static int appendRules( - std::vector *from, - std::vector *to, - std::ostringstream *err) { - int amount_of_rules = 0; - // TODO: std::vector could be replaced with something more efficient. - std::vector v; - for (int i = 0; i < modsecurity::Phases::NUMBER_OF_PHASES; i++) { - std::vector *rules_to = to+i; - v.reserve(rules_to->size()); - for (size_t z = 0; z < rules_to->size(); z++) { - Rule *rule_ckc = rules_to->at(z); - if (rule_ckc->m_secMarker == true) { - continue; - } - v.push_back(rule_ckc->m_ruleId); - } - } - std::sort (v.begin(), v.end()); - - for (int i = 0; i < modsecurity::Phases::NUMBER_OF_PHASES; i++) { - std::vector *rules_from = from+i; - std::vector *rules_to = to+i; - for (size_t j = 0; j < rules_from->size(); j++) { - Rule *rule = rules_from->at(j); - if (std::binary_search(v.begin(), v.end(), rule->m_ruleId)) { - if (err != NULL) { - *err << "Rule id: " << std::to_string(rule->m_ruleId) \ - << " is duplicated" << std::endl; - } - return -1; - } - amount_of_rules++; - rule->refCountIncrease(); - rules_to->push_back(rule); - } - } - return amount_of_rules; - } - - - std::vector *getRulesForPhase(int phase) { - if (phase >= modsecurity::Phases::NUMBER_OF_PHASES) { - return NULL; - } - return &m_rules[phase]; + return 1; } @@ -551,7 +486,6 @@ class RulesSetProperties { ConfigString m_secArgumentSeparator; ConfigString m_secWebAppId; std::vector m_defaultActions[modsecurity::Phases::NUMBER_OF_PHASES]; - std::vector m_rules[modsecurity::Phases::NUMBER_OF_PHASES]; ConfigUnicodeMap m_unicodeMapTable; }; diff --git a/src/parser/driver.cc b/src/parser/driver.cc index 90415255..5de8c29a 100644 --- a/src/parser/driver.cc +++ b/src/parser/driver.cc @@ -45,7 +45,7 @@ int Driver::addSecMarker(std::string marker) { for (int i = 0; i < modsecurity::Phases::NUMBER_OF_PHASES; i++) { Rule *rule = new Rule(marker); rule->m_phase = i; - m_rules[i].push_back(rule); + m_rulesSetPhases.insert(rule); } return 0; } @@ -58,14 +58,15 @@ int Driver::addSecAction(Rule *rule) { return false; } - m_rules[rule->m_phase].push_back(rule); + + m_rulesSetPhases.insert(rule); return true; } int Driver::addSecRuleScript(RuleScript *rule) { - m_rules[rule->m_phase].push_back(rule); + m_rulesSetPhases.insert(rule); return true; } @@ -118,7 +119,7 @@ int Driver::addSecRule(Rule *rule) { return false; } for (int i = 0; i < modsecurity::Phases::NUMBER_OF_PHASES; i++) { - std::vector rules = m_rules[i]; + std::vector rules = m_rulesSetPhases[i]; for (int j = 0; j < rules.size(); j++) { if (rules[j]->m_ruleId == rule->m_ruleId) { m_parserError << "Rule id: " << std::to_string(rule->m_ruleId) \ @@ -129,7 +130,7 @@ int Driver::addSecRule(Rule *rule) { } lastRule = rule; - m_rules[rule->m_phase].push_back(rule); + m_rulesSetPhases.insert(rule); return true; } diff --git a/src/parser/driver.h b/src/parser/driver.h index 9cc49ba7..72091f0a 100644 --- a/src/parser/driver.h +++ b/src/parser/driver.h @@ -90,6 +90,8 @@ class Driver : public RulesSetProperties { std::string buffer; Rule *lastRule; + + RulesSetPhases m_rulesSetPhases; }; diff --git a/src/rules_set.cc b/src/rules_set.cc index 0e4987ea..06d49fc0 100644 --- a/src/rules_set.cc +++ b/src/rules_set.cc @@ -110,7 +110,7 @@ int RulesSet::evaluate(int phase, Transaction *t) { return 0; } - std::vector rules = m_rules[phase]; + std::vector rules = m_rulesSetPhases[phase]; ms_dbg_a(t, 9, "This phase consists of " \ + std::to_string(rules.size()) + " rule(s)."); @@ -222,7 +222,10 @@ int RulesSet::evaluate(int phase, Transaction *t) { int RulesSet::merge(Driver *from) { int amount_of_rules = 0; - amount_of_rules = mergeProperties( + + amount_of_rules = m_rulesSetPhases.append(&from->m_rulesSetPhases, + &m_parserError); + mergeProperties( dynamic_cast(from), dynamic_cast(this), &m_parserError); @@ -233,7 +236,10 @@ int RulesSet::merge(Driver *from) { int RulesSet::merge(RulesSet *from) { int amount_of_rules = 0; - amount_of_rules = mergeProperties( + + amount_of_rules = m_rulesSetPhases.append(&from->m_rulesSetPhases, + &m_parserError); + mergeProperties( dynamic_cast(from), dynamic_cast(this), &m_parserError); @@ -254,7 +260,7 @@ void RulesSet::debug(int level, const std::string &id, void RulesSet::dump() const { std::cout << "Rules: " << std::endl; for (int i = 0; i < modsecurity::Phases::NUMBER_OF_PHASES; i++) { - std::vector rules = m_rules[i]; + std::vector rules = m_rulesSetPhases[i]; std::cout << "Phase: " << std::to_string(i); std::cout << " (" << std::to_string(rules.size()); std::cout << " rules)" << std::endl; diff --git a/test/optimization/optimization.cc b/test/optimization/optimization.cc index 6dd16e02..4caf40be 100644 --- a/test/optimization/optimization.cc +++ b/test/optimization/optimization.cc @@ -67,8 +67,8 @@ int main(int argc, char **argv) { std::cout << std::endl; int nphases = modsecurity::Phases::NUMBER_OF_PHASES; - for (int j = 0; j < nphases; j++) { - std::vector rules = modsecRules->m_rules[i]; + for (int i = 0; i < nphases; i++) { + std::vector rules = modsecRules->m_rulesSetPhases[i]; if (rules.size() == 0) { continue; } diff --git a/test/test-cases/regression/config-phases.json b/test/test-cases/regression/config-phases.json new file mode 100644 index 00000000..5a620757 --- /dev/null +++ b/test/test-cases/regression/config-phases.json @@ -0,0 +1,199 @@ +[ + { + "enabled":1, + "version_min":300000, + "title":"Testing Config :: Phases (1/n)", + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*" + }, + "uri":"/?key=value&key=other_value", + "method":"GET" + }, + "response":{ + "headers":{ + "Date":"Mon, 13 Jul 2015 20:02:41 GMT", + "Last-Modified":"Sun, 26 Oct 2014 22:33:37 GMT", + "Content-Type":"text/html" + }, + "body":[ + "no need." + ] + }, + "expected":{ + "debug_log":"SecRules 0.*\n.*This phase consists of 1 rule\\(s\\)" + }, + "rules":[ + "SecRuleEngine On", + "SecRule ARGS \"@unconditionalMatch other_value\" \"id:1,phase:0,block,status:404\"" + ] + }, + { + "enabled":1, + "version_min":300000, + "title":"Testing Config :: Phases (2/n)", + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*" + }, + "uri":"/?key=value&key=other_value", + "method":"GET" + }, + "response":{ + "headers":{ + "Date":"Mon, 13 Jul 2015 20:02:41 GMT", + "Last-Modified":"Sun, 26 Oct 2014 22:33:37 GMT", + "Content-Type":"text/html" + }, + "body":[ + "no need." + ] + }, + "expected":{ + "debug_log":"SecRules 1.*\n.*This phase consists of 1 rule\\(s\\)" + }, + "rules":[ + "SecRuleEngine On", + "SecRule ARGS \"@unconditionalMatch other_value\" \"id:1,phase:1,block,status:404\"" + ] +}, +{ + "enabled":1, + "version_min":300000, + "title":"Testing Config :: Phases (3/n)", + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*" + }, + "uri":"/?key=value&key=other_value", + "method":"GET" + }, + "response":{ + "headers":{ + "Date":"Mon, 13 Jul 2015 20:02:41 GMT", + "Last-Modified":"Sun, 26 Oct 2014 22:33:37 GMT", + "Content-Type":"text/html" + }, + "body":[ + "no need." + ] + }, + "expected":{ + "debug_log":"SecRules 3.*\n.*This phase consists of 1 rule\\(s\\)" + }, + "rules":[ + "SecRuleEngine On", + "SecRule ARGS \"@unconditionalMatch other_value\" \"id:1,phase:3,block,status:404\"" + ] +}, +{ + "enabled":1, + "version_min":300000, + "title":"Testing Config :: Phases (4/n)", + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*" + }, + "uri":"/?key=value&key=other_value", + "method":"GET" + }, + "response":{ + "headers":{ + "Date":"Mon, 13 Jul 2015 20:02:41 GMT", + "Last-Modified":"Sun, 26 Oct 2014 22:33:37 GMT", + "Content-Type":"text/html" + }, + "body":[ + "no need." + ] + }, + "expected":{ + "debug_log":"SecRules 4.*\n.*This phase consists of 1 rule\\(s\\)" + }, + "rules":[ + "SecRuleEngine On", + "SecResponseBodyAccess On", + "SecRule ARGS \"@unconditionalMatch other_value\" \"id:1,phase:4,block,status:404\"" + ] +}, +{ + "enabled":1, + "version_min":300000, + "title":"Testing Config :: Phases (5/n)", + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*" + }, + "uri":"/?key=value&key=other_value", + "method":"GET" + }, + "response":{ + "headers":{ + "Date":"Mon, 13 Jul 2015 20:02:41 GMT", + "Last-Modified":"Sun, 26 Oct 2014 22:33:37 GMT", + "Content-Type":"text/html" + }, + "body":[ + "no need." + ] + }, + "expected":{ + "debug_log":"SecRules 5.*\n.*This phase consists of 1 rule\\(s\\)" + }, + "rules":[ + "SecRuleEngine On", + "SecRule ARGS \"@unconditionalMatch other_value\" \"id:1,phase:5,block,status:404\"" + ] +} +] +