Skip to content

Commit 6421ff0

Browse files
author
Felipe Zimmerle
committed
Forces disruptive to be first-rule-only
ModSecurity version 3 is capable to handle disruptive actions in different rules from the chain. However, lets get it working in the same fashion that we have in version 2.
1 parent 7e59250 commit 6421ff0

File tree

9 files changed

+70
-36
lines changed

9 files changed

+70
-36
lines changed

‎headers/modsecurity/rule.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ class Rule {
7676
std::vector<std::string> getActionNames();
7777
std::vector<actions::Action *> getActionsByName(const std::string& name);
7878
bool containsTag(const std::string& name, Transaction *t);
79-
79+
bool containsDisruptiveAction();
8080

8181
int refCountDecreaseAndCheck() {
8282
m_referenceCount--;

‎src/actions/disruptive/allow.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ class Allow : public Action {
6060

6161
bool init(std::string *error) override;
6262
bool evaluate(Rule *rule, Transaction *transaction) override;
63+
bool isDisruptive() override { return true; }
6364

6465
AllowType m_allowType;
6566

‎src/parser/driver.cc

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -70,23 +70,34 @@ int Driver::addSecRule(Rule *rule) {
7070
return false;
7171
}
7272

73-
if (lastRule && lastRule->m_chained && lastRule->m_chainedRule == NULL) {
74-
rule->m_phase = lastRule->m_phase;
75-
lastRule->m_chainedRule = rule;
76-
return true;
77-
}
78-
79-
if (lastRule && lastRule->m_chained && lastRule->m_chainedRule != NULL) {
80-
Rule *a = lastRule->m_chainedRule;
81-
while (a->m_chained && a->m_chainedRule != NULL) {
82-
a = a->m_chainedRule;
83-
}
84-
if (a->m_chained && a->m_chainedRule == NULL) {
85-
a->m_chainedRule = rule;
73+
if (lastRule && lastRule->m_chained) {
74+
if (lastRule->m_chainedRule == NULL) {
75+
rule->m_phase = lastRule->m_phase;
76+
lastRule->m_chainedRule = rule;
77+
if (rule->containsDisruptiveAction()) {
78+
m_parserError << "Disruptive actions can only be specified by";
79+
m_parserError << " chain starter rules.";
80+
return false;
81+
}
8682
return true;
83+
} else {
84+
Rule *a = lastRule->m_chainedRule;
85+
while (a->m_chained && a->m_chainedRule != NULL) {
86+
a = a->m_chainedRule;
87+
}
88+
if (a->m_chained && a->m_chainedRule == NULL) {
89+
a->m_chainedRule = rule;
90+
if (a->containsDisruptiveAction()) {
91+
m_parserError << "Disruptive actions can only be ";
92+
m_parserError << "specified by chain starter rules.";
93+
return false;
94+
}
95+
return true;
96+
}
8797
}
8898
}
8999

100+
90101
/*
91102
* Checking if the rule has an ID and also checking if this ID is not used
92103
* by other rule

‎src/rule.cc

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -706,6 +706,27 @@ bool Rule::evaluate(Transaction *trans,
706706
}
707707

708708

709+
bool Rule::containsDisruptiveAction() {
710+
for (Action *a : m_actionsRuntimePos) {
711+
if (a->isDisruptive() == true) {
712+
return true;
713+
}
714+
}
715+
for (Action *a : m_actionsRuntimePre) {
716+
if (a->isDisruptive() == true) {
717+
return true;
718+
}
719+
}
720+
for (Action *a : m_actionsConf) {
721+
if (a->isDisruptive() == true) {
722+
return true;
723+
}
724+
}
725+
726+
return false;
727+
}
728+
729+
709730
std::vector<actions::Action *> Rule::getActionsByName(const std::string& name) {
710731
std::vector<actions::Action *> ret;
711732
for (auto &z : m_actionsRuntimePos) {

‎test/test-cases/regression/config-calling_phases_by_name.json

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
{
33
"enabled":1,
44
"version_min":300000,
5-
"title":"Testing Variables :: MATCHED_VAR (1/2)",
5+
"title":"Testing Config :: Phases by name (1/2)",
66
"client":{
77
"ip":"200.249.12.31",
88
"port":123
@@ -35,14 +35,14 @@
3535
},
3636
"rules":[
3737
"SecRuleEngine On",
38-
"SecRule ARGS:key \"@contains other_value\" \"id:1,phase:request,chain\"",
39-
"SecRule MATCHED_VAR \"@contains asdf\" \"phase:request,pass\""
38+
"SecRule ARGS:key \"@contains other_value\" \"id:1,phase:request,pass,chain\"",
39+
"SecRule MATCHED_VAR \"@contains asdf\" \"\""
4040
]
4141
},
4242
{
4343
"enabled":1,
4444
"version_min":300000,
45-
"title":"Testing Variables :: MATCHED_VAR (2/2)",
45+
"title":"Testing Config :: Phases by name (2/2)",
4646
"client":{
4747
"ip":"200.249.12.31",
4848
"port":123
@@ -75,8 +75,8 @@
7575
},
7676
"rules":[
7777
"SecRuleEngine On",
78-
"SecRule ARGS:key \"@contains other_value\" \"chain,phase:response,id:28\"",
79-
"SecRule MATCHED_VAR \"@contains Aasdf\" \"pass\"",
78+
"SecRule ARGS:key \"@contains other_value\" \"chain,pass,phase:response,id:28\"",
79+
"SecRule MATCHED_VAR \"@contains Aasdf\" \"\"",
8080
"SecRule MATCHED_VAR \"@contains other_value\" \"id:29,phase:response,pass\"",
8181
"SecRule MATCHED_VAR \"@contains other_value\" \"id:30,phase:response,pass\""
8282
]

‎test/test-cases/regression/variable-MATCHED_VAR.json

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,8 @@
3535
},
3636
"rules":[
3737
"SecRuleEngine On",
38-
"SecRule ARGS:key \"@contains other_value\" \"chain,id:28\"",
39-
"SecRule MATCHED_VAR \"@contains asdf\" \"pass\""
38+
"SecRule ARGS:key \"@contains other_value\" \"chain,id:28,pass\"",
39+
"SecRule MATCHED_VAR \"@contains asdf\" \"\""
4040
]
4141
},
4242
{
@@ -75,8 +75,9 @@
7575
},
7676
"rules":[
7777
"SecRuleEngine On",
78-
"SecRule ARGS:key \"@contains other_value\" \"chain,id:28\"",
79-
"SecRule MATCHED_VAR \"@contains Aasdf\" \"pass\"",
78+
"SecRule ARGS:key \"@contains other_value\" \"chain,id:28,pass\"",
79+
"SecRule MATCHED_VAR \"@contains Aasdf\" \"\"",
80+
8081
"SecRule MATCHED_VAR \"@contains other_value\" \"id:29,pass\"",
8182
"SecRule MATCHED_VAR \"@contains other_value\" \"id:30,pass\""
8283
]

‎test/test-cases/regression/variable-MATCHED_VARS.json

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,9 @@
3535
},
3636
"rules":[
3737
"SecRuleEngine On",
38-
"SecRule ARGS:keyI \"@contains value\" \"chain,id:28\"",
38+
"SecRule ARGS:keyI \"@contains value\" \"chain,id:28,pass\"",
3939
"SecRule ARGS:keyII \"@contains other_value\" \"chain\"",
40-
"SecRule MATCHED_VARS \"@contains asdf\" \"pass\""
40+
"SecRule MATCHED_VARS \"@contains asdf\" \"\""
4141
]
4242
},
4343
{
@@ -76,9 +76,9 @@
7676
},
7777
"rules":[
7878
"SecRuleEngine On",
79-
"SecRule ARGS:keyI \"@contains value\" \"chain,id:28\"",
79+
"SecRule ARGS:keyI \"@contains value\" \"chain,id:28,pass\"",
8080
"SecRule ARGS:keyII \"@contains other_value\" \"chain\"",
81-
"SecRule MATCHED_VARS \"@contains asdf\" \"pass\"",
81+
"SecRule MATCHED_VARS \"@contains asdf\" \"\"",
8282
"SecRule MATCHED_VARS \"@contains value\" \"id:29\""
8383
]
8484
}

‎test/test-cases/regression/variable-MATCHED_VARS_NAMES.json

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,9 @@
3535
},
3636
"rules":[
3737
"SecRuleEngine On",
38-
"SecRule ARGS:keyI \"@contains value\" \"chain,id:28\"",
38+
"SecRule ARGS:keyI \"@contains value\" \"chain,id:28,pass\"",
3939
"SecRule ARGS:keyII \"@contains other_value\" \"chain\"",
40-
"SecRule MATCHED_VARS_NAMES \"@contains asdf\" \"pass\""
40+
"SecRule MATCHED_VARS_NAMES \"@contains asdf\" \"\""
4141
]
4242
},
4343
{
@@ -76,9 +76,9 @@
7676
},
7777
"rules":[
7878
"SecRuleEngine On",
79-
"SecRule ARGS:keyI \"@contains value\" \"chain,id:28\"",
79+
"SecRule ARGS:keyI \"@contains value\" \"chain,id:28,pass\"",
8080
"SecRule ARGS:keyII \"@contains other_value\" \"chain\"",
81-
"SecRule MATCHED_VARS_NAMES \"@contains asdf\" \"pass\"",
81+
"SecRule MATCHED_VARS_NAMES \"@contains asdf\" \"\"",
8282
"SecRule MATCHED_VARS_NAMES \"@contains value\" \"id:29\""
8383
]
8484
}

‎test/test-cases/regression/variable-MATCHED_VAR_NAME.json

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,9 +35,9 @@
3535
},
3636
"rules":[
3737
"SecRuleEngine On",
38-
"SecRule ARGS:keyI \"@contains value\" \"chain,id:28\"",
38+
"SecRule ARGS:keyI \"@contains value\" \"chain,id:28,pass\"",
3939
"SecRule ARGS:keyII \"@contains other_value\" \"chain\"",
40-
"SecRule MATCHED_VAR_NAME \"@contains asdf\" \"pass\""
40+
"SecRule MATCHED_VAR_NAME \"@contains asdf\" \"\""
4141
]
4242
},
4343
{
@@ -76,9 +76,9 @@
7676
},
7777
"rules":[
7878
"SecRuleEngine On",
79-
"SecRule ARGS:keyI \"@contains value\" \"chain,id:28\"",
79+
"SecRule ARGS:keyI \"@contains value\" \"chain,id:28,pass\"",
8080
"SecRule ARGS:keyII \"@contains other_value\" \"chain\"",
81-
"SecRule MATCHED_VAR_NAME \"@contains asdf\" \"pass\"",
81+
"SecRule MATCHED_VAR_NAME \"@contains asdf\" \"\"",
8282
"SecRule MATCHED_VAR_NAME \"@contains value\" \"id:29\""
8383
]
8484
}

0 commit comments

Comments
 (0)