Merge pull request owasp-modsecurity#3120 from marcstern/v2/mst/nullcheck2

Check for null pointer dereference (almost) everywhere

Author: airween

.github/workflows/ci.yml

@@ -32,7 +32,7 @@ jobs:
       - 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`

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

apache2/mod_security2.c

@@ -475,6 +475,8 @@ static modsec_rec *retrieve_tx_context(request_rec *r) {
  * phases, redirections, or subrequests.
  */
 static void store_tx_context(modsec_rec *msr, request_rec *r) {
+    assert(msr != NULL);
+    assert(r != NULL);
     apr_table_setn(r->notes, NOTE_MSR, (void *)msr);
 }
 
@@ -491,7 +493,10 @@ static modsec_rec *create_tx_context(request_rec *r) {
     apr_allocator_create(&allocator);
     apr_allocator_max_free_set(allocator, 1024);
     apr_pool_create_ex(&msr->mp, r->pool, NULL, allocator);
-    if (msr->mp == NULL) return NULL;
+    if (msr->mp == NULL) {
+        apr_allocator_destroy(allocator);
+        return NULL;
+    }
     apr_allocator_owner_set(allocator, msr->mp);
 
     msr->modsecurity = modsecurity;
@@ -863,6 +868,9 @@ static int hook_request_early(request_rec *r) {
      */
     msr = create_tx_context(r);
     if (msr == NULL) return DECLINED;
+    if (msr->txcfg->debuglog_level >= 9) {
+        msr_log(msr, 9, "Context created after request failure.");
+    }
 
 #ifdef REQUEST_EARLY
 
@@ -1150,17 +1158,12 @@ static void hook_error_log(const char *file, int line, int level, apr_status_t s
 #endif
     if (msr_ap_server) {
 #if AP_SERVER_MAJORVERSION_NUMBER > 1 && AP_SERVER_MINORVERSION_NUMBER > 2
-        msr = create_tx_context((request_rec *)info->r);
+        msr = create_tx_context((request_rec*)info->r);
 #else
-        msr = create_tx_context((request_rec *)r);
+        msr = create_tx_context((request_rec*)r);
 #endif
-        if (msr != NULL && msr->txcfg->debuglog_level >= 9) {
-            if (msr == NULL) {
-                msr_log(msr, 9, "Failed to create context after request failure.");
-            }
-            else {
-                msr_log(msr, 9, "Context created after request failure.");
-            }
+        if (msr && msr->txcfg->debuglog_level >= 9) {
+            msr_log(msr, 9, "Context created after request failure.");
         }
     }
     if (msr == NULL) return;