Merge branch 'v2/master' of https://github.com/marcstern/ModSecurity into v2/master

Author: Marc Stern

.github/security2.conf

@@ -0,0 +1,6 @@
+LoadModule security2_module /usr/lib/apache2/modules/mod_security2.so
+
+<IfModule security2_module>
+    SecDataDir /var/cache/modsecurity
+    Include /etc/apache2/modsecurity.conf
+</IfModule>

.github/workflows/ci.yml

@@ -10,22 +10,42 @@ jobs:
     strategy:
       matrix:
         os: [ubuntu-22.04]
-        platform: [x64]
-        compiler: [gcc]
+        platform: [x32, x64]
+        compiler: [gcc, clang]
         configure:
-          - {label: "with pcre2", opt: "--with-pcre2" }
-          - {label: "with lua",   opt: "--with-lua" }
-          - {label: "wo lua",     opt: "--without-lua" }
+          - {label: "with pcre, no study, no jit",      opt: "--enable-pcre-study=no" }
+          - {label: "with pcre, with study, no jit",    opt: "--enable-pcre-study=yes" }
+          - {label: "with pcre, no study, with jit",    opt: "--enable-pcre-study=no --enable-pcre-jit" }
+          - {label: "with pcre, with study, with jit",  opt: "--enable-pcre-study=yes --enable-pcre-jit" }
+          - {label: "with pcre2",                       opt: "--with-pcre2 --enable-pcre-study=no" }
+          - {label: "with pcre2, with study, no jit",   opt: "--with-pcre2 --enable-pcre-study=yes" }
+          - {label: "with pcre2, no study, with jit",   opt: "--with-pcre2 --enable-pcre-study=no --enable-pcre-jit" }
+          - {label: "with pcre2, with study, with jit", opt: "--with-pcre2 --enable-pcre-study=yes --enable-pcre-jit" }
+          - {label: "with lua",                         opt: "--with-lua" }
+          - {label: "wo lua",                           opt: "--without-lua" }
     steps:
       - name: Setup Dependencies
         run: |
           sudo apt-get update -y -qq
-          sudo apt-get install -y apache2-dev libxml2-dev liblua5.1-0-dev libcurl4-gnutls-dev libpcre2-dev pkg-config libyajl-dev
+          sudo apt-get install -y apache2-dev libxml2-dev liblua5.1-0-dev libcurl4-gnutls-dev libpcre2-dev pkg-config libyajl-dev apache2 apache2-bin apache2-data
       - uses: actions/checkout@v2
       - name: autogen.sh
         run: ./autogen.sh
       - name: configure ${{ matrix.configure.label }}
-        run: ./configure ${{ matrix.configure.opt }}
+        run: ./configure --enable-assertions ${{ matrix.configure.opt }}
       - uses: ammaraskar/gcc-problem-matcher@master
       - name: make
         run: make -j `nproc`
+      - name: install module
+        run: sudo make install
+      - name: prepare config
+        run: |
+          sudo cp .github/security2.conf /etc/apache2/mods-enabled/
+          sudo cp modsecurity.conf-recommended /etc/apache2/modsecurity.conf
+          sudo cp unicode.mapping /etc/apache2/
+          sudo mkdir -p /var/cache/modsecurity
+          sudo chown -R www-data:www-data /var/cache/modsecurity
+      - name: start apache with module
+        run: |
+          sudo systemctl restart apache2.service
+

CHANGES

@@ -1,6 +1,8 @@
 DD mmm YYYY - 2.9.x (to be released)
 -------------------
 
+ * Fix possible segfault in collection_unpack
+   [Issue #3072 - @twouters]
  * Set the minimum security protocol version for SecRemoteRules
    [Issue security/code-scanning/2 - @airween]
  * Allow lua version 5.4

apache2/apache2_config.c

@@ -30,6 +30,15 @@
     APLOG_USE_MODULE(security2);
 #endif
 
+// Returns the rule id if existing, otherwise the file name & line number
+const char* id_log(msre_rule* rule) {
+    assert(rule != NULL);
+    assert(rule->actionset != NULL);
+    const char* id = rule->actionset->id;
+    if (!id || !*id || id == NOT_SET_P) id = apr_psprintf(rule->ruleset->mp, "%s (%d)", rule->filename, rule->line_num);
+	return id;
+}
+
 /* -- Directory context creation and initialisation -- */
 
 /**
@@ -239,19 +248,19 @@ static void copy_rules_phase(apr_pool_t *mp,
 
             if (copy > 0) {
 #ifdef DEBUG_CONF
-                ap_log_perror(APLOG_MARK, APLOG_STARTUP|APLOG_NOERRNO, 0, mp, "Copy rule %pp [id \"%s\"]", rule, rule->actionset->id);
+                ap_log_perror(APLOG_MARK, APLOG_STARTUP|APLOG_NOERRNO, 0, mp, "Copy rule %pp [id \"%s\"]", rule, id_log(rule));
 #endif
 
                 /* Copy the rule. */
                 *(msre_rule **)apr_array_push(child_phase_arr) = rule;
-                if (rule->actionset && rule->actionset->is_chained) mode = 2;
+                if (rule->actionset->is_chained) mode = 2;
             } else {
-                if (rule->actionset && rule->actionset->is_chained) mode = 1;
+                if (rule->actionset->is_chained) mode = 1;
             }
         } else {
             if (mode == 2) {
 #ifdef DEBUG_CONF
-                ap_log_perror(APLOG_MARK, APLOG_STARTUP|APLOG_NOERRNO, 0, mp, "Copy chain %pp for rule %pp [id \"%s\"]", rule, rule->chain_starter, rule->chain_starter->actionset->id);
+                ap_log_perror(APLOG_MARK, APLOG_STARTUP|APLOG_NOERRNO, 0, mp, "Copy chain %pp for rule %pp [id \"%s\"]", rule, rule->chain_starter, id_log(rule->chain_starter));
 #endif
 
                 /* Copy the rule (it belongs to the chain we want to include. */
@@ -906,16 +915,14 @@ static const char *add_rule(cmd_parms *cmd, directory_config *dcfg, int type,
      */
     rule->actionset = msre_actionset_merge(modsecurity->msre, cmd->pool, dcfg->tmp_default_actionset,
         rule->actionset, 1);
+    if (rule->actionset == NULL) return apr_psprintf(cmd->pool, "ModSecurity: cannot merge actionset (memory full?).");
 
     /* Keep track of the parent action for "block" */
-    if (rule->actionset) {
-        rule->actionset->parent_intercept_action_rec = dcfg->tmp_default_actionset->intercept_action_rec;
-        rule->actionset->parent_intercept_action = dcfg->tmp_default_actionset->intercept_action;
-    }
+    rule->actionset->parent_intercept_action_rec = dcfg->tmp_default_actionset->intercept_action_rec;
+    rule->actionset->parent_intercept_action = dcfg->tmp_default_actionset->intercept_action;
 
     /* Must NOT specify a disruptive action in logging phase. */
-    if ((rule->actionset != NULL)
-        && (rule->actionset->phase == PHASE_LOGGING)
+    if (   (rule->actionset->phase == PHASE_LOGGING)
         && (rule->actionset->intercept_action != ACTION_ALLOW)
         && (rule->actionset->intercept_action != ACTION_ALLOW_REQUEST)
         && (rule->actionset->intercept_action != ACTION_NONE)
@@ -926,9 +933,7 @@ static const char *add_rule(cmd_parms *cmd, directory_config *dcfg, int type,
 
     if (dcfg->tmp_chain_starter != NULL) {
         rule->chain_starter = dcfg->tmp_chain_starter;
-        if (rule->actionset) {
-            rule->actionset->phase = rule->chain_starter->actionset->phase;
-        }
+        rule->actionset->phase = rule->chain_starter->actionset->phase;
     }
 
     if (rule->actionset->is_chained != 1) {
@@ -967,8 +972,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 phase=%d id=\"%s\".", rule, rule->actionset->phase, (rule->actionset->id == NOT_SET_P
-        ? "(none)" : rule->actionset->id));
+        "Adding rule %pp phase=%d id=\"%s\".", rule, rule->actionset->phase, id_log(rule));
     #endif
 
     /* Add rule to the recipe. */
@@ -1042,8 +1046,7 @@ static const char *add_marker(cmd_parms *cmd, directory_config *dcfg,
     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));
+            "Adding marker %pp phase=%d id=\"%s\".", rule, p, id_log(rule));
         #endif
 
         if (msre_ruleset_rule_add(dcfg->ruleset, rule, p) < 0) {
@@ -1091,11 +1094,7 @@ static const char *update_rule_action(cmd_parms *cmd, directory_config *dcfg,
         return NULL;
     }
 
-    /* Check the rule actionset */
-    /* ENH: Can this happen? */
-    if (rule->actionset == NULL) {
-        return apr_psprintf(cmd->pool, "ModSecurity: Attempt to update action for rule \"%s\" failed: Rule does not have an actionset.", p1);
-    }
+    assert(rule->actionset != NULL);
 
     /* Create a new actionset */
     new_actionset = msre_actionset_create(modsecurity->msre, cmd->pool, p2, &my_error_msg);
@@ -1117,16 +1116,15 @@ static const char *update_rule_action(cmd_parms *cmd, directory_config *dcfg,
         char *actions = msre_actionset_generate_action_string(ruleset->mp, rule->actionset);
         ap_log_perror(APLOG_MARK, APLOG_STARTUP|APLOG_NOERRNO, 0, cmd->pool,
             "Update rule %pp id=\"%s\" old action: \"%s\"",
-            rule,
-            (rule->actionset->id == NOT_SET_P ? "(none)" : rule->actionset->id),
-            actions);
+            rule, id_log(rule), actions);
     }
     #endif
 
     /* Merge new actions with the rule */
     /* ENH: Will this leak the old actionset? */
     rule->actionset = msre_actionset_merge(modsecurity->msre, cmd->pool, rule->actionset,
         new_actionset, 1);
+    if (rule->actionset == NULL) return apr_psprintf(cmd->pool, "ModSecurity: cannot merge actionset (memory full?).");
     msre_actionset_set_defaults(rule->actionset);
 
     /* Update the unparsed rule */
@@ -1137,9 +1135,7 @@ static const char *update_rule_action(cmd_parms *cmd, directory_config *dcfg,
         char *actions = msre_actionset_generate_action_string(ruleset->mp, rule->actionset);
         ap_log_perror(APLOG_MARK, APLOG_STARTUP|APLOG_NOERRNO, 0, cmd->pool,
             "Update rule %pp id=\"%s\" new action: \"%s\"",
-            rule,
-            (rule->actionset->id == NOT_SET_P ? "(none)" : rule->actionset->id),
-            actions);
+            rule, id_log(rule), actions);
     }
     #endif
 
@@ -1746,6 +1742,9 @@ char *parser_conn_limits_operator(apr_pool_t *mp, const char *p2,
 
     config_orig_path = apr_pstrndup(mp, filename,
         strlen(filename) - strlen(apr_filepath_name_get(filename)));
+    if (config_orig_path == NULL) {
+        return apr_psprintf(mp, "ModSecurity: failed to duplicate filename in parser_conn_limits_operator");
+    }
 
     apr_filepath_merge(&file, config_orig_path, param, APR_FILEPATH_TRUENAME,
         mp);
@@ -2452,8 +2451,12 @@ static const char *cmd_rule_remove_by_id(cmd_parms *cmd, void *_dcfg,
                                          const char *p1)
 {
     directory_config *dcfg = (directory_config *)_dcfg;
-    rule_exception *re = apr_pcalloc(cmd->pool, sizeof(rule_exception));
     if (dcfg == NULL) return NULL;
+    rule_exception* re = apr_pcalloc(cmd->pool, sizeof(rule_exception));
+    if (re == NULL) {
+        ap_log_perror(APLOG_MARK, APLOG_STARTUP | APLOG_NOERRNO, 0, cmd->pool, "cmd_rule_remove_by_id: Cannot allocate memory");
+        return NULL;
+    }
 
     re->type = RULE_EXCEPTION_REMOVE_ID;
     re->param = p1;

apache2/apache2_io.c

@@ -179,12 +179,13 @@ apr_status_t input_filter(ap_filter_t *f, apr_bucket_brigade *bb_out,
  * Reads request body from a client.
  */
 apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
+    assert(msr != NULL);
+    assert(error_msg!= NULL);
     request_rec *r = msr->r;
     unsigned int finished_reading;
     apr_bucket_brigade *bb_in;
     apr_bucket *bucket;
 
-    if (error_msg == NULL) return -1;
     *error_msg = NULL;
 
     if (msr->reqbody_should_exist != 1) {
@@ -368,6 +369,8 @@ apr_status_t read_request_body(modsec_rec *msr, char **error_msg) {
  * run or not.
  */
 static int output_filter_should_run(modsec_rec *msr, request_rec *r) {
+    assert(msr != NULL);
+    assert(r != NULL);
     char *content_type = NULL;
 
     /* Check configuration. */
@@ -429,10 +432,13 @@ static int output_filter_should_run(modsec_rec *msr, request_rec *r) {
 static apr_status_t output_filter_init(modsec_rec *msr, ap_filter_t *f,
         apr_bucket_brigade *bb_in)
 {
+    assert(msr != NULL);
+    assert(f != NULL);
     request_rec *r = f->r;
     const char *s_content_length = NULL;
     apr_status_t rc;
 
+    assert(msr != NULL);
     msr->of_brigade = apr_brigade_create(msr->mp, f->c->bucket_alloc);
     if (msr->of_brigade == NULL) {
         msr_log(msr, 1, "Output filter: Failed to create brigade.");
@@ -496,6 +502,8 @@ static apr_status_t output_filter_init(modsec_rec *msr, ap_filter_t *f,
  * and to the client.
  */
 static apr_status_t send_of_brigade(modsec_rec *msr, ap_filter_t *f) {
+    assert(msr != NULL);
+    assert(f != NULL);
     apr_status_t rc;
 
     rc = ap_pass_brigade(f->next, msr->of_brigade);
@@ -537,6 +545,8 @@ static apr_status_t send_of_brigade(modsec_rec *msr, ap_filter_t *f) {
  *
  */
 static void inject_content_to_of_brigade(modsec_rec *msr, ap_filter_t *f) {
+    assert(msr != NULL);
+    assert(f != NULL);
     apr_bucket *b;
 
     if (msr->txcfg->content_injection_enabled && msr->stream_output_data != NULL) {
@@ -563,6 +573,8 @@ static void inject_content_to_of_brigade(modsec_rec *msr, ap_filter_t *f) {
  *
  */
 static void prepend_content_to_of_brigade(modsec_rec *msr, ap_filter_t *f) {
+    assert(msr != NULL);
+    assert(f != NULL);
     if ((msr->txcfg->content_injection_enabled) && (msr->content_prepend) && (!msr->of_skipping)) {
         apr_bucket *bucket_ci = NULL;
 
@@ -1008,6 +1020,12 @@ apr_status_t output_filter(ap_filter_t *f, apr_bucket_brigade *bb_in) {
     /* Now send data down the filter stream
      * (full-buffering only).
      */
+    if (!eos_bucket) {
+        ap_log_error(APLOG_MARK, APLOG_ERR | APLOG_NOERRNO, 0, f->r->server,
+            "ModSecurity: Internal Error: eos_bucket is NULL.");
+        return APR_EGENERAL;
+    }
+
     if ((msr->of_skipping == 0)&&(!msr->of_partial)) {
         if(msr->of_stream_changed == 1) {
             inject_content_to_of_brigade(msr,f);

apache2/apache2_util.c

@@ -25,6 +25,8 @@
  * Sends a brigade with an error bucket down the filter chain.
  */
 apr_status_t send_error_bucket(modsec_rec *msr, ap_filter_t *f, int status) {
+    assert(msr != NULL);
+    assert(f != NULL);
     apr_bucket_brigade *brigade = NULL;
     apr_bucket *bucket = NULL;
 
@@ -61,6 +63,9 @@ apr_status_t send_error_bucket(modsec_rec *msr, ap_filter_t *f, int status) {
  * the "output" parameter.
  */
 int apache2_exec(modsec_rec *msr, const char *command, const char **argv, char **output) {
+    assert(msr != NULL);
+    assert(command != NULL);
+
     apr_procattr_t *procattr = NULL;
     apr_proc_t *procnew = NULL;
     apr_status_t rc = APR_SUCCESS;
@@ -204,6 +209,9 @@ char *get_env_var(request_rec *r, char *name) {
 static void internal_log_ex(request_rec *r, directory_config *dcfg, modsec_rec *msr,
     int level, int fixup, const char *text, va_list ap)
 {
+    assert(r != NULL);
+    assert(msr != NULL);
+    assert(text != NULL);
     apr_size_t nbytes, nbytes_written;
     apr_file_t *debuglog_fd = NULL;
     int filter_debug_level = 0;
@@ -303,6 +311,8 @@ static void internal_log_ex(request_rec *r, directory_config *dcfg, modsec_rec *
  * Apache error log if the message is important enough.
  */
 void msr_log(modsec_rec *msr, int level, const char *text, ...) {
+    assert(msr != NULL);
+    assert(text != NULL);
     va_list ap;
 
     va_start(ap, text);
@@ -316,6 +326,8 @@ void msr_log(modsec_rec *msr, int level, const char *text, ...) {
  * Apache error log. This is intended for error callbacks.
  */
 void msr_log_error(modsec_rec *msr, const char *text, ...) {
+    assert(msr != NULL);
+    assert(text != NULL);
     va_list ap;
 
     va_start(ap, text);
@@ -330,6 +342,8 @@ void msr_log_error(modsec_rec *msr, const char *text, ...) {
  * The 'text' will first be escaped.
  */
 void msr_log_warn(modsec_rec *msr, const char *text, ...) {
+    assert(msr != NULL);
+    assert(text != NULL);
     va_list ap;
 
     va_start(ap, text);