Fixed crash on configuration if SecMarker is used before any rules.

Fixed SecRuleUpdateActionById so that it will work on chain starters (MODSEC-37).

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.

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;
 }

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))
         {

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",
+	),
+},