Skip to content

Release the libmodsecurity error string on failed rule loading and merging #398

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

Open
tomsommer wants to merge 1 commit into
base: master
Choose a base branch
from
+214 −4
Open

Release the libmodsecurity error string on failed rule loading and merging #398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
61 changes: 57 additions & 4 deletions src/ngx_http_modsecurity_module.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -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;
Expand All @@ -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);
Expand All @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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,
Expand All @@ -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)
Expand Down
157 changes: 157 additions & 0 deletions tests/modsecurity-config-error.t
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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');

###############################################################################
Loading