Merge branch 'v2/master' into v2/mst/nullcheck2

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

@@ -0,0 +1,51 @@
+name: Quality Assurance
+
+on:
+  push:
+  pull_request:
+
+jobs:
+  build-linux:
+    runs-on: ${{ matrix.os }}
+    strategy:
+      matrix:
+        os: [ubuntu-22.04]
+        platform: [x32, x64]
+        compiler: [gcc, clang]
+        configure:
+          - {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 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 }}
+      - 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/msc_json.c

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

apache2/msc_logging.c

@@ -237,7 +237,15 @@ 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 = get_username(mp);
+    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);
+    }
 
     apr_time_exp_lt(&t, apr_time_now());
 

apache2/msc_pcre.c

@@ -31,7 +31,11 @@ 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) {
@@ -148,15 +152,19 @@ 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_EXTRA_NEEDED|PCRE_STUDY_JIT_COMPILE, &errptr);
+                pe = pcre_study(regex->re, PCRE_STUDY_JIT_COMPILE, &errptr);
         #else
-                pe = pcre_study(regex->re, PCRE_STUDY_EXTRA_NEEDED, &errptr);
+                pe = pcre_study(regex->re, 0, &errptr);
         #endif
     #endif
 
     /* Setup the pcre_extra record if pcre_study did not already do it */
     if (pe == NULL) {
-        pe = (pcre_extra*)pcre_malloc(sizeof(pcre_extra));
+#if defined(VERSION_NGINX)
+        pe = pcre_malloc(sizeof(pcre_extra));
+#else
+        pe = malloc(sizeof(pcre_extra));
+#endif
         if (pe == NULL) {
             return NULL;
         }

apache2/msc_util.c

@@ -2850,14 +2850,3 @@ char* strtok_r(
 }
 #endif
 
-// 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;
-}

apache2/msc_util.h

@@ -160,8 +160,6 @@ 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);

apache2/persist_dbm.c

@@ -813,4 +813,4 @@ int collections_remove_stale(modsec_rec *msr, const char *col_name) {
     }
 
     return -1;
-}
+}

apache2/re.c

@@ -84,7 +84,6 @@ static int fetch_target_exception(msre_rule *rule, modsec_rec *msr, msre_var *va
     assert(exceptions != NULL);
 
     {
-
         myvar = apr_pstrdup(msr->mp, var->name);
 
         c = strchr(myvar,':');
@@ -363,11 +362,11 @@ char *update_rule_target_ex(modsec_rec *msr, msre_ruleset *ruleset, msre_rule *r
                     rc = msre_parse_targets(ruleset, p, rule->targets, &my_error_msg);
                     if (rc < 0) {
                         if(msr) {
-                            msr_log(msr, 9, "Error parsing rule targets to replace variable: %s", my_error_msg);
+                            msr_log(msr, 9, "Error parsing rule targets to replace variable");
                         }
 #if !defined(MSC_TEST)
                         else {
-                            ap_log_error(APLOG_MARK, APLOG_ERR, 0, NULL, " ModSecurity: Error parsing rule targets to replace variable: %s", my_error_msg);
+                            ap_log_error(APLOG_MARK, APLOG_ERR, 0, NULL, " ModSecurity: Error parsing rule targets to replace variable");
                         }
 #endif
                         goto end;
@@ -388,21 +387,16 @@ char *update_rule_target_ex(modsec_rec *msr, msre_ruleset *ruleset, msre_rule *r
                     }
 #if !defined(MSC_TEST)
                     else {
-                        ap_log_error(APLOG_MARK, APLOG_ERR, 0, NULL, " ModSecurity: Cannot find variable to replace");
+                        ap_log_error(APLOG_MARK, APLOG_ERR, 0, NULL, " ModSecurity: Cannot find varibale to replace");
                     }
 #endif
                     goto end;
                 }
             } else {
 
                 target = strdup(p);
-                if(target == NULL) {
-                   if(target_list != NULL)
-                       free(target_list);
-                   if(replace != NULL)
-                       free(replace);
-                   return NULL;
-                  }
+                if(target == NULL)
+                    return NULL;
 
                 is_negated = is_counting = 0;
                 param = name = value = NULL;
@@ -436,8 +430,6 @@ char *update_rule_target_ex(modsec_rec *msr, msre_ruleset *ruleset, msre_rule *r
                         free(target_list);
                     if(replace != NULL)
                         free(replace);
-                    if(target != NULL)
-                        free(target);
                     if(msr) {
                         msr_log(msr, 9, "Error to update target - [%s] is not valid target", name);
                     }
@@ -516,7 +508,7 @@ char *update_rule_target_ex(modsec_rec *msr, msre_ruleset *ruleset, msre_rule *r
         if(var_appended == 1)  {
             current_targets = msre_generate_target_string(ruleset->mp, rule);
             rule->unparsed = msre_rule_generate_unparsed(ruleset->mp, rule, current_targets, NULL, NULL);
-            rule->p1 = current_targets;
+            rule->p1 = apr_pstrdup(ruleset->mp, current_targets);
             if(msr) {
                 msr_log(msr, 9, "Successfully appended variable");
             }
@@ -529,12 +521,18 @@ char *update_rule_target_ex(modsec_rec *msr, msre_ruleset *ruleset, msre_rule *r
     }
 
 end:
-    if(target_list != NULL)
+    if(target_list != NULL) {
         free(target_list);
-    if(replace != NULL)
+        target_list = NULL;
+    }
+    if(replace != NULL) {
         free(replace);
-    if(target != NULL)
+        replace = NULL;
+    }
+    if(target != NULL)  {
         free(target);
+        target = NULL;
+    }
     return NULL;
 }
 
@@ -648,10 +646,7 @@ static char *msre_generate_target_string(apr_pool_t *pool, msre_rule *rule)  {
 /**
  * Generate an action string from an actionset.
  */
-#ifndef DEBUG_CONF
- static 
-#endif
-char *msre_actionset_generate_action_string(apr_pool_t *pool, const msre_actionset *actionset)  {
+static char *msre_actionset_generate_action_string(apr_pool_t *pool, const msre_actionset *actionset)  {
     const apr_array_header_t *tarr = NULL;
     const apr_table_entry_t *telts = NULL;
     char *actions = NULL;
@@ -1071,12 +1066,6 @@ int msre_parse_generic(apr_pool_t *mp, const char *text, apr_table_t *vartable,
         /* ignore whitespace */
         while(isspace(*p)) p++;
         if (*p == '\0') return count;
- 
-        /* ignore empty action */
-        if (*p == ',') {
-          	p++;
-           continue;
-        }
 
         /* we are at the beginning of the name */
         name = p;