github_mirrors/ModSecurity
3
0
Fork 1
mirror of https://github.com/owasp-modsecurity/ModSecurity.git synced 2025-11-16 01:22:18 +03:00
Code Issues Packages Projects Releases Wiki Activity

Refactoring in the Rule class to make it more elegant

Browse Source
This commit is contained in:
Felipe Zimmerle
2019-02-25 20:22:38 -03:00
parent e0703a4774
commit 35a341e473
23 changed files with 191 additions and 210 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
src/actions/rule_id.cc
View File

@@ -49,7 +49,7 @@ bool RuleId::init(std::string *error) {
bool RuleId::evaluate(RuleWithActions *rule, Transaction *transaction) {
rule->m_ruleId = m_ruleId;
rule->setId(m_ruleId);
return true;
}

2
src/actions/transformations/escape_seq_decode.h
View File

@@ -35,7 +35,7 @@ class EscapeSeqDecode : public Transformation {
std::string evaluate(const std::string &exp,
Transaction *transaction) override;
int ansi_c_sequences_decode_inplace(unsigned char *input, int input_len);
static int ansi_c_sequences_decode_inplace(unsigned char *input, int input_len);
};
} // namespace transformations

2
src/actions/transformations/url_encode.h
View File

@@ -36,7 +36,7 @@ class UrlEncode : public Transformation {
std::string evaluate(const std::string &exp,
Transaction *transaction) override;
std::string url_enc(const char *input,
static std::string url_enc(const char *input,
unsigned int input_len, int *changed);
};

42
src/parser/driver.cc
View File

@@ -88,77 +88,79 @@ int Driver::addSecRule(std::unique_ptr<RuleWithActions> r) {
m_parserError << " chain starter rules.";
return false;
}
m_lastRule->m_chainedRuleChild = std::move(r);
m_lastRule->m_chainedRuleChild->m_chainedRuleParent = m_lastRule;
m_lastRule = m_lastRule->m_chainedRuleChild.get();
m_lastRule->setChainedNext(std::move(r));
m_lastRule->getChainedNext()->setChainedParent(m_lastRule);
m_lastRule = m_lastRule->getChainedNext();
/* Lets set all meta-data to the first rule */
RuleWithActions *firstRule = m_lastRule;
if (!firstRule->hasChainAction()) {
while (firstRule->m_chainedRuleParent != nullptr) {
while (firstRule->getChainedParent() != nullptr) {
if (firstRule->hasMessageAction()) {
firstRule->m_chainedRuleParent->setMessageAction(
firstRule->getChainedParent()->setMessageAction(
firstRule->getMessageAction()
);
firstRule->setMessageAction(nullptr);
}
if (firstRule->hasLogDataAction()) {
firstRule->m_chainedRuleParent->setLogDataAction(
firstRule->getChainedParent()->setLogDataAction(
firstRule->getLogDataAction()
);
firstRule->setLogDataAction(nullptr);
}
if (firstRule->hasSeverityAction()) {
firstRule->m_chainedRuleParent->setSeverity(
firstRule->getChainedParent()->setSeverity(
firstRule->getSeverity()
);
}
if (firstRule->hasRevisionAction()) {
firstRule->m_chainedRuleParent->setRevision(
firstRule->getChainedParent()->setRevision(
firstRule->getRevision()
);
}
if (firstRule->hasVersionAction()) {
firstRule->m_chainedRuleParent->setVersion(
firstRule->getChainedParent()->setVersion(
firstRule->getVersion()
);
}
if (firstRule->hasAccuracyAction()) {
firstRule->m_chainedRuleParent->setAccuracy(
firstRule->getChainedParent()->setAccuracy(
firstRule->getAccuracy()
);
}
if (firstRule->hasMaturityAction()) {
firstRule->m_chainedRuleParent->setMaturity(
firstRule->getChainedParent()->setMaturity(
firstRule->getMaturity()
);
}
if (firstRule->hasTagAction()) {
firstRule->m_chainedRuleParent->setTags(
firstRule->getChainedParent()->setTags(
firstRule->getTagsAction()
);
firstRule->cleanTags();
}
if (firstRule->hasDisruptiveAction()) {
firstRule->m_chainedRuleParent->setDisruptiveAction(
firstRule->getChainedParent()->setDisruptiveAction(
firstRule->getDisruptiveAction()
);
firstRule->setDisruptiveAction(nullptr);
}
firstRule->m_chainedRuleParent->setHasBlockAction(
firstRule->getChainedParent()->setHasBlockAction(
firstRule->hasBlockAction()
);
firstRule->m_chainedRuleParent->setHasLogAction(
firstRule->getChainedParent()->setHasLogAction(
firstRule->hasLogAction()
);
firstRule->m_chainedRuleParent->setHasLogAction(
firstRule->getChainedParent()->setHasLogAction(
firstRule->hasNoLogAction()
);
firstRule = firstRule->m_chainedRuleParent;
firstRule = firstRule->getChainedParent();
}
}
return true;
}
@@ -167,7 +169,7 @@ int Driver::addSecRule(std::unique_ptr<RuleWithActions> r) {
* Checking if the rule has an ID and also checking if this ID is not used
* by other rule
*/
if (rule->m_ruleId == 0) {
if (rule->getId() == 0) {
m_parserError << "Rules must have an ID. File: ";
m_parserError << rule->getFileName() << " at line: ";
m_parserError << std::to_string(rule->getLineNumber()) << std::endl;
@@ -178,8 +180,8 @@ int Driver::addSecRule(std::unique_ptr<RuleWithActions> r) {
Rules *rules = m_rulesSetPhases[i];
for (int j = 0; j < rules->size(); j++) {
RuleWithOperator *lr = dynamic_cast<RuleWithOperator *>(rules->at(j).get());
if (lr && lr->m_ruleId == rule->m_ruleId) {
m_parserError << "Rule id: " << std::to_string(rule->m_ruleId) \
if (lr && lr->getId() == rule->getId()) {
m_parserError << "Rule id: " << std::to_string(rule->getId()) \
<< " is duplicated" << std::endl;
return false;
}

7
src/rule_script.cc
View File

@@ -26,15 +26,12 @@ bool RuleScript::evaluate(Transaction *trans) {
ms_dbg_a(trans, 4, " Executing script: " + m_name + ".");
bool containsDisruptive = false;
executeActionsIndependentOfChainedRuleResult(trans,
&containsDisruptive);
executeActionsIndependentOfChainedRuleResult(trans);
bool ret = m_lua->run(trans);
if (ret) {
executeActionsAfterFullMatch(trans, containsDisruptive);
executeActionsAfterFullMatch(trans);
}
return ret;

10
src/rule_unconditional.cc
View File

@@ -22,16 +22,12 @@ namespace modsecurity {
bool RuleUnconditional::evaluate(Transaction *trans) {
RuleWithActions::evaluate(trans);
// FIXME: This needs to be romeved on the runtime exeption review.
bool containsBlock = false;
ms_dbg_a(trans, 4, "(Rule: " + std::to_string(m_ruleId) \
ms_dbg_a(trans, 4, "(Rule: " + std::to_string(getId()) \
+ ") Executing unconditional rule...");
executeActionsIndependentOfChainedRuleResult(trans,
&containsBlock);
executeActionsIndependentOfChainedRuleResult(trans);
executeActionsAfterFullMatch(trans, containsBlock);
executeActionsAfterFullMatch(trans);
trans->logMatchLastRuleOnTheChain(this);

139
src/rule_with_actions.cc
View File

@@ -18,47 +18,38 @@
#include <stdio.h>
#include <algorithm>
#include <iostream>
#include <string>
#include <cstring>
#include <iostream>
#include <list>
#include <utility>
#include <memory>
#include <string>
#include <utility>
#include "modsecurity/rules_set.h"
#include "src/operators/operator.h"
#include "modsecurity/actions/action.h"
#include "modsecurity/modsecurity.h"
#include "src/actions/transformations/none.h"
#include "src/actions/tag.h"
#include "src/utils/string.h"
#include "modsecurity/rule_message.h"
#include "modsecurity/rules_set.h"
#include "modsecurity/rule_with_actions.h"
#include "src/actions/accuracy.h"
#include "src/actions/maturity.h"
#include "src/actions/block.h"
#include "src/actions/capture.h"
#include "src/actions/log_data.h"
#include "src/actions/msg.h"
#include "src/actions/maturity.h"
#include "src/actions/multi_match.h"
#include "src/actions/rev.h"
#include "src/actions/log.h"
#include "src/actions/no_log.h"
#include "src/actions/log_data.h"
#include "src/actions/rev.h"
#include "src/actions/severity.h"
#include "src/actions/capture.h"
#include "src/actions/multi_match.h"
#include "src/actions/set_var.h"
#include "src/actions/block.h"
#include "src/variables/variable.h"
#include "src/actions/severity.h"
#include "src/actions/tag.h"
#include "src/actions/transformations/transformation.h"
#include "src/actions/xmlns.h"
#include "src/utils/string.h"
namespace modsecurity {
using operators::Operator;
using actions::Action;
using variables::Variable;
using actions::transformations::None;
using actions::transformations::Transformation;
RuleWithActions::RuleWithActions(
Actions *actions,
Transformations *transformations,
@@ -74,6 +65,7 @@ RuleWithActions::RuleWithActions(
m_actionsRuntimePos(),
m_actionsSetVar(),
m_actionsTag(),
m_XmlNSs(),
m_defaultActionDisruptiveAction(nullptr),
m_defaultActionLogData(nullptr),
m_defaultActionMsg(nullptr),
@@ -215,8 +207,10 @@ void RuleWithActions::addAction(actions::Action *a) {
} else if (dynamic_cast<actions::Tag *>(a)) {
m_actionsTag.push_back(std::unique_ptr<actions::Tag>(dynamic_cast<actions::Tag *>(a)));
} else if (dynamic_cast<actions::Block *>(a)) {
m_actionsRuntimePos.push_back(std::unique_ptr<actions::Block>(dynamic_cast<actions::Block*>(a)));
m_actionsRuntimePos.push_back(std::unique_ptr<actions::Block>(dynamic_cast<actions::Block *>(a)));
m_containsStaticBlockAction = true;
} else if (dynamic_cast<actions::XmlNS *>(a)) {
m_XmlNSs.push_back(std::unique_ptr<actions::XmlNS>(dynamic_cast<actions::XmlNS *>(a)));
} else if (a->isDisruptive() == true) {
m_disruptiveAction = std::unique_ptr<Action>(a);
} else {
@@ -237,12 +231,6 @@ RuleWithActions::~RuleWithActions() { }
bool RuleWithActions::evaluate(Transaction *transaction) {
/* Rule evaluate is pure virtual.
*
* Rule::evaluate(transaction);
*/
/* Matched vars needs to be clear at every new rule execution */
transaction->m_matched.clear();
@@ -250,8 +238,7 @@ bool RuleWithActions::evaluate(Transaction *transaction) {
}
void RuleWithActions::executeActionsIndependentOfChainedRuleResult(Transaction *trans,
bool *containsBlock) {
void RuleWithActions::executeActionsIndependentOfChainedRuleResult(Transaction *trans) {
for (actions::SetVar *a : getSetVarsActionsPtr()) {
ms_dbg_a(trans, 4, "Running [independent] (non-disruptive) " \
@@ -268,7 +255,6 @@ void RuleWithActions::executeActionsIndependentOfChainedRuleResult(Transaction *
actions::Action *a = dynamic_cast<actions::Action*>(b.second.get());
if (a->isDisruptive() == true && *a->m_name.get() == "block") {
ms_dbg_a(trans, 9, "Rule contains a `block' action");
*containsBlock = true;
} else if (*a->m_name.get() == "setvar") {
ms_dbg_a(trans, 4, "Running [independent] (non-disruptive) " \
"action: " + *a->m_name.get());
@@ -278,8 +264,7 @@ void RuleWithActions::executeActionsIndependentOfChainedRuleResult(Transaction *
}
void RuleWithActions::executeActionsAfterFullMatch(Transaction *trans,
bool containsBlock) {
void RuleWithActions::executeActionsAfterFullMatch(Transaction *trans) {
bool disruptiveAlreadyExecuted = false;
#if 0
@@ -288,7 +273,8 @@ void RuleWithActions::executeActionsAfterFullMatch(Transaction *trans,
continue;
}
if (!a.get()->isDisruptive()) {
executeAction(trans, containsBlock, a.get(), true);
executeAction(trans, a.get(), true);
}
}
#endif
@@ -299,13 +285,18 @@ void RuleWithActions::executeActionsAfterFullMatch(Transaction *trans,
a->evaluate(this, trans, *trans->messageGetLast());
}
/**
*
* FIXME: SecRuleUpdateActionBy should be runtime
*
*/
for (auto &b :
trans->m_rules->m_exceptions.m_action_pos_update_target_by_id) {
if (m_ruleId != b.first) {
continue;
}
actions::Action *a = dynamic_cast<actions::Action*>(b.second.get());
executeAction(trans, containsBlock, a, false);
executeAction(trans, a, false);
disruptiveAlreadyExecuted = true;
}
@@ -325,22 +316,22 @@ void RuleWithActions::executeActionsAfterFullMatch(Transaction *trans,
if (!a->isDisruptive()
&& !(disruptiveAlreadyExecuted
&& dynamic_cast<actions::Block *>(a))) {
executeAction(trans, containsBlock, a, false);
executeAction(trans, a, false);
}
}
if (!disruptiveAlreadyExecuted && m_disruptiveAction != nullptr) {
executeAction(trans, containsBlock,
executeAction(trans,
m_disruptiveAction.get(), false);
} else if (!disruptiveAlreadyExecuted && hasBlockAction()
&& m_defaultActionDisruptiveAction != nullptr) {
executeAction(trans, containsBlock,
executeAction(trans,
m_defaultActionDisruptiveAction.get(), false);
}
}
void RuleWithActions::executeAction(Transaction *trans,
bool containsBlock,
Action *a, bool defaultContext) {
if (a->isDisruptive() == false && *a->m_name.get() != "block") {
ms_dbg_a(trans, 9, "Running " \
@@ -349,15 +340,15 @@ void RuleWithActions::executeAction(Transaction *trans,
return;
}
if (defaultContext && !containsBlock) {
if (defaultContext && !hasBlockAction()) {
ms_dbg_a(trans, 4, "Ignoring action: " + *a->m_name.get() + \
" (rule does not cotains block)");
return;
}
if (trans->getRuleEngineState() == RulesSet::EnabledRuleEngine) {
ms_dbg_a(trans, 4, "Running (disruptive) action: " + *a->m_name.get() + \
".");
ms_dbg_a(trans, 4, "Running (disruptive) action: " +
*a->m_name.get() + ".");
a->evaluate(this, trans, *trans->messageGetLast());
return;
}
@@ -372,8 +363,7 @@ inline void RuleWithActions::executeTransformation(
std::shared_ptr<std::string> *value,
Transaction *trans,
TransformationResults *ret,
std::string *path,
int *nth) const {
std::string *path) const {
std::string *oldValue = (*value).get();
std::string newValue = a->evaluate(*oldValue, trans);
@@ -382,7 +372,6 @@ inline void RuleWithActions::executeTransformation(
std::shared_ptr<std::string> u(new std::string(newValue));
if (m_containsMultiMatchAction) {
ret->push_back(std::make_pair(u, a->m_name));
(*nth)++;
}
*value = u;
}
@@ -393,16 +382,16 @@ inline void RuleWithActions::executeTransformation(
path->append("," + *a->m_name.get());
}
ms_dbg_a(trans, 9, " T (" + \
std::to_string(*nth) + ") " + \
ms_dbg_a(trans, 9, "Transformation " + \
*a->m_name.get() + ": \"" + \
utils::string::limitTo(80, newValue) +"\"");
utils::string::limitTo(80, newValue) + "\"");
}
void RuleWithActions::executeTransformations(
Transaction *trans, const std::string &in, TransformationResults &ret) {
Transaction *trans,
const std::string &in,
TransformationResults &ret) {
int none = 0;
int transformations = 0;
std::string path("");
std::shared_ptr<std::string> value =
std::shared_ptr<std::string>(new std::string(in));
@@ -422,9 +411,7 @@ void RuleWithActions::executeTransformations(
for (Transformation *a : getTransformationPtr()) {
if (none == 0) {
Transformation *t = dynamic_cast<Transformation *>(a);
executeTransformation(t, &value, trans, &ret, &path,
&transformations);
executeTransformation(a, &value, trans, &ret, &path);
}
if (a->m_isNone) {
none--;
@@ -451,9 +438,7 @@ void RuleWithActions::executeTransformations(
}
Transformation *a = dynamic_cast<Transformation*>(b.second.get());
if (none == 0) {
Transformation *t = dynamic_cast<Transformation *>(a);
executeTransformation(t, &value, trans, &ret, &path,
&transformations);
executeTransformation(a, &value, trans, &ret, &path);
}
if (a->m_isNone) {
none--;
@@ -489,42 +474,6 @@ bool RuleWithActions::containsMsg(const std::string& name, Transaction *t) {
}
std::vector<actions::Action *> RuleWithActions::getActionsByName(const std::string& name,
Transaction *trans) {
std::vector<actions::Action *> ret;
for (auto &z : getMatchActionsPtr()) {
if (*z->m_name.get() == name) {
ret.push_back(z);
}
}
for (auto &z : getTransformationPtr()) {
if (*z->m_name.get() == name) {
ret.push_back(z);
}
}
for (auto &b :
trans->m_rules->m_exceptions.m_action_pre_update_target_by_id) {
if (m_ruleId != b.first) {
continue;
}
actions::Action *z = dynamic_cast<actions::Action*>(b.second.get());
if (*z->m_name.get() == name) {
ret.push_back(z);
}
}
for (auto &b :
trans->m_rules->m_exceptions.m_action_pos_update_target_by_id) {
if (m_ruleId != b.first) {
continue;
}
actions::Action *z = dynamic_cast<actions::Action*>(b.second.get());
if (*z->m_name.get() == name) {
ret.push_back(z);
}
}
return ret;
}
std::string RuleWithActions::getLogData(Transaction *t) { return m_logData->data(t); }
std::string RuleWithActions::getMessage(Transaction *t) { return m_msg->data(t); }

28
src/rule_with_operator.cc
View File

@@ -150,7 +150,7 @@ void RuleWithOperator::getVariablesExceptions(Transaction *t,
}
for (auto &a : t->m_rules->m_exceptions.m_variable_update_target_by_id) {
if (m_ruleId != a.first) {
if (getId() != a.first) {
continue;
}
Variable *b = a.second.get();
@@ -178,7 +178,7 @@ inline void RuleWithOperator::getFinalVars(variables::Variables *vars,
if (std::find_if(trans->m_ruleRemoveTargetById.begin(),
trans->m_ruleRemoveTargetById.end(),
[&, variable, this](std::pair<int, std::string> &m) -> bool {
return m.first == m_ruleId
return m.first == getId()
&& m.second == *variable->m_fullName.get();
}) != trans->m_ruleRemoveTargetById.end()) {
continue;
@@ -216,18 +216,18 @@ bool RuleWithOperator::evaluate(Transaction *trans) {
// FIXME: Make a class runTimeException to handle this cases.
for (auto &i : trans->m_ruleRemoveById) {
if (m_ruleId != i) {
if (getId() != i) {
continue;
}
ms_dbg_a(trans, 9, "Rule id: " + std::to_string(m_ruleId) +
ms_dbg_a(trans, 9, "Rule id: " + std::to_string(getId()) +
" was skipped due to a ruleRemoveById action...");
return true;
}
for (auto &i : trans->m_ruleRemoveByIdRange) {
if (!(i.first <= m_ruleId && i.second >= m_ruleId)) {
if (!(i.first <= getId() && i.second >= getId())) {
continue;
}
ms_dbg_a(trans, 9, "Rule id: " + std::to_string(m_ruleId) +
ms_dbg_a(trans, 9, "Rule id: " + std::to_string(getId()) +
" was skipped due to a ruleRemoveById action...");
return true;
}
@@ -241,14 +241,15 @@ bool RuleWithOperator::evaluate(Transaction *trans) {
} else {
eparam = "\"" + eparam + "\"";
}
ms_dbg_a(trans, 4, "(Rule: " + std::to_string(m_ruleId) \
ms_dbg_a(trans, 4, "(Rule: " + std::to_string(getId()) \
+ ") Executing operator \"" + getOperatorName() \
+ "\" with param " \
+ eparam \
+ " against " \
+ variables + ".");
} else {
ms_dbg_a(trans, 4, "(Rule: " + std::to_string(m_ruleId) \
ms_dbg_a(trans, 4, "(Rule: " + std::to_string(getId()
) \
+ ") Executing operator \"" + getOperatorName() \
+ " against " \
+ variables + ".");
@@ -271,7 +272,7 @@ bool RuleWithOperator::evaluate(Transaction *trans) {
std::find_if(trans->m_ruleRemoveTargetById.begin(),
trans->m_ruleRemoveTargetById.end(),
[&, v, this](std::pair<int, std::string> &m) -> bool {
return m.first == m_ruleId && m.second == v->getKeyWithCollection();
return m.first == getId() && m.second == v->getKeyWithCollection();
}) != trans->m_ruleRemoveTargetById.end()
) {
delete v;
@@ -311,8 +312,7 @@ bool RuleWithOperator::evaluate(Transaction *trans) {
trans->messageGetLast()->m_reference.append(*valueTemp.second);
updateMatchedVars(trans, key, valueAfterTrans);
executeActionsIndependentOfChainedRuleResult(trans,
&containsBlock);
executeActionsIndependentOfChainedRuleResult(trans);
globalRet = true;
}
@@ -336,14 +336,14 @@ bool RuleWithOperator::evaluate(Transaction *trans) {
}
/* FIXME: this check should happens on the parser. */
if (this->m_chainedRuleChild == nullptr) {
if (getChainedNext() == nullptr) {
ms_dbg_a(trans, 4, "Rule is marked as chained but there " \
"isn't a subsequent rule.");
goto end_clean;
}
ms_dbg_a(trans, 4, "Executing chained rule.");
recursiveGlobalRet = m_chainedRuleChild->evaluate(trans);
recursiveGlobalRet = getChainedNext()->evaluate(trans);
if (recursiveGlobalRet == true) {
goto end_exec;
@@ -353,7 +353,7 @@ end_clean:
return false;
end_exec:
executeActionsAfterFullMatch(trans, containsBlock);
executeActionsAfterFullMatch(trans);
/* last rule in the chain. */
trans->logMatchLastRuleOnTheChain(this);

2
src/rules_set.cc
View File

@@ -198,7 +198,7 @@ int RulesSet::evaluate(int phase, Transaction *t) {
Rule *base = rule.get();
RuleWithActions *ruleWithActions = dynamic_cast<RuleWithActions *>(base);
// FIXME: Those should be treated inside the rule itself
if (ruleWithActions && m_exceptions.contains(ruleWithActions->m_ruleId)) {
if (ruleWithActions && m_exceptions.contains(ruleWithActions->getId())) {
ms_dbg_a(t, 9, "Skipped rule id '" + rule->getReference() \
+ "'. Removed by an SecRuleRemove directive.");
continue;

2
src/rules_set_phases.cc
View File

@@ -50,7 +50,7 @@ int RulesSetPhases::append(RulesSetPhases *from, std::ostringstream *err) {
if (!rule_ckc) {
continue;
}
v.push_back(rule_ckc->m_ruleId);
v.push_back(rule_ckc->getId());
}
}
std::sort (v.begin(), v.end());

4
src/variables/rule.h
View File

@@ -42,11 +42,11 @@ class Rule_DictElement : public VariableDictElement { \
std::vector<const VariableValue *> *l) {
RuleWithActions *r = rule;
if (!r || r->m_ruleId == 0) {
if (!r || r->getId() == 0) {
return;
}
std::unique_ptr<VariableOrigin> origin(new VariableOrigin());
std::string *a = new std::string(std::to_string(r->m_ruleId));
std::string *a = new std::string(std::to_string(r->getId()));
VariableValue *var = new VariableValue(&m_rule, &m_rule_id,
a
);

5
src/variables/xml.cc
View File

@@ -89,9 +89,8 @@ void XML::evaluate(Transaction *t,
if (rule == NULL) {
ms_dbg_a(t, 2, "XML: Can't look for xmlns, internal error.");
} else {
std::vector<actions::Action *> acts = rule->getActionsByName("xmlns", t);
for (auto &x : acts) {
actions::XmlNS *z = (actions::XmlNS *)x;
std::vector<actions::XmlNS *> acts = rule->getXmlNSsPtr();
for (auto &z : acts) {
if (xmlXPathRegisterNs(xpathCtx, (const xmlChar*)z->m_scope.c_str(),
(const xmlChar*)z->m_href.c_str()) != 0) {
ms_dbg_a(t, 1, "Failed to register XML namespace href \"" + \