Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Memory leaks + enhanced logging #3156

Closed
wants to merge 7 commits into from
Closed

Memory leaks + enhanced logging #3156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c5a6d6b
Revert to OWASP
marcstern Feb 20, 2024
ca7b4b4
Merge branch 'v2/master' of https://github.com/marcstern/ModSecurity …
marcstern May 16, 2024
d45c4ba
spaces
marcstern May 16, 2024
c7c7881
space
marcstern May 16, 2024
e803cdd
Merge branch 'v2/master' of https://github.com/marcstern/ModSecurity …
marcstern May 24, 2024
f74fea6
Log audit lock name in case of problem.
marcstern May 28, 2024
313ed6e
Fixed return value
marcstern May 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions apache2/msc_logging.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1474,8 +1474,8 @@ void sec_audit_logger_json(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;
Expand Down
114 changes: 43 additions & 71 deletions apache2/re.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,22 +252,21 @@ char *update_rule_target_ex(modsec_rec *msr, msre_ruleset *ruleset, msre_rule *r
if(rule != NULL) {

target_list = strdup(p2);
if(target_list == NULL)
return apr_psprintf(ruleset->mp, "Error to update target - memory allocation");;
if (target_list == NULL) {
my_error_msg = apr_psprintf(ruleset->mp, "Error to update target - memory allocation");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here (and at some other cases) you removed the return statement and after set of my_error_msg you jump to the label end. There you call an msr_log(msr, 9, my_error_msg) and always return with NULL.

But the calling places return this value, so may be that char * needs there - isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I fixed the return value

goto end;
}

if(p3 != NULL) {
if (p3 != NULL) {
replace = strdup(p3);
if(replace == NULL) {
free(target_list);
target_list = NULL;
return apr_psprintf(ruleset->mp, "Error to update target - memory allocation");;
if (replace == NULL) {
my_error_msg = apr_psprintf(ruleset->mp, "Error to update target - memory allocation");
goto end;
}
}

if(replace != NULL) {

if (replace != NULL) {
opt = strchr(replace,'!');

if(opt != NULL) {
*opt = '\0';
opt++;
Expand All @@ -283,30 +282,21 @@ char *update_rule_target_ex(modsec_rec *msr, msre_ruleset *ruleset, msre_rule *r
}

opt = strchr(param,':');

if(opt != NULL) {
if (opt != NULL) {
name = apr_strtok(param,":",&value);
} else {
name = param;
}

if(apr_table_get(ruleset->engine->variables, name) == NULL) {
if(target_list != NULL)
free(target_list);
if(replace != NULL)
free(replace);
if(msr) {
msr_log(msr, 9, "Error to update target - [%s] is not valid target", name);
}
return apr_psprintf(ruleset->mp, "Error to update target - [%s] is not valid target", name);
if (apr_table_get(ruleset->engine->variables, name) == NULL) {
my_error_msg = apr_psprintf(ruleset->mp, "Error to update target - [%s] is not valid target", name);
goto end;
}

name_len = strlen(name);
if (value != NULL) value_len = strlen(value);

if(value != NULL)
value_len = strlen(value);

if(msr) {
if (msr) {
msr_log(msr, 9, "Trying to replace by variable name [%s] value [%s]", name, value);
}
#if !defined(MSC_TEST)
Expand All @@ -318,13 +308,13 @@ char *update_rule_target_ex(modsec_rec *msr, msre_ruleset *ruleset, msre_rule *r
targets = (msre_var **)rule->targets->elts;
// TODO need a good way to remove the element from array, maybe change array by tables or rings
for (i = 0; i < rule->targets->nelts; i++) {
if((strlen(targets[i]->name) == strlen(name)) &&
if ((strlen(targets[i]->name) == strlen(name)) &&
(strncasecmp(targets[i]->name,name,strlen(targets[i]->name)) == 0) &&
(targets[i]->is_negated == is_negated) &&
(targets[i]->is_counting == is_counting)) {

if(value != NULL && targets[i]->param != NULL) {
if((strlen(targets[i]->param) == strlen(value)) &&
if (value != NULL && targets[i]->param != NULL) {
if ((strlen(targets[i]->param) == strlen(value)) &&
strncasecmp(targets[i]->param,value,strlen(targets[i]->param)) == 0) {
memset(targets[i]->name,0,strlen(targets[i]->name));
memset(targets[i]->param,0,strlen(targets[i]->param));
Expand All @@ -346,9 +336,9 @@ char *update_rule_target_ex(modsec_rec *msr, msre_ruleset *ruleset, msre_rule *r

p = apr_strtok(target_list, ",", &savedptr);

while(p != NULL) {
if(replace != NULL) {
if(match == 1) {
while (p != NULL) {
if (replace != NULL) {
if (match == 1) {
rc = msre_parse_targets(ruleset, p, rule->targets, &my_error_msg);
if (rc < 0) {
if(msr) {
Expand All @@ -361,7 +351,7 @@ char *update_rule_target_ex(modsec_rec *msr, msre_ruleset *ruleset, msre_rule *r
#endif
goto end;
}
if(msr) {
if (msr) {
msr_log(msr, 9, "Successfully replaced variable");
}
#if !defined(MSC_TEST)
Expand All @@ -370,30 +360,29 @@ char *update_rule_target_ex(modsec_rec *msr, msre_ruleset *ruleset, msre_rule *r
}
#endif
var_appended = 1;

} else {
if(msr) {
if (msr) {
msr_log(msr, 9, "Cannot find variable to replace");
}
#if !defined(MSC_TEST)
else {
ap_log_error(APLOG_MARK, APLOG_ERR, 0, NULL, " ModSecurity: Cannot find varibale to replace");
ap_log_error(APLOG_MARK, APLOG_ERR, 0, NULL, " ModSecurity: Cannot find variable to replace");
}
#endif
goto end;
}
} else {

target = strdup(p);
if(target == NULL)
return NULL;
if (target == NULL) {
my_error_msg = apr_psprintf(ruleset->mp, "Error to update target - memory allocation");
goto end;
}

is_negated = is_counting = 0;
param = name = value = NULL;

opt = strchr(target,'!');

if(opt != NULL) {
if (opt != NULL) {
*opt = '\0';
opt++;
param = opt;
Expand All @@ -408,30 +397,21 @@ char *update_rule_target_ex(modsec_rec *msr, msre_ruleset *ruleset, msre_rule *r
}

opt = strchr(param,':');

if(opt != NULL) {
if (opt != NULL) {
name = apr_strtok(param,":",&value);
} else {
name = param;
}

if(apr_table_get(ruleset->engine->variables, name) == NULL) {
if(target_list != NULL)
free(target_list);
if(replace != NULL)
free(replace);
if(msr) {
msr_log(msr, 9, "Error to update target - [%s] is not valid target", name);
}
return apr_psprintf(ruleset->mp, "Error to update target - [%s] is not valid target", name);
if (apr_table_get(ruleset->engine->variables, name) == NULL) {
my_error_msg = apr_psprintf(ruleset->mp, "Error to update target - [%s] is not valid target", name);
goto end;
}

name_len = strlen(name);
if (value != NULL) value_len = strlen(value);

if(value != NULL)
value_len = strlen(value);

if(msr) {
if (msr) {
msr_log(msr, 9, "Trying to append variable name [%s] value [%s]", name, value);
}
#if !defined(MSC_TEST)
Expand Down Expand Up @@ -461,12 +441,12 @@ char *update_rule_target_ex(modsec_rec *msr, msre_ruleset *ruleset, msre_rule *r
}
}

if(target != NULL) {
if (target != NULL) {
free(target);
target = NULL;
}

if(match == 0 ) {
if (match == 0 ) {
rc = msre_parse_targets(ruleset, p, rule->targets, &my_error_msg);
if (rc < 0) {
if(msr) {
Expand Down Expand Up @@ -495,7 +475,7 @@ char *update_rule_target_ex(modsec_rec *msr, msre_ruleset *ruleset, msre_rule *r
p = apr_strtok(NULL,",",&savedptr);
}

if(var_appended == 1) {
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 = apr_pstrdup(ruleset->mp, current_targets);
Expand All @@ -511,19 +491,11 @@ char *update_rule_target_ex(modsec_rec *msr, msre_ruleset *ruleset, msre_rule *r
}

end:
if(target_list != NULL) {
free(target_list);
target_list = NULL;
}
if(replace != NULL) {
free(replace);
replace = NULL;
}
if(target != NULL) {
free(target);
target = NULL;
}
return NULL;
if (msr && my_error_msg) msr_log(msr, 9, my_error_msg);
if (target_list != NULL) free(target_list);
if (replace != NULL) free(replace);
if (target != NULL) free(target);
return my_error_msg;
}

int msre_ruleset_rule_matches_exception(msre_rule *rule, rule_exception *re) {
Expand Down
Loading