CI improvement: First check syntax & always display error.log

.github/security2.conf

@@ -4,3 +4,5 @@ LoadModule security2_module /usr/lib/apache2/modules/mod_security2.so
     SecDataDir /var/cache/modsecurity
     Include /etc/apache2/modsecurity.conf
 </IfModule>
+
+SecAuditLog /var/log/apache2/modsec_audit.log

.github/workflows/ci.yml

@@ -45,8 +45,14 @@ jobs:
           sudo cp unicode.mapping /etc/apache2/
           sudo mkdir -p /var/cache/modsecurity
           sudo chown -R www-data:www-data /var/cache/modsecurity
+      - name: first check config (to get syntax errors)
+        run: sudo apachectl configtest
       - name: start apache with module
-        run: |
-          sudo systemctl restart apache2.service
-          sudo cat /var/log/apache2/error.log
-
+        run: sudo systemctl restart apache2.service
+      - name: Show httpd error log
+        if: always()
+        run: sudo cat /var/log/apache2/error.log
+      - name: Show mod_security2 audit log
+        if: always()
+        run: sudo cat /var/log/apache2/modsec_audit.log
+        # For non-regression tests: /home/runner/work/ModSecurity/ModSecurity/tests/regression/server_root/logs/audit/audit.log

apache2/apache2_config.c

@@ -30,13 +30,10 @@
     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);
+static const char* id_log(msre_rule* rule) {
     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;
+    if (id == NOT_SET_P || !*id) id = apr_psprintf(rule->ruleset->mp, "%s (%d)", rule->filename, rule->line_num);
+    return id;
 }
 
 /* -- Directory context creation and initialisation -- */
@@ -903,7 +900,7 @@ static const char *add_rule(cmd_parms *cmd, directory_config *dcfg, int type,
         }
 
         /* Must NOT use skip. */
-        if (rule->actionset->skip_count != NOT_SET) {
+        if (rule->actionset && rule->actionset->skip_count != NOT_SET) {
             return apr_psprintf(cmd->pool, "ModSecurity: The skip action can only be used "
                 " by chain starter rules. ");
         }
@@ -936,7 +933,7 @@ static const char *add_rule(cmd_parms *cmd, directory_config *dcfg, int type,
         rule->actionset->phase = rule->chain_starter->actionset->phase;
     }
 
-    if (rule->actionset->is_chained != 1) {
+    if (rule->actionset && rule->actionset->is_chained != 1) {
         /* If this rule is part of the chain but does
          * not want more rules to follow in the chain
          * then cut it (the chain).
@@ -959,7 +956,7 @@ static const char *add_rule(cmd_parms *cmd, directory_config *dcfg, int type,
     }
 
     /* Keep track of any rule IDs we need to skip after */
-    if (rule->actionset->skip_after != NOT_SET_P) {
+    if (rule->actionset && 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);
 
@@ -976,33 +973,33 @@ static const char *add_rule(cmd_parms *cmd, directory_config *dcfg, int type,
     #endif
 
     /* Add rule to the recipe. */
-    if (msre_ruleset_rule_add(dcfg->ruleset, rule, rule->actionset->phase) < 0) {
+    if (rule->actionset && 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)) {
+    if (rule->actionset && (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));
         if (phrule == NULL) {
             return FATAL_ERROR;
         }
 
         #ifdef DEBUG_CONF
         ap_log_perror(APLOG_MARK, APLOG_STARTUP|APLOG_NOERRNO, 0, cmd->pool,
-            "Adding placeholder %pp for rule %pp id=\"%s\".", phrule, rule, rule->actionset->id);
+            "Adding placeholder %pp for rule %pp id=\"%s\".", phrule, rule, id_log(rule));
         #endif
 
         /* shallow copy of original rule with placeholder marked as target */
         memcpy(phrule, rule, sizeof(msre_rule));
         phrule->placeholder = RULE_PH_SKIPAFTER;
 
         /* Add placeholder. */
-        if (msre_ruleset_rule_add(dcfg->ruleset, phrule, phrule->actionset->phase) < 0) {
+        if (phrule->actionset && msre_ruleset_rule_add(dcfg->ruleset, phrule, phrule->actionset->phase) < 0) {
             return "Internal Error: Failed to add placeholder to the ruleset.";
         }
 
         /* No longer need to search for the ID */
-        apr_table_unset(dcfg->tmp_rule_placeholders, rule->actionset->id);
+        if (rule->actionset) apr_table_unset(dcfg->tmp_rule_placeholders, rule->actionset->id);
     }
 
     /* Update the unparsed rule */
@@ -1055,7 +1052,7 @@ static const char *add_marker(cmd_parms *cmd, directory_config *dcfg,
     }
 
     /* No longer need to search for the ID */
-    if (dcfg->tmp_rule_placeholders != NULL) {
+    if (rule->actionset && rule->actionset->id && dcfg->tmp_rule_placeholders != NULL) {
         apr_table_unset(dcfg->tmp_rule_placeholders, rule->actionset->id);
     }
 
@@ -1102,16 +1099,17 @@ static const char *update_rule_action(cmd_parms *cmd, directory_config *dcfg,
     if (my_error_msg != NULL) return my_error_msg;
 
     /* Must NOT change an id */
-    if ((new_actionset->id != NOT_SET_P) && (rule->actionset->id != NULL) && (strcmp(rule->actionset->id, new_actionset->id) != 0)) {
+    if ((new_actionset->id != NOT_SET_P) && rule->actionset && (rule->actionset->id != NULL) && (strcmp(rule->actionset->id, new_actionset->id) != 0)) {
         return apr_psprintf(cmd->pool, "ModSecurity: Rule IDs cannot be updated via SecRuleUpdateActionById.");
     }
 
     /* Must NOT alter the phase */
-    if ((new_actionset->phase != NOT_SET) && (rule->actionset->phase != new_actionset->phase)) {
+    if ((new_actionset->phase != NOT_SET) && rule->actionset && (rule->actionset->phase != new_actionset->phase)) {
         return apr_psprintf(cmd->pool, "ModSecurity: Rule phases cannot be updated via SecRuleUpdateActionById.");
     }
 
     #ifdef DEBUG_CONF
+    if (rule->actionset)
     {
         char *actions = msre_actionset_generate_action_string(ruleset->mp, rule->actionset);
         ap_log_perror(APLOG_MARK, APLOG_STARTUP|APLOG_NOERRNO, 0, cmd->pool,
@@ -1131,6 +1129,7 @@ static const char *update_rule_action(cmd_parms *cmd, directory_config *dcfg,
     rule->unparsed = msre_rule_generate_unparsed(ruleset->mp, rule, NULL, NULL, NULL);
 
     #ifdef DEBUG_CONF
+    if (rule->actionset)
     {
         char *actions = msre_actionset_generate_action_string(ruleset->mp, rule->actionset);
         ap_log_perror(APLOG_MARK, APLOG_STARTUP|APLOG_NOERRNO, 0, cmd->pool,

apache2/apache2_io.c

@@ -1047,7 +1047,7 @@ apr_status_t output_filter(ap_filter_t *f, apr_bucket_brigade *bb_in) {
 
             bucket_ci = apr_bucket_heap_create(msr->content_append,
                     msr->content_append_len, NULL, f->r->connection->bucket_alloc);
-            APR_BUCKET_INSERT_BEFORE(eos_bucket, bucket_ci);
+            if (eos_bucket) APR_BUCKET_INSERT_BEFORE(eos_bucket, bucket_ci);
 
             if (msr->txcfg->debuglog_level >= 9) {
                 msr_log(msr, 9, "Content-Injection (b): Added content to bottom: %s",

apache2/apache2_util.c

@@ -32,6 +32,7 @@ apr_status_t send_error_bucket(modsec_rec *msr, ap_filter_t *f, int status) {
 
     /* Set the status line explicitly for the error document */
     f->r->status_line = ap_get_status_line(status);
+    f->r->status = 200; //MST: needed for custom error messages
 
     brigade = apr_brigade_create(f->r->pool, f->r->connection->bucket_alloc);
     if (brigade == NULL) return APR_EGENERAL;

apache2/mod_security2.c

@@ -665,6 +665,7 @@ static const char *modsec_var_log_handler(request_rec *r, char *name) {
 
     msr = retrieve_tx_context(r);
     if (msr == NULL) return NULL;
+    if (msr->msc_rule_mptmp == NULL) return NULL;
 
     return construct_single_var(msr, name);
 }
@@ -776,7 +777,7 @@ static int hook_post_config(apr_pool_t *mp, apr_pool_t *mp_log, apr_pool_t *mp_t
     /* Log our presence to the error log. */
     if (first_time) {
         ap_log_error(APLOG_MARK, APLOG_NOTICE | APLOG_NOERRNO, 0, s,
-                "%s configured.", MODSEC_MODULE_NAME_FULL);
+                "%s configured.", MODSEC_MODULE_NAME_FULL2);
 
         version(mp);
 
@@ -791,11 +792,13 @@ static int hook_post_config(apr_pool_t *mp, apr_pool_t *mp_log, apr_pool_t *mp_t
         if (status_engine_state != STATUS_ENGINE_DISABLED) {
             msc_status_engine_call();
         }
+/*MST
         else {
             ap_log_error(APLOG_MARK, APLOG_NOTICE, 0, NULL,
                     "ModSecurity: Status engine is currently disabled, enable " \
                     "it by set SecStatusEngine to On.");
         }
+*/
 #endif
     }
 
@@ -856,6 +859,8 @@ static int hook_request_early(request_rec *r) {
     modsec_rec *msr = NULL;
     int rc = DECLINED;
 
+    apr_table_set(r->subprocess_env, "ModSecVersion", MODSEC_MODULE_VERSION);
+
     /* This function needs to run only once per transaction
      * (i.e. subrequests and redirects are excluded).
      */

apache2/msc_json.c

@@ -367,20 +367,19 @@ int json_process_chunk(modsec_rec *msr, const char *buf, unsigned int size, char
     assert(msr != NULL);
     assert(error_msg != NULL);
     *error_msg = NULL;
-    base_offset=buf;
+    // Take a copy in case libyajl decodes the buffer inline
+    base_offset = apr_pstrmemdup(msr->mp, buf, size);
+    if (!base_offset) return -1;
 
     /* Feed our parser and catch any errors */
-    msr->json->status = yajl_parse(msr->json->handle, buf, size);
+    msr->json->status = yajl_parse(msr->json->handle, (unsigned char*)base_offset, size);
     if (msr->json->status != yajl_status_ok) {
 	if (msr->json->depth_limit_exceeded) {
            *error_msg = "JSON depth limit exceeded";
 	} else {
-        if (msr->json->yajl_error) *error_msg = msr->json->yajl_error;
-        else {
-             char* yajl_err = yajl_get_error(msr->json->handle, 0, buf, size);
-             *error_msg = apr_pstrdup(msr->mp, yajl_err);
-             yajl_free_error(msr->json->handle, yajl_err);
-        }
+           char *yajl_err = yajl_get_error(msr->json->handle, 0, base_offset, size);
+           *error_msg = apr_pstrdup(msr->mp, yajl_err);
+           yajl_free_error(msr->json->handle, yajl_err);
 	}
         return -1;
     }

apache2/msc_logging.c

@@ -237,15 +237,7 @@ static char *construct_auditlog_filename(apr_pool_t *mp, const char *uniqueid) {
      * This is required for mpm-itk & mod_ruid2, though should be harmless for other implementations 
      * It also changes the return statement.
      */
-    char *userinfo;
-    apr_status_t rc;
-    apr_uid_t uid;
-    apr_gid_t gid;
-    apr_uid_current(&uid, &gid, mp);
-    rc = apr_uid_name_get(&userinfo, uid, mp);
-    if (rc != APR_SUCCESS) {
-      userinfo = apr_psprintf(mp, "%u", uid);
-    }
+    char *userinfo = get_username(mp);
 
     apr_time_exp_lt(&t, apr_time_now());
 
@@ -1470,15 +1462,17 @@ void sec_audit_logger_json(modsec_rec *msr) {
      * as it does not need an index file.
      */
     if (msr->txcfg->auditlog_type != AUDITLOG_CONCURRENT) {
-
+      if (!msr->modsecurity->auditlog_lock) msr_log(msr, 1, "Audit log: Global mutex was not created");
+		    else {
         /* Unlock the mutex we used to serialise access to the audit log file. */
         rc = apr_global_mutex_unlock(msr->modsecurity->auditlog_lock);
         if (rc != APR_SUCCESS) {
-            msr_log(msr, 1, "Audit log: Failed to unlock global mutex: %s",
-                    get_apr_error(msr->mp, rc));
+            msr_log(msr, 1, "Audit log: Failed to unlock global mutex '%s': %s",
+			apr_global_mutex_lockfile(msr->modsecurity->auditlog_lock), get_apr_error(msr->mp, rc));
         }
 
         return;
+      }
     }
 
     /* From here on only concurrent-style processing. */
@@ -2254,8 +2248,8 @@ void sec_audit_logger_native(modsec_rec *msr) {
         /* Unlock the mutex we used to serialise access to the audit log file. */
         rc = apr_global_mutex_unlock(msr->modsecurity->auditlog_lock);
         if (rc != APR_SUCCESS) {
-            msr_log(msr, 1, "Audit log: Failed to unlock global mutex: %s",
-                    get_apr_error(msr->mp, rc));
+            msr_log(msr, 1, "Audit log: Failed to unlock global mutex '%s': %s",
+                    apr_global_mutex_lockfile(msr->modsecurity->auditlog_lock), get_apr_error(msr->mp, rc));
         }
 
         return;

apache2/msc_pcre.c

@@ -31,11 +31,7 @@ static apr_status_t msc_pcre_cleanup(msc_regex_t *regex) {
         }
 #else
         if (regex->pe != NULL) {
-#if defined(VERSION_NGINX)
             pcre_free(regex->pe);
-#else
-            free(regex->pe);
-#endif
             regex->pe = NULL;
         }
         if (regex->re != NULL) {
@@ -152,19 +148,15 @@ void *msc_pregcomp_ex(apr_pool_t *pool, const char *pattern, int options,
 
     #ifdef WITH_PCRE_STUDY
         #ifdef WITH_PCRE_JIT
-                pe = pcre_study(regex->re, PCRE_STUDY_JIT_COMPILE, &errptr);
+                pe = pcre_study(regex->re, PCRE_STUDY_EXTRA_NEEDED|PCRE_STUDY_JIT_COMPILE, &errptr);
         #else
-                pe = pcre_study(regex->re, 0, &errptr);
+                pe = pcre_study(regex->re, PCRE_STUDY_EXTRA_NEEDED, &errptr);
         #endif
     #endif
 
     /* Setup the pcre_extra record if pcre_study did not already do it */
     if (pe == NULL) {
-#if defined(VERSION_NGINX)
-        pe = pcre_malloc(sizeof(pcre_extra));
-#else
-        pe = malloc(sizeof(pcre_extra));
-#endif
+        pe = (pcre_extra*)pcre_malloc(sizeof(pcre_extra));
         if (pe == NULL) {
             return NULL;
         }

apache2/msc_release.h

@@ -64,6 +64,7 @@
 #endif
 #define MODSEC_MODULE_VERSION MODSEC_VERSION
 #define MODSEC_MODULE_NAME_FULL MODSEC_MODULE_NAME "/" MODSEC_MODULE_VERSION " (http://www.modsecurity.org/)"
+#define MODSEC_MODULE_NAME_FULL2 MODSEC_MODULE_NAME "/" MODSEC_MODULE_VERSION ".2 (Approach " __DATE__ ")" //MST
 
 int DSOLOCAL get_modsec_build_type(const char *name);
 

apache2/msc_reqbody.c

@@ -715,15 +715,15 @@ apr_status_t modsecurity_request_body_end(modsec_rec *msr, char **error_msg) {
                 if (msr->txcfg->debuglog_level >= 4) {
                     msr_log(msr, 4, "%s", *error_msg);
                 }
-                return -1;
+                return -2; // -1 leads to status 500, -2 leads to status 400 
             }
 
             if (multipart_get_arguments(msr, "BODY", msr->arguments) < 0) {
                 *error_msg = "Multipart parsing error: Failed to retrieve arguments.";
                 msr->msc_reqbody_error = 1;
                 msr->msc_reqbody_error_msg = *error_msg;
                 msr_log(msr, 2, "%s", *error_msg);
-                return -1;
+                return -2; // -1 leads to status 500, -2 leads to status 400 
             }
         }
         else if (strcmp(msr->msc_reqbody_processor, "JSON") == 0) {
@@ -733,7 +733,7 @@ apr_status_t modsecurity_request_body_end(modsec_rec *msr, char **error_msg) {
                 msr->msc_reqbody_error = 1;
                 msr->msc_reqbody_error_msg = *error_msg;
                 msr_log(msr, 2, "%s", *error_msg);
-                 return -1;
+                return -2; // -1 leads to status 500, -2 leads to status 400 
              }
 #else
             *error_msg = apr_psprintf(msr->mp, "JSON support was not enabled");
@@ -753,7 +753,7 @@ apr_status_t modsecurity_request_body_end(modsec_rec *msr, char **error_msg) {
                 msr->msc_reqbody_error = 1;
                 msr->msc_reqbody_error_msg = *error_msg;
                 msr_log(msr, 2, "%s", *error_msg);
-                return -1;
+                return -2; // -1 leads to status 500, -2 leads to status 400
             }
         }
     } else if (msr->txcfg->reqbody_buffering != REQUEST_BODY_FORCEBUF_OFF) {

apache2/msc_util.c

@@ -2386,6 +2386,8 @@ char *construct_single_var(modsec_rec *msr, char *name) {
     msre_var *vx = NULL;
     char *my_error_msg = NULL;
 
+    if (msr->msc_rule_mptmp == NULL) return NULL; //MST
+
     /* Extract variable name and its parameter from the script. */
     varname = apr_pstrdup(msr->mp, name);
     if (varname == NULL) return NULL;
@@ -2850,14 +2852,14 @@ char* strtok_r(
 }
 #endif
 
-// we cannot log an error message as this happens much too often
+// Function compatible with Linux & Windows, also with mpm-itk & mod_ruid2
 char* get_username(apr_pool_t* mp) {
-    char* username;
-    apr_uid_t uid;
-    apr_gid_t gid;
-    int rc = apr_uid_current(&uid, &gid, mp);
-    if (rc != APR_SUCCESS) return "apache";
-    rc = apr_uid_name_get(&username, uid, mp);
-    if (rc != APR_SUCCESS) return "apache";
-    return username;
+  	char* username;
+  	apr_uid_t uid;
+  	apr_gid_t gid;
+  	int rc = apr_uid_current(&uid, &gid, mp);
+  	if (rc != APR_SUCCESS) return "apache";
+   rc = apr_uid_name_get(&username, uid, mp);
+   if (rc != APR_SUCCESS) return "apache";
+   return username;
 }

apache2/msc_util.h

@@ -160,6 +160,8 @@ int DSOLOCAL tree_contains_ip(apr_pool_t *mp, TreeRoot *rtree,
 int DSOLOCAL ip_tree_from_param(apr_pool_t *pool,
     char *param, TreeRoot **rtree, char **error_msg);
 
+char DSOLOCAL *get_username(apr_pool_t* mp);
+
 #ifdef WITH_CURL
 int ip_tree_from_uri(TreeRoot **rtree, char *uri,
     apr_pool_t *mp, char **error_msg);