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

@@ -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

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)
         {

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;
 

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,
+                if (remote_rules_fail_message == NULL)
-                         "Failed to download \"%s\" error: %s ",
+                {
-                         uri, curl_easy_strerror(res));
+                    remote_rules_fail_message = "";
+                }
+
+                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;
+    downloaded_content.size = 0;
-    chunk_encrypted.memory = NULL;
+    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(&downloaded_content);
-        msc_remote_clean_chunk(&chunk_encrypted);
     }
     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 " \

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.");
         }

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])

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,

tests/regression/misc/30-pmfromfile.t

@@ -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')--"
-	),
-},
-

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],
+	},
+},
+

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')--"
+	),
+},
+