Added placeholder support for skipAfter so that it works with removed rules. See #258.

apache2/apache2_config.c

@@ -27,7 +27,7 @@ void *create_directory_config(apr_pool_t *mp, char *path) {
     if (dcfg == NULL) return NULL;
 
     #ifdef DEBUG_CONF
-    fprintf(stderr, "Created directory config %p path %s\n", dcfg, path);
+    ap_log_perror(APLOG_MARK, APLOG_STARTUP|APLOG_NOERRNO, 0, mp, "Created directory config %pp path %s", dcfg, path);
     #endif
 
     dcfg->mp = mp;
@@ -75,6 +75,7 @@ void *create_directory_config(apr_pool_t *mp, char *path) {
     /* These are only used during the configuration process. */
     dcfg->tmp_chain_starter = NULL;
     dcfg->tmp_default_actionset = NULL;
+    dcfg->tmp_rule_placeholders = NULL;
 
     /* Misc */
     dcfg->data_dir = NOT_SET_P;
@@ -195,7 +196,7 @@ void *merge_directory_configs(apr_pool_t *mp, void *_parent, void *_child) {
     directory_config *merged = create_directory_config(mp, NULL);
 
     #ifdef DEBUG_CONF
-    fprintf(stderr, "Merge parent %p child %p RESULT %p\n", _parent, _child, merged);
+    ap_log_perror(APLOG_MARK, APLOG_STARTUP|APLOG_NOERRNO, 0, mp, "Merge parent %pp child %pp RESULT %pp", _parent, _child, merged);
     #endif
 
     if (merged == NULL) return NULL;
@@ -580,11 +581,48 @@ static const char *add_rule(cmd_parms *cmd, directory_config *dcfg, const char *
         dcfg->upload_validates_files = 1;
     }
 
+    /* Create skip table if one does not already exist. */
+    if (dcfg->tmp_rule_placeholders == NULL) {
+        dcfg->tmp_rule_placeholders = apr_table_make(cmd->pool, 10);
+        if (dcfg->tmp_rule_placeholders == NULL) return FATAL_ERROR;
+    }
+
+    /* Keep track of any rule IDs we need to skip after */
+    if (rule->actionset->skip_after != NOT_SET_P) {
+        char *tmp_id = apr_pstrdup(cmd->pool, rule->actionset->skip_after);
+        apr_table_setn(dcfg->tmp_rule_placeholders, tmp_id, tmp_id);
+
+        #ifdef DEBUG_CONF
+        ap_log_perror(APLOG_MARK, APLOG_STARTUP|APLOG_NOERRNO, 0, cmd->pool, "Watching for skipafter target rule id=\"%s\".", tmp_id);
+        #endif
+
+    }
+
     /* Add rule to the recipe. */
     if (msre_ruleset_rule_add(dcfg->ruleset, rule, rule->actionset->phase) < 0) {
         return "Internal Error: Failed to add rule to the ruleset.";
     }
 
+    /* Add an additional placeholder if this rule ID is on the list */
+    if ((rule->actionset->id != NULL) && apr_table_get(dcfg->tmp_rule_placeholders, rule->actionset->id)) {
+        msre_rule *phrule = apr_palloc(rule->ruleset->mp, sizeof(msre_rule));
+
+        #ifdef DEBUG_CONF
+        ap_log_perror(APLOG_MARK, APLOG_STARTUP|APLOG_NOERRNO, 0, cmd->pool, "Adding placeholder for rule id=\"%s\".", rule->actionset->id);
+        #endif
+
+        /* shallow copy of original rule with placeholder marked as target */
+        memcpy(phrule, rule, sizeof(msre_rule));
+        phrule->placeholder = RULE_PH_TARGET;
+
+        /* Add placeholder. */
+        if (msre_ruleset_rule_add(dcfg->ruleset, phrule, phrule->actionset->phase) < 0) {
+            return "Internal Error: Failed to add placeholder to the ruleset.";
+        }
+
+        apr_table_unset(dcfg->tmp_rule_placeholders, rule->actionset->id);
+    }
+        
     return NULL;    
 }
 
@@ -1132,7 +1170,7 @@ static const char *cmd_rule_remove_by_msg(cmd_parms *cmd, void *_dcfg, const cha
     msre_ruleset_rule_remove_with_exception(dcfg->ruleset, re);
 
     #ifdef DEBUG_CONF
-    fprintf(stderr, "Added exception %p (%d %s) to dcfg %p.\n", re, re->type, re->param, dcfg);
+    ap_log_perror(APLOG_MARK, APLOG_STARTUP|APLOG_NOERRNO, 0, cmd->pool, "Added exception %pp (%d %s) to dcfg %pp.", re, re->type, re->param, dcfg);
     #endif
 
     return NULL;

apache2/mod_security2.c

@@ -418,6 +418,10 @@ static int hook_post_config(apr_pool_t *mp, apr_pool_t *mp_log, apr_pool_t *mp_t
     void *init_flag = NULL;
     int first_time = 0;
 
+    /* ENH Figure out a way to validate config before we start
+     * - skipafter: need to make sure we found all of our targets
+     */
+
     /* Figure out if we are here for the first time */
     apr_pool_userdata_get(&init_flag, "modsecurity-init-flag", s->process->pool);
     if (init_flag == NULL) {

apache2/modsecurity.h

@@ -423,6 +423,7 @@ struct directory_config {
     /* Used only in the configuration phase. */
     msre_rule           *tmp_chain_starter;
     msre_actionset      *tmp_default_actionset;
+    apr_table_t         *tmp_rule_placeholders;
 
     /* Misc */
     const char          *data_dir;

apache2/re.c

@@ -673,27 +673,34 @@ apr_status_t msre_ruleset_process_phase(msre_ruleset *ruleset, modsec_rec *msr)
         apr_time_t time1 = 0;
 #endif
 
-        // TODO: Still need to skip over placeholders
-
         /* SKIP_RULES is used to skip all rules until we hit a placeholder
          * with the specified rule ID and then resume execution after that.
          */
         if (mode == SKIP_RULES) {
             /* Go to the next rule if we have not yet hit the skip_after ID */
-            // TODO: must be a placeholder as well
+            if ((rule->placeholder != RULE_PH_NONE) && ((rule->actionset->id == NULL) || (strcmp(skip_after, rule->actionset->id) != 0))) {
-            if ((rule->actionset->id == NULL) || (strcmp(skip_after, rule->actionset->id) != 0)) {
                 if (msr->txcfg->debuglog_level >= 9) {
-                    msr_log(msr, 9, "Skipping rule id=\"%s\" while looking for id=\"%s\"", (rule->actionset->id ? rule->actionset->id : "(none)"), skip_after);
+                    msr_log(msr, 9, "Skipping rule id=\"%s\": Skipping until after id=\"%s\"", (rule->actionset->id ? rule->actionset->id : "(none)"), skip_after);
+
                 }
                 continue;
             }
-            if (msr->txcfg->debuglog_level >= 4) {
+            if (msr->txcfg->debuglog_level >= 9) {
-                msr_log(msr, 4, "Continuing execution after rule id=\"%s\"", skip_after);
+                msr_log(msr, 9, "Found rule id=\"%s\"%s.", skip_after, (rule->placeholder ? " placeholder" : ""));
             }
-            skip_after = NULL;
-            mode = NEXT_RULE;
 
             /* Go to the rule *after* this one to continue execution. */
+            if (msr->txcfg->debuglog_level >= 4) {
+                msr_log(msr, 4, "Continuing execution after rule id=\"%s\".", skip_after);
+            }
+
+            skip_after = NULL;
+            mode = NEXT_RULE;
+            continue;
+        }
+
+        /* Skip any rule marked as a placeholder */
+        if (rule->placeholder != RULE_PH_NONE) {
             continue;
         }
 

apache2/re.h

@@ -109,6 +109,9 @@ int DSOLOCAL msre_ruleset_phase_rule_remove_with_exception(msre_ruleset *ruleset
 #define RULE_NO_MATCH           0
 #define RULE_MATCH              1
 
+#define RULE_PH_NONE            0  /* Not a placeholder */
+#define RULE_PH_TARGET          1  /* Placeholder for skipafter targets */
+
 struct msre_rule {
     apr_array_header_t      *targets;
     const char              *op_name;
@@ -120,6 +123,7 @@ struct msre_rule {
     const char              *p1;
     const char              *filename;
     int                      line_num;
+    int                      placeholder;
     
     msre_ruleset            *ruleset;
     msre_rule               *chain_starter;

doc/modsecurity2-apache-reference.xml

@@ -1939,7 +1939,7 @@ SecRule REQUEST_HEADERS:Host "!^$" "deny,<emphasis>phase:1</emphasis>"</programl
     different phases, they would not happen one after the other. The order of
     rules in the configuration file is important only within the rules of each
     phase. This is especially important when using the <literal>skip</literal>
-    action.</para>
+    and <literal>skipAfter</literal> actions.</para>
 
     <section>
       <title>Phase Request Headers</title>
@@ -2333,8 +2333,8 @@ SecRule <emphasis>TX:MYMATCH</emphasis> "@eq ARGS:param" deny</programlisting>
       intended to be used to check the build number prior to using a feature
       that is available only in a certain build. Example:</para>
 
-      <programlisting format="linespecific">SecRule <emphasis>MODSEC_BUILD</emphasis> "!@ge 02050102" skip:1
+      <programlisting format="linespecific">SecRule <emphasis>MODSEC_BUILD</emphasis> "!@ge 02050102" skipAfter:12345
-SecRule ARGS "@pm some key words" deny,status:500</programlisting>
+SecRule ARGS "@pm some key words" id:12345,deny,status:500</programlisting>
     </section>
 
     <section>
@@ -3106,10 +3106,10 @@ SecRule REQUEST_HEADERS:Transfer-Encoding "!^$"</programlisting>
       <programlisting format="linespecific">SecDefaultAction log,deny,status:403,phase:2
 SecRule REQUEST_HEADERS:Content-Type ^text/xml$ \
     phase:1,t:lowercase,nolog,pass,ctl:requestBodyProcessor=<emphasis>XML</emphasis>
-SecRule REQBODY_PROCESSOR "<emphasis>!^XML$</emphasis>" skip:2
+SecRule REQBODY_PROCESSOR "<emphasis>!^XML$</emphasis>" skipAfter:12345
 SecRule <emphasis>XML:/employees/employee/name/text()</emphasis> Fred
 SecRule <emphasis>XML:/xq:employees/employee/name/text()</emphasis> Fred \
-    xmlns:xq=http://www.example.com/employees</programlisting>
+    id:12345,xmlns:xq=http://www.example.com/employees</programlisting>
 
       <para>The first XPath expression does not use namespaces. It would match
       against payload such as this one:</para>
@@ -3576,8 +3576,8 @@ SecRule REQUEST_HEADER:Content-Length ^$</programlisting>
       first portion of the chained rule will only be triggered if all of the
       variable checks return positive hits. If one aspect of the chained rule
       is negative, then the entire rule chain is negative. Also note that
-      disruptive actions, execution phases, metadata actions (id, rev, msg)
+      disruptive actions, execution phases, metadata actions (id, rev, msg),
-      and skip actions can only be specified on by the chain starter
+      skip and skipAfter actions can only be specified on by the chain starter
       rule.</para>
     </section>
 
@@ -4443,6 +4443,34 @@ SecRule &amp;REQUEST_HEADERS:Accept "@eq 0" \
       skip.</para>
     </section>
 
+    <section>
+      <title><literal>skipAfter</literal></title>
+
+      <para><emphasis>Description:</emphasis> Skips rules (or chains) on
+      successful match resuming rule execution after the specified rule id is
+      found.</para>
+
+      <para><emphasis>Action Group:</emphasis> Non-Disruptive</para>
+
+      <para>Example:</para>
+
+      <para><programlisting format="linespecific">SecRule REQUEST_URI "^/$" "chain,<emphasis>skipAfter:960015</emphasis>"
+SecRule REMOTE_ADDR "^127\.0\.0\.1$" "chain"
+SecRule REQUEST_HEADERS:User-Agent "^Apache \(internal dummy connection\)$" "t:none"  
+SecRule &amp;REQUEST_HEADERS:Host "@eq 0" \
+    "deny,log,status:400,id:960008,severity:4,msg:'Request Missing a Host Header'"
+SecRule &amp;REQUEST_HEADERS:Accept "@eq 0" \
+    "log,deny,log,status:400,id:960015,msg:'Request Missing an Accept Header'"</programlisting></para>
+
+      <para><emphasis>Note</emphasis></para>
+
+      <para>SkipAfter only applies to the current processing phase and not
+      necessarily the order in which the rules appear in the configuration
+      file. If you group rules by processing phases, then skip should work as
+      expected. This action can not be used to skip rules within one chain.
+      Accepts a single paramater denoting the last rule ID to skip.</para>
+    </section>
+
     <section>
       <title><literal>status</literal></title>
 
@@ -4837,8 +4865,8 @@ SecRule REQUEST_HEADERS:Ip-Address "!<emphasis>@streq %{TX.1}</emphasis>"</progr
       <programlisting format="linespecific">SecDefaultAction log,deny,status:403,phase:2
 SecRule REQUEST_HEADERS:Content-Type ^text/xml$ \
     phase:1,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML
-SecRule REQBODY_PROCESSOR "!^XML$" nolog,pass,skip:1
+SecRule REQBODY_PROCESSOR "!^XML$" nolog,pass,skipAfter:12345
-SecRule XML "<emphasis>@validateDTD /path/to/apache2/conf/xml.dtd</emphasis>"</programlisting>
+SecRule XML "<emphasis>@validateDTD /path/to/apache2/conf/xml.dtd</emphasis>,id:12345"</programlisting>
     </section>
 
     <section>
@@ -4852,8 +4880,8 @@ SecRule XML "<emphasis>@validateDTD /path/to/apache2/conf/xml.dtd</emphasis>"</p
       <programlisting format="linespecific">SecDefaultAction log,deny,status:403,phase:2
 SecRule REQUEST_HEADERS:Content-Type ^text/xml$ \
     phase:1,t:lowercase,nolog,pass,ctl:requestBodyProcessor=XML
-SecRule REQBODY_PROCESSOR "!^XML$" nolog,pass,skip:1
+SecRule REQBODY_PROCESSOR "!^XML$" nolog,pass,skipAfter:12345
-SecRule XML "<emphasis>@validateSchema /path/to/apache2/conf/xml.xsd</emphasis>"</programlisting>
+SecRule XML "<emphasis>@validateSchema /path/to/apache2/conf/xml.xsd</emphasis>,id:12345"</programlisting>
 
       <para>This operator requires request body to be processed as XML.</para>
     </section>