Skip to content

fix: replace strdup(error) with nginx pool allocation in config handlers - #382

Open
eilandert wants to merge 3 commits into
owasp-modsecurity:masterfrom
eilandert:fix/strdup-config-error-leaks
Open

eilandert wants to merge 3 commits into
owasp-modsecurity:masterfrom
eilandert:fix/strdup-config-error-leaks

Conversation

@eilandert

Copy link
Copy Markdown

In ngx_conf_set_rules, ngx_conf_set_rules_file, and ngx_conf_set_rules_remote, rule load errors returned a strdup()- allocated string. Nginx treats the returned char* as a constant string and never frees it, causing a heap leak on every failed nginx -s reload when rule files have errors.

Replace with ngx_pstrdup(cf->pool, ...) so the error string is allocated from the config pool and freed automatically when nginx reloads configuration.

eilandert added 3 commits May 13, 2026 14:09
In ngx_conf_set_rules, ngx_conf_set_rules_file, and
ngx_conf_set_rules_remote, rule load errors returned a strdup()-
allocated string. Nginx treats the returned char* as a constant string
and never frees it, causing a heap leak on every failed nginx -s reload
when rule files have errors.

Replace with ngx_pstrdup(cf->pool, ...) so the error string is
allocated from the config pool and freed automatically when nginx
reloads configuration.

Severity: High
Reported-by: Security audit 2026-05-13
ngx_pstrdup returns u_char* but the config handler return type is char*.
nginx builds with -Werror=pointer-sign so this produced a compile error.
Add explicit (char*) cast at all three call sites.
@sonarqubecloud

Copy link
Copy Markdown

@tomsommer

Copy link
Copy Markdown
Contributor

Confirming the problem this PR addresses: the strdup() in the three modsecurity_rules* handlers is never released, and under valgrind it shows as a per-failed-parse leak (nginx -t with a bad rule, or a bad configuration delivered by SIGHUP to a running master; a -s reload from a separate process does not accumulate because that process exits).

Two points on the replacement:

  1. ngx_pstrdup() allocates src->len bytes and copies them without a terminating NUL (src/core/ngx_string.c), but ngx_conf_handler() prints the returned message with "%s" (src/core/ngx_conf_file.c), so the emerg line reads past the end of the allocation. An explicit ngx_pnalloc(cf->pool, len + 1) plus rv[len] = '\0' avoids that.
  2. If ngx_pstrdup() returns NULL, the handler returns NULL, which is NGX_CONF_OK: the configuration error would be silently accepted. Returning NGX_CONF_ERROR in that case keeps nginx failing closed.

libmodsecurity's own error string is a separate leak (it is heap-allocated by libmodsecurity and must be released with msc_rules_error_cleanup(), available since 3.0.13); #398 handles that and the equivalent path in merge_conf(), and deliberately leaves the strdup lines to this PR to avoid overlapping. Found during a valgrind audit of the connector done with Claude Fable 5.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants