Skip to content

Commit ce4cf24

Browse files
author
Felipe Zimmerle
committed
Refactoring external resources download warn messages
Holding the message to be displayed when Apache is ready to write on the error_log instead of the default output. Regression tests were added.
1 parent d4a055e commit ce4cf24

11 files changed

+204
-49
lines changed

‎CHANGES

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,11 @@
11
DD mmm YYYY - 2.9.????? (To be released)
22
-----------------------
33

4+
* Adds missing 'ModSecurity:' prefix in some warnings messages.
5+
[Walter Hop and ModSecurity team]
6+
* Refactoring external resources download warn messages. Holding the message
7+
to be displayed when Apache is ready to write on the error_log.
8+
[ModSecurity team]
49
* Remote resources loading process is now failing in case of HTTP error.
510
[Walter Hop and ModSecurity team]
611
* Fixed start up crash on Apache with mod_ssl configured. Crash was happening

‎apache2/mod_security2.c

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ unsigned long int DSOLOCAL msc_pcre_match_limit_recursion = 0;
7272
msc_remote_rules_server DSOLOCAL *remote_rules_server = NULL;
7373
#endif
7474
int DSOLOCAL remote_rules_fail_action = REMOTE_RULES_ABORT_ON_FAIL;
75+
char DSOLOCAL *remote_rules_fail_message = NULL;
7576

7677
int DSOLOCAL status_engine_state = STATUS_ENGINE_DISABLED;
7778

@@ -761,6 +762,13 @@ static int hook_post_config(apr_pool_t *mp, apr_pool_t *mp_log, apr_pool_t *mp_t
761762
}
762763
#endif
763764

765+
if (remote_rules_fail_message != NULL)
766+
{
767+
ap_log_error(APLOG_MARK, APLOG_NOTICE, 0, NULL, "ModSecurity: " \
768+
"Problems loading external resources: %s",
769+
remote_rules_fail_message);
770+
}
771+
764772
#ifdef WITH_REMOTE_RULES
765773
if (remote_rules_server != NULL)
766774
{

‎apache2/modsecurity.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,7 @@ extern DSOLOCAL unsigned long int msc_pcre_match_limit_recursion;
150150
extern DSOLOCAL msc_remote_rules_server *remote_rules_server;
151151
#endif
152152
extern DSOLOCAL int remote_rules_fail_action;
153+
extern DSOLOCAL char *remote_rules_fail_message;
153154

154155
extern DSOLOCAL int status_engine_state;
155156

‎apache2/msc_remote_rules.c

Lines changed: 29 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -328,16 +328,22 @@ int msc_remote_download_content(apr_pool_t *mp, const char *uri, const char *key
328328
{
329329
if (remote_rules_fail_action == REMOTE_RULES_WARN_ON_FAIL)
330330
{
331-
ap_log_error(APLOG_MARK, APLOG_NOTICE, 0, NULL,
332-
"Failed to download \"%s\" error: %s ",
333-
uri, curl_easy_strerror(res));
331+
if (remote_rules_fail_message == NULL)
332+
{
333+
remote_rules_fail_message = "";
334+
}
334335

335-
ret = -2;
336-
goto failed;
336+
remote_rules_fail_message = apr_psprintf(mp, "%sFailed to " \
337+
"download: \"%s\" error: %s. ",
338+
remote_rules_fail_message, uri,
339+
curl_easy_strerror(res));
340+
341+
ret = -2;
342+
goto failed;
337343
}
338344
else
339345
{
340-
*error_msg = apr_psprintf(mp, "Failed to download \"%s\" " \
346+
*error_msg = apr_psprintf(mp, "Failed to download: \"%s\" " \
341347
"error: %s ",
342348
uri, curl_easy_strerror(res));
343349

@@ -581,6 +587,11 @@ int msc_remote_decrypt(apr_pool_t *pool,
581587
}
582588

583589
apr_crypto_block_cleanup(block);
590+
apr_crypto_cleanup(f);
591+
592+
// Shutdown the apr_crypto seems to be the correct thing to do.
593+
// However it seems to add instability especially if mod_ssl is enabled.
594+
// apr_crypto_shutdown(driver);
584595

585596
return 0;
586597
}
@@ -611,7 +622,7 @@ int msc_remote_add_rules_from_uri(cmd_parms *orig_parms,
611622
{
612623

613624
#ifdef WITH_REMOTE_RULES
614-
struct msc_curl_memory_buffer_t chunk_encrypted;
625+
struct msc_curl_memory_buffer_t downloaded_content;
615626
unsigned char *plain_text = NULL;
616627
int len = 0;
617628
int start = 0;
@@ -622,11 +633,11 @@ int msc_remote_add_rules_from_uri(cmd_parms *orig_parms,
622633

623634
apr_pool_t *mp = orig_parms->pool;
624635

625-
chunk_encrypted.size = 0;
626-
chunk_encrypted.memory = NULL;
636+
downloaded_content.size = 0;
637+
downloaded_content.memory = NULL;
627638

628639
res = msc_remote_download_content(mp, remote_rules_server->uri,
629-
remote_rules_server->key, &chunk_encrypted, error_msg);
640+
remote_rules_server->key, &downloaded_content, error_msg);
630641
if (*error_msg != NULL)
631642
{
632643
return -1;
@@ -642,26 +653,26 @@ int msc_remote_add_rules_from_uri(cmd_parms *orig_parms,
642653
if (remote_rules_server->crypto == 1)
643654
{
644655
#ifdef WITH_APU_CRYPTO
645-
msc_remote_decrypt(mp, remote_rules_server->key, &chunk_encrypted,
656+
msc_remote_decrypt(mp, remote_rules_server->key, &downloaded_content,
646657
&plain_text,
647658
&plain_text_len,
648659
error_msg);
660+
649661
if (*error_msg != NULL)
650662
{
651-
msc_remote_clean_chunk(&chunk_encrypted);
663+
msc_remote_clean_chunk(&downloaded_content);
652664
return -1;
653665
}
654666
#else
655667
*error_msg = "ModSecurity was not compiled with crypto support.\n";
656-
msc_remote_clean_chunk(&chunk_encrypted);
668+
msc_remote_clean_chunk(&downloaded_content);
657669
return -1;
658670
#endif
659-
660-
msc_remote_clean_chunk(&chunk_encrypted);
671+
msc_remote_clean_chunk(&downloaded_content);
661672
}
662673
else
663674
{
664-
plain_text = chunk_encrypted.memory;
675+
plain_text = downloaded_content.memory;
665676
plain_text_len = strlen(plain_text);
666677
}
667678

@@ -740,9 +751,9 @@ int msc_remote_add_rules_from_uri(cmd_parms *orig_parms,
740751

741752
remote_rules_server->amount_of_rules = added_rules;
742753

743-
if (remote_rules_server->crypto == 1)
754+
if (remote_rules_server->crypto != 1)
744755
{
745-
msc_remote_clean_chunk(&chunk_encrypted);
756+
msc_remote_clean_chunk(&downloaded_content);
746757
}
747758
#else
748759
*error_msg = "SecRemoteRules was not enabled during ModSecurity " \

‎apache2/re_operators.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -268,7 +268,7 @@ static int msre_op_ipmatchFromFile_execute(modsec_rec *msr, msre_rule *rule,
268268

269269
if (rtree == NULL)
270270
{
271-
if (msr->txcfg->debuglog_level >= 4)
271+
if (msr->txcfg->debuglog_level >= 6)
272272
{
273273
msr_log(msr, 1, "ipMatchFromFile: tree value is null.");
274274
}
@@ -1394,7 +1394,7 @@ static int msre_op_pm_execute(modsec_rec *msr, msre_rule *rule, msre_var *var, c
13941394

13951395
if (rule->op_param_data == NULL)
13961396
{
1397-
if (msr->txcfg->debuglog_level >= 4)
1397+
if (msr->txcfg->debuglog_level >= 6)
13981398
{
13991399
msr_log(msr, 1, "ACMPTree is null.");
14001400
}

‎configure.ac

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -743,6 +743,9 @@ AC_CONFIG_FILES([build/apxs-wrapper], [chmod +x build/apxs-wrapper])
743743
if test -e "$PERL"; then
744744
if test "$build_mlogc" -ne 0; then
745745
AC_CONFIG_FILES([mlogc/mlogc-batch-load.pl], [chmod +x mlogc/mlogc-batch-load.pl])
746+
AC_CONFIG_FILES([tests/regression/misc/40-secRemoteRules.t])
747+
AC_CONFIG_FILES([tests/regression/misc/50-ipmatchfromfile-external.t])
748+
AC_CONFIG_FILES([tests/regression/misc/60-pmfromfile-external.t])
746749
fi
747750
AC_CONFIG_FILES([tests/run-unit-tests.pl], [chmod +x tests/run-unit-tests.pl])
748751
AC_CONFIG_FILES([tests/run-regression-tests.pl], [chmod +x tests/run-regression-tests.pl])

‎tests/msc_test.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ unsigned long int DSOLOCAL msc_pcre_match_limit = 0;
7979
unsigned long int DSOLOCAL msc_pcre_match_limit_recursion = 0;
8080
char DSOLOCAL *real_server_signature = NULL;
8181
int DSOLOCAL remote_rules_fail_action = REMOTE_RULES_ABORT_ON_FAIL;
82+
char DSOLOCAL *remote_rules_fail_message = NULL;
8283
module AP_MODULE_DECLARE_DATA security2_module = {
8384
NULL,
8485
NULL,

‎tests/regression/misc/30-pmfromfile.t

Lines changed: 0 additions & 29 deletions
This file was deleted.

‎tests/regression/misc/40-secRemoteRules.t renamed to ‎tests/regression/misc/40-secRemoteRules.t.in

File renamed without changes.

‎tests/regression/misc/50-ipmatchfromfile-external.t.in

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
### ipMatchFromFile external resource
2+
3+
{
4+
type => "misc",
5+
comment => "ipMatchFromFile",
6+
conf => qq(
7+
SecRuleEngine On
8+
SecDebugLog $ENV{DEBUG_LOG}
9+
SecDebugLogLevel 9
10+
SecRequestBodyAccess On
11+
SecRule REMOTE_ADDR "\@ipMatchFromFile https://www.modsecurity.org/modsecurity-regression-test.txt" "id:10500,pass"
12+
),
13+
match_log => {
14+
error => [ qr/ModSecurity: Warning. IPmatchFromFile: \"127.0.0.1\" matched at REMOTE_ADDR./, 1],
15+
debug => [ qr/IPmatchFromFile: \"127.0.0.1\" matched at REMOTE_ADDR./, 1 ],
16+
-error => [ qr/ModSecurity: Problems loading external resources:/, 1],
17+
},
18+
match_response => {
19+
status => qr/^404$/,
20+
},
21+
request => new HTTP::Request(
22+
POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/127.0.0.1.html",
23+
[
24+
"Content-Type" => "application/x-www-form-urlencoded",
25+
],
26+
# Args
27+
"some_variable=-1' and 1=1 union/* foo */select load_file('/etc/passwd')--"
28+
),
29+
},
30+
{
31+
type => "misc",
32+
comment => "ipMatchFromFile - 404 download",
33+
conf => qq(
34+
SecRuleEngine On
35+
SecDebugLog $ENV{DEBUG_LOG}
36+
SecDebugLogLevel 9
37+
SecRequestBodyAccess On
38+
SecRemoteRulesFailAction Warn
39+
SecRule REMOTE_ADDR "\@ipMatchFromFile https://www.modsecurity.org/modsecurity-regression-test-404.txt" "id:10500,pass"
40+
),
41+
match_log => {
42+
error => [ qr/ModSecurity: Problems loading external resources: Failed to download: \"https:\/\/www.modsecurity.org\/modsecurity-regression-test-404.txt\" error: HTTP response code said error./, 1],
43+
},
44+
match_response => {
45+
status => qr/^404$/,
46+
},
47+
request => new HTTP::Request(
48+
POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/127.0.0.1.html",
49+
[
50+
"Content-Type" => "application/x-www-form-urlencoded",
51+
],
52+
# Args
53+
"some_variable=-1' and 1=1 union/* foo */select load_file('/etc/passwd')--"
54+
),
55+
},
56+
{
57+
type => "misc",
58+
comment => "ipMatchFromFile - bad certificate name",
59+
conf => qq(
60+
SecRuleEngine On
61+
SecDebugLog $ENV{DEBUG_LOG}
62+
SecDebugLogLevel 9
63+
SecRequestBodyAccess On
64+
SecRemoteRulesFailAction Warn
65+
SecRule REMOTE_ADDR "\@ipMatchFromFile https://status.modsecurity.org/modsecurity-regression-test-huge-ip-list.txt" "id:10500,pass"
66+
),
67+
match_log => {
68+
error => [ qr/ModSecurity: Problems loading external resources: Failed to download: \"https:\/\/status.modsecurity.org\/modsecurity-regression-test-huge-ip-list.txt\" error: SSL peer certificate or SSH remote key was not OK./, 1],
69+
},
70+
},
71+

0 commit comments

Comments
 (0)