diff --git a/headers/modsecurity/actions/action.h b/headers/modsecurity/actions/action.h index 2d4cc8e1..6caa9a1e 100644 --- a/headers/modsecurity/actions/action.h +++ b/headers/modsecurity/actions/action.h @@ -67,7 +67,6 @@ class Action { } virtual bool init(std::string *error) { return true; } virtual bool isDisruptive() { return false; } - virtual void fillIntervention(ModSecurityIntervention *intervention); static Action *instantiate(const std::string& name); diff --git a/headers/modsecurity/rule_message.h b/headers/modsecurity/rule_message.h index 7a96306e..3af3e327 100644 --- a/headers/modsecurity/rule_message.h +++ b/headers/modsecurity/rule_message.h @@ -18,6 +18,7 @@ #include #include #include +#include #endif #ifndef HEADERS_MODSECURITY_RULE_MESSAGE_H_ @@ -50,6 +51,7 @@ class RuleMessage { { } std::string errorLog(Transaction *trans); + std::string disruptiveErrorLog(Transaction *trans, std::string log2); std::string m_match; @@ -65,7 +67,6 @@ class RuleMessage { int m_accuracy; std::list m_tags; - std::vector m_tmp_actions; std::list m_server_logs; Rule *m_rule; diff --git a/headers/modsecurity/transaction.h b/headers/modsecurity/transaction.h index 65295326..1f4646f1 100644 --- a/headers/modsecurity/transaction.h +++ b/headers/modsecurity/transaction.h @@ -73,8 +73,10 @@ class Rules; class RuleMessage; namespace actions { class Action; +namespace disruptive { enum AllowType : int; } +} namespace RequestBodyProcessor { class XML; class JSON; @@ -339,7 +341,7 @@ class Transaction { /** * If allow action was utilized, this variable holds the allow type. */ - modsecurity::actions::AllowType m_allowType; + modsecurity::actions::disruptive::AllowType m_allowType; /** * Holds the decode URI. Notice that m_uri holds the raw version @@ -351,7 +353,8 @@ class Transaction { * Actions (disruptive?) that should be taken by the connector related to * that transaction. */ - std::vector m_actions; + std::vector m_actions; + ModSecurityIntervention m_it; /** * Holds the creation time stamp, using std::time. diff --git a/src/Makefile.am b/src/Makefile.am index 6cb628e4..123b05a2 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -86,9 +86,7 @@ VARIABLES = \ ACTIONS = \ actions/accuracy.cc \ actions/action.cc \ - actions/allow.cc \ actions/audit_log.cc \ - actions/block.cc \ actions/capture.cc \ actions/chain.cc \ actions/ctl/audit_log_parts.cc \ @@ -98,8 +96,12 @@ ACTIONS = \ actions/ctl/rule_remove_target_by_id.cc \ actions/ctl/rule_remove_by_id.cc \ actions/ctl/request_body_access.cc\ + actions/disruptive/allow.cc \ + actions/disruptive/block.cc \ + actions/disruptive/deny.cc \ + actions/disruptive/redirect.cc \ + actions/disruptive/pass.cc \ actions/init_col.cc \ - actions/deny.cc \ actions/log.cc \ actions/log_data.cc \ actions/maturity.cc \ @@ -107,16 +109,14 @@ ACTIONS = \ actions/multi_match.cc \ actions/no_audit_log.cc \ actions/no_log.cc \ - actions/pass.cc \ actions/phase.cc \ - actions/redirect.cc \ actions/rev.cc \ actions/rule_id.cc \ actions/severity.cc \ actions/set_sid.cc \ actions/set_uid.cc \ actions/set_var.cc \ - actions/status.cc \ + actions/data/status.cc \ actions/skip.cc \ actions/skip_after.cc \ actions/tag.cc \ diff --git a/src/actions/action.cc b/src/actions/action.cc index 5ae116d0..0437787e 100644 --- a/src/actions/action.cc +++ b/src/actions/action.cc @@ -22,16 +22,16 @@ #include "modsecurity/rule.h" #include "src/utils/string.h" -#include "src/actions/block.h" +#include "src/actions/disruptive/block.h" #include "src/actions/chain.h" -#include "src/actions/deny.h" -#include "src/actions/redirect.h" -#include "src/actions/status.h" +#include "src/actions/disruptive/deny.h" +#include "src/actions/disruptive/redirect.h" +#include "src/actions/data/status.h" #include "src/actions/rule_id.h" #include "src/actions/phase.h" #include "src/actions/severity.h" #include "src/actions/capture.h" -#include "src/actions/pass.h" +#include "src/actions/disruptive/pass.h" #include "src/actions/log.h" #include "src/actions/no_log.h" #include "src/actions/multi_match.h" @@ -55,9 +55,6 @@ bool Action::evaluate(Rule *rule, Transaction *transaction) { } -void Action::fillIntervention(ModSecurityIntervention *i) { -} - Action *Action::instantiate(const std::string& name) { std::string status("status:"); std::string redirect("redirect:"); @@ -66,13 +63,13 @@ Action *Action::instantiate(const std::string& name) { std::string rule_id("id:"); if (name.compare(0, status.length(), status) == 0) { - return new Status(name); + return new data::Status(name); } if (name.compare(0, redirect.length(), redirect) == 0) { - return new Redirect(name); + return new disruptive::Redirect(name); } if (name.compare(0, block.length(), block) == 0) { - return new Block(name); + return new disruptive::Block(name); } if (name.compare(0, phase.length(), phase) == 0) { return new Phase(name); @@ -87,10 +84,10 @@ Action *Action::instantiate(const std::string& name) { return new Capture(name); } if (name == "pass") { - return new Pass(name); + return new disruptive::Pass(name); } if (name == "deny") { - return new Deny(name); + return new disruptive::Deny(name); } if (name == "log") { return new Log(name); diff --git a/src/actions/status.cc b/src/actions/data/status.cc similarity index 84% rename from src/actions/status.cc rename to src/actions/data/status.cc index 49cbbe6e..3621f8b8 100644 --- a/src/actions/status.cc +++ b/src/actions/data/status.cc @@ -13,7 +13,7 @@ * */ -#include "src/actions/status.h" +#include "src/actions/data/status.h" #include #include @@ -23,7 +23,7 @@ namespace modsecurity { namespace actions { - +namespace data { bool Status::init(std::string *error) { try { @@ -38,16 +38,11 @@ bool Status::init(std::string *error) { bool Status::evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) { - rm->m_tmp_actions.push_back(this); + transaction->m_it.status = m_status; return true; } -void Status::fillIntervention(ModSecurityIntervention *i) { - i->status = m_status; - i->log = "Status"; -} - - +} // namespace data } // namespace actions } // namespace modsecurity diff --git a/src/actions/status.h b/src/actions/data/status.h similarity index 93% rename from src/actions/status.h rename to src/actions/data/status.h index 839670e1..1ceef328 100644 --- a/src/actions/status.h +++ b/src/actions/data/status.h @@ -27,6 +27,8 @@ class Transaction; namespace modsecurity { class Transaction; namespace actions { +namespace data { + class Status : public Action { public: @@ -36,12 +38,12 @@ class Status : public Action { bool init(std::string *error) override; bool evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) override; - void fillIntervention(ModSecurityIntervention *i) override; - protected: int m_status; }; + +} // namespace data } // namespace actions } // namespace modsecurity #endif diff --git a/src/actions/allow.cc b/src/actions/disruptive/allow.cc similarity index 94% rename from src/actions/allow.cc rename to src/actions/disruptive/allow.cc index c7b81286..c534e689 100644 --- a/src/actions/allow.cc +++ b/src/actions/disruptive/allow.cc @@ -13,7 +13,7 @@ * */ -#include "src/actions/allow.h" +#include "src/actions/disruptive/allow.h" #include #include @@ -26,6 +26,8 @@ namespace modsecurity { namespace actions { +namespace disruptive { + bool Allow::init(std::string *error) { std::string a = utils::string::tolower(m_parser_payload); @@ -56,5 +58,7 @@ bool Allow::evaluate(Rule *rule, Transaction *transaction) { return true; } + +} // namespace disruptive } // namespace actions } // namespace modsecurity diff --git a/src/actions/allow.h b/src/actions/disruptive/allow.h similarity index 97% rename from src/actions/allow.h rename to src/actions/disruptive/allow.h index 3b186189..1cd905c8 100644 --- a/src/actions/allow.h +++ b/src/actions/disruptive/allow.h @@ -28,6 +28,8 @@ class Transaction; class Rule; namespace actions { +namespace disruptive { + enum AllowType : int { /** @@ -76,6 +78,8 @@ class Allow : public Action { } }; + +} // namespace disruptive } // namespace actions } // namespace modsecurity #endif diff --git a/src/actions/block.cc b/src/actions/disruptive/block.cc similarity index 67% rename from src/actions/block.cc rename to src/actions/disruptive/block.cc index 9d495f36..dd5a84c7 100644 --- a/src/actions/block.cc +++ b/src/actions/disruptive/block.cc @@ -13,36 +13,38 @@ * */ -#include "src/actions/block.h" +#include "src/actions/disruptive/block.h" #include #include #include "modsecurity/transaction.h" #include "modsecurity/rule.h" +#include "modsecurity/rules.h" #include "modsecurity/intervention.h" +#include "src/actions/data/status.h" namespace modsecurity { namespace actions { +namespace disruptive { bool Block::evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) { -#ifndef NO_LOGS - transaction->debug(8, "Running action block"); -#endif - for (Action *a : rule->m_actionsRuntimePos) { - if (a->isDisruptive() == true) { - rm->m_tmp_actions.push_back(a); + std::string log; + + transaction->debug(8, "Marking request as disruptive."); + + for (Action *a : transaction->m_rules->defaultActions[rule->phase]) { + if (a->isDisruptive() == false) { + continue; } + a->evaluate(rule, transaction, rm); } + return true; } -void Block::fillIntervention(ModSecurityIntervention *i) { - i->disruptive = true; -} - - +} // namespace disruptive } // namespace actions } // namespace modsecurity diff --git a/src/actions/block.h b/src/actions/disruptive/block.h similarity index 94% rename from src/actions/block.h rename to src/actions/disruptive/block.h index c9d4d9f7..33c3417f 100644 --- a/src/actions/block.h +++ b/src/actions/disruptive/block.h @@ -28,6 +28,7 @@ namespace modsecurity { class Transaction; namespace actions { +namespace disruptive { class Block : public Action { @@ -36,11 +37,11 @@ class Block : public Action { bool evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) override; - void fillIntervention(ModSecurityIntervention *i) override; bool isDisruptive() override { return true; } }; +} // namespace disruptive } // namespace actions } // namespace modsecurity #endif diff --git a/src/actions/deny.cc b/src/actions/disruptive/deny.cc similarity index 63% rename from src/actions/deny.cc rename to src/actions/disruptive/deny.cc index d0e9d2df..458e2218 100644 --- a/src/actions/deny.cc +++ b/src/actions/disruptive/deny.cc @@ -13,34 +13,41 @@ * */ -#include "src/actions/deny.h" +#include "src/actions/disruptive/deny.h" #include #include +#include +#include #include "modsecurity/transaction.h" namespace modsecurity { namespace actions { +namespace disruptive { bool Deny::evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) { #ifndef NO_LOGS transaction->debug(8, "Running action deny"); #endif - rm->m_tmp_actions.push_back(this); + std::string log; + + if (transaction->m_it.status == 200) { + transaction->m_it.status = 403; + } + + log.append("Access denied with code %d"); + log.append(" (phase "); + log.append(std::to_string(rm->m_rule->phase - 1) + "). "); + + transaction->m_it.disruptive = true; + transaction->m_it.log = strdup(rm->disruptiveErrorLog(transaction, log).c_str()); + return true; } -void Deny::fillIntervention(ModSecurityIntervention *i) { - if (i->status == 200) { - i->status = 403; - } - i->log = "Deny action"; - i->disruptive = true; -} - - +} // namespace disruptive } // namespace actions } // namespace modsecurity diff --git a/src/actions/deny.h b/src/actions/disruptive/deny.h similarity index 94% rename from src/actions/deny.h rename to src/actions/disruptive/deny.h index cf5acc7f..fda5d8e5 100644 --- a/src/actions/deny.h +++ b/src/actions/disruptive/deny.h @@ -24,6 +24,7 @@ namespace modsecurity { namespace actions { +namespace disruptive { class Deny : public Action { @@ -32,11 +33,11 @@ class Deny : public Action { bool evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) override; - void fillIntervention(ModSecurityIntervention *i) override; bool isDisruptive() override { return true; } }; +} // namespace disruptive } // namespace actions } // namespace modsecurity diff --git a/src/actions/pass.cc b/src/actions/disruptive/pass.cc similarity index 72% rename from src/actions/pass.cc rename to src/actions/disruptive/pass.cc index f9e43245..3b737fbc 100644 --- a/src/actions/pass.cc +++ b/src/actions/disruptive/pass.cc @@ -13,7 +13,7 @@ * */ -#include "src/actions/pass.h" +#include "src/actions/disruptive/pass.h" #include #include @@ -24,13 +24,22 @@ namespace modsecurity { namespace actions { +namespace disruptive { bool Pass::evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) { - rm->m_tmp_actions.clear(); + transaction->m_it.status = 200; + transaction->m_it.disruptive = false; + transaction->m_it.url = NULL; + transaction->m_it.log = NULL; + transaction->m_it.pause = 0; + + transaction->debug(8, "Running action pass"); + return true; } +} // namespace disruptive } // namespace actions } // namespace modsecurity diff --git a/src/actions/pass.h b/src/actions/disruptive/pass.h similarity index 84% rename from src/actions/pass.h rename to src/actions/disruptive/pass.h index 14272e6a..172d1aef 100644 --- a/src/actions/pass.h +++ b/src/actions/disruptive/pass.h @@ -18,11 +18,12 @@ #include "modsecurity/actions/action.h" #include "modsecurity/transaction.h" -#ifndef SRC_ACTIONS_PASS_H_ -#define SRC_ACTIONS_PASS_H_ +#ifndef SRC_ACTIONS_DISRUPTIVE_PASS_H_ +#define SRC_ACTIONS_DISRUPTIVE_PASS_H_ namespace modsecurity { namespace actions { +namespace disruptive { class Pass : public Action { @@ -34,8 +35,10 @@ class Pass : public Action { bool isDisruptive() override { return true; } }; + +} // namespace disruptive } // namespace actions } // namespace modsecurity -#endif // SRC_ACTIONS_PASS_H_ +#endif // SRC_ACTIONS_DISRUPTIVE_PASS_H_ diff --git a/src/actions/redirect.cc b/src/actions/disruptive/redirect.cc similarity index 60% rename from src/actions/redirect.cc rename to src/actions/disruptive/redirect.cc index 1d40daed..533f9fec 100644 --- a/src/actions/redirect.cc +++ b/src/actions/disruptive/redirect.cc @@ -13,16 +13,19 @@ * */ -#include "src/actions/redirect.h" +#include "src/actions/disruptive/redirect.h" #include #include +#include + #include "modsecurity/transaction.h" #include "src/macro_expansion.h" namespace modsecurity { namespace actions { +namespace disruptive { bool Redirect::init(std::string *error) { @@ -32,23 +35,26 @@ bool Redirect::init(std::string *error) { } -bool Redirect::evaluate(Rule *rule, Transaction *transaction) { +bool Redirect::evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) { m_urlExpanded = MacroExpansion::expand(m_url, transaction); - transaction->m_actions.push_back(this); + std::string log; + + /* if it was changed before, lets keep it. */ + if (transaction->m_it.status == 200) { + transaction->m_it.status = m_status; + } + log.append("Access denied with code %d"); + log.append(" (phase "); + log.append(std::to_string(rm->m_rule->phase - 1) + "). "); + + transaction->m_it.url = strdup(m_urlExpanded.c_str()); + transaction->m_it.disruptive = true; + transaction->m_it.log = strdup(rm->disruptiveErrorLog(transaction, log).c_str()); + return true; } -void Redirect::fillIntervention(ModSecurityIntervention *i) { - /* if it was changed before, lets keep it. */ - if (i->status == 200) { - i->status = m_status; - } - i->url = m_urlExpanded.c_str(); - i->log = "Redirecting"; - i->disruptive = true; -} - - +} // namespace disruptive } // namespace actions } // namespace modsecurity diff --git a/src/actions/redirect.h b/src/actions/disruptive/redirect.h similarity index 87% rename from src/actions/redirect.h rename to src/actions/disruptive/redirect.h index a12f764d..6d8612f2 100644 --- a/src/actions/redirect.h +++ b/src/actions/disruptive/redirect.h @@ -16,6 +16,7 @@ #include #include "modsecurity/actions/action.h" +#include "modsecurity/rule_message.h" #ifndef SRC_ACTIONS_REDIRECT_H_ #define SRC_ACTIONS_REDIRECT_H_ @@ -27,6 +28,8 @@ namespace modsecurity { class Transaction; namespace actions { +namespace disruptive { + class Redirect : public Action { public: @@ -36,9 +39,8 @@ class Redirect : public Action { m_urlExpanded(""), m_url("") { } - bool evaluate(Rule *rule, Transaction *transaction) override; + bool evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) override; bool init(std::string *error) override; - void fillIntervention(ModSecurityIntervention *i) override; bool isDisruptive() override { return true; } private: @@ -47,6 +49,8 @@ class Redirect : public Action { std::string m_url; }; + +} // namespace disruptive } // namespace actions } // namespace modsecurity #endif diff --git a/src/actions/msg.cc b/src/actions/msg.cc index 165aa637..0da24a8d 100644 --- a/src/actions/msg.cc +++ b/src/actions/msg.cc @@ -48,10 +48,12 @@ namespace actions { bool Msg::evaluate(Rule *rule, Transaction *transaction, RuleMessage *rm) { std::string msg = data(transaction); - transaction->debug(9, "Saving msg: " + msg); rm->m_message = msg; + transaction->debug(9, "Saving msg: " + msg); transaction->m_collections.storeOrUpdateFirst("RULE:msg", msg); + + rm->m_server_logs.push_back(rm->errorLog(transaction)); return true; } diff --git a/src/operators/pm.cc b/src/operators/pm.cc index b6354e50..a567e391 100644 --- a/src/operators/pm.cc +++ b/src/operators/pm.cc @@ -26,6 +26,7 @@ #include "src/operators/operator.h" #include "src/utils/acmp.h" +#include "src/utils/string.h" namespace modsecurity { namespace operators { @@ -78,15 +79,6 @@ void Pm::postOrderTraversal(acmp_btree_node_t *node) { node = NULL; } -void Pm::replaceAll(std::string str, const std::string& from, - const std::string& to) { - size_t start_pos = 0; - while ((start_pos = str.find(from, start_pos)) != std::string::npos) { - size_t end_pos = start_pos + from.length(); - str.replace(start_pos, end_pos, to); - start_pos += to.length(); - } -} bool Pm::evaluate(Transaction *transaction, Rule *rule, const std::string &input) { @@ -119,8 +111,6 @@ bool Pm::init(const std::string &file, std::string *error) { std::istringstream *iss; const char *err = NULL; - replaceAll(m_param, "\\", "\\\\"); - char *content = parse_pm_content(m_param.c_str(), m_param.length(), &err); if (content == NULL) { iss = new std::istringstream(m_param); diff --git a/src/operators/pm.h b/src/operators/pm.h index 9c0562fb..d81ac5d5 100644 --- a/src/operators/pm.h +++ b/src/operators/pm.h @@ -35,8 +35,6 @@ class Pm : public Operator { m_p = acmp_create(0); } ~Pm(); - void replaceAll(std::string str, const std::string& from, - const std::string& to); bool evaluate(Transaction *transaction, Rule *rule, const std::string &input) override; bool evaluate(Transaction *transaction, diff --git a/src/parser/seclang-parser.yy b/src/parser/seclang-parser.yy index 14a717db..7edeb03b 100644 --- a/src/parser/seclang-parser.yy +++ b/src/parser/seclang-parser.yy @@ -22,7 +22,7 @@ class Driver; #include "src/actions/accuracy.h" #include "modsecurity/actions/action.h" -#include "src/actions/allow.h" +#include "src/actions/disruptive/allow.h" #include "src/actions/audit_log.h" #include "src/actions/ctl/audit_log_parts.h" #include "src/actions/ctl/request_body_access.h" @@ -36,7 +36,7 @@ class Driver; #include "src/actions/maturity.h" #include "src/actions/msg.h" #include "src/actions/phase.h" -#include "src/actions/redirect.h" +#include "src/actions/disruptive/redirect.h" #include "src/actions/rev.h" #include "src/actions/set_sid.h" #include "src/actions/set_uid.h" @@ -102,7 +102,7 @@ using modsecurity::Variables::Variations::Exclusion; using modsecurity::Variables::XML; using modsecurity::actions::Accuracy; using modsecurity::actions::Action; -using modsecurity::actions::Allow; +using modsecurity::actions::disruptive::Allow; using modsecurity::actions::ctl::AuditLogParts; using modsecurity::actions::ctl::RequestBodyProcessorJSON; using modsecurity::actions::ctl::RequestBodyProcessorXML; @@ -111,7 +111,7 @@ using modsecurity::actions::LogData; using modsecurity::actions::Maturity; using modsecurity::actions::Msg; using modsecurity::actions::Phase; -using modsecurity::actions::Redirect; +using modsecurity::actions::disruptive::Redirect; using modsecurity::actions::Rev; using modsecurity::actions::SetSID; using modsecurity::actions::SetUID; diff --git a/src/rule.cc b/src/rule.cc index e3615dcd..e0b621eb 100644 --- a/src/rule.cc +++ b/src/rule.cc @@ -251,9 +251,6 @@ void Rule::executeActionsIndependentOfChainedRuleResult(Transaction *trasn, } } } - for (auto &z : ruleMessage->m_tmp_actions) { - trasn->m_actions.push_back(z); - } } @@ -505,16 +502,14 @@ void Rule::executeActionsAfterFullMatch(Transaction *trasn, a->evaluate(this, trasn, ruleMessage); continue; } else { + trasn->debug(4, "_Not_ running (disruptive) action: " + + a->m_name + ". SecRuleEngine is not On."); continue; } trasn->debug(4, "Not running disruptive action: " + \ a->m_name + ". SecRuleEngine is not On"); } - - for (auto &z : ruleMessage->m_tmp_actions) { - trasn->m_actions.push_back(z); - } } @@ -524,6 +519,8 @@ bool Rule::evaluate(Transaction *trasn) { bool recursiveGlobalRet; bool containsDisruptive = false; RuleMessage ruleMessage(this); + std::vector finalVars; + std::string eparam; trasn->m_matched.clear(); @@ -535,8 +532,7 @@ bool Rule::evaluate(Transaction *trasn) { + ") Executing unconditional rule..."); executeActionsIndependentOfChainedRuleResult(trasn, &containsDisruptive, &ruleMessage); - executeActionsAfterFullMatch(trasn, false, &ruleMessage); - return true; + goto end_exec; } for (auto &i : trasn->m_ruleRemoveById) { @@ -548,7 +544,7 @@ bool Rule::evaluate(Transaction *trasn) { return true; } - std::string eparam = MacroExpansion::expand(this->op->m_param, trasn); + eparam = MacroExpansion::expand(this->op->m_param, trasn); if (this->op->m_param != eparam) { eparam = "\"" + eparam + "\" Was: \"" + this->op->m_param + "\""; @@ -565,7 +561,7 @@ bool Rule::evaluate(Transaction *trasn) { updateRulesVariable(trasn); - std::vector finalVars = getFinalVars(trasn); + finalVars = getFinalVars(trasn); for (const collection::Variable *v : finalVars) { std::string value = v->m_value; @@ -583,13 +579,6 @@ bool Rule::evaluate(Transaction *trasn) { updateMatchedVars(trasn, v->m_key, value); executeActionsIndependentOfChainedRuleResult(trasn, &containsDisruptive, &ruleMessage); - std::string msg2save = ruleMessage.errorLog(trasn); - if (ruleMessage.m_message.empty() == false) { - trasn->debug(4, - "Scheduled to be saved on the server log: " \ - + msg2save + ""); - ruleMessage.m_server_logs.push_back(msg2save); - } globalRet = true; } } @@ -604,14 +593,7 @@ bool Rule::evaluate(Transaction *trasn) { trasn->debug(4, "Rule returned 1."); if (this->chained == false) { - executeActionsAfterFullMatch(trasn, containsDisruptive, &ruleMessage); - trasn->debug(4, "Merging temporary actions to the main transaction."); - - for (const auto &u : ruleMessage.m_server_logs) { - trasn->debug(8, "To save: " + u); - trasn->serverLog(u); - } - return true; + goto end_exec; } if (this->chainedRule == NULL) { @@ -624,15 +606,18 @@ bool Rule::evaluate(Transaction *trasn) { recursiveGlobalRet = this->chainedRule->evaluate(trasn); if (recursiveGlobalRet == true) { - executeActionsAfterFullMatch(trasn, containsDisruptive, &ruleMessage); - trasn->debug(4, "Merging temporary actions to the main transaction."); - for (const auto &u : ruleMessage.m_server_logs) { - trasn->serverLog(u); - } - return true; + goto end_exec; } return false; + +end_exec: + executeActionsAfterFullMatch(trasn, containsDisruptive, &ruleMessage); + for (const auto &u : ruleMessage.m_server_logs) { + trasn->serverLog(u); + } + + return true; } diff --git a/src/rule_message.cc b/src/rule_message.cc index 4dbfaffb..fd67bfff 100644 --- a/src/rule_message.cc +++ b/src/rule_message.cc @@ -23,6 +23,36 @@ namespace modsecurity { +std::string RuleMessage::disruptiveErrorLog(Transaction *trans, std::string msg2) { + std::string msg; + + msg.append("[client " + std::string(trans->m_clientIpAddress) + "]"); + msg.append(" ModSecurity: "); + msg.append(msg2); + msg.append(m_match); + msg.append(" [file \"" + std::string(m_ruleFile) + "\"]"); + msg.append(" [line \"" + std::to_string(m_ruleLine) + "\"]"); + msg.append(" [id \"" + std::to_string(m_ruleId) + "\"]"); + msg.append(" [rev \"" + m_rev + "\"]"); + msg.append(" [msg \"" + m_message + "\"]"); + msg.append(" [data \"" + m_data + "\"]"); + msg.append(" [severity \"" + + std::to_string(m_severity) + "\"]"); + msg.append(" [ver \"" + m_ver + "\"]"); + msg.append(" [maturity \"" + std::to_string(m_maturity) + "\"]"); + msg.append(" [accuracy \"" + std::to_string(m_accuracy) + "\"]"); + for (auto &a : m_tags) { + msg.append(" [tag \"" + a + "\"]"); + } + msg.append(" [hostname \"" + std::string(trans->m_serverIpAddress) \ + + "\"]"); + msg.append(" [uri \"" + trans->m_uri_no_query_string_decoded + "\"]"); + msg.append(" [unique_id \"" + trans->m_id + "\"]"); + + return modsecurity::utils::string::toHexIfNeeded(msg); + +} + std::string RuleMessage::errorLog(Transaction *trans) { std::string msg; diff --git a/src/rules.cc b/src/rules.cc index 8b2aa0ae..85bea585 100644 --- a/src/rules.cc +++ b/src/rules.cc @@ -189,20 +189,20 @@ int Rules::evaluate(int phase, Transaction *transaction) { debug(9, "This phase consists of " + std::to_string(rules.size()) + \ " rule(s)."); - if (transaction->m_allowType == actions::FromNowOneAllowType + if (transaction->m_allowType == actions::disruptive::FromNowOneAllowType && phase != modsecurity::Phases::LoggingPhase) { debug(9, "Skipping all rules evaluation on this phase as request " \ "through the utilization of an `allow' action."); return true; } - if (transaction->m_allowType == actions::RequestAllowType + if (transaction->m_allowType == actions::disruptive::RequestAllowType && phase <= modsecurity::Phases::RequestBodyPhase) { debug(9, "Skipping all rules evaluation on this phase as request " \ "through the utilization of an `allow' action."); return true; } - if (transaction->m_allowType != actions::NoneAllowType) { - transaction->m_allowType = actions::NoneAllowType; + if (transaction->m_allowType != actions::disruptive::NoneAllowType) { + transaction->m_allowType = actions::disruptive::NoneAllowType; } for (int i = 0; i < rules.size(); i++) { @@ -223,7 +223,8 @@ int Rules::evaluate(int phase, Transaction *transaction) { debug(9, "Skipped rule id '" + std::to_string(rule->rule_id) \ + "' due to a `skip' action. Still " + \ std::to_string(transaction->m_skip_next) + " to be skipped."); - } else if (transaction->m_allowType != actions::NoneAllowType) { + } else if (transaction->m_allowType + != actions::disruptive::NoneAllowType) { debug(9, "Skipped rule id '" + std::to_string(rule->rule_id) \ + "' as request trough the utilization of an `allow' action."); } else if (m_exceptions.contains(rule->rule_id)) { @@ -231,6 +232,11 @@ int Rules::evaluate(int phase, Transaction *transaction) { + "'. Removed by an SecRuleRemove directive."); } else { rule->evaluate(transaction); + if (transaction->m_it.disruptive == true) { + debug(8, "Skipping this phase as this " \ + "request was already intercepted."); + break; + } } } return 1; diff --git a/src/transaction.cc b/src/transaction.cc index e9ca8759..5f1578cc 100644 --- a/src/transaction.cc +++ b/src/transaction.cc @@ -33,7 +33,7 @@ #include #include "modsecurity/actions/action.h" -#include "src/actions/deny.h" +#include "src/actions/disruptive/deny.h" #include "modsecurity/intervention.h" #include "modsecurity/modsecurity.h" #include "src/request_body_processor/multipart.h" @@ -48,7 +48,7 @@ #include "modsecurity/rule.h" #include "modsecurity/rule_message.h" #include "modsecurity/rules_properties.h" -#include "src/actions/allow.h" +#include "src/actions/disruptive/allow.h" @@ -123,7 +123,7 @@ Transaction::Transaction(ModSecurity *ms, Rules *rules, void *logCbData) m_responseContentType(NULL), m_requestBodyAccess(Rules::PropertyNotSetConfigBoolean), m_marker(""), - m_allowType(modsecurity::actions::NoneAllowType), + m_allowType(modsecurity::actions::disruptive::NoneAllowType), m_skip_next(0), m_creationTimeStamp(utils::cpu_seconds()), m_logCbData(logCbData), @@ -158,8 +158,14 @@ Transaction::Transaction(ModSecurity *ms, Rules *rules, void *logCbData) m_collections.storeOrUpdateFirst("URLENCODED_ERROR", "0"); #ifndef NO_LOGS - this->debug(4, "Initialising transaction"); + this->debug(4, "Initializing transaction"); #endif + + m_it.status = 200; + m_it.disruptive = false; + m_it.url = NULL; + m_it.log = NULL; + m_it.pause = 0; } @@ -879,9 +885,9 @@ int Transaction::appendRequestBody(const unsigned char *buf, size_t len) { debug(5, "Request body limit is marked to reject the " \ "request"); #endif - Action *a = new actions::Deny("deny"); - a->temporaryAction = true; - m_actions.push_back(a); + m_it.log = "Request body limit is marked to reject the request"; + m_it.status = 403; + m_it.disruptive = true; } return true; } @@ -1136,9 +1142,10 @@ int Transaction::appendResponseBody(const unsigned char *buf, size_t len) { debug(5, "Response body limit is marked to reject the " \ "request"); #endif - Action *a = new actions::Deny("deny"); - a->temporaryAction = true; - m_actions.push_back(a); + + m_it.log = "Response body limit is marked to reject the request"; + m_it.status = 403; + m_it.disruptive = true; } return true; } @@ -1285,20 +1292,26 @@ int Transaction::processLogging() { * */ bool Transaction::intervention(ModSecurityIntervention *it) { - it->status = 200; - it->url = NULL; - it->disruptive = false; - if (m_actions.size() > 0) { - for (Action *a : m_actions) { - if (a->action_kind == Action::Kind::RunTimeOnlyIfMatchKind) { - a->fillIntervention(it); - } - if (a->temporaryAction) { - delete a; - } + if (m_it.disruptive) { + it->url = m_it.url; + it->disruptive = m_it.disruptive; + it->status = m_it.status; + + if (m_it.log != NULL) { + std::string log(""); + const char *log_str; + log.append(m_it.log); + utils::string::replaceAll(&log, std::string("%d"), std::to_string(it->status)); + log_str = strdup(log.c_str()); + it->log = log_str; } - m_actions.clear(); + m_it.status = 200; + m_it.disruptive = false; + m_it.url = NULL; + m_it.log = NULL; + m_it.pause = 0; } + return it->disruptive; } diff --git a/src/utils/string.cc b/src/utils/string.cc index 36b7c3bf..5fedb1b9 100644 --- a/src/utils/string.cc +++ b/src/utils/string.cc @@ -206,6 +206,17 @@ unsigned char *c2x(unsigned what, unsigned char *where) { } +void replaceAll(std::string *str, const std::string& from, + const std::string& to) { + size_t start_pos = 0; + while ((start_pos = str->find(from, start_pos)) != std::string::npos) { + size_t end_pos = start_pos + from.length(); + str->replace(start_pos, from.length(), to); + start_pos += to.length(); + } +} + + } // namespace string } // namespace utils } // namespace modsecurity diff --git a/src/utils/string.h b/src/utils/string.h index 72e4f375..72963f78 100644 --- a/src/utils/string.h +++ b/src/utils/string.h @@ -66,6 +66,8 @@ std::string tolower(std::string str); std::string toupper(std::string str); std::vector split(std::string str, char delimiter); void chomp(std::string *str); +void replaceAll(std::string *str, const std::string& from, + const std::string& to); unsigned char x2c(unsigned char *what); unsigned char xsingle2c(unsigned char *what); diff --git a/test/test-cases/regression/action-disruptive.json b/test/test-cases/regression/action-disruptive.json index 543ac4f8..11e80ff6 100644 --- a/test/test-cases/regression/action-disruptive.json +++ b/test/test-cases/regression/action-disruptive.json @@ -32,7 +32,7 @@ "version_min":300000, "title":"Testing Disruptive actions (3/n)", "expected":{ - "debug_log": "Running action block", + "debug_log": "Running .disruptive. action: block", "http_code":404 }, "rules":[ diff --git a/test/test-cases/regression/secruleengine.json b/test/test-cases/regression/secruleengine.json index ca4439b3..b290becc 100644 --- a/test/test-cases/regression/secruleengine.json +++ b/test/test-cases/regression/secruleengine.json @@ -34,14 +34,14 @@ "version_min":300000, "title":"Testing Disruptive actions (3/n)", "expected":{ - "debug_log": "_Not_ running action: deny. Rule _does not_contains a disruptive action, but SecRuleEngine is not On.", + "debug_log": "_Not_ running .disruptive. action: block. SecRuleEngine is not On", "http_code":200 }, "rules":[ "SecRuleEngine On", "SecRuleEngine DetectionOnly", "SecDefaultAction \"phase:2,deny,status:404\"", - "SecAction \"id:'1',phase:request,nolog,block,t:none\"" + "SecAction \"id:'1',phase:request,nolog,nolog,block,t:none\"" ] }, {