From fe1021e36949a5132e8989eaf5f546e49fd1a00b Mon Sep 17 00:00:00 2001 From: brectanus Date: Fri, 28 Sep 2007 20:02:02 +0000 Subject: [PATCH] More cleanup of error messages and marking as relevant. See #4. --- CHANGES | 3 ++- apache2/apache2_io.c | 12 +++++++++--- apache2/apache2_util.c | 16 +++++++++++----- apache2/mod_security2.c | 17 +++++++++-------- apache2/msc_logging.c | 5 ++++- apache2/msc_reqbody.c | 22 +++++++++++++++++++++- apache2/pdf_protect.c | 2 ++ apache2/re_operators.c | 9 +++++---- 8 files changed, 63 insertions(+), 23 deletions(-) diff --git a/CHANGES b/CHANGES index 9c9b0d2f..83c5194c 100644 --- a/CHANGES +++ b/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. diff --git a/apache2/apache2_io.c b/apache2/apache2_io.c index 0e21d3f1..ea73f71f 100644 --- a/apache2/apache2_io.c +++ b/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 diff --git a/apache2/apache2_util.c b/apache2/apache2_util.c index f890683f..e9c75b73 100644 --- a/apache2/apache2_util.c +++ b/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; diff --git a/apache2/mod_security2.c b/apache2/mod_security2.c index 4f6eeea9..53faa31d 100644 --- a/apache2/mod_security2.c +++ b/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_NOTICE | APLOG_NOERRNO, 0, s, - * "ModSecurity: Failed to initialise engine."); - */ + ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL, + "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; diff --git a/apache2/msc_logging.c b/apache2/msc_logging.c index 2b4ac634..334761f5 100644 --- a/apache2/msc_logging.c +++ b/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); } } diff --git a/apache2/msc_reqbody.c b/apache2/msc_reqbody.c index 76603764..8f96505d 100644 --- a/apache2/msc_reqbody.c +++ b/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; diff --git a/apache2/pdf_protect.c b/apache2/pdf_protect.c index 4ba611dd..fe26837d 100644 --- a/apache2/pdf_protect.c +++ b/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. */ diff --git a/apache2/re_operators.c b/apache2/re_operators.c index 5515172b..663593fd 100644 --- a/apache2/re_operators.c +++ b/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;