From ce4cf24f6ea67fe51f95b9a05792c4f6abb3c17e Mon Sep 17 00:00:00 2001 From: Felipe Zimmerle Date: Thu, 27 Nov 2014 13:11:04 -0800 Subject: [PATCH] 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. --- CHANGES | 5 ++ apache2/mod_security2.c | 8 ++ apache2/modsecurity.h | 1 + apache2/msc_remote_rules.c | 47 +++++++---- apache2/re_operators.c | 4 +- configure.ac | 3 + tests/msc_test.c | 1 + tests/regression/misc/30-pmfromfile.t | 29 ------- ...ecRemoteRules.t => 40-secRemoteRules.t.in} | 0 .../misc/50-ipmatchfromfile-external.t.in | 71 ++++++++++++++++ .../misc/60-pmfromfile-external.t.in | 84 +++++++++++++++++++ 11 files changed, 204 insertions(+), 49 deletions(-) delete mode 100644 tests/regression/misc/30-pmfromfile.t rename tests/regression/misc/{40-secRemoteRules.t => 40-secRemoteRules.t.in} (100%) create mode 100644 tests/regression/misc/50-ipmatchfromfile-external.t.in create mode 100644 tests/regression/misc/60-pmfromfile-external.t.in diff --git a/CHANGES b/CHANGES index 0fc9f546..39b655b3 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,11 @@ DD mmm YYYY - 2.9.????? (To be released) ----------------------- + * Adds missing 'ModSecurity:' prefix in some warnings messages. + [Walter Hop and ModSecurity team] + * Refactoring external resources download warn messages. Holding the message + to be displayed when Apache is ready to write on the error_log. + [ModSecurity team] * Remote resources loading process is now failing in case of HTTP error. [Walter Hop and ModSecurity team] * Fixed start up crash on Apache with mod_ssl configured. Crash was happening diff --git a/apache2/mod_security2.c b/apache2/mod_security2.c index 693ac500..047fd9f1 100644 --- a/apache2/mod_security2.c +++ b/apache2/mod_security2.c @@ -72,6 +72,7 @@ unsigned long int DSOLOCAL msc_pcre_match_limit_recursion = 0; msc_remote_rules_server DSOLOCAL *remote_rules_server = NULL; #endif int DSOLOCAL remote_rules_fail_action = REMOTE_RULES_ABORT_ON_FAIL; +char DSOLOCAL *remote_rules_fail_message = NULL; int DSOLOCAL status_engine_state = STATUS_ENGINE_DISABLED; @@ -761,6 +762,13 @@ static int hook_post_config(apr_pool_t *mp, apr_pool_t *mp_log, apr_pool_t *mp_t } #endif + if (remote_rules_fail_message != NULL) + { + ap_log_error(APLOG_MARK, APLOG_NOTICE, 0, NULL, "ModSecurity: " \ + "Problems loading external resources: %s", + remote_rules_fail_message); + } + #ifdef WITH_REMOTE_RULES if (remote_rules_server != NULL) { diff --git a/apache2/modsecurity.h b/apache2/modsecurity.h index 8ab9b220..fc5be7de 100644 --- a/apache2/modsecurity.h +++ b/apache2/modsecurity.h @@ -150,6 +150,7 @@ extern DSOLOCAL unsigned long int msc_pcre_match_limit_recursion; extern DSOLOCAL msc_remote_rules_server *remote_rules_server; #endif extern DSOLOCAL int remote_rules_fail_action; +extern DSOLOCAL char *remote_rules_fail_message; extern DSOLOCAL int status_engine_state; diff --git a/apache2/msc_remote_rules.c b/apache2/msc_remote_rules.c index 197ad602..798c9f72 100644 --- a/apache2/msc_remote_rules.c +++ b/apache2/msc_remote_rules.c @@ -328,16 +328,22 @@ int msc_remote_download_content(apr_pool_t *mp, const char *uri, const char *key { if (remote_rules_fail_action == REMOTE_RULES_WARN_ON_FAIL) { - ap_log_error(APLOG_MARK, APLOG_NOTICE, 0, NULL, - "Failed to download \"%s\" error: %s ", - uri, curl_easy_strerror(res)); + if (remote_rules_fail_message == NULL) + { + remote_rules_fail_message = ""; + } - ret = -2; - goto failed; + remote_rules_fail_message = apr_psprintf(mp, "%sFailed to " \ + "download: \"%s\" error: %s. ", + remote_rules_fail_message, uri, + curl_easy_strerror(res)); + + ret = -2; + goto failed; } else { - *error_msg = apr_psprintf(mp, "Failed to download \"%s\" " \ + *error_msg = apr_psprintf(mp, "Failed to download: \"%s\" " \ "error: %s ", uri, curl_easy_strerror(res)); @@ -581,6 +587,11 @@ int msc_remote_decrypt(apr_pool_t *pool, } apr_crypto_block_cleanup(block); + apr_crypto_cleanup(f); + + // Shutdown the apr_crypto seems to be the correct thing to do. + // However it seems to add instability especially if mod_ssl is enabled. + // apr_crypto_shutdown(driver); return 0; } @@ -611,7 +622,7 @@ int msc_remote_add_rules_from_uri(cmd_parms *orig_parms, { #ifdef WITH_REMOTE_RULES - struct msc_curl_memory_buffer_t chunk_encrypted; + struct msc_curl_memory_buffer_t downloaded_content; unsigned char *plain_text = NULL; int len = 0; int start = 0; @@ -622,11 +633,11 @@ int msc_remote_add_rules_from_uri(cmd_parms *orig_parms, apr_pool_t *mp = orig_parms->pool; - chunk_encrypted.size = 0; - chunk_encrypted.memory = NULL; + downloaded_content.size = 0; + downloaded_content.memory = NULL; res = msc_remote_download_content(mp, remote_rules_server->uri, - remote_rules_server->key, &chunk_encrypted, error_msg); + remote_rules_server->key, &downloaded_content, error_msg); if (*error_msg != NULL) { return -1; @@ -642,26 +653,26 @@ int msc_remote_add_rules_from_uri(cmd_parms *orig_parms, if (remote_rules_server->crypto == 1) { #ifdef WITH_APU_CRYPTO - msc_remote_decrypt(mp, remote_rules_server->key, &chunk_encrypted, + msc_remote_decrypt(mp, remote_rules_server->key, &downloaded_content, &plain_text, &plain_text_len, error_msg); + if (*error_msg != NULL) { - msc_remote_clean_chunk(&chunk_encrypted); + msc_remote_clean_chunk(&downloaded_content); return -1; } #else *error_msg = "ModSecurity was not compiled with crypto support.\n"; - msc_remote_clean_chunk(&chunk_encrypted); + msc_remote_clean_chunk(&downloaded_content); return -1; #endif - - msc_remote_clean_chunk(&chunk_encrypted); + msc_remote_clean_chunk(&downloaded_content); } else { - plain_text = chunk_encrypted.memory; + plain_text = downloaded_content.memory; plain_text_len = strlen(plain_text); } @@ -740,9 +751,9 @@ next: remote_rules_server->amount_of_rules = added_rules; - if (remote_rules_server->crypto == 1) + if (remote_rules_server->crypto != 1) { - msc_remote_clean_chunk(&chunk_encrypted); + msc_remote_clean_chunk(&downloaded_content); } #else *error_msg = "SecRemoteRules was not enabled during ModSecurity " \ diff --git a/apache2/re_operators.c b/apache2/re_operators.c index d318b20b..33ce83cf 100644 --- a/apache2/re_operators.c +++ b/apache2/re_operators.c @@ -268,7 +268,7 @@ static int msre_op_ipmatchFromFile_execute(modsec_rec *msr, msre_rule *rule, if (rtree == NULL) { - if (msr->txcfg->debuglog_level >= 4) + if (msr->txcfg->debuglog_level >= 6) { msr_log(msr, 1, "ipMatchFromFile: tree value is null."); } @@ -1394,7 +1394,7 @@ static int msre_op_pm_execute(modsec_rec *msr, msre_rule *rule, msre_var *var, c if (rule->op_param_data == NULL) { - if (msr->txcfg->debuglog_level >= 4) + if (msr->txcfg->debuglog_level >= 6) { msr_log(msr, 1, "ACMPTree is null."); } diff --git a/configure.ac b/configure.ac index 8ff19ee2..b2bf4b31 100644 --- a/configure.ac +++ b/configure.ac @@ -743,6 +743,9 @@ AC_CONFIG_FILES([build/apxs-wrapper], [chmod +x build/apxs-wrapper]) if test -e "$PERL"; then if test "$build_mlogc" -ne 0; then AC_CONFIG_FILES([mlogc/mlogc-batch-load.pl], [chmod +x mlogc/mlogc-batch-load.pl]) + AC_CONFIG_FILES([tests/regression/misc/40-secRemoteRules.t]) + AC_CONFIG_FILES([tests/regression/misc/50-ipmatchfromfile-external.t]) + AC_CONFIG_FILES([tests/regression/misc/60-pmfromfile-external.t]) fi AC_CONFIG_FILES([tests/run-unit-tests.pl], [chmod +x tests/run-unit-tests.pl]) AC_CONFIG_FILES([tests/run-regression-tests.pl], [chmod +x tests/run-regression-tests.pl]) diff --git a/tests/msc_test.c b/tests/msc_test.c index 18139039..7c794a5d 100644 --- a/tests/msc_test.c +++ b/tests/msc_test.c @@ -79,6 +79,7 @@ unsigned long int DSOLOCAL msc_pcre_match_limit = 0; unsigned long int DSOLOCAL msc_pcre_match_limit_recursion = 0; char DSOLOCAL *real_server_signature = NULL; int DSOLOCAL remote_rules_fail_action = REMOTE_RULES_ABORT_ON_FAIL; +char DSOLOCAL *remote_rules_fail_message = NULL; module AP_MODULE_DECLARE_DATA security2_module = { NULL, NULL, diff --git a/tests/regression/misc/30-pmfromfile.t b/tests/regression/misc/30-pmfromfile.t deleted file mode 100644 index db93581e..00000000 --- a/tests/regression/misc/30-pmfromfile.t +++ /dev/null @@ -1,29 +0,0 @@ -### pmfromfile external resource - -{ - type => "misc", - comment => "pmfromfile", - conf => qq( - SecRuleEngine On - SecDebugLog $ENV{DEBUG_LOG} - SecDebugLogLevel 9 - SecRequestBodyAccess On - SecRule REQUEST_FILENAME "\@pmFromFile https://www.modsecurity.org/modsecurity-regression-test.txt" "id:'123',phase:2,log,pass,t:none" - ), - match_log => { - error => [ qr/ModSecurity: Warning. Matched phrase \"127.0.0.1\" at REQUEST_FILENAME./, 1], - debug => [ qr/Matched phrase \"127.0.0.1\" at REQUEST_FILENAME/, 1 ], - }, - match_response => { - status => qr/^404$/, - }, - request => new HTTP::Request( - POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/127.0.0.1.html", - [ - "Content-Type" => "application/x-www-form-urlencoded", - ], - # Args - "some_variable=-1' and 1=1 union/* foo */select load_file('/etc/passwd')--" - ), -}, - diff --git a/tests/regression/misc/40-secRemoteRules.t b/tests/regression/misc/40-secRemoteRules.t.in similarity index 100% rename from tests/regression/misc/40-secRemoteRules.t rename to tests/regression/misc/40-secRemoteRules.t.in diff --git a/tests/regression/misc/50-ipmatchfromfile-external.t.in b/tests/regression/misc/50-ipmatchfromfile-external.t.in new file mode 100644 index 00000000..effe97bf --- /dev/null +++ b/tests/regression/misc/50-ipmatchfromfile-external.t.in @@ -0,0 +1,71 @@ +### ipMatchFromFile external resource + +{ + type => "misc", + comment => "ipMatchFromFile", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRequestBodyAccess On + SecRule REMOTE_ADDR "\@ipMatchFromFile https://www.modsecurity.org/modsecurity-regression-test.txt" "id:10500,pass" + ), + match_log => { + error => [ qr/ModSecurity: Warning. IPmatchFromFile: \"127.0.0.1\" matched at REMOTE_ADDR./, 1], + debug => [ qr/IPmatchFromFile: \"127.0.0.1\" matched at REMOTE_ADDR./, 1 ], + -error => [ qr/ModSecurity: Problems loading external resources:/, 1], + }, + match_response => { + status => qr/^404$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/127.0.0.1.html", + [ + "Content-Type" => "application/x-www-form-urlencoded", + ], + # Args + "some_variable=-1' and 1=1 union/* foo */select load_file('/etc/passwd')--" + ), +}, +{ + type => "misc", + comment => "ipMatchFromFile - 404 download", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRequestBodyAccess On + SecRemoteRulesFailAction Warn + SecRule REMOTE_ADDR "\@ipMatchFromFile https://www.modsecurity.org/modsecurity-regression-test-404.txt" "id:10500,pass" + ), + match_log => { + 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], + }, + match_response => { + status => qr/^404$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/127.0.0.1.html", + [ + "Content-Type" => "application/x-www-form-urlencoded", + ], + # Args + "some_variable=-1' and 1=1 union/* foo */select load_file('/etc/passwd')--" + ), +}, +{ + type => "misc", + comment => "ipMatchFromFile - bad certificate name", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRequestBodyAccess On + SecRemoteRulesFailAction Warn + SecRule REMOTE_ADDR "\@ipMatchFromFile https://status.modsecurity.org/modsecurity-regression-test-huge-ip-list.txt" "id:10500,pass" + ), + match_log => { + 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], + }, +}, + diff --git a/tests/regression/misc/60-pmfromfile-external.t.in b/tests/regression/misc/60-pmfromfile-external.t.in new file mode 100644 index 00000000..bfa4038a --- /dev/null +++ b/tests/regression/misc/60-pmfromfile-external.t.in @@ -0,0 +1,84 @@ +### pmfromfile external resource + +{ + type => "misc", + comment => "pmfromfile", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRequestBodyAccess On + SecRule REQUEST_FILENAME "\@pmFromFile https://www.modsecurity.org/modsecurity-regression-test.txt" "id:'123',phase:2,log,pass,t:none" + ), + match_log => { + error => [ qr/ModSecurity: Warning. Matched phrase \"127.0.0.1\" at REQUEST_FILENAME./, 1], + debug => [ qr/Matched phrase \"127.0.0.1\" at REQUEST_FILENAME/, 1 ], + -error => [ qr/ModSecurity: Problems loading external resources:/, 1], + }, + match_response => { + status => qr/^404$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/127.0.0.1.html", + [ + "Content-Type" => "application/x-www-form-urlencoded", + ], + # Args + "some_variable=-1' and 1=1 union/* foo */select load_file('/etc/passwd')--" + ), +}, +{ + type => "misc", + comment => "pmfromfile - 404 download", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRequestBodyAccess On + SecRemoteRulesFailAction Warn + SecRule REQUEST_FILENAME "\@pmFromFile https://www.modsecurity.org/modsecurity-regression-test-404.txt" "id:'123',phase:2,log,pass,t:none" + + ), + match_log => { + 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], + }, + match_response => { + status => qr/^404$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/127.0.0.1.html", + [ + "Content-Type" => "application/x-www-form-urlencoded", + ], + # Args + "some_variable=-1' and 1=1 union/* foo */select load_file('/etc/passwd')--" + ), +}, +{ + type => "misc", + comment => "pmfromfile - bad certificate name", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRequestBodyAccess On + SecRemoteRulesFailAction Warn + SecRule REQUEST_FILENAME "\@pmFromFile https://status.modsecurity.org/modsecurity-regression-test.txt" "id:'123',phase:2,log,pass,t:none" + + ), + match_log => { + error => [ qr/ModSecurity: Problems loading external resources: Failed to download: \"https:\/\/status.modsecurity.org\/modsecurity-regression-test.txt\" error: SSL peer certificate or SSH remote key was not OK./, 1], + }, + match_response => { + status => qr/^404$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/127.0.0.1.html", + [ + "Content-Type" => "application/x-www-form-urlencoded", + ], + # Args + "some_variable=-1' and 1=1 union/* foo */select load_file('/etc/passwd')--" + ), +}, +