diff --git a/.cdtproject b/.cdtproject deleted file mode 100644 index 93d4fd4b..00000000 --- a/.cdtproject +++ /dev/null @@ -1,1192 +0,0 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - make - - all - false - true - - - make - - test - false - true - - - gksudo - make - install - false - false - - - make - - clean - false - true - - - make - - dist-clean - false - true - - - - - - - - - - - - - - - - - - - - - - - - - \ No newline at end of file diff --git a/.jupiter b/.jupiter deleted file mode 100644 index 4bd771a5..00000000 --- a/.jupiter +++ /dev/null @@ -1,200 +0,0 @@ - - - - property.default.description - - 1970-01-01 :: 00:00:00:000 GMT-10:00 - review - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - Pre 2.5 Review - brian - 2008-01-04 :: 10:09:23:000 GMT-08:00 - review - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - diff --git a/.project b/.project deleted file mode 100644 index 6f0bc9dc..00000000 --- a/.project +++ /dev/null @@ -1,85 +0,0 @@ - - - ModSecurity - - - - - - org.eclipse.cdt.make.core.makeBuilder - clean,full,incremental, - - - org.eclipse.cdt.make.core.append_environment - true - - - org.eclipse.cdt.make.core.enableCleanBuild - true - - - org.eclipse.cdt.make.core.build.command - make - - - org.eclipse.cdt.make.core.useDefaultBuildCmd - true - - - org.eclipse.cdt.make.core.build.target.auto - all - - - org.eclipse.cdt.make.core.stopOnError - false - - - org.eclipse.cdt.make.core.build.location - - - - org.eclipse.cdt.make.core.build.target.inc - all - - - org.eclipse.cdt.make.core.build.arguments - - - - org.eclipse.cdt.core.errorOutputParser - org.eclipse.cdt.core.MakeErrorParser;org.eclipse.cdt.core.GCCErrorParser;org.eclipse.cdt.core.GASErrorParser;org.eclipse.cdt.core.GLDErrorParser;org.eclipse.cdt.core.VCErrorParser; - - - org.eclipse.cdt.make.core.enableAutoBuild - false - - - org.eclipse.cdt.make.core.environment - - - - org.eclipse.cdt.make.core.enabledIncrementalBuild - true - - - org.eclipse.cdt.make.core.build.target.clean - clean - - - org.eclipse.cdt.make.core.enableFullBuild - true - - - - - org.eclipse.cdt.make.core.ScannerConfigBuilder - - - - - - org.eclipse.cdt.core.cnature - org.eclipse.cdt.make.core.makeNature - org.eclipse.cdt.make.core.ScannerConfigNature - - diff --git a/.settings/org.eclipse.cdt.core.prefs b/.settings/org.eclipse.cdt.core.prefs deleted file mode 100644 index 2222ff26..00000000 --- a/.settings/org.eclipse.cdt.core.prefs +++ /dev/null @@ -1,3 +0,0 @@ -#Fri Jan 04 09:57:26 GMT-08:00 2008 -eclipse.preferences.version=1 -indexerId=org.eclipse.cdt.core.fastIndexer diff --git a/CHANGES b/CHANGES index b02ee7d7..ea8a1e9b 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,9 @@ -07 Oct 2008 - 2.5.8 +21 Nov 2008 - 2.5.8 ------------------- + * Fixed PDF XSS issue where a non-GET request for a PDF file would crash the + Apache httpd process. Discovered by Steve Grubb at Red Hat. + * Removed an invalid "Internal error: Issuing "%s" for unspecified error." message that was logged when denying with nolog/noauditlog set and causing the request to be audited. diff --git a/apache2/msc_release.h b/apache2/msc_release.h index 0b346379..011fafc0 100644 --- a/apache2/msc_release.h +++ b/apache2/msc_release.h @@ -48,8 +48,8 @@ extern DSOLOCAL modsec_build_type_rec modsec_build_type[]; #define MODSEC_VERSION_MAJOR "2" #define MODSEC_VERSION_MINOR "5" #define MODSEC_VERSION_MAINT "8" -#define MODSEC_VERSION_TYPE "-dev" -#define MODSEC_VERSION_RELEASE "1" +#define MODSEC_VERSION_TYPE "" +#define MODSEC_VERSION_RELEASE "0" #define MODSEC_VERSION_SUFFIX MODSEC_VERSION_TYPE MODSEC_VERSION_RELEASE diff --git a/apache2/pdf_protect.c b/apache2/pdf_protect.c index d5c26482..0c708955 100644 --- a/apache2/pdf_protect.c +++ b/apache2/pdf_protect.c @@ -365,7 +365,6 @@ apr_status_t pdfp_output_filter(ap_filter_t *f, apr_bucket_brigade *bb_in) { */ int pdfp_check(modsec_rec *msr) { const char *token = NULL; - directory_config *cfg = NULL; char *uri = NULL; char *p = NULL; @@ -426,10 +425,12 @@ int pdfp_check(modsec_rec *msr) { /* Ignore request methods other than GET and HEAD if * configured to do so. + * + * TODO: Code here is only GET, not GET|HEAD as comment states */ - if ((msr->r->method_number != M_GET)&&(cfg->pdfp_only_get != 0)) { + if ((msr->r->method_number != M_GET)&&(msr->txcfg->pdfp_only_get != 0)) { if (msr->txcfg->debuglog_level >= 4) { - msr_log(msr, 4, "PdfProtect: Not intercepting a GET/HEAD request " + msr_log(msr, 4, "PdfProtect: Not intercepting request " "(method=%s/%d).", log_escape_nq(msr->mp, msr->r->method), msr->r->method_number); } diff --git a/apache2/t/action/.empty b/apache2/t/action/.empty new file mode 100644 index 00000000..e69de29b diff --git a/apache2/t/regression/misc/20-pdf-xss.t b/apache2/t/regression/misc/20-pdf-xss.t new file mode 100644 index 00000000..92d72303 --- /dev/null +++ b/apache2/t/regression/misc/20-pdf-xss.t @@ -0,0 +1,54 @@ +# PDF XSS Protection + +{ + type => "misc", + comment => "pdf-xss - GET", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + + SecPdfProtect On + SecPdfProtectMethod TokenRedirection + SecPdfProtectSecret FooBar + SecPdfProtectTimeout 10 + ), + match_log => { + debug => [ qr/PdfProtect: PDF request without a token - redirecting to/, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + GET => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.pdf", + ), +}, +{ + type => "misc", + comment => "pdf-xss - POST", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + + SecPdfProtect On + SecPdfProtectMethod TokenRedirection + SecPdfProtectSecret FooBar + SecPdfProtectTimeout 10 + ), + match_log => { + -error => [ qr/exit signal/, 1 ], + debug => [ qr/PdfProtect: Not intercepting.*method=POST\/2/, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.pdf", + [ + "Content-Type" => "application/x-www-form-urlencoded", + ], + # Args + "a=1&b=2", + ), +}, diff --git a/apache2/t/regression/server_root/data/.empty b/apache2/t/regression/server_root/data/.empty new file mode 100644 index 00000000..e69de29b diff --git a/apache2/t/regression/server_root/htdocs/test.pdf b/apache2/t/regression/server_root/htdocs/test.pdf new file mode 100644 index 00000000..a6ec2d04 Binary files /dev/null and b/apache2/t/regression/server_root/htdocs/test.pdf differ diff --git a/apache2/t/regression/server_root/logs/audit/.empty b/apache2/t/regression/server_root/logs/audit/.empty new file mode 100644 index 00000000..e69de29b diff --git a/apache2/t/regression/server_root/logs/subdir/.empty b/apache2/t/regression/server_root/logs/subdir/.empty new file mode 100644 index 00000000..e69de29b diff --git a/apache2/t/regression/server_root/tmp/.empty b/apache2/t/regression/server_root/tmp/.empty new file mode 100644 index 00000000..e69de29b diff --git a/apache2/t/regression/server_root/upload/.empty b/apache2/t/regression/server_root/upload/.empty new file mode 100644 index 00000000..e69de29b diff --git a/doc/modsecurity2-apache-reference.xml b/doc/modsecurity2-apache-reference.xml index f9fac9cb..0afbbfd4 100644 --- a/doc/modsecurity2-apache-reference.xml +++ b/doc/modsecurity2-apache-reference.xml @@ -6,7 +6,7 @@ Manual - Version 2.5.8-dev1 (October 6, 2008) + Version 2.5.8 (November 21, 2008) 2004-2008 diff --git a/review/pre-2.5-brian.review b/review/pre-2.5-brian.review deleted file mode 100644 index 0d2e9b5a..00000000 --- a/review/pre-2.5-brian.review +++ /dev/null @@ -1,3436 +0,0 @@ - - - - - 2008-01-04 :: 10:55:40:361 GMT-08:00 - 2008-01-11 :: 16:47:15:701 GMT-08:00 - - brian - brian - apache2/re_variables.c - item.type.label.suggestion - item.severity.label.normal - Is ENV really cacheable? It could change via setenv. - /* ENV */ - msre_engine_variable_register(engine, - "ENV", - VAR_LIST, - 0, 1, - var_env_validate, - var_env_generate, - VAR_CACHE, - PHASE_REQUEST_HEADERS - ); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 10:57:54:279 GMT-08:00 - 2008-01-11 :: 16:43:38:825 GMT-08:00 - - brian - brian - apache2/re_variables.c - item.type.label.suggestion - item.severity.label.trivial - GEO is probably not cacheable as it changes with every @geoLookup operator. - /* GEO */ - msre_engine_variable_register(engine, - "GEO", - VAR_LIST, - 1, 1, - var_generic_list_validate, - var_geo_generate, - VAR_CACHE, - PHASE_REQUEST_HEADERS - ); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 11:02:30:450 GMT-08:00 - 2008-01-11 :: 16:43:20:652 GMT-08:00 - - brian - brian - apache2/re_variables.c - item.type.label.suggestion - item.severity.label.trivial - GLOBAL is not documented. Is it cacheable? - /* GLOBAL */ - msre_engine_variable_register(engine, - "GLOBAL", - VAR_LIST, - 1, 1, - var_generic_list_validate, - var_global_generate, - VAR_CACHE, - PHASE_REQUEST_HEADERS - ); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 11:06:50:690 GMT-08:00 - 2008-01-11 :: 16:43:02:916 GMT-08:00 - - brian - brian - apache2/re_variables.c - item.type.label.suggestion - item.severity.label.trivial - IP undocumented. Probably not cacheable as it can change via setvar, etc. - /* IP */ - msre_engine_variable_register(engine, - "IP", - VAR_LIST, - 1, 1, - var_generic_list_validate, - var_ip_generate, - VAR_CACHE, - PHASE_REQUEST_HEADERS - ); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 11:08:36:733 GMT-08:00 - 2008-01-11 :: 16:42:44:589 GMT-08:00 - - brian - brian - apache2/re_variables.c - item.type.label.suggestion - item.severity.label.trivial - RESOURCE is undocumented. Probably not cacheable as it is easily changed. - /* RESOURCE */ - msre_engine_variable_register(engine, - "RESOURCE", - VAR_LIST, - 1, 1, - var_generic_list_validate, - var_resource_generate, - VAR_CACHE, - PHASE_REQUEST_HEADERS - ); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 11:14:32:043 GMT-08:00 - 2008-01-11 :: 16:41:31:558 GMT-08:00 - - brian - brian - apache2/re_variables.c - item.type.label.suggestion - item.severity.label.trivial - SESSION is probably not cacheable since it is modifyable via setvar. - /* SESSION */ - msre_engine_variable_register(engine, - "SESSION", - VAR_LIST, - 1, 1, - var_generic_list_validate, - var_session_generate, - VAR_CACHE, - PHASE_REQUEST_HEADERS - ); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 11:25:25:879 GMT-08:00 - 2008-01-11 :: 17:00:23:571 GMT-08:00 - - brian - brian - apache2/apache2_util.c - item.type.label.suggestion - item.severity.label.trivial - Portable way to format sizeof()? - msr_log(msr, 1, "Exec: Unable to allocate %lu bytes.", (unsigned long)sizeof(*procnew)); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 11:48:52:563 GMT-08:00 - 2008-01-04 :: 11:50:30:473 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.irrelevant - item.severity.label.trivial - This #if 0'd out code should be removed. - /* Removed %0-9 macros as it messes up urlEncoding in the match - * where having '%0a' will be treated as %{TX.0}a, which is incorrect. - * */ -#if 0 - else if ((*(p + 1) >= '0')&&(*(p + 1) <= '9')) { - /* Special case for regex captures. */ - var_name = "TX"; - var_value = apr_pstrmemdup(mptmp, p + 1, 1); - next_text_start = p + 2; - } -#endif - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 11:53:45:291 GMT-08:00 - 2008-01-04 :: 14:51:42:573 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.optimization - item.severity.label.trivial - Use apr_array_pstrcat(msr->mp, arr, NULL) instead? - /* If there's more than one member of the array that - * means there was at least one macro present. Combine - * text parts into a single string now. - */ - if (arr->nelts > 1) { - /* Figure out the required size for the string. */ - var->value_len = 0; - for(i = 0; i < arr->nelts; i++) { - part = ((msc_string **)arr->elts)[i]; - var->value_len += part->value_len; - } - - /* Allocate the string. */ - var->value = apr_palloc(msr->mp, var->value_len + 1); - if (var->value == NULL) return -1; - - /* Combine the parts. */ - offset = 0; - for(i = 0; i < arr->nelts; i++) { - part = ((msc_string **)arr->elts)[i]; - memcpy((char *)(var->value + offset), part->value, part->value_len); - offset += part->value_len; - } - var->value[offset] = '\0'; - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 12:00:50:877 GMT-08:00 - 2008-01-04 :: 12:03:50:156 GMT-08:00 - - brian - brian - apache2/re_operators.c - Suggestion - item.severity.label.trivial - Use resolve_relative_path() instead? Maybe a config_relative_path() to just get the path? - /* Get the path of the rule filename to use as a base */ - rulefile_path = apr_pstrndup(rule->ruleset->mp, rule->filename, strlen(rule->filename) - strlen(apr_filepath_name_get(rule->filename))); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 13:23:49:834 GMT-08:00 - 2008-01-11 :: 16:26:24:257 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.clarity - item.severity.label.trivial - Add parens for clarity. - *next++ = '\0'; - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 14:54:09:456 GMT-08:00 - 2008-01-04 :: 14:55:05:945 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.missing - item.severity.label.trivial - Need to check return code and log an error on failure. - acmp_add_pattern(p, buf, NULL, NULL, strlen(buf)); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 14:55:53:308 GMT-08:00 - 2008-01-04 :: 14:56:17:267 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.missing - item.severity.label.trivial - Need to check return code and log an error on failure. - acmp_prepare(p); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 14:58:08:006 GMT-08:00 - 2008-01-04 :: 15:00:17:190 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.optimization - item.severity.label.minor - See if apr_strmatch is faster. - msre_op_within_execute - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 14:59:32:341 GMT-08:00 - 2008-01-04 :: 14:59:59:858 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.optimization - item.severity.label.minor - See if apr_strmatch is faster. - msre_op_contains_execute - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 15:01:39:208 GMT-08:00 - 2008-01-04 :: 15:02:04:258 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.optimization - item.severity.label.minor - See if apr_strmatch is faster. - msre_op_containsWord_execute - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 15:03:12:792 GMT-08:00 - 2008-01-04 :: 15:04:40:965 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.optimization - item.severity.label.minor - This implementation comment needs to be coded as many string operators now attempt to resolve macros. - /* IMP1 Duplicate the string and create the array on - * demand, thus not having to do it if there are - * no macros in the input data. - */ - - data = apr_pstrdup(mptmp, var->value); /* IMP1 Are we modifying data anywhere? */ - arr = apr_array_make(mptmp, 16, sizeof(msc_string *)); - if ((data == NULL)||(arr == NULL)) return -1; - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 15:12:55:300 GMT-08:00 - 2008-01-04 :: 15:13:19:677 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.missing - item.severity.label.minor - Need more unit tests for operators. Start with new operators. - - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 15:36:54:292 GMT-08:00 - 2008-01-04 :: 15:37:50:111 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.missing - item.severity.label.trivial - @m operator is not documented. This does the same as @contains, so it was suggested earlier to use the @m algorithm for contains (if faster) and drop @m. - /* m */ - msre_engine_op_register(engine, - "m", - msre_op_m_param_init, - msre_op_m_execute - ); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 15:43:40:931 GMT-08:00 - 2008-01-11 :: 16:38:59:940 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.missing - item.severity.label.trivial - @geoLookup should set error_msg on success to something like "Successful geograpical lookup of \"%s\" at %s." - msre_op_geoLookup_execute - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 15:47:09:574 GMT-08:00 - 2008-01-11 :: 16:40:07:483 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.missing - item.severity.label.trivial - @rbl fails to set the var name in error_msg. Should append "at %s". - rc = apr_sockaddr_info_get(&sa, name_to_check, - APR_UNSPEC/*msr->r->connection->remote_addr->family*/, 0, 0, msr->mp); - if (rc == APR_SUCCESS) { - *error_msg = apr_psprintf(msr->r->pool, "RBL lookup of %s succeeded.", - log_escape_nq(msr->mp, name_to_check)); - return 1; /* Match. */ - } - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 15:49:05:878 GMT-08:00 - 2008-01-11 :: 14:56:55:081 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.suggestion - item.severity.label.major - Change from TODO to ENH. - - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 15:54:24:055 GMT-08:00 - 2008-01-11 :: 14:57:08:520 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.suggestion - item.severity.label.trivial - Need to remove the LUA #ifdef's - #ifdef WITH_LUA - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 15:56:23:415 GMT-08:00 - 2008-01-11 :: 14:57:21:641 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.suggestion - item.severity.label.trivial - The LUA #ifdef's should be removed, but if it is decided not to, then this lua call needs to be #ifdef'd. - } else { - /* Execute internally, as Lua script. */ - char *target = apr_pstrmemdup(msr->mp, var->value, var->value_len); - msc_script *script = (msc_script *)rule->op_param_data; - int rc; - - rc = lua_execute(script, target, msr, rule, error_msg); - if (rc < 0) { - /* Error. */ - return -1; - } - - return rc; - } - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 15:58:01:918 GMT-08:00 - 2008-01-04 :: 15:58:58:621 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.missing - item.severity.label.major - Need an error_msg set for lua execution error. - rc = lua_execute(script, target, msr, rule, error_msg); - if (rc < 0) { - /* Error. */ - return -1; - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 16:00:09:204 GMT-08:00 - 2008-01-11 :: 16:38:08:350 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.missing - item.severity.label.trivial - @validateByteRange does not output the VAR name on match. Need to append " at %s." - msre_op_validateByteRange_execute - - - item.resolution.label.invalidWontfix - item.status.label.closed - - - - 2008-01-04 :: 16:02:02:403 GMT-08:00 - 2008-01-04 :: 16:06:22:410 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.missing - item.severity.label.trivial - @validateurlEncoding does not output VAR name nor offset in error_msg on match. - msre_op_validateUrlEncoding_execute - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 16:03:51:127 GMT-08:00 - 2008-01-11 :: 16:30:07:550 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.missing - item.severity.label.trivial - Numeric operators (@eq, etc) do not output VAR name on match. - msre_op_eq_execute -msre_op_gt_execute -msre_op_lt_execute -msre_op_ge_execute -msre_op_le_execute - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 16:12:30:943 GMT-08:00 - 2008-01-11 :: 14:57:45:711 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - No. - /* ENH Do we want to support %{DIGIT} as well? */ - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 16:14:51:515 GMT-08:00 - 2008-01-07 :: 14:22:10:119 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.missing - item.severity.label.trivial - Implement. Need to check if Apache will return an invalid status code - /* status */ -static char *msre_action_status_validate(msre_engine *engine, msre_action *action) { - /* ENH action->param must be a valid HTTP status code. */ - return NULL; -} - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 16:15:55:149 GMT-08:00 - 2008-01-04 :: 16:16:52:467 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.missing - item.severity.label.trivial - Implement. - /* pause */ -static char *msre_action_pause_validate(msre_engine *engine, msre_action *action) { - /* ENH Validate a positive number. */ - return NULL; -} - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 16:17:34:464 GMT-08:00 - 2008-01-04 :: 16:22:35:501 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.missing - item.severity.label.trivial - Implement as a valid URI check with apr_uri_parse()? - /* redirect */ - -static char *msre_action_redirect_validate(msre_engine *engine, msre_action *action) { - /* ENH Add validation. */ - return NULL; -} - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 16:22:43:274 GMT-08:00 - 2008-01-04 :: 16:22:54:679 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.missing - item.severity.label.trivial - Implement as a valid URI check with apr_uri_parse()? - /* proxy */ - -static char *msre_action_proxy_validate(msre_engine *engine, msre_action *action) { - /* ENH Add validation. */ - return NULL; -} - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 16:24:58:322 GMT-08:00 - 2008-01-11 :: 16:13:48:178 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.irrelevant - item.severity.label.trivial - I believe this is already done and comment needs removed. - // TODO: Need to keep track of skipAfter IDs so we can insert placeholders after - // we get to the real rule with that ID. - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 16:26:46:732 GMT-08:00 - 2008-01-04 :: 16:27:54:881 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.irrelevant - item.severity.label.trivial - I do not see a need to validate beyound what is already done in the init function. - msre_action_skip_validate -msre_action_skipAfter_validate - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 16:28:48:332 GMT-08:00 - 2008-01-04 :: 16:29:03:587 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.missing - item.severity.label.trivial - Implement. - /* phase */ - -static char *msre_action_phase_validate(msre_engine *engine, msre_action *action) { - /* ENH Add validation. */ - return NULL; -} - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 16:51:52:029 GMT-08:00 - 2008-01-04 :: 17:03:32:872 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - Probably should also calc length and validate a length > 0 instead of just checking NULL. Other checks would benefit from checking a length as well, so no harm in calculating that. - if (value == NULL) { - return apr_psprintf(engine->mp, "Missing ctl value for name: %s", name); - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 16:56:10:995 GMT-08:00 - 2008-01-04 :: 16:59:47:800 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.irrelevant - item.severity.label.trivial - Why register init() if we do not use it? - static apr_status_t msre_action_ctl_init(msre_engine *engine, msre_actionset *actionset, - msre_action *action) -{ - /* Do nothing. */ - return 1; -} - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 17:00:49:340 GMT-08:00 - 2008-01-04 :: 17:02:15:141 GMT-08:00 - - brian - brian - apache2/msc_logging.c - item.type.label.programLogic - item.severity.label.trivial - This allows an empty string as a valid part. This misvalidates "ctl:auditLogParts=+", etc. - is_valid_parts_specification - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 17:04:51:155 GMT-08:00 - 2008-01-11 :: 16:12:33:664 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.optimization - item.severity.label.trivial - Inner if's should be else if's. - if (strcasecmp(name, "ruleEngine") == 0) { - if (strcasecmp(value, "on") == 0) { - msr->txcfg->is_enabled = MODSEC_ENABLED; - msr->usercfg->is_enabled = MODSEC_ENABLED; - } - - if (strcasecmp(value, "off") == 0) { - msr->txcfg->is_enabled = MODSEC_DISABLED; - msr->usercfg->is_enabled = MODSEC_DISABLED; - } - - if (strcasecmp(value, "detectiononly") == 0) { - msr->txcfg->is_enabled = MODSEC_DETECTION_ONLY; - msr->usercfg->is_enabled = MODSEC_DETECTION_ONLY; - } - - return 1; - } else - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 17:05:44:757 GMT-08:00 - 2008-01-11 :: 16:12:12:876 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.optimization - item.severity.label.trivial - TODO needs looked into. - if (strcasecmp(name, "auditEngine") == 0) { - if (strcasecmp(value, "on") == 0) { - msr->txcfg->auditlog_flag = AUDITLOG_ON; - msr->usercfg->auditlog_flag = AUDITLOG_ON; - } - - if (strcasecmp(value, "off") == 0) { - msr->txcfg->auditlog_flag = AUDITLOG_OFF; - msr->usercfg->auditlog_flag = AUDITLOG_OFF; - } - - if (strcasecmp(value, "relevantonly") == 0) { - msr->txcfg->auditlog_flag = AUDITLOG_RELEVANT; - msr->usercfg->auditlog_flag = AUDITLOG_RELEVANT; - } - - msr_log(msr, 4, "Ctl: Set auditEngine to %d.", msr->txcfg->auditlog_flag); // TODO - - return 1; - } else - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-04 :: 17:08:00:396 GMT-08:00 - 2008-01-11 :: 14:59:05:782 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - That warning quieter should be s++. An evil typo that was fixed in 2.1.x, but not trunk! - while(*s != '\0') { - if (*s != c) { - *d++ = *s++; - } else { - (*s)++; /* parens quiet compiler warning */ - } - } - *d = '\0'; - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-04 :: 17:13:11:886 GMT-08:00 - 2008-01-04 :: 17:13:40:681 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.missing - item.severity.label.trivial - Should log an internal error here. - else { - /* ENH Should never happen, but log if it does. */ - return -1; - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 11:14:40:912 GMT-08:00 - 2008-01-07 :: 11:15:54:834 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - Should log a level 9 msg here. - } else { - /* We could not identify a valid macro so add it as text. */ - part = (msc_string *)apr_pcalloc(mptmp, sizeof(msc_string)); - if (part == NULL) return -1; - part->value_len = p - text_start + 1; /* len(text)+len("%") */ - part->value = apr_pstrmemdup(mptmp, text_start, part->value_len); - *(msc_string **)apr_array_push(arr) = part; - - next_text_start = p + 1; - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 11:37:09:009 GMT-08:00 - 2008-01-07 :: 11:41:55:614 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - Probably should use apr_strtoi64 where we can tell if there was an error in conversion since we are potentially taking a value from a macro expansion. Also may want to look for overflow. - value += atoi(var_value); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 11:43:13:789 GMT-08:00 - 2008-01-11 :: 16:10:24:140 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.missing - item.severity.label.trivial - Missing error log needs implemented. - } else { - /* ENH Log warning detected variable name but no collection. */ - return 0; - } - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 11:52:38:857 GMT-08:00 - 2008-01-07 :: 11:53:56:791 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - Not sure why we would not want to deprecate a TX var. Further rules could use this even if TX is not persisted. - /* IMP1 Add message TX variables cannot deprecate in value. */ - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 11:54:14:673 GMT-08:00 - 2008-01-11 :: 15:04:13:383 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - Missing error log needs implemented. - } else { - /* ENH Log warning detected variable name but no collection. */ - return 0; - } - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 12:03:17:626 GMT-08:00 - 2008-01-07 :: 23:10:15:221 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - The timeout is hardcoded to 3600. The docs state TIMEOUT is read-only, but this is not true. So, you can modify TIMEOUT. - /* IMP1 Is the timeout hard-coded to 3600? */ - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 14:12:19:250 GMT-08:00 - 2008-01-07 :: 14:14:28:669 GMT-08:00 - - brian - brian - apache2/msc_logging.c - item.type.label.suggestion - item.severity.label.trivial - apr_dir_make_recursive will attempt to create the dir straight away and if that fails keep backing off a dir until it can start creating, so I see no need to cache. Besides, what happens if you cache, then someone deletes the path from outside apache? - /* IMP1 Surely it would be more efficient to check the folders for - * the audit log repository base path in the configuration phase, to reduce - * the work we do on every request. Also, since our path depends on time, - * we could cache the time we last checked and don't check if we know - * the folder is there. - */ - rc = apr_dir_make_recursive(entry_basename, CREATEMODE_DIR, msr->mp); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 14:24:02:378 GMT-08:00 - 2008-01-07 :: 14:50:55:762 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - We already have support for relative filenames, but cannot get to this data from here. This needs solved by passing more data to the validate function (cmd_parms rec). Maybe need a warning here stating we do not support them yet, or it might be confusing to users that we do not here but do elsewhere. - /* TODO Support relative filenames. */ - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 14:33:59:432 GMT-08:00 - 2008-01-07 :: 15:59:35:056 GMT-08:00 - - brian - brian - apache2/re.h - item.type.label.suggestion - item.severity.label.trivial - Why not stored in op_param_data like @rx, etc. The param_data is used w/exec action for lua. - /* Compiled Lua script. */ - msc_script *script; - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 15:55:08:220 GMT-08:00 - 2008-01-07 :: 16:02:36:938 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - This assumes lua is the only type (which it is now), but should be re-writen with a script_rec stored in param_data. - if (action->param_data != NULL) { /* Lua */ - msc_script *script = (msc_script *)action->param_data; - char *my_error_msg = NULL; - - if (lua_execute(script, NULL, msr, rule, &my_error_msg) < 0) { - msr_log(msr, 1, "%s", my_error_msg); - return 0; - } - } else { /* Execute as shell script. */ - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 16:00:51:901 GMT-08:00 - 2008-01-07 :: 16:04:13:185 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - Not sure using an extension is a good idea here. Better I think would be to specify a type: "exec:[type=]/path/to/file" as in "exec:lua=/path/to/script" and make param_data a script_rec with a type and value. Also we use the abstract param_data here vs using a specific field as in SecRuleScript. - /* Process Lua scripts internally. */ - if (strlen(filename) > 4) { - char *p = filename + strlen(filename) - 4; - if ((p[0] == '.')&&(p[1] == 'l')&&(p[2] == 'u')&&(p[3] == 'a')) { - /* It's a Lua script. */ - msc_script *script = NULL; - - /* Compile script. */ - char *msg = lua_compile(&script, filename, engine->mp); - if (msg != NULL) return msg; - - action->param_data = script; - } - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 16:08:53:365 GMT-08:00 - 2008-01-11 :: 15:25:58:269 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.suggestion - item.severity.label.trivial - Should not log_escape the actions as they will get double escaped (once now and again when logged). - } else { - rule->unparsed = apr_psprintf(ruleset->mp, "SecRuleScript \"%s\" \"%s\"", - script_filename, log_escape(ruleset->mp, actions)); - } - - - item.resolution.label.invalidWontfix - item.status.label.closed - - - - 2008-01-07 :: 16:17:58:895 GMT-08:00 - 2008-01-11 :: 15:26:18:631 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.suggestion - item.severity.label.trivial - Should not log_escape the actions as they will get double escaped (once now and again when logged). - /* Add the unparsed rule */ - if ((strcmp(SECACTION_TARGETS, targets) == 0) && (strcmp(SECACTION_ARGS, args) == 0)) { - rule->unparsed = apr_psprintf(ruleset->mp, "SecAction \"%s\"", - log_escape(ruleset->mp, actions)); - } - else - if ((strcmp(SECMARKER_TARGETS, targets) == 0) - && (strcmp(SECMARKER_ARGS, args) == 0) - && (strncmp(SECMARKER_BASE_ACTIONS, actions, strlen(SECMARKER_BASE_ACTIONS)) == 0)) - { - rule->unparsed = apr_psprintf(ruleset->mp, "SecMarker \"%s\"", - log_escape(ruleset->mp, actions + strlen(SECMARKER_BASE_ACTIONS))); - } - else { - if (actions == NULL) { - rule->unparsed = apr_psprintf(ruleset->mp, "SecRule \"%s\" \"%s\"", - log_escape(ruleset->mp, targets), log_escape(ruleset->mp, args)); - } else { - rule->unparsed = apr_psprintf(ruleset->mp, "SecRule \"%s\" \"%s\" \"%s\"", - log_escape(ruleset->mp, targets), log_escape(ruleset->mp, args), - log_escape(ruleset->mp, actions)); - } - } - - - item.resolution.label.invalidWontfix - item.status.label.closed - - - - 2008-01-07 :: 16:22:43:295 GMT-08:00 - 2008-01-11 :: 15:26:48:665 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.suggestion - item.severity.label.trivial - No logging should be done here as we are passing the error_msg back to the parent and they are responsible for this. - if (*error_msg != NULL) { - /* ENH Shouldn't we log the problem? */ - return NULL; - } - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 16:30:41:403 GMT-08:00 - 2008-01-07 :: 16:31:00:930 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.missing - item.severity.label.trivial - Need to log on failure. - var = msre_create_var(ruleset, telts[i].key, telts[i].val, NULL, error_msg); - if (var == NULL) return -1; - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 16:36:31:466 GMT-08:00 - 2008-01-07 :: 16:36:54:189 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.suggestion - item.severity.label.trivial - Should replace with isvarnamechar() if possible. - while((*p != '\0')&&(*p != '|')&&(*p != ':')&&(*p != ',')&&(!isspace(*p))) p++; /* ENH replace with isvarnamechar() */ - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 16:39:21:316 GMT-08:00 - 2008-01-11 :: 15:28:02:997 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.suggestion - item.severity.label.trivial - Fix or remove TODO. - // TODO better 64-bit support here - *error_msg = apr_psprintf(mp, "Missing closing quote at position %d: %s", - (int)(p - text), text); - free(value); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 16:39:47:318 GMT-08:00 - 2008-01-11 :: 15:28:24:355 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.suggestion - item.severity.label.trivial - Fix or remove TODO. - // TODO better 64-bit support here - *error_msg = apr_psprintf(mp, "Invalid quoted pair at position %d: %s", - (int)(p - text), text); - free(value); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 16:40:15:634 GMT-08:00 - 2008-01-11 :: 16:07:58:168 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.clarity - item.severity.label.trivial - Add parens for clarity. - *d++ = *p++; - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 16:40:43:464 GMT-08:00 - 2008-01-11 :: 16:07:34:306 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.clarity - item.severity.label.trivial - Add parens for clarity. - *d++ = *p++; - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 20:46:50:832 GMT-08:00 - 2008-01-11 :: 16:06:54:324 GMT-08:00 - - brian - brian - apache2/acmp.c - item.type.label.clarity - item.severity.label.trivial - Add parens for clarity. - *ucs_chars++ = *c++; - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 20:47:54:093 GMT-08:00 - 2008-01-11 :: 16:06:02:231 GMT-08:00 - - brian - brian - apache2/msc_multipart.c - item.type.label.clarity - item.severity.label.trivial - Add parens for clarity. - *t++ = *p++; - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 20:48:42:400 GMT-08:00 - 2008-01-11 :: 16:05:23:686 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.clarity - item.severity.label.trivial - Add parens for clarity. - *d++ = *s++; - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 21:42:51:192 GMT-08:00 - 2008-01-07 :: 21:44:12:097 GMT-08:00 - - brian - brian - apache2/apache2_io.c - item.type.label.programLogic - item.severity.label.major - Returning here may fail to free chunks data due to modsecurity_request_body_end() not being called. - int rcbs = modsecurity_request_body_store(msr, buf, buflen, error_msg); - if (rcbs < 0) { - if (rcbs == -5) { - *error_msg = apr_psprintf(msr->mp, "Requests body no files data length is larger than the " - "configured limit (%lu).", msr->txcfg->reqbody_no_files_limit); - return -5; - } - - return -1; - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 21:50:32:890 GMT-08:00 - 2008-01-07 :: 21:52:29:239 GMT-08:00 - - brian - brian - apache2/modsecurity.c - item.type.label.suggestion - item.severity.label.major - Good. This looks to solve the other issues noted as possible memory leaks in body chunk data due to modsecurity_request_body_end() not being called. Need to verify, though. - /* Register TX cleanup */ - apr_pool_cleanup_register(msr->mp, msr, modsecurity_tx_cleanup, apr_pool_cleanup_null); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 21:59:33:579 GMT-08:00 - 2008-01-11 :: 15:29:21:240 GMT-08:00 - - brian - brian - apache2/msc_util.c - item.type.label.suggestion - item.severity.label.trivial - Actually, the parens are *required* for correctness, so remove the comments. - (*invalid_count)++; /* parens quiet compiler warning */ - } - } else { - /* Not enough bytes available, copy the raw bytes. */ - *d++ = input[i++]; - count ++; - (*invalid_count)++; /* parens quiet compiler warning */ - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 22:03:31:138 GMT-08:00 - 2008-01-07 :: 22:04:07:108 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.irrelevant - item.severity.label.trivial - Does not appear to be used anywhere. - /** - * Destroys an engine instance, releasing the consumed memory. - */ -void msre_engine_destroy(msre_engine *engine) { - /* Destroyed automatically by the parent pool. - * apr_pool_destroy(engine->mp); - */ -} - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 22:04:22:398 GMT-08:00 - 2008-01-07 :: 22:04:35:724 GMT-08:00 - - brian - brian - apache2/re.h - item.type.label.irrelevant - item.severity.label.trivial - Does not appear to be used anywhere. - void DSOLOCAL msre_engine_destroy(msre_engine *engine); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 22:23:48:909 GMT-08:00 - 2008-01-11 :: 16:04:30:061 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.clarity - item.severity.label.trivial - This version should be moved up next to the normal version. - #if defined(PERFORMANCE_MEASUREMENT) -apr_status_t msre_ruleset_process_phase(msre_ruleset *ruleset, modsec_rec *msr) { - ... -} - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 22:30:42:215 GMT-08:00 - 2008-01-11 :: 16:02:09:916 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.missing - item.severity.label.major - Hmm, I thought this had already been fixed in trunk. Missing logging phase. Need to fix in 2.1.5 as well. - /** - * Removes from the ruleset all rules that match the given exception. - */ -int msre_ruleset_rule_remove_with_exception(msre_ruleset *ruleset, rule_exception *re) { - int count = 0; - - if (ruleset == NULL) return 0; - - count += msre_ruleset_phase_rule_remove_with_exception(ruleset, re, ruleset->phase_request_headers); - count += msre_ruleset_phase_rule_remove_with_exception(ruleset, re, ruleset->phase_request_body); - count += msre_ruleset_phase_rule_remove_with_exception(ruleset, re, ruleset->phase_response_headers); - count += msre_ruleset_phase_rule_remove_with_exception(ruleset, re, ruleset->phase_response_body); - - return count; -} - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 22:46:51:423 GMT-08:00 - 2008-01-11 :: 16:01:31:111 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.optimization - item.severity.label.trivial - Should move this to a static global for performance. - static const char *const severities[] = { - "EMERGENCY", - "ALERT", - "CRITICAL", - "ERROR", - "WARNING", - "NOTICE", - "INFO", - "DEBUG", - NULL, - }; - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-07 :: 22:49:04:822 GMT-08:00 - 2008-01-07 :: 22:49:16:740 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.suggestion - item.severity.label.trivial - Implement TODO. - //TODO: restrict to 512 bytes - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-07 :: 22:51:40:727 GMT-08:00 - 2008-01-07 :: 22:53:13:118 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.optimization - item.severity.label.trivial - tags set to NULL would be a bit better as it would stop apr_pstrcat() earlier, but tags *must* remain last or wierd results. - char *tags = ""; - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-08 :: 11:44:13:484 GMT-08:00 - 2008-01-08 :: 11:45:29:864 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.optimization - item.severity.label.trivial - This causes two loops through the action list. Perhaps there is a more performant way to do these at the same time? Maybe split into two lists? - /* Perform non-disruptive actions. */ - msre_perform_nondisruptive_actions(msr, rule, rule->actionset, mptmp); - - /* Perform disruptive actions, but only if - * this rule is not part of a chain. - */ - if (rule->actionset->is_chained == 0) { - msre_perform_disruptive_actions(msr, rule, acting_actionset, mptmp, my_error_msg); - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-08 :: 12:16:20:280 GMT-08:00 - 2008-01-11 :: 15:59:48:300 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.irrelevant - item.severity.label.trivial - These do not appear to be needed. - tfnspath = NULL; - tfnskey = NULL; - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-08 :: 12:20:29:491 GMT-08:00 - 2008-01-11 :: 15:59:23:210 GMT-08:00 - - brian - brian - apache2/re.c - item.type.label.programLogic - item.severity.label.major - This does not appear to work as the tfnskey is not being built here. Need to build the tfnskey in this loop for this to work. - /* check cache, saving the 'most complete' */ - crec = (msre_cache_rec *)apr_table_get(cachetab, tfnskey); - if (crec != NULL) { - last_crec = crec; - last_cached_tfn = tfnscount; - } - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-08 :: 21:29:14:889 GMT-08:00 - 2008-01-08 :: 21:30:41:759 GMT-08:00 - - brian - brian - apache2/re_tfns.c - item.type.label.optimization - item.severity.label.trivial - No need to set this on all. Only set it once when we find the first non-space char. - (*rval)[i] = '\0'; - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-08 :: 22:09:10:463 GMT-08:00 - 2008-01-11 :: 14:20:53:411 GMT-08:00 - - brian - brian - apache2/re_variables.c - item.type.label.suggestion - item.severity.label.trivial - Indention off. - return var_simple_generate_ex(var, vartab, mptmp, - apr_pmemdup(mptmp, - msr->matched_var->value, - msr->matched_var->value_len), - msr->matched_var->value_len); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-08 :: 22:09:35:664 GMT-08:00 - 2008-01-11 :: 14:21:28:242 GMT-08:00 - - brian - brian - apache2/re_variables.c - item.type.label.suggestion - item.severity.label.trivial - Indention off. - return var_simple_generate_ex(var, vartab, mptmp, - apr_pmemdup(mptmp, - msr->matched_var->name, - msr->matched_var->name_len), - msr->matched_var->name_len); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 10:55:38:639 GMT-08:00 - 2008-01-11 :: 14:25:15:860 GMT-08:00 - - brian - brian - apache2/acmp.c - item.type.label.suggestion - item.severity.label.trivial - Remove comment. - //return acmp_child_for_code(node, letter) - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 11:01:01:815 GMT-08:00 - 2008-01-11 :: 15:55:50:363 GMT-08:00 - - brian - brian - apache2/acmp.c - item.type.label.suggestion - item.severity.label.trivial - Change to #idef DEBUG_ACMP or similar. - /* printf("%c ->left %c \n", node->node->letter, node->left->node->letter); */ - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 11:01:41:424 GMT-08:00 - 2008-01-11 :: 15:56:10:175 GMT-08:00 - - brian - brian - apache2/acmp.c - item.type.label.suggestion - item.severity.label.trivial - Change to #idef DEBUG_ACMP or similar. - /* printf("%c ->right %c \n", node->node->letter, node->right->node->letter); */ - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 11:02:22:279 GMT-08:00 - 2008-01-11 :: 15:56:41:045 GMT-08:00 - - brian - brian - apache2/acmp.c - item.type.label.suggestion - item.severity.label.trivial - Change to #idef DEBUG_ACMP or similar. - /* printf("fail direction: *%s* => *%s*\n", child->text, child->fail->text); */ - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 11:02:42:897 GMT-08:00 - 2008-01-11 :: 15:54:16:807 GMT-08:00 - - brian - brian - apache2/acmp.c - item.type.label.suggestion - item.severity.label.trivial - Change to #idef DEBUG_ACMP or similar. - /* printf("fail direction: *%s* => *%s*\n", node->text, node->fail->text); */ - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 11:10:25:157 GMT-08:00 - 2008-01-11 :: 14:24:57:212 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - The 'tag' and 'severity' metadata actions should be included. Some actions are missing from the log msg. - /* Must NOT use metadata actions. */ - if ((rule->actionset->id != NOT_SET_P) - ||(rule->actionset->rev != NOT_SET_P) - ||(rule->actionset->msg != NOT_SET_P) - ||(rule->actionset->logdata != NOT_SET_P)) - { - return apr_psprintf(cmd->pool, "ModSecurity: Metadata actions (id, rev, msg) " - " can only be specified by chain starter rules."); - } - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 11:18:38:672 GMT-08:00 - 2008-01-11 :: 14:29:49:134 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Probably should check we were able to allocate. - msre_rule *phrule = apr_palloc(rule->ruleset->mp, sizeof(msre_rule)); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:05:40:949 GMT-08:00 - 2008-01-11 :: 14:30:01:771 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Comment or remove. - TODO - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:06:48:653 GMT-08:00 - 2008-01-11 :: 14:30:40:514 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - ENH instead of TODO - TODO - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:09:17:114 GMT-08:00 - 2008-01-11 :: 14:31:50:812 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - The 'tag' and 'severity' metadata actions should be included. Some actions are missing from the log msg. - /* Must not use metadata actions. */ - if ((dcfg->tmp_default_actionset->id != NOT_SET_P) - ||(dcfg->tmp_default_actionset->rev != NOT_SET_P) - ||(dcfg->tmp_default_actionset->msg != NOT_SET_P) - ||(dcfg->tmp_default_actionset->logdata != NOT_SET_P)) - { - return apr_psprintf(cmd->pool, "ModSecurity: SecDefaultAction must not " - "contain any metadata actions (id, rev, msg)."); - } - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:17:06:712 GMT-08:00 - 2008-01-11 :: 14:33:23:560 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or make ENH - // TODO Validate encoding - // return apr_psprintf(cmd->pool, "ModSecurity: Invalid value for SecRequestBodyAccess: %s", p1); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:17:55:545 GMT-08:00 - 2008-01-09 :: 12:25:33:617 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.irrelevant - item.severity.label.trivial - Remove code? - /* -static const char *cmd_rule_import_by_id(cmd_parms *cmd, void *_dcfg, const char *p1) { - directory_config *dcfg = (directory_config *)_dcfg; - rule_exception *re = apr_pcalloc(cmd->pool, sizeof(rule_exception)); - if (dcfg == NULL) return NULL; - - re->type = RULE_EXCEPTION_IMPORT_ID; - // TODO verify p1 - re->param = p1; - *(rule_exception **)apr_array_push(dcfg->rule_exceptions) = re; - - return NULL; -} - -static const char *cmd_rule_import_by_msg(cmd_parms *cmd, void *_dcfg, const char *p1) { - directory_config *dcfg = (directory_config *)_dcfg; - rule_exception *re = apr_pcalloc(cmd->pool, sizeof(rule_exception)); - if (dcfg == NULL) return NULL; - - re->type = RULE_EXCEPTION_IMPORT_MSG; - // TODO verify p1 - re->param = p1; - *(rule_exception **)apr_array_push(dcfg->rule_exceptions) = re; - - return NULL; -} -*/ - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 12:18:34:399 GMT-08:00 - 2008-01-11 :: 14:34:07:673 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix or TODO->ENH - // TODO enforce format (letters, digits, ., _, -) - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:20:16:234 GMT-08:00 - 2008-01-11 :: 14:40:53:633 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Need to allow for relative filename based of rule file. - /* -- Geo Lookup configuration -- */ - -static const char *cmd_geo_lookup_db(cmd_parms *cmd, void *_dcfg, - const char *p1) -{ - char *error_msg; - directory_config *dcfg = (directory_config *)_dcfg; - if (dcfg == NULL) return NULL; - - if (geo_init(dcfg, p1, &error_msg) <= 0) { - return error_msg; - } - - return NULL; -} - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:22:58:798 GMT-08:00 - 2008-01-09 :: 12:23:11:462 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Portable? - /* The NOT_SET indicator is -1, a signed long, and therfore - * we cannot be >= the unsigned value of NOT_SET. - */ - if ((unsigned long)intval >= (unsigned long)NOT_SET) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations minlen must be less than: %lu", (unsigned long)NOT_SET); - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 12:23:33:676 GMT-08:00 - 2008-01-09 :: 12:23:39:437 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Portable? - /* The NOT_SET indicator is -1, a signed long, and therfore - * we cannot be >= the unsigned value of NOT_SET. - */ - if ((unsigned long)intval >= (unsigned long)NOT_SET) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations maxlen must be less than: %lu", (unsigned long)NOT_SET); - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 12:24:06:001 GMT-08:00 - 2008-01-09 :: 12:24:23:994 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Could use strtol as Ivan has as well. - intval = apr_atoi64(charval); - if (errno == ERANGE) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations minlen out of range: %s", charval); - } - if (intval < 0) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations minlen must be positive: %s", charval); - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 12:24:37:453 GMT-08:00 - 2008-01-09 :: 12:24:42:462 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Could use strtol as Ivan has as well. - intval = apr_atoi64(charval); - if (errno == ERANGE) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations maxlen out of range: %s", charval); - } - if (intval < 0) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations maxlen must be positive: %s", charval); - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 12:25:36:799 GMT-08:00 - 2008-01-09 :: 12:31:35:264 GMT-08:00 - - brian - brian - apache2/apache2_io.c - item.type.label.programLogic - item.severity.label.major - Remove code? It is actually used below, so need to verify. - #if 0 -static void dummy_free_func(void *data) {} -#endif - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 12:27:10:252 GMT-08:00 - 2008-01-09 :: 12:29:31:617 GMT-08:00 - - brian - brian - apache2/apache2_io.c - item.type.label.programLogic - item.severity.label.major - dummy_free_func() is defined where? It is ifdef'd out at the top of source, so need to verify it is valid. - /* Do not make a copy of the data we received in the chunk. */ - bucket = apr_bucket_heap_create(chunk->data, chunk->length, dummy_free_func, - f->r->connection->bucket_alloc); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 12:35:21:613 GMT-08:00 - 2008-01-09 :: 13:02:40:668 GMT-08:00 - - brian - brian - apache2/apache2_io.c - item.type.label.suggestion - item.severity.label.trivial - Yes, why do we ignore the rc - why have one at all? - // TODO: Why ignore the return code here? - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 12:55:19:633 GMT-08:00 - 2008-01-11 :: 17:00:04:416 GMT-08:00 - - brian - brian - apache2/msc_reqbody.c - item.type.label.suggestion - item.severity.label.trivial - Portable way to format sizeof()? - *error_msg = apr_psprintf(msr->mp, "Input filter: Failed to allocate %lu bytes for request body chunk.", (unsigned long)sizeof(msc_data_chunk)); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:55:39:585 GMT-08:00 - 2008-01-11 :: 16:58:16:438 GMT-08:00 - - brian - brian - apache2/msc_reqbody.c - item.type.label.suggestion - item.severity.label.trivial - Portable way to format sizeof()? - *error_msg = apr_psprintf(msr->mp, "Failed to allocate %lu bytes for request body disk chunk.", (unsigned long)sizeof(msc_data_chunk)); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:56:05:302 GMT-08:00 - 2008-01-11 :: 16:57:58:049 GMT-08:00 - - brian - brian - apache2/msc_reqbody.c - item.type.label.suggestion - item.severity.label.trivial - Portable way to format sizeof()? - *error_msg = apr_psprintf(msr->mp, "Failed to allocate %lu bytes for request body disk chunk.", (unsigned long)sizeof(msc_data_chunk)); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:56:51:734 GMT-08:00 - 2008-01-11 :: 14:41:50:773 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - // TODO check whether the parameter is a valid MIME type of "null" - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:58:11:331 GMT-08:00 - 2008-01-11 :: 14:42:59:645 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - AP_INIT_TAKE1 ( - "SecAction", - cmd_action, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:58:29:461 GMT-08:00 - 2008-01-11 :: 14:46:15:813 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - AP_INIT_TAKE1 ( - "SecDataDir", - cmd_data_dir, - NULL, - CMD_SCOPE_MAIN, - "" // TODO - ), - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:58:45:647 GMT-08:00 - 2008-01-11 :: 14:46:51:173 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - AP_INIT_TAKE1 ( - "SecDefaultAction", - cmd_default_action, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:59:03:959 GMT-08:00 - 2008-01-11 :: 14:47:05:294 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - AP_INIT_TAKE1 ( - "SecResponseBodyLimit", - cmd_response_body_limit, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 12:59:17:580 GMT-08:00 - 2008-01-11 :: 14:49:00:349 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - AP_INIT_TAKE1 ( - "SecResponseBodyLimitAction", - cmd_response_body_limit_action, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:00:04:205 GMT-08:00 - 2008-01-11 :: 14:50:22:367 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - AP_INIT_TAKE23 ( - "SecRule", - cmd_rule, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:00:30:606 GMT-08:00 - 2008-01-11 :: 14:51:16:772 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - AP_INIT_TAKE12 ( - "SecRuleScript", - cmd_rule_script, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - AP_INIT_ITERATE ( - "SecRuleRemoveById", - cmd_rule_remove_by_id, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - AP_INIT_ITERATE ( - "SecRuleRemoveByMsg", - cmd_rule_remove_by_msg, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:00:56:319 GMT-08:00 - 2008-01-11 :: 14:52:48:192 GMT-08:00 - - brian - brian - apache2/apache2_config.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - AP_INIT_TAKE1 ( - "SecTmpDir", - cmd_tmp_dir, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - AP_INIT_TAKE1 ( - "SecUploadDir", - cmd_upload_dir, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - AP_INIT_TAKE1 ( - "SecUploadKeepFiles", - cmd_upload_keep_files, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - AP_INIT_TAKE1 ( - "SecWebAppId", - cmd_web_app_id, - NULL, - CMD_SCOPE_ANY, - "" // TODO - ), - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:01:32:996 GMT-08:00 - 2008-01-11 :: 14:54:32:209 GMT-08:00 - - brian - brian - apache2/mod_security2.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - /* Update the request headers. They might have changed after - * the body was read (trailers). - */ - // TODO We still need to keep a copy of the original headers - // to log in the audit log. - msr->request_headers = apr_table_copy(msr->mp, r->headers_in); - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:02:05:035 GMT-08:00 - 2008-01-09 :: 13:02:47:482 GMT-08:00 - - brian - brian - apache2/modsecurity.c - item.type.label.suggestion - item.severity.label.trivial - Yes, why do we ignore the rc - why have one at all? - // TODO: Why do we ignore return code here? - modsecurity_request_body_clear(msr, &my_error_msg); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:03:24:928 GMT-08:00 - 2008-01-09 :: 13:04:49:633 GMT-08:00 - - brian - brian - apache2/msc_geo.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO. - offset = -3; - apr_file_seek(geo->db, APR_END, &offset); - /* TODO check offset */ - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:04:20:937 GMT-08:00 - 2008-01-09 :: 13:04:39:295 GMT-08:00 - - brian - brian - apache2/msc_geo.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO. - rc = apr_file_read_full(geo->db, &buf, 1, &nbytes); - /* TODO: check rc */ - geo->dbtype = (int)buf[0]; - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:05:16:204 GMT-08:00 - 2008-01-09 :: 13:05:27:720 GMT-08:00 - - brian - brian - apache2/msc_geo.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO. - apr_file_seek(geo->db, APR_SET, &seekto); - /* TODO: check rc */ - rc = apr_file_read_full(geo->db, &buf, (2 * reclen), &nbytes); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:05:58:953 GMT-08:00 - 2008-01-09 :: 13:06:03:494 GMT-08:00 - - brian - brian - apache2/msc_geo.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO. - apr_file_seek(geo->db, APR_SET, &seekto); - /* TODO: check rc */ - rc = apr_file_read_full(geo->db, &cbuf, sizeof(cbuf), &nbytes); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:06:45:422 GMT-08:00 - 2008-01-11 :: 14:55:10:355 GMT-08:00 - - brian - brian - apache2/msc_logging.c - item.type.label.suggestion - item.severity.label.trivial - Fix TODO or change to ENH. - /* The audit log storage directory should be explicitly - * defined. But if it isn't try to write to the same - * directory where the index file is placed. Of course, - * it is *very* bad practice to allow the Apache user - * to write to the same directory where a root user is - * writing to but it's not us that's causing the problem - * and there isn't anything we can do about that. - * - * TODO Actually there is something we can do! We will make - * SecAuditStorageDir mandatory, ask the user to explicitly - * define the storage location *and* refuse to work if the - * index and the storage location are in the same folder. - */ - if (msr->txcfg->auditlog_storage_dir == NULL) { - entry_filename = file_dirname(msr->mp, msr->txcfg->auditlog_name); - } - else { - entry_filename = msr->txcfg->auditlog_storage_dir; - } - if (entry_filename == NULL) return; - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:07:22:398 GMT-08:00 - 2008-01-11 :: 14:55:42:896 GMT-08:00 - - brian - brian - apache2/msc_logging.c - item.type.label.suggestion - item.severity.label.trivial - Change TODO to ENH. - /* AUDITLOG_PART_UPLOADS */ - // TODO: Implement - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:08:02:613 GMT-08:00 - 2008-01-11 :: 15:51:28:546 GMT-08:00 - - brian - brian - apache2/msc_lua.c - item.type.label.missing - item.severity.label.trivial - Log an error. - } else { - // TODO Error - return NULL; - } - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:08:51:978 GMT-08:00 - 2008-01-09 :: 13:09:44:680 GMT-08:00 - - brian - brian - apache2/msc_util.c - item.type.label.suggestion - item.severity.label.normal - Fix TODO. - char *resolve_relative_path(apr_pool_t *pool, const char *parent_filename, const char *filename) { - if (filename == NULL) return NULL; - // TODO Support paths on operating systems other than Unix. - if (filename[0] == '/') return (char *)filename; - - return apr_pstrcat(pool, apr_pstrndup(pool, parent_filename, - strlen(parent_filename) - strlen(apr_filepath_name_get(parent_filename))), - filename, NULL); -} - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:10:22:786 GMT-08:00 - 2008-01-09 :: 13:10:31:982 GMT-08:00 - - brian - brian - apache2/pdf_protect.c - item.type.label.suggestion - item.severity.label.trivial - Change from TODO to ENH. - // TODO We need ID and REV values for the PDF XSS alert. - -// TODO It would be nice if the user could choose the ID/REV/SEVERITY/MESSAGE, etc. - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:10:53:181 GMT-08:00 - 2008-01-09 :: 13:11:06:931 GMT-08:00 - - brian - brian - apache2/pdf_protect.c - item.type.label.suggestion - item.severity.label.trivial - Change from TODO to ENH. - // TODO Should we look at err_headers_out too? - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:11:23:251 GMT-08:00 - 2008-01-09 :: 13:11:28:132 GMT-08:00 - - brian - brian - apache2/pdf_protect.c - item.type.label.suggestion - item.severity.label.trivial - Change from TODO to ENH. - // TODO application/x-pdf, application/vnd.fdf, application/vnd.adobe.xfdf, - // application/vnd.adobe.xdp+xml, application/vnd.adobe.xfd+xml, application/vnd.pdf - // application/acrobat, text/pdf, text/x-pdf ??? - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:11:54:942 GMT-08:00 - 2008-01-09 :: 13:12:24:617 GMT-08:00 - - brian - brian - apache2/pdf_protect.c - item.type.label.missing - item.severity.label.trivial - Add the missing alert. - // TODO Log alert - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:13:04:159 GMT-08:00 - 2008-01-09 :: 13:13:38:064 GMT-08:00 - - brian - brian - apache2/re_actions.c - item.type.label.suggestion - item.severity.label.trivial - Not positive why the TODO here. Perhaps for a decision as to log and/or at what level? - msr_log(msr, 4, "Ctl: Set auditEngine to %d.", msr->txcfg->auditlog_flag); // TODO - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:14:02:379 GMT-08:00 - 2008-01-11 :: 15:42:21:749 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.suggestion - item.severity.label.trivial - Change from TODO to ENH. - // TODO Write & use string_ends(s, e). - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:14:29:571 GMT-08:00 - 2008-01-11 :: 15:43:53:997 GMT-08:00 - - brian - brian - apache2/re_operators.c - item.type.label.suggestion - item.severity.label.trivial - Remove the ifdef as lua is required? - #ifdef WITH_LUA - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:17:47:759 GMT-08:00 - 2008-01-11 :: 15:41:12:538 GMT-08:00 - - brian - brian - CHANGES - item.type.label.suggestion - item.severity.label.trivial - Remove TODO. - TODO: more to come - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 13:35:00:891 GMT-08:00 - 2008-01-09 :: 13:35:40:975 GMT-08:00 - - brian - brian - apache2/modsecurity.h - item.type.label.optimization - item.severity.label.trivial - Need to re-test implementing this as just a table. - /* data cache */ - apr_hash_t *tcache; - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 13:37:04:264 GMT-08:00 - 2008-01-09 :: 13:37:25:196 GMT-08:00 - - brian - brian - apache2/modsecurity.h - item.type.label.suggestion - item.severity.label.trivial - Should probably use STRINGIFY and define the numeric value. - #define MODSEC_VERSION_MAJOR "2" -#define MODSEC_VERSION_MINOR "5" -#define MODSEC_VERSION_MAINT "0" -#define MODSEC_VERSION_TYPE "rc" -#define MODSEC_VERSION_RELEASE "1" - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 14:08:14:025 GMT-08:00 - 2008-01-11 :: 15:40:12:054 GMT-08:00 - - brian - brian - apache2/msc_logging.c - item.type.label.suggestion - item.severity.label.trivial - Spelling. - throught - - - item.resolution.label.validNeedsfixing - item.status.label.resolved - - - - 2008-01-09 :: 14:21:32:782 GMT-08:00 - 2008-01-09 :: 14:22:32:803 GMT-08:00 - - brian - brian - apache2/msc_geo.h - item.type.label.suggestion - item.severity.label.trivial - This value may not be portable based on endianness. The algorithm compares it to the IP address as int in host order. - #define GEO_COUNTRY_OFFSET 0xffff00 - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 14:31:25:720 GMT-08:00 - 2008-01-09 :: 14:45:20:612 GMT-08:00 - - brian - brian - apache2/msc_geo.c - item.type.label.suggestion - item.severity.label.trivial - Check portability due to endianness. It seems that the DB values are assumed to be in host order. Perhaps the compiled DB is little-endian and we need to compensate? - - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 15:28:14:563 GMT-08:00 - 2008-01-09 :: 15:28:24:203 GMT-08:00 - - brian - brian - apache2/msc_multipart.c - item.type.label.irrelevant - item.severity.label.trivial - Remove code? - #if 0 -static char *multipart_construct_filename(modsec_rec *msr) { - char c, *p, *q = msr->mpd->mpp->filename; - char *filename; - - /* find the last backward slash and consider the - * filename to be only what's right from it - */ - p = strrchr(q, '\\'); - if (p != NULL) q = p + 1; - - /* do the same for the forward slash */ - p = strrchr(q, '/'); - if (p != NULL) q = p + 1; - - /* allow letters, digits and dots, replace - * everything else with underscores - */ - p = filename = apr_pstrdup(msr->mp, q); - while((c = *p) != 0) { - if (!( isalnum(c)||(c == '.') )) *p = '_'; - p++; - } - - return filename; -} -#endif - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 15:35:50:978 GMT-08:00 - 2008-01-09 :: 15:49:54:128 GMT-08:00 - - brian - brian - apache2/msc_multipart.c - item.type.label.programLogic - item.severity.label.trivial - The multipart C-D header is case insensitive (rfc 2183), so we should probably use strncasecmp() here. - /* accept only what we understand */ - if (strncmp(c_d_value, "form-data", 9) != 0) { - return -1; - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 16:05:05:601 GMT-08:00 - 2008-01-09 :: 16:07:17:436 GMT-08:00 - - brian - brian - apache2/msc_multipart.c - item.type.label.suggestion - item.severity.label.trivial - This allows for a name of "", so maybe check for name[0] == '\0' and return an error. Although we catch this later on as an unrecognized name and return -10. - start = p; - while((*p != '\0')&&(*p != '=')&&(*p != '\t')&&(*p != ' ')) p++; - if (*p == '\0') return -4; - - name = apr_pstrmemdup(msr->mp, start, (p - start)); - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 16:14:36:307 GMT-08:00 - 2008-01-09 :: 17:02:47:324 GMT-08:00 - - brian - brian - apache2/msc_multipart.c - item.type.label.suggestion - item.severity.label.trivial - For a quoted value we just include everything until the end quote. The field values should be US-ASCII 'qtext' or 'quoted-pair' (RFC 822) or escaped using RFC 2047. - if (*p == '"') { - *t = '\0'; - break; - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 16:59:12:520 GMT-08:00 - 2008-01-09 :: 17:00:55:566 GMT-08:00 - - brian - brian - apache2/msc_multipart.c - item.type.label.suggestion - item.severity.label.trivial - I think this is wrong. RFC 822 defines a quoted-string as <"> *(qtext/quoted-pair) <"> and quoted-par being able to quote any CHAR. - /* only " and \ can be escaped */ - if ((*(p + 1) == '"')||(*(p + 1) == '\\')) { - p++; - } - else { - /* improper escaping */ - - /* We allow for now because IE sends - * improperly escaped content and there's - * nothing we can do about it. - * - * return -9; - */ - } - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-09 :: 17:04:30:357 GMT-08:00 - 2008-01-09 :: 17:05:19:369 GMT-08:00 - - brian - brian - apache2/msc_multipart.c - item.type.label.suggestion - item.severity.label.trivial - RFC 2045 defines an 'attribute' as "ALWAYS case-insensitive", so these should be strncasecmp() - if (strcmp(name, "name") == 0) { - if (msr->mpd->mpp->name != NULL) return -14; - msr->mpd->mpp->name = value; - - if (msr->txcfg->debuglog_level >= 9) { - msr_log(msr, 9, "Multipart: Content-Disposition name: %s", - log_escape_nq(msr->mp, value)); - } - } - else - if (strcmp(name, "filename") == 0) { - - - item.resolution.label.validNeedsfixing - item.status.label.open - - - - 2008-01-10 :: 12:24:59:317 GMT-08:00 - 2008-01-11 :: 16:58:44:275 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.resolved - - - - 2008-01-10 :: 12:27:34:472 GMT-08:00 - 2008-01-11 :: 15:39:17:001 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.resolved - - - - 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-11 :: 15:38:39:261 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.resolved - - - - 2008-01-10 :: 13:12:54:295 GMT-08:00 - 2008-01-11 :: 15:36:43:726 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.resolved - - - - 2008-01-10 :: 13:19:16:539 GMT-08:00 - 2008-01-11 :: 15:35:53:104 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.resolved - - - - 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-11 :: 16:51:13:950 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.resolved - - - - 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-11 :: 15:34:14:676 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.resolved - - - diff --git a/review/pre-2.5-brian.txt b/review/pre-2.5-brian.txt deleted file mode 100644 index 3f2f49f9..00000000 --- a/review/pre-2.5-brian.txt +++ /dev/null @@ -1,982 +0,0 @@ -File: apache2/apache2_config.c -=================================================================== -[brian] Irrelevant @ 1251 - -Remove code? - -/* -static const char *cmd_rule_import_by_id(cmd_parms *cmd, void *_dcfg, const char *p1) { - directory_config *dcfg = (directory_config *)_dcfg; - rule_exception *re = apr_pcalloc(cmd->pool, sizeof(rule_exception)); - if (dcfg == NULL) return NULL; - - re->type = RULE_EXCEPTION_IMPORT_ID; - // TODO verify p1 - re->param = p1; - *(rule_exception **)apr_array_push(dcfg->rule_exceptions) = re; - - return NULL; -} - -static const char *cmd_rule_import_by_msg(cmd_parms *cmd, void *_dcfg, const char *p1) { - directory_config *dcfg = (directory_config *)_dcfg; - rule_exception *re = apr_pcalloc(cmd->pool, sizeof(rule_exception)); - if (dcfg == NULL) return NULL; - - re->type = RULE_EXCEPTION_IMPORT_MSG; - // TODO verify p1 - re->param = p1; - *(rule_exception **)apr_array_push(dcfg->rule_exceptions) = re; - - return NULL; -} -*/ - - -[brian] Suggestion @ 1514 - -Could use strtol as Ivan has as well. - -intval = apr_atoi64(charval); - if (errno == ERANGE) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations minlen out of range: %s", charval); - } - if (intval < 0) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations minlen must be positive: %s", charval); - } - - -[brian] Suggestion @ 1522 - -Portable? - -/* The NOT_SET indicator is -1, a signed long, and therfore - * we cannot be >= the unsigned value of NOT_SET. - */ - if ((unsigned long)intval >= (unsigned long)NOT_SET) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations minlen must be less than: %lu", (unsigned long)NOT_SET); - } - - -[brian] Suggestion @ 1534 - -Could use strtol as Ivan has as well. - -intval = apr_atoi64(charval); - if (errno == ERANGE) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations maxlen out of range: %s", charval); - } - if (intval < 0) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations maxlen must be positive: %s", charval); - } - - -[brian] Suggestion @ 1542 - -Portable? - -/* The NOT_SET indicator is -1, a signed long, and therfore - * we cannot be >= the unsigned value of NOT_SET. - */ - if ((unsigned long)intval >= (unsigned long)NOT_SET) { - return apr_psprintf(cmd->pool, "ModSecurity: SecCacheTransformations maxlen must be less than: %lu", (unsigned long)NOT_SET); - } - - - -File: apache2/apache2_io.c -=================================================================== -[brian] ProgramLogic @ 17 - -Remove code? It is actually used below, so need to verify. - -#if 0 -static void dummy_free_func(void *data) {} -#endif - - -[brian] ProgramLogic @ 92 - -dummy_free_func() is defined where? It is ifdef'd out at the top of source, so need to verify it is valid. - -/* Do not make a copy of the data we received in the chunk. */ - bucket = apr_bucket_heap_create(chunk->data, chunk->length, dummy_free_func, - f->r->connection->bucket_alloc); - - -[brian] ProgramLogic @ 224 - -Returning here may fail to free chunks data due to modsecurity_request_body_end() not being called. - -int rcbs = modsecurity_request_body_store(msr, buf, buflen, error_msg); - if (rcbs < 0) { - if (rcbs == -5) { - *error_msg = apr_psprintf(msr->mp, "Requests body no files data length is larger than the " - "configured limit (%lu).", msr->txcfg->reqbody_no_files_limit); - return -5; - } - - return -1; - } - - -[brian] Suggestion @ 246 - -Yes, why do we ignore the rc - why have one at all? - -// TODO: Why ignore the return code here? - - - -File: apache2/mod_security2.c -=================================================================== -[brian] Suggestion @ 1074 - -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); - */ - - - -File: apache2/modsecurity.c -=================================================================== -[brian] Irrelevant @ 100 - -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 - - -[brian] Suggestion @ 126 - -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"); - } - - -[brian] Suggestion @ 132 - -This does nothing. - -/** - * Releases resources held by engine instance. - */ -void modsecurity_shutdown(msc_engine *msce) { - if (msce == NULL) return; -} - - -[brian] Suggestion @ 178 - -Yes, why do we ignore the rc - why have one at all? - -// TODO: Why do we ignore return code here? - modsecurity_request_body_clear(msr, &my_error_msg); - - -[brian] Suggestion @ 196 - -Good. This looks to solve the other issues noted as possible memory leaks in body chunk data due to modsecurity_request_body_end() not being called. Need to verify, though. - -/* Register TX cleanup */ - apr_pool_cleanup_register(msr->mp, msr, modsecurity_tx_cleanup, apr_pool_cleanup_null); - - -[brian] Suggestion @ 298 - -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; - - - -File: apache2/modsecurity.h -=================================================================== -[brian] Suggestion @ 63 - -Should probably use STRINGIFY and define the numeric value. - -#define MODSEC_VERSION_MAJOR "2" -#define MODSEC_VERSION_MINOR "5" -#define MODSEC_VERSION_MAINT "0" -#define MODSEC_VERSION_TYPE "rc" -#define MODSEC_VERSION_RELEASE "1" - - -[brian] Optimization @ 363 - -Need to re-test implementing this as just a table. - -/* data cache */ - apr_hash_t *tcache; - - - -File: apache2/msc_geo.c -=================================================================== -[brian] Suggestion - -Check portability due to endianness. It seems that the DB values are assumed to be in host order. Perhaps the compiled DB is little-endian and we need to compensate? - - -[brian] Suggestion @ 153 - -Fix TODO. - -offset = -3; - apr_file_seek(geo->db, APR_END, &offset); - /* TODO check offset */ - - -[brian] Suggestion @ 176 - -Fix TODO. - -rc = apr_file_read_full(geo->db, &buf, 1, &nbytes); - /* TODO: check rc */ - geo->dbtype = (int)buf[0]; - - -[brian] Suggestion @ 325 - -Fix TODO. - -apr_file_seek(geo->db, APR_SET, &seekto); - /* TODO: check rc */ - rc = apr_file_read_full(geo->db, &buf, (2 * reclen), &nbytes); - - -[brian] Suggestion @ 374 - -Fix TODO. - -apr_file_seek(geo->db, APR_SET, &seekto); - /* TODO: check rc */ - rc = apr_file_read_full(geo->db, &cbuf, sizeof(cbuf), &nbytes); - - - -File: apache2/msc_geo.h -=================================================================== -[brian] Suggestion @ 16 - -This value may not be portable based on endianness. The algorithm compares it to the IP address as int in host order. - -#define GEO_COUNTRY_OFFSET 0xffff00 - - - -File: apache2/msc_logging.c -=================================================================== -[brian] ProgramLogic @ 235 - -This allows an empty string as a valid part. This misvalidates "ctl:auditLogParts=+", etc. - -is_valid_parts_specification - - -[brian] Suggestion @ 432 - -apr_dir_make_recursive will attempt to create the dir straight away and if that fails keep backing off a dir until it can start creating, so I see no need to cache. Besides, what happens if you cache, then someone deletes the path from outside apache? - -/* IMP1 Surely it would be more efficient to check the folders for - * the audit log repository base path in the configuration phase, to reduce - * the work we do on every request. Also, since our path depends on time, - * we could cache the time we last checked and don't check if we know - * the folder is there. - */ - rc = apr_dir_make_recursive(entry_basename, CREATEMODE_DIR, msr->mp); - - - -File: apache2/msc_multipart.c -=================================================================== -[brian] Irrelevant @ 18 - -Remove code? - -#if 0 -static char *multipart_construct_filename(modsec_rec *msr) { - char c, *p, *q = msr->mpd->mpp->filename; - char *filename; - - /* find the last backward slash and consider the - * filename to be only what's right from it - */ - p = strrchr(q, '\\'); - if (p != NULL) q = p + 1; - - /* do the same for the forward slash */ - p = strrchr(q, '/'); - if (p != NULL) q = p + 1; - - /* allow letters, digits and dots, replace - * everything else with underscores - */ - p = filename = apr_pstrdup(msr->mp, q); - while((c = *p) != 0) { - if (!( isalnum(c)||(c == '.') )) *p = '_'; - p++; - } - - return filename; -} -#endif - - -[brian] ProgramLogic @ 52 - -The multipart C-D header is case insensitive (rfc 2183), so we should probably use strncasecmp() here. - -/* accept only what we understand */ - if (strncmp(c_d_value, "form-data", 9) != 0) { - return -1; - } - - -[brian] Suggestion @ 75 - -This allows for a name of "", so maybe check for name[0] == '\0' and return an error. Although we catch this later on as an unrecognized name and return -10. - -start = p; - while((*p != '\0')&&(*p != '=')&&(*p != '\t')&&(*p != ' ')) p++; - if (*p == '\0') return -4; - - name = apr_pstrmemdup(msr->mp, start, (p - start)); - - -[brian] Suggestion @ 106 - -I think this is wrong. RFC 822 defines a quoted-string as <"> *(qtext/quoted-pair) <"> and quoted-par being able to quote any CHAR. - -/* only " and \ can be escaped */ - if ((*(p + 1) == '"')||(*(p + 1) == '\\')) { - p++; - } - else { - /* improper escaping */ - - /* We allow for now because IE sends - * improperly escaped content and there's - * nothing we can do about it. - * - * return -9; - */ - } - - -[brian] Suggestion @ 122 - -For a quoted value we just include everything until the end quote. The field values should be US-ASCII 'qtext' or 'quoted-pair' (RFC 822) or escaped using RFC 2047. - -if (*p == '"') { - *t = '\0'; - break; - } - - -[brian] Suggestion @ 143 - -RFC 2045 defines an 'attribute' as "ALWAYS case-insensitive", so these should be strncasecmp() - -if (strcmp(name, "name") == 0) { - if (msr->mpd->mpp->name != NULL) return -14; - msr->mpd->mpp->name = value; - - if (msr->txcfg->debuglog_level >= 9) { - msr_log(msr, 9, "Multipart: Content-Disposition name: %s", - log_escape_nq(msr->mp, value)); - } - } - else - if (strcmp(name, "filename") == 0) { - - -[brian] Suggestion @ 722 - -Cannot find anything that supports that this is or is not allowed. - -/* Flag for whitespace after parameter name. */ - - -[brian] Suggestion @ 735 - -Cannot find anything that supports that this is or is not allowed. - -/* Flag for whitespace before parameter value. */ - - - -File: apache2/msc_util.c -=================================================================== -[brian] Suggestion @ 24 - -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')) - - -[brian] Suggestion @ 1186 - -Fix TODO. - -char *resolve_relative_path(apr_pool_t *pool, const char *parent_filename, const char *filename) { - if (filename == NULL) return NULL; - // TODO Support paths on operating systems other than Unix. - if (filename[0] == '/') return (char *)filename; - - return apr_pstrcat(pool, apr_pstrndup(pool, parent_filename, - strlen(parent_filename) - strlen(apr_filepath_name_get(parent_filename))), - filename, NULL); -} - - - -File: apache2/msc_xml.c -=================================================================== -[brian] Suggestion @ 27 - -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 - - - -File: apache2/pdf_protect.c -=================================================================== -[brian] Suggestion @ 26 - -Change from TODO to ENH. - -// TODO We need ID and REV values for the PDF XSS alert. - -// TODO It would be nice if the user could choose the ID/REV/SEVERITY/MESSAGE, etc. - - -[brian] Suggestion @ 217 - -Change from TODO to ENH. - -// TODO Should we look at err_headers_out too? - - -[brian] Suggestion @ 244 - -Change from TODO to ENH. - -// TODO application/x-pdf, application/vnd.fdf, application/vnd.adobe.xfdf, - // application/vnd.adobe.xdp+xml, application/vnd.adobe.xfd+xml, application/vnd.pdf - // application/acrobat, text/pdf, text/x-pdf ??? - - -[brian] Missing @ 472 - -Add the missing alert. - -// TODO Log alert - - - -File: apache2/re.c -=================================================================== -[brian] Missing @ 47 - -Need to log on failure. - -var = msre_create_var(ruleset, telts[i].key, telts[i].val, NULL, error_msg); - if (var == NULL) return -1; - - -[brian] Suggestion @ 297 - -Should replace with isvarnamechar() if possible. - -while((*p != '\0')&&(*p != '|')&&(*p != ':')&&(*p != ',')&&(!isspace(*p))) p++; /* ENH replace with isvarnamechar() */ - - -[brian] Irrelevant @ 608 - -Does not appear to be used anywhere. - -/** - * Destroys an engine instance, releasing the consumed memory. - */ -void msre_engine_destroy(msre_engine *engine) { - /* Destroyed automatically by the parent pool. - * apr_pool_destroy(engine->mp); - */ -} - - -[brian] Optimization @ 1150 - -tags set to NULL would be a bit better as it would stop apr_pstrcat() earlier, but tags *must* remain last or wierd results. - -char *tags = ""; - - -[brian] Suggestion @ 1179 - -Implement TODO. - -//TODO: restrict to 512 bytes - - -[brian] Optimization @ 1528 - -This causes two loops through the action list. Perhaps there is a more performant way to do these at the same time? Maybe split into two lists? - -/* Perform non-disruptive actions. */ - msre_perform_nondisruptive_actions(msr, rule, rule->actionset, mptmp); - - /* Perform disruptive actions, but only if - * this rule is not part of a chain. - */ - if (rule->actionset->is_chained == 0) { - msre_perform_disruptive_actions(msr, rule, acting_actionset, mptmp, my_error_msg); - } - - - -File: apache2/re.h -=================================================================== -[brian] Irrelevant @ 86 - -Does not appear to be used anywhere. - -void DSOLOCAL msre_engine_destroy(msre_engine *engine); - - -[brian] Suggestion @ 148 - -Why not stored in op_param_data like @rx, etc. The param_data is used w/exec action for lua. - -/* Compiled Lua script. */ - msc_script *script; - - - -File: apache2/re_actions.c -=================================================================== -[brian] Optimization @ 170 - -This implementation comment needs to be coded as many string operators now attempt to resolve macros. - -/* IMP1 Duplicate the string and create the array on - * demand, thus not having to do it if there are - * no macros in the input data. - */ - - data = apr_pstrdup(mptmp, var->value); /* IMP1 Are we modifying data anywhere? */ - arr = apr_array_make(mptmp, 16, sizeof(msc_string *)); - if ((data == NULL)||(arr == NULL)) return -1; - - -[brian] Irrelevant @ 209 - -This #if 0'd out code should be removed. - -/* Removed %0-9 macros as it messes up urlEncoding in the match - * where having '%0a' will be treated as %{TX.0}a, which is incorrect. - * */ -#if 0 - else if ((*(p + 1) >= '0')&&(*(p + 1) <= '9')) { - /* Special case for regex captures. */ - var_name = "TX"; - var_value = apr_pstrmemdup(mptmp, p + 1, 1); - next_text_start = p + 2; - } -#endif - - -[brian] Suggestion @ 257 - -Should log a level 9 msg here. - -} else { - /* We could not identify a valid macro so add it as text. */ - part = (msc_string *)apr_pcalloc(mptmp, sizeof(msc_string)); - if (part == NULL) return -1; - part->value_len = p - text_start + 1; /* len(text)+len("%") */ - part->value = apr_pstrmemdup(mptmp, text_start, part->value_len); - *(msc_string **)apr_array_push(arr) = part; - - next_text_start = p + 1; - } - - -[brian] Optimization @ 276 - -Use apr_array_pstrcat(msr->mp, arr, NULL) instead? - -/* If there's more than one member of the array that - * means there was at least one macro present. Combine - * text parts into a single string now. - */ - if (arr->nelts > 1) { - /* Figure out the required size for the string. */ - var->value_len = 0; - for(i = 0; i < arr->nelts; i++) { - part = ((msc_string **)arr->elts)[i]; - var->value_len += part->value_len; - } - - /* Allocate the string. */ - var->value = apr_palloc(msr->mp, var->value_len + 1); - if (var->value == NULL) return -1; - - /* Combine the parts. */ - offset = 0; - for(i = 0; i < arr->nelts; i++) { - part = ((msc_string **)arr->elts)[i]; - memcpy((char *)(var->value + offset), part->value, part->value_len); - offset += part->value_len; - } - var->value[offset] = '\0'; - } - - -[brian] Missing @ 402 - -Implement. Need to check if Apache will return an invalid status code - -/* status */ -static char *msre_action_status_validate(msre_engine *engine, msre_action *action) { - /* ENH action->param must be a valid HTTP status code. */ - return NULL; -} - - -[brian] Missing @ 422 - -Implement. - -/* pause */ -static char *msre_action_pause_validate(msre_engine *engine, msre_action *action) { - /* ENH Validate a positive number. */ - return NULL; -} - - -[brian] Missing @ 434 - -Implement as a valid URI check with apr_uri_parse()? - -/* redirect */ - -static char *msre_action_redirect_validate(msre_engine *engine, msre_action *action) { - /* ENH Add validation. */ - return NULL; -} - - -[brian] Missing @ 465 - -Implement as a valid URI check with apr_uri_parse()? - -/* proxy */ - -static char *msre_action_proxy_validate(msre_engine *engine, msre_action *action) { - /* ENH Add validation. */ - return NULL; -} - - -[brian] Irrelevant @ 507 - -I do not see a need to validate beyound what is already done in the init function. - -msre_action_skip_validate -msre_action_skipAfter_validate - - -[brian] Missing @ 570 - -Implement. - -/* phase */ - -static char *msre_action_phase_validate(msre_engine *engine, msre_action *action) { - /* ENH Add validation. */ - return NULL; -} - - -[brian] Suggestion @ 612 - -Probably should also calc length and validate a length > 0 instead of just checking NULL. Other checks would benefit from checking a length as well, so no harm in calculating that. - -if (value == NULL) { - return apr_psprintf(engine->mp, "Missing ctl value for name: %s", name); - } - - -[brian] Irrelevant @ 708 - -Why register init() if we do not use it? - -static apr_status_t msre_action_ctl_init(msre_engine *engine, msre_actionset *actionset, - msre_action *action) -{ - /* Do nothing. */ - return 1; -} - - -[brian] Optimization @ 774 - -TODO needs looked into. - -if (strcasecmp(name, "auditEngine") == 0) { - if (strcasecmp(value, "on") == 0) { - msr->txcfg->auditlog_flag = AUDITLOG_ON; - msr->usercfg->auditlog_flag = AUDITLOG_ON; - } - - if (strcasecmp(value, "off") == 0) { - msr->txcfg->auditlog_flag = AUDITLOG_OFF; - msr->usercfg->auditlog_flag = AUDITLOG_OFF; - } - - if (strcasecmp(value, "relevantonly") == 0) { - msr->txcfg->auditlog_flag = AUDITLOG_RELEVANT; - msr->usercfg->auditlog_flag = AUDITLOG_RELEVANT; - } - - msr_log(msr, 4, "Ctl: Set auditEngine to %d.", msr->txcfg->auditlog_flag); // TODO - - return 1; - } else - - -[brian] Suggestion @ 790 - -Not positive why the TODO here. Perhaps for a decision as to log and/or at what level? - -msr_log(msr, 4, "Ctl: Set auditEngine to %d.", msr->txcfg->auditlog_flag); // TODO - - -[brian] Missing @ 855 - -Should log an internal error here. - -else { - /* ENH Should never happen, but log if it does. */ - return -1; - } - - -[brian] Suggestion @ 1152 - -Probably should use apr_strtoi64 where we can tell if there was an error in conversion since we are potentially taking a value from a macro expansion. Also may want to look for overflow. - -value += atoi(var_value); - - -[brian] Suggestion @ 1288 - -Not sure why we would not want to deprecate a TX var. Further rules could use this even if TX is not persisted. - -/* IMP1 Add message TX variables cannot deprecate in value. */ - - -[brian] Suggestion @ 1383 - -The timeout is hardcoded to 3600. The docs state TIMEOUT is read-only, but this is not true. So, you can modify TIMEOUT. - -/* IMP1 Is the timeout hard-coded to 3600? */ - - -[brian] Suggestion @ 1555 - -We already have support for relative filenames, but cannot get to this data from here. This needs solved by passing more data to the validate function (cmd_parms rec). Maybe need a warning here stating we do not support them yet, or it might be confusing to users that we do not here but do elsewhere. - -/* TODO Support relative filenames. */ - - -[brian] Suggestion @ 1557 - -Not sure using an extension is a good idea here. Better I think would be to specify a type: "exec:[type=]/path/to/file" as in "exec:lua=/path/to/script" and make param_data a script_rec with a type and value. Also we use the abstract param_data here vs using a specific field as in SecRuleScript. - -/* Process Lua scripts internally. */ - if (strlen(filename) > 4) { - char *p = filename + strlen(filename) - 4; - if ((p[0] == '.')&&(p[1] == 'l')&&(p[2] == 'u')&&(p[3] == 'a')) { - /* It's a Lua script. */ - msc_script *script = NULL; - - /* Compile script. */ - char *msg = lua_compile(&script, filename, engine->mp); - if (msg != NULL) return msg; - - action->param_data = script; - } - } - - -[brian] Suggestion @ 1578 - -This assumes lua is the only type (which it is now), but should be re-writen with a script_rec stored in param_data. - -if (action->param_data != NULL) { /* Lua */ - msc_script *script = (msc_script *)action->param_data; - char *my_error_msg = NULL; - - if (lua_execute(script, NULL, msr, rule, &my_error_msg) < 0) { - msr_log(msr, 1, "%s", my_error_msg); - return 0; - } - } else { /* Execute as shell script. */ - - - -File: apache2/re_operators.c -=================================================================== -[brian] Missing - -Need more unit tests for operators. Start with new operators. - - -[brian] Suggestion @ 246 - -Use resolve_relative_path() instead? Maybe a config_relative_path() to just get the path? - -/* Get the path of the rule filename to use as a base */ - rulefile_path = apr_pstrndup(rule->ruleset->mp, rule->filename, strlen(rule->filename) - strlen(apr_filepath_name_get(rule->filename))); - - -[brian] Missing @ 310 - -Need to check return code and log an error on failure. - -acmp_add_pattern(p, buf, NULL, NULL, strlen(buf)); - - -[brian] Missing @ 315 - -Need to check return code and log an error on failure. - -acmp_prepare(p); - - -[brian] Optimization @ 379 - -See if apr_strmatch is faster. - -msre_op_within_execute - - -[brian] Optimization @ 442 - -See if apr_strmatch is faster. - -msre_op_contains_execute - - -[brian] Optimization @ 506 - -See if apr_strmatch is faster. - -msre_op_containsWord_execute - - -[brian] Missing @ 1330 - -Need an error_msg set for lua execution error. - -rc = lua_execute(script, target, msr, rule, error_msg); - if (rc < 0) { - /* Error. */ - return -1; - } - - -[brian] Missing @ 1477 - -@validateurlEncoding does not output VAR name nor offset in error_msg on match. - -msre_op_validateUrlEncoding_execute - - -[brian] Missing @ 1885 - -@m operator is not documented. This does the same as @contains, so it was suggested earlier to use the @m algorithm for contains (if faster) and drop @m. - -/* m */ - msre_engine_op_register(engine, - "m", - msre_op_m_param_init, - msre_op_m_execute - ); - - - -File: apache2/re_tfns.c -=================================================================== -[brian] Optimization @ 77 - -No need to set this on all. Only set it once when we find the first non-space char. - -(*rval)[i] = '\0'; - - - diff --git a/review/review-summary.pl b/review/review-summary.pl deleted file mode 100755 index 799f1672..00000000 --- a/review/review-summary.pl +++ /dev/null @@ -1,50 +0,0 @@ -#!/usr/bin/perl -use strict; -use XML::Simple; -#use Data::Dumper; - -my $REVIEW = shift @ARGV; -my %ISSUES = (); - -unless (defined $REVIEW) { - print STDERR "Usage: $0 [ ... ]\n"; - exit 1; -} - -# XML => hashref -my $review = XMLin( - $REVIEW, - KeepRoot => 0, - KeyAttr => { ReviewIssue => "+id" }, - ContentKey => "value", - SuppressEmpty => undef, -); -#print Dumper($review); - -# Reorg hashref to be only open issues by filename -for my $rec (values %{$review->{ReviewIssue} || {}}) { - my $key = defined($rec->{File}->{value}) ? $rec->{File}->{value} : ""; - push @{$ISSUES{$key}}, $rec if ($rec->{Status} =~ m/\.open$/); -} - - -# Write report -for my $fn (@ARGV ? (@ARGV) : (sort keys %ISSUES)) { - print "File: $fn\n"; - print "===================================================================\n"; - for my $r (sort { $a->{File}->{line} <=> $b->{File}->{line} || $a->{ReviewerId} cmp $b->{ReviewerId} } @{$ISSUES{$fn} || []}) { - (my $type = $r->{Type}) =~ s/^.*\.([^\.]+)$/$1/; - $type = ucfirst($type); - (my $res = $r->{Resolution}) =~ s/^.*\.([^\.]+)$/$1/; - my $line = ($r->{File}->{line} and $r->{File}->{line} > 1) ? " @ $r->{File}->{line}" : ""; - my $summary = $r->{Summary} ? "\n$r->{Summary}\n\n" : ""; - my $desc = $r->{Description} ? "$r->{Description}\n\n" : ""; - - print << "EOT"; -[$r->{ReviewerId}] $type$line -$summary$desc -EOT - } - print "\n"; -} -