More cleanup of error messages and marking as relevant. See #4.

CHANGES

@@ -3,7 +3,8 @@
 ------------------------
 
  * Situations where ModSecurity will intercept, generate an error or log
-   an alert to the debug log are now marked as 'relevant'.
+   a level 1-3 message to the debug log are now marked as 'relevant' and may
+   generate an audit log entry.
 
  * Do not process subrequests in phase 2-4.
 

apache2/apache2_io.c

@@ -275,7 +275,10 @@ static int output_filter_should_run(modsec_rec *msr, request_rec *r) {
         char *p = NULL;
 
         content_type = apr_pstrdup(msr->mp, r->content_type);
-        if (content_type == NULL) return -1;
+        if (content_type == NULL) {
+            msr_log(msr, 1, "Output filter: Failed to allocate memory for content type.");
+            return -1;
+        }
 
         /* Hide the character encoding information
          * if present. Sometimes the content type header
@@ -316,11 +319,14 @@ static apr_status_t output_filter_init(modsec_rec *msr, ap_filter_t *f,
     apr_status_t rc;
 
     msr->of_brigade = apr_brigade_create(msr->mp, f->c->bucket_alloc);
-    if (msr->of_brigade == NULL) return -1;
+    if (msr->of_brigade == NULL) {
+        msr_log(msr, 1, "Output filter: Failed to create brigade.");
+        return -1;
+    }
     msr->of_status = OF_STATUS_IN_PROGRESS;
     
     rc = output_filter_should_run(msr, r);
-    if (rc < 0) return -1;
+    if (rc < 0) return -1; /* output_filter_should_run() generates error msg */
     if (rc == 0) return 0;
 
     /* Do not check the output limit if we are willing to

apache2/apache2_util.c

@@ -20,14 +20,20 @@ apr_status_t send_error_bucket(modsec_rec *msr, ap_filter_t *f, int status) {
     apr_bucket_brigade *brigade = NULL;
     apr_bucket *bucket = NULL;
 
-    /* Force relevancy for all errors */
-    if (msr != NULL) {
-        msr->is_relevant++;
-    }
-
     /* Set the status line explicitly for the error document */
     f->r->status_line = ap_get_status_line(status);
 
+    /* Force alert log for any errors that are not already marked relevant
+     * to prevent any missing error messages in the code from going
+     * unnoticed.  To prevent this error, all code should either set
+     * is_relevant, or just use msr_log with a level <= 3 prior to
+     * calling this function.
+     */
+    if ((msr != NULL) && (msr->is_relevant == 0)) {
+        msr_log(msr, 1, "Internal error: Issuing \"%s\" for unspecified error.",
+            f->r->status_line);
+    }
+
     brigade = apr_brigade_create(f->r->pool, f->r->connection->bucket_alloc);
     if (brigade == NULL) return APR_EGENERAL;
 

apache2/mod_security2.c

@@ -338,7 +338,10 @@ static modsec_rec *create_tx_context(request_rec *r) {
     msr->hostname = ap_get_server_name(r);
 
     /* Invoke the engine to continue with initialisation */
-    if (modsecurity_tx_init(msr) < 0) return NULL;
+    if (modsecurity_tx_init(msr) < 0) {
+        msr_log(msr, 1, "Failed to initialising transaction (txid %s).", msr->txid);
+        return NULL;
+    }
 
     store_tx_context(msr, r);
 
@@ -400,10 +403,8 @@ static int hook_pre_config(apr_pool_t *mp, apr_pool_t *mp_log, apr_pool_t *mp_te
     /* Initialise ModSecurity engine */
     modsecurity = modsecurity_create(mp, MODSEC_ONLINE);
     if (modsecurity == NULL) {
-        /* ENH Since s not available, how do we log from here? stderr?
+        ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL,
-         * ap_log_error(APLOG_MARK, APLOG_NOTICE | APLOG_NOERRNO, 0, s,
+            "ModSecurity: Failed to initialise engine.");
-         *    "ModSecurity: Failed to initialise engine.");
-         */
         return HTTP_INTERNAL_SERVER_ERROR;
     }    
 
@@ -639,20 +640,20 @@ static int hook_request_late(request_rec *r) {
     if (rc < 0) {
         switch(rc) {
             case -1 :
-                if ((my_error_msg != NULL)||(msr->is_relevant == 0)) {
+                if (my_error_msg != NULL) {
                     msr_log(msr, 1, "%s", my_error_msg);
                 }
                 return HTTP_INTERNAL_SERVER_ERROR;
                 break;
             case -4 : /* Timeout. */
-                if ((my_error_msg != NULL)||(msr->is_relevant == 0)) {
+                if (my_error_msg != NULL) {
                     msr_log(msr, 4, "%s", my_error_msg);
                 }
                 r->connection->keepalive = AP_CONN_CLOSE;
                 return HTTP_REQUEST_TIME_OUT;
                 break;
             case -5 : /* Request body limit reached. */
-                if ((my_error_msg != NULL)||(msr->is_relevant == 0)) {
+                if (my_error_msg != NULL) {
                     msr_log(msr, 1, "%s", my_error_msg);
                 }
                 r->connection->keepalive = AP_CONN_CLOSE;

apache2/msc_logging.c

@@ -596,11 +596,14 @@ void sec_audit_logger(modsec_rec *msr) {
                     }
 
                     if (rc <= 0) {
-                        msr_log(msr, 1, "Audit log: %s", my_error_msg);
                         break;
                     }
                 }
 
+                if (rc < 0) {
+                    msr_log(msr, 1, "Audit log: %s", my_error_msg);
+                }
+
                 modsecurity_request_body_retrieve_end(msr);
             }
         }

apache2/msc_reqbody.c

@@ -17,6 +17,8 @@
  * Prepare to accept the request body (part 2).
  */
 static apr_status_t modsecurity_request_body_start_init(modsec_rec *msr, char **error_msg) {
+    *error_msg = NULL;
+
     if(msr->msc_reqbody_storage == MSC_REQBODY_MEMORY) {
         /* Prepare to store request body in memory. */
 
@@ -55,6 +57,7 @@ static apr_status_t modsecurity_request_body_start_init(modsec_rec *msr, char **
  * Prepare to accept the request body (part 1).
  */
 apr_status_t modsecurity_request_body_start(modsec_rec *msr, char **error_msg) {
+    *error_msg = NULL;
     msr->msc_reqbody_length = 0;
 
     /* Create a separate memory pool that will be used
@@ -104,7 +107,11 @@ apr_status_t modsecurity_request_body_start(modsec_rec *msr, char **error_msg) {
 static apr_status_t modsecurity_request_body_store_disk(modsec_rec *msr,
     const char *data, apr_size_t length, char **error_msg)
 {
-    apr_size_t i = write(msr->msc_reqbody_fd, data, length);
+    apr_size_t i;
+
+    *error_msg = NULL;
+
+    i = write(msr->msc_reqbody_fd, data, length);
     if (i != length) {
         *error_msg = apr_psprintf(msr->mp, "Input filter: Failed writing %" APR_SIZE_T_FMT " bytes to temporary file (rc %" APR_SIZE_T_FMT ").", length, i);
         return -1;
@@ -119,6 +126,8 @@ static apr_status_t modsecurity_request_body_store_disk(modsec_rec *msr,
 static apr_status_t modsecurity_request_body_store_memory(modsec_rec *msr,
     const char *data, apr_size_t length, char **error_msg)
 {
+    *error_msg = NULL;
+
     /* Would storing this chunk mean going over the limit? */
     if ((msr->msc_reqbody_spilltodisk)
         && (msr->msc_reqbody_length + length > (apr_size_t)msr->txcfg->reqbody_inmemory_limit))
@@ -242,6 +251,8 @@ static apr_status_t modsecurity_request_body_store_memory(modsec_rec *msr,
 apr_status_t modsecurity_request_body_store(modsec_rec *msr,
     const char *data, apr_size_t length, char **error_msg)
 {
+    *error_msg = NULL;
+
     /* If we have a processor for this request body send
      * data to it first (but only if it did not report an
      * error on previous invocations).
@@ -300,6 +311,8 @@ static apr_status_t modsecurity_request_body_end_urlencoded(modsec_rec *msr, cha
     int i, sofar;
     int invalid_count = 0;
 
+    *error_msg = NULL;
+
     /* Allocate a buffer large enough to hold the request body. */
 
     if (msr->msc_reqbody_length + 1 == 0) {
@@ -367,6 +380,7 @@ static apr_status_t modsecurity_request_body_end_urlencoded(modsec_rec *msr, cha
  * Stops receiving the request body.
  */
 apr_status_t modsecurity_request_body_end(modsec_rec *msr, char **error_msg) {
+    *error_msg = NULL;
 
     /* Close open file descriptors, if any. */
     if (msr->msc_reqbody_storage == MSC_REQBODY_DISK) {
@@ -420,6 +434,8 @@ apr_status_t modsecurity_request_body_end(modsec_rec *msr, char **error_msg) {
  * Prepares to forward the request body.
  */
 apr_status_t modsecurity_request_body_retrieve_start(modsec_rec *msr, char **error_msg) {
+    *error_msg = NULL;
+
     if (msr->msc_reqbody_storage == MSC_REQBODY_MEMORY) {
         msr->msc_reqbody_chunk_position = 0;
         msr->msc_reqbody_chunk_offset = 0;
@@ -485,6 +501,8 @@ apr_status_t modsecurity_request_body_retrieve(modsec_rec *msr,
 {
     msc_data_chunk **chunks;
 
+    *error_msg = NULL;
+
     if (chunk == NULL) {
         *error_msg = apr_pstrdup(msr->mp, "Internal error, retrieving request body chunk.");
         return -1;
@@ -581,6 +599,8 @@ apr_status_t modsecurity_request_body_retrieve(modsec_rec *msr,
  *
  */
 apr_status_t modsecurity_request_body_clear(modsec_rec *msr, char **error_msg) {
+    *error_msg = NULL;
+
     /* Release memory we used to store request body data. */    
     if (msr->msc_reqbody_chunks != NULL) {
         msc_data_chunk **chunks = (msc_data_chunk **)msr->msc_reqbody_chunks->elts;

apache2/pdf_protect.c

@@ -312,6 +312,8 @@ apr_status_t pdfp_output_filter(ap_filter_t *f, apr_bucket_brigade *bb_in) {
 
                     apr_table_set(r->headers_out, "Location", new_uri);
 
+                    ap_remove_output_filter(f);
+
                     return send_error_bucket(msr, f, REDIRECT_STATUS);
                 }
             } else { /* Token found. */

apache2/re_operators.c

@@ -161,15 +161,16 @@ static int msre_op_rx_execute(modsec_rec *msr, msre_rule *rule, msre_var *var, c
     {
     */
     if (rc != PCRE_ERROR_NOMATCH) { /* Match. */
-        char *pattern_escaped = log_escape(msr->mp, regex->pattern);
+        /* We no longer escape the pattern here as it is done when logging */
+        char *pattern = apr_pstrdup(msr->mp, regex->pattern);
 
         /* This message will be logged. */
-        if (strlen(pattern_escaped) > 252) {
+        if (strlen(pattern) > 252) {
             *error_msg = apr_psprintf(msr->mp, "Pattern match \"%.252s ...\" at %s.",
-                pattern_escaped, var->name);
+                pattern, var->name);
         } else {
             *error_msg = apr_psprintf(msr->mp, "Pattern match \"%s\" at %s.",
-                pattern_escaped, var->name);
+                pattern, var->name);
         }
 
         return 1;