From 2d034c5ce64ff163546cb1d497bd694c146bd36f Mon Sep 17 00:00:00 2001 From: brectanus Date: Thu, 10 Jan 2008 22:02:47 +0000 Subject: [PATCH] Finished with pre-2.5 source code review. --- review/pre-2.5-brian.review | 372 +++++++++++++++++++++++++++++++++--- 1 file changed, 344 insertions(+), 28 deletions(-) diff --git a/review/pre-2.5-brian.review b/review/pre-2.5-brian.review index dc9f5802..2ffb1ec9 100644 --- a/review/pre-2.5-brian.review +++ b/review/pre-2.5-brian.review @@ -1406,34 +1406,6 @@ static char *msre_action_phase_validate(msre_engine *engine, msre_action *action item.resolution.label.validNeedsfixing item.status.label.open - - - 2008-01-07 :: 21:28:41:095 GMT-08:00 - 2008-01-07 :: 21:40:15:511 GMT-08:00 - - brian - brian - apache2/msc_reqbody.c - item.type.label.programLogic - item.severity.label.major - Potential memory leak if modsecurity_request_body_store_disk() fails. Returning here causes modsecurity_request_body_end() to never be called and never free chunk data. See also notes in read_request_body() in apache_io.c. - /* Write the data we keep in memory */ - chunks = (msc_data_chunk **)msr->msc_reqbody_chunks->elts; - for(i = 0; i < msr->msc_reqbody_chunks->nelts; i++) { - disklen += chunks[i]->length; - - if (modsecurity_request_body_store_disk(msr, chunks[i]->data, chunks[i]->length, error_msg) < 0) { - return -1; - } - - free(chunks[i]->data); - chunks[i]->data = NULL; - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - 2008-01-07 :: 21:42:51:192 GMT-08:00 @@ -3116,5 +3088,349 @@ static char *multipart_construct_filename(modsec_rec *msr) { item.resolution.label.validNeedsfixing item.status.label.open + + + 2008-01-10 :: 12:24:59:317 GMT-08:00 + 2008-01-10 :: 12:25:40:036 GMT-08:00 + + brian + brian + apache2/msc_multipart.c + item.type.label.suggestion + item.severity.label.trivial + len is always assumed to be at least 1. What is preventing a len of 0? + if (len > 1) { + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-10 :: 12:27:34:472 GMT-08:00 + 2008-01-10 :: 13:16:19:644 GMT-08:00 + + brian + brian + apache2/msc_multipart.c + item.type.label.suggestion + item.severity.label.trivial + Use MULTIPART_BUF_SIZE for the constant. + if (strlen(new_value) > 4096) { + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-10 :: 13:04:27:048 GMT-08:00 + 2008-01-10 :: 13:04:57:229 GMT-08:00 + + brian + brian + apache2/msc_multipart.c + item.type.label.suggestion + item.severity.label.trivial + Cannot find anything that supports that this is or is not allowed. + /* Flag for whitespace after parameter name. */ + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-10 :: 13:05:06:226 GMT-08:00 + 2008-01-10 :: 13:05:11:552 GMT-08:00 + + brian + brian + apache2/msc_multipart.c + item.type.label.suggestion + item.severity.label.trivial + Cannot find anything that supports that this is or is not allowed. + /* Flag for whitespace before parameter value. */ + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-10 :: 13:12:20:299 GMT-08:00 + 2008-01-10 :: 13:12:45:495 GMT-08:00 + + brian + brian + apache2/msc_multipart.c + item.type.label.suggestion + item.severity.label.trivial + Use '\r' vs 0x0d as is done elsewhere. + if ((c == 0x0d)&&(msr->mpd->bufleft == 1)) { + /* we don't want to take 0x0d as the last byte in the buffer */ + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-10 :: 13:12:54:295 GMT-08:00 + 2008-01-10 :: 13:13:12:297 GMT-08:00 + + brian + brian + apache2/msc_multipart.c + item.type.label.suggestion + item.severity.label.trivial + Use '\n' vs 0x0a as is done elsewhere. + if ((c == 0x0a)||(msr->mpd->bufleft == 0)||(process_buffer)) { + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-10 :: 13:19:16:539 GMT-08:00 + 2008-01-10 :: 13:20:00:685 GMT-08:00 + + brian + brian + apache2/msc_parsers.c + item.type.label.suggestion + item.severity.label.trivial + Headers are not used. I think they were added for iconv support, but Ivan removed it. + #include "iconv.h" +#include <errno.h> + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-10 :: 13:26:56:609 GMT-08:00 + 2008-01-10 :: 13:27:21:817 GMT-08:00 + + brian + brian + apache2/msc_util.c + item.type.label.suggestion + item.severity.label.trivial + Be more consistent in naming. + #define VALID_HEX(X) (((X >= '0')&&(X <= '9')) || ((X >= 'a')&&(X <= 'f')) || ((X >= 'A')&&(X <= 'F'))) +#define ISODIGIT(X) ((X >= '0')&&(X <= '7')) + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-10 :: 13:32:29:278 GMT-08:00 + 2008-01-10 :: 13:34:07:812 GMT-08:00 + + brian + brian + apache2/msc_util.c + item.type.label.optimization + item.severity.label.trivial + No need to do a strlen twice *and* loop through the string. Just loop and exit on non-space. + if (strlen(string) == 0) return 1; + + for(i = 0; i < strlen(string); i++) { + if (!isspace(string[i])) { + return 0; + } + } + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-10 :: 13:35:40:307 GMT-08:00 + 2008-01-10 :: 13:36:03:933 GMT-08:00 + + brian + brian + apache2/modsecurity.c + item.type.label.irrelevant + item.severity.label.trivial + Remove commented code? + /* Serial audit log mutext */ + rc = apr_global_mutex_create(&msce->auditlog_lock, NULL, APR_LOCK_DEFAULT, mp); + if (rc != APR_SUCCESS) { + //ap_log_error(APLOG_MARK, APLOG_ERR, rv, s, "mod_security: Could not create modsec_auditlog_lock"); + //return HTTP_INTERNAL_SERVER_ERROR; + return -1; + } + + #ifdef __SET_MUTEX_PERMS + rc = unixd_set_global_mutex_perms(msce->auditlog_lock); + if (rc != APR_SUCCESS) { + // ap_log_error(APLOG_MARK, APLOG_ERR, rc, s, "mod_security: Could not set permissions on modsec_auditlog_lock; check User and Group directives"); + // return HTTP_INTERNAL_SERVER_ERROR; + return -1; + } + #endif + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-10 :: 13:36:28:229 GMT-08:00 + 2008-01-10 :: 13:36:52:702 GMT-08:00 + + brian + brian + apache2/modsecurity.c + item.type.label.suggestion + item.severity.label.trivial + What *should* we do on error here? + if (rc != APR_SUCCESS) { + // ap_log_error(APLOG_MARK, APLOG_ERR, rs, s, "Failed to child-init auditlog mutex"); + } + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-10 :: 13:37:17:326 GMT-08:00 + 2008-01-10 :: 13:37:25:682 GMT-08:00 + + brian + brian + apache2/modsecurity.c + item.type.label.suggestion + item.severity.label.trivial + This does nothing. + /** + * Releases resources held by engine instance. + */ +void modsecurity_shutdown(msc_engine *msce) { + if (msce == NULL) return; +} + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-10 :: 13:39:01:265 GMT-08:00 + 2008-01-10 :: 13:39:22:070 GMT-08:00 + + brian + brian + apache2/modsecurity.c + item.type.label.suggestion + item.severity.label.trivial + Figure out the optimal initial size for the arrays/tables. + /* Collections. */ + msr->tx_vars = apr_table_make(msr->mp, 32); + if (msr->tx_vars == NULL) return -1; + + msr->geo_vars = apr_table_make(msr->mp, 8); + if (msr->geo_vars == NULL) return -1; + + msr->collections = apr_table_make(msr->mp, 8); + if (msr->collections == NULL) return -1; + msr->collections_dirty = apr_table_make(msr->mp, 8); + if (msr->collections_dirty == NULL) return -1; + + /* Other */ + msr->tcache = apr_hash_make(msr->mp); + if (msr->tcache == NULL) return -1; + + msr->matched_rules = apr_array_make(msr->mp, 16, sizeof(void *)); + if (msr->matched_rules == NULL) return -1; + + msr->matched_var = (msc_string *)apr_pcalloc(msr->mp, sizeof(msc_string)); + if (msr->matched_var == NULL) return -1; + + msr->highest_severity = 255; /* high, invalid value */ + + msr->removed_rules = apr_array_make(msr->mp, 16, sizeof(char *)); + if (msr->removed_rules == NULL) return -1; + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-10 :: 13:42:33:518 GMT-08:00 + 2008-01-10 :: 13:42:50:351 GMT-08:00 + + brian + brian + apache2/msc_xml.c + item.type.label.suggestion + item.severity.label.trivial + Remove #if 0'd code? + #if 0 +static void xml_receive_sax_error(void *data, const char *msg, ...) { + modsec_rec *msr = (modsec_rec *)data; + char message[256]; + + if (msr == NULL) return; + + apr_snprintf(message, sizeof(message), "%s (line %d offset %d)", + log_escape_nq(msr->mp, msr->xml->parsing_ctx->lastError.message), + msr->xml->parsing_ctx->lastError.line, + msr->xml->parsing_ctx->lastError.int2); + + msr_log(msr, 5, "XML: Parsing error: %s", message); +} +#endif + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-10 :: 13:54:16:193 GMT-08:00 + 2008-01-10 :: 13:54:45:610 GMT-08:00 + + brian + brian + apache2/mod_security2.c + item.type.label.suggestion + item.severity.label.trivial + What is the history of this? + /* Our own hook to handle RPC transactions (not used at the moment). + * // ap_hook_handler(hook_handler, NULL, NULL, APR_HOOK_MIDDLE); + */ + + + item.resolution.label.validNeedsfixing + item.status.label.open + + + + 2008-01-10 :: 13:56:58:413 GMT-08:00 + 2008-01-10 :: 13:57:23:209 GMT-08:00 + + brian + brian + apache2/msc_lua.c + item.type.label.suggestion + item.severity.label.trivial + Change C++ to C style comment. + // Get the response from the script. + + + item.resolution.label.validNeedsfixing + item.status.label.open +