diff --git a/CHANGES b/CHANGES index d18684a6..06945e73 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,10 @@ 03 Aug 2009 - 2.5.10 -------------------- + * Fixed crash on configuration if SecMarker is used before any rules. + + * Fixed SecRuleUpdateActionById so that it will work on chain starters. + * Cleanup build system for mlogc. * Allow mlogc to periodically flush memory pools. diff --git a/apache2/apache2_config.c b/apache2/apache2_config.c index 8168f610..67e6e995 100644 --- a/apache2/apache2_config.c +++ b/apache2/apache2_config.c @@ -565,6 +565,11 @@ static const char *add_rule(cmd_parms *cmd, directory_config *dcfg, int type, msre_rule *rule = NULL; extern msc_engine *modsecurity; + #ifdef DEBUG_CONF + ap_log_perror(APLOG_MARK, APLOG_STARTUP|APLOG_NOERRNO, 0, cmd->pool, + "Rule: type=%d p1='%s' p2='%s' p3='%s'", type, p1, p2, p3); + #endif + /* Create a ruleset if one does not exist. */ if ((dcfg->ruleset == NULL)||(dcfg->ruleset == NOT_SET_P)) { dcfg->ruleset = msre_ruleset_create(modsecurity->msre, cmd->pool); @@ -698,7 +703,7 @@ static const char *add_rule(cmd_parms *cmd, directory_config *dcfg, int type, #ifdef DEBUG_CONF ap_log_perror(APLOG_MARK, APLOG_STARTUP|APLOG_NOERRNO, 0, cmd->pool, - "Adding rule %pp id=\"%s\".", rule, (rule->actionset->id == NOT_SET_P + "Adding rule %pp phase=%d id=\"%s\".", rule, rule->actionset->phase, (rule->actionset->id == NOT_SET_P ? "(none)" : rule->actionset->id)); #endif @@ -749,6 +754,11 @@ static const char *add_marker(cmd_parms *cmd, directory_config *dcfg, const char extern msc_engine *modsecurity; int p; + #ifdef DEBUG_CONF + ap_log_perror(APLOG_MARK, APLOG_STARTUP|APLOG_NOERRNO, 0, cmd->pool, + "Rule: type=%d p1='%s' p2='%s' p3='%s'", RULE_TYPE_MARKER, p1, p2, p3); + #endif + /* Create a ruleset if one does not exist. */ if ((dcfg->ruleset == NULL)||(dcfg->ruleset == NOT_SET_P)) { dcfg->ruleset = msre_ruleset_create(modsecurity->msre, cmd->pool); @@ -766,13 +776,21 @@ static const char *add_marker(cmd_parms *cmd, directory_config *dcfg, const char /* Add placeholder to each phase */ for (p = PHASE_FIRST; p <= PHASE_LAST; p++) { + #ifdef DEBUG_CONF + ap_log_perror(APLOG_MARK, APLOG_STARTUP|APLOG_NOERRNO, 0, cmd->pool, + "Adding marker %pp phase=%d id=\"%s\".", rule, p, (rule->actionset->id == NOT_SET_P + ? "(none)" : rule->actionset->id)); + #endif + if (msre_ruleset_rule_add(dcfg->ruleset, rule, p) < 0) { return "Internal Error: Failed to add marker to the ruleset."; } } /* No longer need to search for the ID */ - apr_table_unset(dcfg->tmp_rule_placeholders, rule->actionset->id); + if (dcfg->tmp_rule_placeholders != NULL) { + apr_table_unset(dcfg->tmp_rule_placeholders, rule->actionset->id); + } return NULL; } diff --git a/apache2/re.c b/apache2/re.c index 7f9da78a..4b5e2ae3 100644 --- a/apache2/re.c +++ b/apache2/re.c @@ -1188,8 +1188,9 @@ static msre_rule * msre_ruleset_fetch_phase_rule(const msre_ruleset *ruleset, co for (i = 0; i < phase_arr->nelts; i++) { msre_rule *rule = (msre_rule *)rules[i]; + /* Rule with an action, not a sub-rule (chain) and a matching id */ if ( (rule->actionset != NULL) - && !rule->actionset->is_chained + && (!rule->actionset->is_chained || !rule->chain_starter) && (rule->actionset->id != NULL) && (strcmp(rule->actionset->id, id) == 0)) { diff --git a/apache2/t/regression/rule/20-exceptions.t b/apache2/t/regression/rule/20-exceptions.t index 43ccf215..12c058e9 100644 --- a/apache2/t/regression/rule/20-exceptions.t +++ b/apache2/t/regression/rule/20-exceptions.t @@ -1,10 +1,8 @@ ### Tests for rule exceptions -# SecRuleRemoveByMsg - # SecRuleRemoveById { - type => "config", + type => "rule", comment => "SecRuleRemoveById (single)", conf => qq( SecRuleEngine On @@ -27,7 +25,7 @@ ), }, { - type => "config", + type => "rule", comment => "SecRuleRemoveById (multiple)", conf => qq( SecRuleEngine On @@ -52,7 +50,7 @@ ), }, { - type => "config", + type => "rule", comment => "SecRuleRemoveById (range)", conf => qq( SecRuleEngine On @@ -77,7 +75,7 @@ ), }, { - type => "config", + type => "rule", comment => "SecRuleRemoveById (multiple + range)", conf => qq( SecRuleEngine On @@ -105,7 +103,7 @@ # SecRuleRemoveByMsg { - type => "config", + type => "rule", comment => "SecRuleRemoveByMsg", conf => qq( SecRuleEngine On @@ -127,3 +125,52 @@ GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", ), }, + +# SecRuleUpdateActionById +{ + type => "rule", + comment => "SecRuleUpdateActionById", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRule REQUEST_URI "test" "phase:1,deny,status:500,id:1,msg:'testing rule'" + SecRuleUpdateActionById 1 "pass,nolog" + ), + match_log => { + -error => [ qr/ModSecurity: /, 1 ], + -audit => [ qr/./, 1 ], + debug => [ qr/id:1,.*,pass,nolog/, 1 ], + -debug => [ qr/Access denied/, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + ), +}, +{ + type => "rule", + comment => "SecRuleUpdateActionById (chain)", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRule REQUEST_URI "test" "phase:1,deny,status:500,id:1,msg:'testing rule',chain" + SecRule ARGS "bar" + SecRuleUpdateActionById 1 "pass,nolog" + ), + match_log => { + -error => [ qr/ModSecurity: /, 1 ], + -audit => [ qr/./, 1 ], + debug => [ qr/id:1,.*,pass,nolog/, 1 ], + -debug => [ qr/Access denied/, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt?foo=bar", + ), +},