Adds support to config warnings

This commit is contained in:
Felipe Zimmerle 2020-12-22 18:20:46 -03:00
parent bf87f11036
commit 62d35fbf97
No known key found for this signature in database
GPG Key ID: E6DFB08CE8B11277
17 changed files with 1197 additions and 939 deletions

View File

@ -127,6 +127,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-warning.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

View File

@ -97,6 +97,7 @@ int main (int argc, char **argv)
{
int ret;
const char *error = NULL;
const char *warn = NULL;
int i = 0;
pid_t pid;
int f;
@ -108,7 +109,7 @@ int main (int argc, char **argv)
rules = msc_create_rules_set();
ret = msc_rules_add_file(rules, main_rule_uri, &error);
ret = msc_rules_add_file(rules, main_rule_uri, &warn, &error);
if (ret < 0) {
fprintf(stderr, "Problems loading the rules --\n");
fprintf(stderr, "%s\n", error);

View File

@ -27,6 +27,7 @@ int main (int argc, char **argv)
{
int ret;
const char *error = NULL;
const char *warn = NULL;
ModSecurity *modsec;
Transaction *transaction = NULL;
RulesSet *rules;
@ -38,7 +39,7 @@ int main (int argc, char **argv)
rules = msc_create_rules_set();
ret = msc_rules_add_file(rules, main_rule_uri, &error);
ret = msc_rules_add_file(rules, main_rule_uri, &warn, &error);
if (ret < 0) {
fprintf(stderr, "Problems loading the rules --\n");
fprintf(stderr, "%s\n", error);
@ -48,7 +49,7 @@ int main (int argc, char **argv)
ret = msc_rules_add_remote(rules, "test",
"https://www.modsecurity.org/modsecurity-regression-test-secremoterules.txt",
&error);
&warn, &error);
if (ret < 0) {
fprintf(stderr, "Problems loading the rules --\n");
fprintf(stderr, "%s\n", error);

View File

@ -120,6 +120,19 @@ class Action {
}
};
class ActionNotSupported : public Action {
public:
ActionNotSupported() : Action()
{ };
explicit ActionNotSupported(const std::string& action)
: Action(action)
{ };
ActionNotSupported(const ActionNotSupported &other) = delete;
ActionNotSupported &operator=(const ActionNotSupported& a) = delete;
};
} // namespace actions
} // namespace modsecurity

View File

@ -75,6 +75,7 @@ class RulesSet : public RulesSetProperties {
int evaluate(int phase, Transaction *transaction);
std::string getParserError();
std::string getParserWarnings();
void debug(int level, const std::string &id, const std::string &uri,
const std::string &msg);
@ -95,11 +96,14 @@ extern "C" {
RulesSet *msc_create_rules_set(void);
void msc_rules_dump(RulesSet *rules);
int msc_rules_merge(RulesSet *rules_dst, RulesSet *rules_from, const char **error);
int msc_rules_merge(RulesSet *rules_dst, RulesSet *rules_from,
const char **warn, const char **error);
int msc_rules_add_remote(RulesSet *rules, const char *key, const char *uri,
const char **error);
int msc_rules_add_file(RulesSet *rules, const char *file, const char **error);
int msc_rules_add(RulesSet *rules, const char *plain_rules, const char **error);
const char **warn, const char **error);
int msc_rules_add_file(RulesSet *rules, const char *file,
const char **warn, const char **error);
int msc_rules_add(RulesSet *rules, const char *plain_rules,
const char **warn, const char **error);
int msc_rules_cleanup(RulesSet *rules);
#ifdef __cplusplus

View File

@ -457,6 +457,7 @@ class RulesSetProperties {
RulesExceptions m_exceptions;
std::list<std::string> m_components;
std::ostringstream m_parserError;
std::ostringstream m_parserWarn;
ConfigSet m_responseBodyTypeToBeInspected;
ConfigString m_httpblKey;
ConfigString m_uploadDirectory;

View File

@ -180,7 +180,7 @@ int Driver::addSecRule(std::unique_ptr<RuleWithActions> r) {
*/
if (rule->getId() == 0) {
m_parserError << "Rules must have an ID. File: ";
m_parserError << rule->getFileName() << " at line: ";
m_parserError << *rule->getFileName() << " at line: ";
m_parserError << std::to_string(rule->getLineNumber()) << std::endl;
return false;
}
@ -282,5 +282,30 @@ void Driver::error(const yy::location& l, const std::string& m,
}
void Driver::warn(const yy::location& l, const std::string& m) {
warn(l, m, "");
}
void Driver::warn(const yy::location& l, const std::string& m,
const std::string& c) {
if (m_parserWarn.tellp() != 0) {
m_parserWarn << std::endl;
}
m_parserWarn << "Warning. ";
m_parserWarn << "File: " << *l.end.filename << ". ";
m_parserWarn << "Line: " << l.end.line << ". ";
m_parserWarn << "Column: " << l.end.column - 1 << ". ";
if (m.empty() == false) {
m_parserWarn << "" << m << " ";
}
if (c.empty() == false) {
m_parserWarn << c;
}
}
} // namespace Parser
} // namespace modsecurity

View File

@ -86,6 +86,10 @@ class Driver : public RulesSetProperties {
void error(const yy::location& l, const std::string& m,
const std::string& c);
void warn(const yy::location& l, const std::string& m);
void warn(const yy::location& l, const std::string& m,
const std::string& c);
std::list<yy::location *> loc;
std::string buffer;

File diff suppressed because it is too large Load Diff

View File

@ -324,7 +324,13 @@ using namespace modsecurity::operators;
if (t)
#define ACTION_NOT_SUPPORTED(a, b) \
#define ACTION_NOT_SUPPORTED(a, b, c) \
std::unique_ptr<actions::Action> d(new actions::ActionNotSupported(b)); \
a = std::move(d); \
driver.warn(c, "Action " + std::string(b) + " is not supported in version 3.");
#define ACTION_NOT_SUPPORTED_OLD(a, b) \
driver.error(b, "Action: " + std::string(a) + " is not yet supported."); \
YYERROR;
@ -354,7 +360,7 @@ using namespace modsecurity::operators;
a = std::move(c);
#line 358 "seclang-parser.hh"
#line 364 "seclang-parser.hh"
# include <cassert>
# include <cstdlib> // std::abort
@ -488,7 +494,7 @@ using namespace modsecurity::operators;
#endif
namespace yy {
#line 492 "seclang-parser.hh"
#line 498 "seclang-parser.hh"
@ -8629,7 +8635,7 @@ switch (yykind)
}
} // yy
#line 8633 "seclang-parser.hh"
#line 8639 "seclang-parser.hh"

View File

@ -285,7 +285,13 @@ using namespace modsecurity::operators;
if (t)
#define ACTION_NOT_SUPPORTED(a, b) \
#define ACTION_NOT_SUPPORTED(a, b, c) \
std::unique_ptr<actions::Action> d(new actions::ActionNotSupported(b)); \
a = std::move(d); \
driver.warn(c, "Action " + std::string(b) + " is not supported in version 3.");
#define ACTION_NOT_SUPPORTED_OLD(a, b) \
driver.error(b, "Action: " + std::string(a) + " is not yet supported."); \
YYERROR;
@ -1285,6 +1291,8 @@ expression:
}
| CONFIG_CONN_ENGINE CONFIG_VALUE_OFF
{
driver.error(@0, "SecConnEngine is not yet supported.");
YYERROR;
}
| CONFIG_SEC_WEB_APP_ID
{
@ -1308,6 +1316,8 @@ expression:
}
| CONFIG_SEC_DISABLE_BACKEND_COMPRESS CONFIG_VALUE_OFF
{
driver.error(@0, "SecDisableBackendCompression is not supported.");
YYERROR;
}
| CONFIG_CONTENT_INJECTION CONFIG_VALUE_ON
{
@ -1316,6 +1326,8 @@ expression:
}
| CONFIG_CONTENT_INJECTION CONFIG_VALUE_OFF
{
driver.error(@0, "SecContentInjection is not yet supported.");
YYERROR;
}
| CONFIG_SEC_CHROOT_DIR
{
@ -1329,6 +1341,8 @@ expression:
}
| CONFIG_SEC_HASH_ENGINE CONFIG_VALUE_OFF
{
driver.error(@0, "SecHashEngine is not yet supported.");
YYERROR;
}
| CONFIG_SEC_HASH_KEY
{
@ -1367,6 +1381,8 @@ expression:
}
| CONFIG_SEC_INTERCEPT_ON_ERROR CONFIG_VALUE_OFF
{
driver.error(@0, "SecInterceptOnError is not yet supported.");
YYERROR;
}
| CONFIG_SEC_CONN_R_STATE_LIMIT
{
@ -1390,6 +1406,8 @@ expression:
}
| CONFIG_SEC_RULE_INHERITANCE CONFIG_VALUE_OFF
{
driver.error(@0, "SecRuleInheritance is not yet supported.");
YYERROR;
}
| CONFIG_SEC_RULE_PERF_TIME
{
@ -1639,15 +1657,19 @@ expression:
driver.m_remoteRulesActionOnFailed = RulesSet::OnFailedRemoteRulesAction::WarnOnFailedRemoteRulesAction;
}
| CONFIG_DIR_PCRE_MATCH_LIMIT_RECURSION
{
/* Parser error disabled to avoid breaking default installations with modsecurity.conf-recommended
driver.error(@0, "SecPcreMatchLimitRecursion is not currently supported. Default PCRE values are being used for now");
YYERROR;
*/
}
| CONFIG_DIR_PCRE_MATCH_LIMIT
{
/* Parser error disabled to avoid breaking default installations with modsecurity.conf-recommended
driver.error(@0, "SecPcreMatchLimit is not currently supported. Default PCRE values are being used for now");
YYERROR;
*/
}
| CONGIG_DIR_RESPONSE_BODY_MP
{
std::istringstream buf($1);
@ -1686,6 +1708,7 @@ expression:
*/
}
| CONGIG_DIR_SEC_DATA_DIR
{
/* Parser error disabled to avoid breaking default installations with modsecurity.conf-recommended
std::stringstream ss;
ss << "SecDataDir is not currently supported.";
@ -1694,6 +1717,7 @@ expression:
driver.error(@0, ss.str());
YYERROR;
*/
}
| CONGIG_DIR_SEC_ARG_SEP
| CONGIG_DIR_SEC_COOKIE_FORMAT
{
@ -1708,10 +1732,12 @@ expression:
YYERROR;
}
| CONGIG_DIR_SEC_STATUS_ENGINE
{
/* Parser error disabled to avoid breaking default installations with modsecurity.conf-recommended
driver.error(@0, "SecStatusEngine is not yet supported.");
YYERROR;
*/
}
| CONFIG_DIR_UNICODE_MAP_FILE
{
std::string error;
@ -2606,7 +2632,7 @@ act:
}
| ACTION_APPEND
{
ACTION_NOT_SUPPORTED("Append", @0);
ACTION_NOT_SUPPORTED_OLD("Append", @0);
}
| ACTION_AUDIT_LOG
{
@ -2626,18 +2652,15 @@ act:
}
| ACTION_CTL_AUDIT_ENGINE CONFIG_VALUE_ON
{
ACTION_NOT_SUPPORTED("CtlAuditEngine", @0);
//ACTION_CONTAINER($$, new actions::Action($1));
ACTION_NOT_SUPPORTED_OLD("ctl:auditEngine", @0);
}
| ACTION_CTL_AUDIT_ENGINE CONFIG_VALUE_OFF
{
ACTION_NOT_SUPPORTED("CtlAuditEngine", @0);
//ACTION_CONTAINER($$, new actions::Action($1));
ACTION_NOT_SUPPORTED_OLD("ctl:auditEngine", @0);
}
| ACTION_CTL_AUDIT_ENGINE CONFIG_VALUE_RELEVANT_ONLY
{
ACTION_NOT_SUPPORTED("CtlAuditEngine", @0);
//ACTION_CONTAINER($$, new actions::Action($1));
ACTION_NOT_SUPPORTED_OLD("ctl:auditEngine", @0);
}
| ACTION_CTL_AUDIT_LOG_PARTS
{
@ -2657,13 +2680,11 @@ act:
}
| ACTION_CTL_FORCE_REQ_BODY_VAR CONFIG_VALUE_ON
{
ACTION_NOT_SUPPORTED("CtlForceRequestBodyVariable", @0);
//ACTION_CONTAINER($$, new actions::Action($1));
ACTION_NOT_SUPPORTED($$, "ctl:forceRequestBodyVariable", @0);
}
| ACTION_CTL_FORCE_REQ_BODY_VAR CONFIG_VALUE_OFF
{
ACTION_NOT_SUPPORTED("CtlForceRequestBodyVariable", @0);
//ACTION_CONTAINER($$, new actions::Action($1));
ACTION_NOT_SUPPORTED($$, "ctl:forceRequestBodyVariable", @0);
}
| ACTION_CTL_REQUEST_BODY_ACCESS CONFIG_VALUE_ON
{
@ -2707,7 +2728,7 @@ act:
}
| ACTION_DEPRECATE_VAR
{
ACTION_NOT_SUPPORTED("DeprecateVar", @0);
ACTION_NOT_SUPPORTED_OLD("DeprecateVar", @0);
}
| ACTION_DROP
{
@ -2763,7 +2784,7 @@ act:
}
| ACTION_PAUSE
{
ACTION_NOT_SUPPORTED("Pause", @0);
ACTION_NOT_SUPPORTED_OLD("Pause", @0);
}
| ACTION_PHASE
{
@ -2771,11 +2792,11 @@ act:
}
| ACTION_PREPEND
{
ACTION_NOT_SUPPORTED("Prepend", @0);
ACTION_NOT_SUPPORTED_OLD("Prepend", @0);
}
| ACTION_PROXY
{
ACTION_NOT_SUPPORTED("Proxy", @0);
ACTION_NOT_SUPPORTED_OLD("Proxy", @0);
}
| ACTION_REDIRECT run_time_string
{
@ -2787,23 +2808,23 @@ act:
}
| ACTION_SANITISE_ARG
{
ACTION_NOT_SUPPORTED("SanitiseArg", @0);
ACTION_NOT_SUPPORTED_OLD("SanitiseArg", @0);
}
| ACTION_SANITISE_MATCHED
{
ACTION_NOT_SUPPORTED("SanitiseMatched", @0);
ACTION_NOT_SUPPORTED_OLD("SanitiseMatched", @0);
}
| ACTION_SANITISE_MATCHED_BYTES
{
ACTION_NOT_SUPPORTED("SanitiseMatchedBytes", @0);
ACTION_NOT_SUPPORTED_OLD("SanitiseMatchedBytes", @0);
}
| ACTION_SANITISE_REQUEST_HEADER
{
ACTION_NOT_SUPPORTED("SanitiseRequestHeader", @0);
ACTION_NOT_SUPPORTED_OLD("SanitiseRequestHeader", @0);
}
| ACTION_SANITISE_RESPONSE_HEADER
{
ACTION_NOT_SUPPORTED("SanitiseResponseHeader", @0);
ACTION_NOT_SUPPORTED_OLD("SanitiseResponseHeader", @0);
}
| ACTION_SETENV run_time_string
{

View File

@ -87,7 +87,9 @@ RuleWithActions::RuleWithActions(
std::vector<std::shared_ptr<Action>> newActions;
if (actions) {
for (auto &a : *actions) {
if (std::dynamic_pointer_cast<ActionTypeRuleMetaData>(a)) {
if (std::dynamic_pointer_cast<actions::ActionNotSupported>(a)) {
continue;
} else if (std::dynamic_pointer_cast<ActionTypeRuleMetaData>(a)) {
confs.push_back(std::dynamic_pointer_cast<ActionTypeRuleMetaData>(a));
continue;
} else if (std::dynamic_pointer_cast<ActionDisruptive>(a)) {

View File

@ -58,6 +58,11 @@ int RulesSet::loadFromUri(const char *uri) {
return -1;
}
auto warn = driver->m_parserWarn.str();
if (warn.size() > 0) {
m_parserWarn << warn;
}
int rules = this->merge(driver);
delete driver;
@ -73,12 +78,19 @@ int RulesSet::load(const char *file, const std::string &ref) {
delete driver;
return -1;
}
int rules = this->merge(driver);
if (rules == -1) {
m_parserError << driver->m_parserError.str();
delete driver;
return -1;
}
auto warn = driver->m_parserWarn.str();
if (warn.size() > 0) {
m_parserWarn << warn;
}
delete driver;
return rules;
@ -150,10 +162,14 @@ bool RulesSet::containsDuplicatedIds(RulesWarnings *warning, RulesErrors *error)
std::string RulesSet::getParserError() {
return this->m_parserError.str();
return m_parserError.str();
}
std::string RulesSet::getParserWarnings() {
return m_parserWarn.str();
}
int RulesSet::evaluate(int phase, Transaction *t) {
if (phase >= modsecurity::Phases::NUMBER_OF_PHASES) {
return 0;
@ -344,41 +360,67 @@ extern "C" void msc_rules_dump(RulesSet *rules) {
extern "C" int msc_rules_merge(RulesSet *rules_dst,
RulesSet *rules_from, const char **error) {
RulesSet *rules_from,
const char **warn, const char **error) {
int ret = rules_dst->merge(rules_from);
if (ret < 0) {
*error = strdup(rules_dst->getParserError().c_str());
}
auto warnings = rules_dst->getParserWarnings();
if (warnings.size() > 0) {
*warn = strdup(warnings.c_str());
}
return ret;
}
extern "C" int msc_rules_add_remote(RulesSet *rules,
const char *key, const char *uri, const char **error) {
const char *key, const char *uri,
const char **warn, const char **error) {
int ret = rules->loadRemote(key, uri);
if (ret < 0) {
*error = strdup(rules->getParserError().c_str());
}
auto warnings = rules->getParserWarnings();
if (warnings.size() > 0) {
*warn = strdup(warnings.c_str());
}
return ret;
}
extern "C" int msc_rules_add_file(RulesSet *rules, const char *file,
const char **error) {
const char **warn, const char **error) {
int ret = rules->loadFromUri(file);
if (ret < 0) {
*error = strdup(rules->getParserError().c_str());
}
auto warnings = rules->getParserWarnings();
if (warnings.size() > 0) {
*warn = strdup(warnings.c_str());
}
return ret;
}
extern "C" int msc_rules_add(RulesSet *rules, const char *plain_rules,
const char **error) {
const char **warn, const char **error) {
int ret = rules->load(plain_rules);
if (ret < 0) {
*error = strdup(rules->getParserError().c_str());
}
auto warnings = rules->getParserWarnings();
if (warnings.size() > 0) {
*warn = strdup(warnings.c_str());
}
return ret;
}

View File

@ -157,7 +157,7 @@ modsecurity::ModSecurity *setupModSec() {
}
modsecurity::RulesSet *setupModSecRules(RegressionTestResult *r) {
modsecurity::RulesSet *setupModSecRules(RegressionTestResult *r, std::string &warning) {
CustomDebugLog *debug_log = new CustomDebugLog();
auto rules = new modsecurity::RulesSet(debug_log);
rules->load("SecDebugLogLevel 9");
@ -165,8 +165,11 @@ modsecurity::RulesSet *setupModSecRules(RegressionTestResult *r) {
if (rules->load(r->getRules().c_str(), r->getFileName()) >= 0 &&
r->getExpectedParserError().empty()) {
warning.assign(rules->getParserWarnings());
return rules;
}
warning.assign(rules->getParserWarnings());
if (!r->getExpectedParserError().empty()) {
Regex re(r->getExpectedParserError());
@ -287,6 +290,7 @@ void processLogs(RegressionTest *t,
const std::string &serverLog,
const std::string &audit_log,
const std::string &debug_log,
const std::string &parser_warning,
int status_code) {
@ -318,6 +322,13 @@ void processLogs(RegressionTest *t,
reason << KWHT << "Expecting: " << RESET \
<< t->audit_log + "";
testRes->failed(reason.str());
} else if (!contains(parser_warning, t->parser_warn)) {
std::stringstream reason;
reason << "Parser warning was not matching the " \
<< "expected results." << std::endl;
reason << KWHT << "Expecting: " << RESET \
<< t->parser_warn + "";
testRes->failed(reason.str());
} else {
testRes->passed();
return;
@ -331,6 +342,8 @@ void processLogs(RegressionTest *t,
testRes->reason << serverLog << std::endl;
testRes->reason << KWHT << "Audit log:" << RESET << std::endl;
testRes->reason << audit_log << std::endl;
testRes->reason << KWHT << "Parser warning:" << RESET << std::endl;
testRes->reason << parser_warning << std::endl;
}
}
@ -346,6 +359,7 @@ RegressionTestResult *perform_regression_test(
std::string error_log;
std::string audit_log;
std::string debug_log;
std::string parser_warning;
int status_code = 200;
if (t->enabled == 0) {
@ -363,7 +377,7 @@ RegressionTestResult *perform_regression_test(
goto ret;
}
modsec_rules = setupModSecRules(testRes);
modsec_rules = setupModSecRules(testRes, parser_warning);
if (modsec_rules == nullptr) {
goto ret;
}
@ -381,6 +395,7 @@ RegressionTestResult *perform_regression_test(
error_log,
audit_log,
debug_log,
parser_warning,
status_code);
ret:

View File

@ -197,6 +197,9 @@ RegressionTest *RegressionTest::from_yajl_node(const yajl_val &node) {
if (strcmp(key2, "parser_error") == 0) {
u->parser_error = YAJL_GET_STRING(val2);
}
if (strcmp(key2, "parser_warn") == 0) {
u->parser_warn = YAJL_GET_STRING(val2);
}
}
}
if (strcmp(key, "rules") == 0) {

View File

@ -58,6 +58,7 @@ class RegressionTest {
std::string debug_log;
std::string error_log;
std::string parser_error;
std::string parser_warn;
std::string clientIp;
std::string serverIp;

View File

@ -0,0 +1,63 @@
[
{
"enabled":1,
"version_min":300000,
"title":"Testing parser warning (1/n)",
"expected":{
"parser_warn": "Action ctl:forceRequestBodyVariable is not supported in version 3"
},
"client":{
"ip":"200.249.12.31",
"port":12300
},
"request":{
"headers":{
"Host":"a.b.com",
"Accept":"*/*",
"User-Agent":"My sweet little browser",
"Cookie": "PHPSESSID=rAAAAAAA2t5uvjq435r4q7ib3vtdjq120"
},
"uri":"/path1",
"method":"GET"
},
"server":{
"ip":"200.249.12.31",
"port":80
},
"rules":[
"SecRuleEngine On",
"SecRule REQUEST_URI \"@contains path1\" \"phase:1,block,id:5,ctl:forceRequestBodyVariable=Off\""
]
},
{
"enabled":1,
"version_min":300000,
"title":"Testing parser warning (2/n)",
"expected":{
"parser_warn": "config-warning.json. Line: 2. Column: 58. Action ctl:forceRequestBodyVariable is not supported in version 3."
},
"client":{
"ip":"200.249.12.31",
"port":12300
},
"request":{
"headers":{
"Host":"a.b.com",
"Accept":"*/*",
"User-Agent":"My sweet little browser",
"Cookie": "PHPSESSID=rAAAAAAA2t5uvjq435r4q7ib3vtdjq120"
},
"uri":"/path1",
"method":"GET"
},
"server":{
"ip":"200.249.12.31",
"port":80
},
"rules":[
"SecRuleEngine On",
"SecRule REQUEST_URI \"@contains path1\" \"phase:1,block,id:5,ctl:forceRequestBodyVariable=Off\"",
"SecRule REQUEST_URI \"@contains path1\" \"phase:1,block,id:7,ctl:forceRequestBodyVariable=On\""
]
}
]