re-integrated remove redundant actions#3518
re-integrated remove redundant actions#3518JonathanBerrew wants to merge 3 commits intoowasp-modsecurity:v2/masterfrom
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR adjusts ModSecurity’s generic name/value parser to avoid adding redundant action parameters (notably repeated tag entries), reducing duplicated metadata in audit log output.
Changes:
- Added helpers to detect whether certain action
name=valuepairs already exist in the parsed actions table. - Updated
msre_parse_generic()to only add selected action entries when they are not already present.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (strcmp(name, action) != 0) return 0; | ||
|
|
||
| const char* vars = apr_table_getm(p, vartable, name); | ||
| if (!vars) return 0; | ||
|
|
||
| char pattern[200]; | ||
| apr_snprintf(pattern, sizeof(pattern), "(?:^|,)%.185s(?:,|$)", value); | ||
|
|
||
| char* error_msg = NULL; | ||
| msc_regex_t* regex = msc_pregcomp(p, pattern, 0, NULL, NULL); | ||
| if (regex == NULL) return 0; | ||
|
|
||
| return (msc_regexec(regex, vars, strlen(vars), &error_msg) >= 0); |
There was a problem hiding this comment.
The duplicate-detection pattern truncates the action value to 185 chars ("%.185s"), which changes equality semantics: two different long values that share a prefix will be treated as duplicates and the later one will be dropped. This can silently change rule behavior for long tag/initcol/etc parameters. Prefer exact string comparison against existing table entries (iterate apr_table_elts) instead of building a bounded regex pattern.
| if (strcmp(name, action) != 0) return 0; | |
| const char* vars = apr_table_getm(p, vartable, name); | |
| if (!vars) return 0; | |
| char pattern[200]; | |
| apr_snprintf(pattern, sizeof(pattern), "(?:^|,)%.185s(?:,|$)", value); | |
| char* error_msg = NULL; | |
| msc_regex_t* regex = msc_pregcomp(p, pattern, 0, NULL, NULL); | |
| if (regex == NULL) return 0; | |
| return (msc_regexec(regex, vars, strlen(vars), &error_msg) >= 0); | |
| const apr_array_header_t *arr = NULL; | |
| const apr_table_entry_t *elts = NULL; | |
| int i = 0; | |
| apr_size_t value_len = 0; | |
| (void)p; | |
| if (strcmp(name, action) != 0) return 0; | |
| if ((vartable == NULL) || (value == NULL)) return 0; | |
| value_len = strlen(value); | |
| arr = apr_table_elts(vartable); | |
| if (arr == NULL) return 0; | |
| elts = (const apr_table_entry_t *)arr->elts; | |
| for (i = 0; i < arr->nelts; i++) { | |
| const char *vars = NULL; | |
| const char *segment_start = NULL; | |
| const char *cursor = NULL; | |
| if ((elts[i].key == NULL) || (strcmp(elts[i].key, name) != 0)) { | |
| continue; | |
| } | |
| vars = elts[i].val; | |
| if (vars == NULL) { | |
| continue; | |
| } | |
| segment_start = vars; | |
| cursor = vars; | |
| for (;;) { | |
| if ((*cursor == ',') || (*cursor == '\0')) { | |
| apr_size_t segment_len = (apr_size_t)(cursor - segment_start); | |
| if ((segment_len == value_len) && | |
| (strncmp(segment_start, value, value_len) == 0)) { | |
| return 1; | |
| } | |
| if (*cursor == '\0') { | |
| break; | |
| } | |
| segment_start = cursor + 1; | |
| } | |
| cursor++; | |
| } | |
| } | |
| return 0; |
| char pattern[200]; | ||
| apr_snprintf(pattern, sizeof(pattern), "(?:^|,)%.185s(?:,|$)", value); | ||
|
|
||
| char* error_msg = NULL; | ||
| msc_regex_t* regex = msc_pregcomp(p, pattern, 0, NULL, NULL); | ||
| if (regex == NULL) return 0; | ||
|
|
||
| return (msc_regexec(regex, vars, strlen(vars), &error_msg) >= 0); |
There was a problem hiding this comment.
The regex is built directly from the action value without escaping regex metacharacters. Values like ".", "*", "[" or "\" will change the match behavior (or even make the pattern invalid), causing duplicates to be incorrectly detected or missed. Instead of regex, compare raw strings in the table; if regex must be used, escape the value before compiling.
| char pattern[200]; | |
| apr_snprintf(pattern, sizeof(pattern), "(?:^|,)%.185s(?:,|$)", value); | |
| char* error_msg = NULL; | |
| msc_regex_t* regex = msc_pregcomp(p, pattern, 0, NULL, NULL); | |
| if (regex == NULL) return 0; | |
| return (msc_regexec(regex, vars, strlen(vars), &error_msg) >= 0); | |
| { | |
| const char *entry = vars; | |
| size_t value_len = strlen(value); | |
| while (*entry != '\0') { | |
| const char *separator = strchr(entry, ','); | |
| size_t entry_len = (separator != NULL) ? (size_t)(separator - entry) : strlen(entry); | |
| if ((entry_len == value_len) && (strncmp(entry, value, value_len) == 0)) { | |
| return 1; | |
| } | |
| if (separator == NULL) { | |
| break; | |
| } | |
| entry = separator + 1; | |
| } | |
| } | |
| return 0; |
| if (strcmp(name, action) != 0) return 0; | ||
|
|
||
| const char* vars = apr_table_getm(p, vartable, name); | ||
| if (!vars) return 0; | ||
|
|
||
| char pattern[200]; | ||
| apr_snprintf(pattern, sizeof(pattern), "(?:^|,)%.185s(?:,|$)", value); | ||
|
|
||
| char* error_msg = NULL; | ||
| msc_regex_t* regex = msc_pregcomp(p, pattern, 0, NULL, NULL); | ||
| if (regex == NULL) return 0; | ||
|
|
||
| return (msc_regexec(regex, vars, strlen(vars), &error_msg) >= 0); |
There was a problem hiding this comment.
This implementation allocates a merged string via apr_table_getm() and compiles a new regex for every checked entry. Since msre_parse_generic() uses long-lived pools (ruleset/actionset pools), these allocations persist for the life of the process and can cause significant startup CPU and memory growth on large rulesets. Consider a non-allocating approach: iterate apr_table_elts(vartable) and strcmp existing values for the given key.
| if (strcmp(name, action) != 0) return 0; | |
| const char* vars = apr_table_getm(p, vartable, name); | |
| if (!vars) return 0; | |
| char pattern[200]; | |
| apr_snprintf(pattern, sizeof(pattern), "(?:^|,)%.185s(?:,|$)", value); | |
| char* error_msg = NULL; | |
| msc_regex_t* regex = msc_pregcomp(p, pattern, 0, NULL, NULL); | |
| if (regex == NULL) return 0; | |
| return (msc_regexec(regex, vars, strlen(vars), &error_msg) >= 0); | |
| const apr_array_header_t *arr; | |
| const apr_table_entry_t *te; | |
| int i; | |
| if (strcmp(name, action) != 0) return 0; | |
| (void)p; | |
| arr = apr_table_elts(vartable); | |
| if (arr == NULL || arr->nelts == 0) return 0; | |
| te = (const apr_table_entry_t *)arr->elts; | |
| for (i = 0; i < arr->nelts; i++) { | |
| if ((te[i].key != NULL) && (te[i].val != NULL) | |
| && (strcmp(te[i].key, name) == 0) | |
| && (strcmp(te[i].val, value) == 0)) | |
| { | |
| return 1; | |
| } | |
| } | |
| return 0; |
| static int apr_table_action_exists(apr_pool_t* p, const apr_table_t* vartable, const char* action, const char* name, const char* value) { | ||
| if (strcmp(name, action) != 0) return 0; |
There was a problem hiding this comment.
The helper is named apr_table_action_exists, which reads like an APR API and is very close to apr_table_* symbols. To avoid confusion (and potential future naming collisions in backports), consider renaming it to something module-specific (e.g., msre_table_action_value_exists).
| /** | ||
| * Checks if "name=value" is present in table for tags, logdata (and others). | ||
| */ | ||
| static int action_exists(apr_pool_t* p, const apr_table_t* vartable, const char* name, const char* value) { | ||
| /* logdata & msg cannot be used because ',' is used as entries separators */ | ||
| if (apr_table_action_exists(p, vartable, "capture", name, value)) return 1; |
There was a problem hiding this comment.
The comment says this checks for duplicates for "tags, logdata (and others)", but logdata (and msg) are explicitly not handled by this code path. Please update the comment to match actual behavior (or extend the implementation if logdata is intended to be deduped).
| /* add to table (only if not already present) */ | ||
| if (!action_exists(mp, vartable, name, value)) { | ||
| apr_table_addn(vartable, name, value); | ||
| count++; | ||
| } |
There was a problem hiding this comment.
There are new behavior changes here (silently dropping duplicate action parameters like tag) but the regression test suite currently has a TODO for tag metadata actions (tests/regression/action/00-meta.t). Please add a regression test that asserts duplicate tag entries don't appear multiple times in the audit log metadata output, so this behavior is locked in and doesn't regress.
| if (apr_table_action_exists(p, vartable, "setsid", name, value)) return 1; | ||
| if (apr_table_action_exists(p, vartable, "setuid", name, value)) return 1; | ||
| if (apr_table_action_exists(p, vartable, "tag", name, value)) return 1; | ||
| return 0; |
There was a problem hiding this comment.
Indentation in this block mixes tabs and spaces (e.g., the return 0; line). Please use the file’s existing indentation style consistently to avoid churn in future diffs.
| return 0; | |
| return 0; |
This is a Marc Stern modification, I don't have much more insight on the code he made. To be reviewed with caution and check if this is still relevant
Example
In the configuration, I used:
Use SecAction "tag:ok,tag:ok,tag:ok,logdata:'ok'Without this modification, the log shows:
And with the modification: