From 3899ef6e9e4dc8d7c77f3467fdc151c221fce59b Mon Sep 17 00:00:00 2001 From: Tom Sommer Date: Sat, 19 Sep 2026 16:31:08 +0200 Subject: [PATCH] Release the libmodsecurity error string on failed rule loading and merging msc_rules_add(), msc_rules_add_file(), msc_rules_add_remote() and msc_rules_merge() hand out a strdup()'ed error message and transfer its ownership to the caller. The connector never released it, so every failed config parse leaked the message. When the master process itself parses the broken config (a direct SIGHUP, as systemd's ExecReload does) the leak is retained for the lifetime of the master and grows with every failed reload. Add ngx_http_modsecurity_rules_error_free(), which uses the documented msc_rules_error_cleanup() from libmodsecurity v3.0.13 and falls back to free() on older releases, and call it in the three ngx_conf_set_rules* handlers and in ngx_http_modsecurity_merge_conf(). The connector's own strdup() of the message in the three handlers is left untouched on purpose: replacing it with a pool allocation is PR #382's change. This commit only adds the release of the string libmodsecurity handed out, which #382 does not do, and fixes merge_conf(), which #382 does not touch. In merge_conf() the message is now copied into cf->pool before it is released. The copy is explicitly NUL-terminated because ngx_conf_handler() prints it with "%s". msc_rules_merge() is also wrapped in the pcre_malloc_init()/pcre_malloc_done() pair, like the three ngx_conf_set_rules* handlers already do. tests/modsecurity-config-error.t checks that nginx -t rejects an inline rules syntax error, a rules file syntax error and a rule id duplicated between a server and a location, and that the libmodsecurity message reaches the emerg line complete, with nginx's "in :" suffix attached. --- src/ngx_http_modsecurity_module.c | 61 +++++++++++- tests/modsecurity-config-error.t | 157 ++++++++++++++++++++++++++++++ 2 files changed, 214 insertions(+), 4 deletions(-) create mode 100644 tests/modsecurity-config-error.t diff --git a/src/ngx_http_modsecurity_module.c b/src/ngx_http_modsecurity_module.c index d3d9624d..b540ec51 100644 --- a/src/ngx_http_modsecurity_module.c +++ b/src/ngx_http_modsecurity_module.c @@ -111,6 +111,25 @@ ngx_http_modsecurity_pcre_malloc_done(ngx_pool_t *old_pool) } #endif +/* + * Release the error message handed out by msc_rules_add(), msc_rules_add_file(), + * msc_rules_add_remote() and msc_rules_merge(). libmodsecurity allocates it with + * strdup() and transfers the ownership to the caller; msc_rules_error_cleanup() + * is the documented release call, available since libmodsecurity v3.0.13. + */ +static ngx_inline void +ngx_http_modsecurity_rules_error_free(const char *error) +{ + if (error == NULL) { + return; + } +#if defined(MODSECURITY_CHECK_VERSION) && (MODSECURITY_VERSION_NUM >= 30130100) + msc_rules_error_cleanup(error); +#else + free((void *) error); +#endif +} + /* * ngx_string's are not null-terminated in common case, so we need to convert * them into null-terminated ones before passing to ModSecurity @@ -348,6 +367,7 @@ ngx_conf_set_rules(ngx_conf_t *cf, ngx_command_t *cmd, void *conf) { int res; char *rules; + char *rv; ngx_str_t *value; const char *error; ngx_pool_t *old_pool; @@ -367,7 +387,9 @@ ngx_conf_set_rules(ngx_conf_t *cf, ngx_command_t *cmd, void *conf) if (res < 0) { dd("Failed to load the rules: '%s' - reason: '%s'", rules, error); - return strdup(error); + rv = strdup(error); + ngx_http_modsecurity_rules_error_free(error); + return rv; } mmcf = ngx_http_conf_get_module_main_conf(cf, ngx_http_modsecurity_module); @@ -382,6 +404,7 @@ ngx_conf_set_rules_file(ngx_conf_t *cf, ngx_command_t *cmd, void *conf) { int res; char *rules_set; + char *rv; ngx_str_t *value; const char *error; ngx_pool_t *old_pool; @@ -401,7 +424,9 @@ ngx_conf_set_rules_file(ngx_conf_t *cf, ngx_command_t *cmd, void *conf) if (res < 0) { dd("Failed to load the rules from: '%s' - reason: '%s'", rules_set, error); - return strdup(error); + rv = strdup(error); + ngx_http_modsecurity_rules_error_free(error); + return rv; } mmcf = ngx_http_conf_get_module_main_conf(cf, ngx_http_modsecurity_module); @@ -415,6 +440,7 @@ char * ngx_conf_set_rules_remote(ngx_conf_t *cf, ngx_command_t *cmd, void *conf) { int res; + char *rv; ngx_str_t *value; const char *error; const char *rules_remote_key, *rules_remote_server; @@ -440,7 +466,9 @@ ngx_conf_set_rules_remote(ngx_conf_t *cf, ngx_command_t *cmd, void *conf) if (res < 0) { dd("Failed to load the rules from: '%s' - reason: '%s'", rules_remote_server, error); - return strdup(error); + rv = strdup(error); + ngx_http_modsecurity_rules_error_free(error); + return rv; } mmcf = ngx_http_conf_get_module_main_conf(cf, ngx_http_modsecurity_module); @@ -752,7 +780,10 @@ ngx_http_modsecurity_merge_conf(ngx_conf_t *cf, void *parent, void *child) ngx_http_core_loc_conf_t *clcf = ngx_http_conf_get_module_loc_conf(cf, ngx_http_core_module); #endif int rules; + size_t len; + char *rv; const char *error = NULL; + ngx_pool_t *old_pool; dd("merging loc config [%s] - parent: '%p' child: '%p'", ngx_str_to_char(clcf->name, cf->pool), parent, @@ -774,10 +805,32 @@ ngx_http_modsecurity_merge_conf(ngx_conf_t *cf, void *parent, void *child) dd("CHILD RULES"); msc_rules_dump(c->rules_set); #endif + old_pool = ngx_http_modsecurity_pcre_malloc_init(cf->pool); rules = msc_rules_merge(c->rules_set, p->rules_set, &error); + ngx_http_modsecurity_pcre_malloc_done(old_pool); if (rules < 0) { - return strdup(error); + if (error == NULL) { + /* libmodsecurity could not even allocate the message */ + return NGX_CONF_ERROR; + } + + /* + * ngx_conf_handler() prints the returned message with "%s", so the + * copy has to be NUL-terminated -- note that ngx_pstrdup() would + * not be usable here, it does not append the terminator. + */ + len = ngx_strlen(error); + rv = ngx_pnalloc(cf->pool, len + 1); + + if (rv != NULL) { + ngx_memcpy(rv, error, len); + rv[len] = '\0'; + } + + ngx_http_modsecurity_rules_error_free(error); + + return rv != NULL ? rv : NGX_CONF_ERROR; } #if defined(MODSECURITY_DDEBUG) && (MODSECURITY_DDEBUG) diff --git a/tests/modsecurity-config-error.t b/tests/modsecurity-config-error.t new file mode 100644 index 00000000..a0fcb7cb --- /dev/null +++ b/tests/modsecurity-config-error.t @@ -0,0 +1,157 @@ +#!/usr/bin/perl + +# Tests for ModSecurity-nginx connector (configuration error reporting). +# +# The connector hands nginx the error message produced by libmodsecurity. The +# message buffer is owned by the connector once libmodsecurity returned it, so +# it has to be copied before it is released. These tests assert that the +# message nginx prints is complete and correctly terminated -- a copy that is +# released too early, or one that is not NUL-terminated, shows up here as a +# truncated or garbled emerg line. + +############################################################################### + +use warnings; +use strict; + +use Test::More; + +BEGIN { use FindBin; chdir($FindBin::Bin); } + +use lib 'lib'; +use Test::Nginx; + +############################################################################### + +select STDERR; $| = 1; +select STDOUT; $| = 1; + +my $t = Test::Nginx->new()->has(qw/http/)->plan(6); + +$t->write_file('error.log', ''); + +$t->write_file('bad-rules.conf', <<'EOF'); +SecRuleEngine On +SecRule REQUEST_HEADERS:User-Agent "@rx bad-ua" "id:4242,phase:1,deny,nosuchaction" +EOF + +$t->write_file_expand('bad-inline.conf', <<'EOF'); + +%%TEST_GLOBALS%% + +events { +} + +http { + %%TEST_GLOBALS_HTTP%% + + server { + listen 127.0.0.1:%%PORT_8080%%; + server_name localhost; + + location / { + modsecurity on; + modsecurity_rules ' + SecRuleEngine On + SecRule ARGS "@rx attack" "id:4141,phase:1,deny,nosuchaction" + '; + } + } +} + +EOF + +$t->write_file_expand('bad-file.conf', <<'EOF'); + +%%TEST_GLOBALS%% + +events { +} + +http { + %%TEST_GLOBALS_HTTP%% + + server { + listen 127.0.0.1:%%PORT_8080%%; + server_name localhost; + + location / { + modsecurity on; + modsecurity_rules_file %%TESTDIR%%/bad-rules.conf; + } + } +} + +EOF + +$t->write_file_expand('bad-merge.conf', <<'EOF'); + +%%TEST_GLOBALS%% + +events { +} + +http { + %%TEST_GLOBALS_HTTP%% + + server { + listen 127.0.0.1:%%PORT_8080%%; + server_name localhost; + + modsecurity on; + modsecurity_rules ' + SecRuleEngine On + SecRule ARGS "@rx parent" "id:7777,phase:1,deny,status:403" + '; + + location /child { + modsecurity_rules ' + SecRule ARGS "@rx child" "id:7777,phase:1,deny,status:403" + '; + } + } +} + +EOF + +############################################################################### + +# Run "nginx -t" on one of the configurations above, the way Test::Nginx's own +# dump_config() builds its command line, and return the exit code together with +# everything the test printed on stdout/stderr. + +sub nginx_t { + my ($t, $conf) = @_; + my $testdir = $t->testdir(); + + # No -g here: every configuration above was written with + # write_file_expand(), so %%TEST_GLOBALS%% already put "pid" and + # "error_log" into the file itself. + my $command = "$Test::Nginx::NGINX -t -p $testdir/ -c $conf " + . "-e error.log"; + + my $out = qx/$command 2>&1/; + + return ($?, $out); +} + +############################################################################### + +my ($rc, $out); + +($rc, $out) = nginx_t($t, 'bad-inline.conf'); +isnt($rc, 0, 'inline rules syntax error rejected'); +like($out, qr/Rules error\..*got:\s+nosuchaction" in \S*bad-inline\.conf:\d+$/m, + 'inline rules error message intact'); + +($rc, $out) = nginx_t($t, 'bad-file.conf'); +isnt($rc, 0, 'rules file syntax error rejected'); +like($out, qr/Rules error\..*bad-rules\.conf.*got:\s+nosuchaction" in \S*bad-file\.conf:\d+$/m, + 'rules file error message intact'); + +($rc, $out) = nginx_t($t, 'bad-merge.conf'); +isnt($rc, 0, 'duplicated rule id rejected on merge'); +like($out, qr/Rule id: 7777 is duplicated\s+in \S*bad-merge\.conf:\d+$/m, + 'merge error message intact'); + +###############################################################################