mirror of
https://github.com/owasp-modsecurity/ModSecurity.git
synced 2025-08-13 21:36:00 +03:00
Merge 2.5 changes.
This commit is contained in:
b1v1r
parent
a06d8f8ce7
commit
fa96c349e5
1192
.cdtproject
1192
.cdtproject
File diff suppressed because it is too large
Load Diff
200
.jupiter
200
.jupiter
@ -1,200 +0,0 @@
|
||||
<?xml version="1.0" encoding="UTF-8"?>
|
||||
<Property>
|
||||
<Review id="DEFAULT">
|
||||
<Description>property.default.description</Description>
|
||||
<Author />
|
||||
<CreationDate format="yyyy-MM-dd :: HH:mm:ss:SSS z">1970-01-01 :: 00:00:00:000 GMT-10:00</CreationDate>
|
||||
<Directory>review</Directory>
|
||||
<Reviewers />
|
||||
<Files />
|
||||
<FieldItems>
|
||||
<FieldItem id="Type" default="item.label.unset">
|
||||
<Entry name="item.label.unset" />
|
||||
<Entry name="item.type.label.codingStandards" />
|
||||
<Entry name="item.type.label.programLogic" />
|
||||
<Entry name="item.type.label.optimization" />
|
||||
<Entry name="item.type.label.usability" />
|
||||
<Entry name="item.type.label.clarity" />
|
||||
<Entry name="item.type.label.missing" />
|
||||
<Entry name="item.type.label.irrelevant" />
|
||||
<Entry name="item.type.label.suggestion" />
|
||||
<Entry name="item.type.label.other" />
|
||||
</FieldItem>
|
||||
<FieldItem id="Severity" default="item.label.unset">
|
||||
<Entry name="item.label.unset" />
|
||||
<Entry name="item.severity.label.critical" />
|
||||
<Entry name="item.severity.label.major" />
|
||||
<Entry name="item.severity.label.normal" />
|
||||
<Entry name="item.severity.label.minor" />
|
||||
<Entry name="item.severity.label.trivial" />
|
||||
</FieldItem>
|
||||
<FieldItem id="Resolution" default="item.label.unset">
|
||||
<Entry name="item.label.unset" />
|
||||
<Entry name="item.resolution.label.validNeedsfixing" />
|
||||
<Entry name="item.resolution.label.validFixlater" />
|
||||
<Entry name="item.resolution.label.validDuplicate" />
|
||||
<Entry name="item.resolution.label.validWontfix" />
|
||||
<Entry name="item.resolution.label.invalidWontfix" />
|
||||
<Entry name="item.resolution.label.unsureValidity" />
|
||||
</FieldItem>
|
||||
<FieldItem id="Status" default="item.status.label.open">
|
||||
<Entry name="item.status.label.open" />
|
||||
<Entry name="item.status.label.resolved" />
|
||||
<Entry name="item.status.label.closed" />
|
||||
<Entry name="item.status.label.reopened" />
|
||||
</FieldItem>
|
||||
</FieldItems>
|
||||
<Filters>
|
||||
<Phase name="phase.individual" enabled="true">
|
||||
<Filter name="Interval" value="7" enabled="false" />
|
||||
<Filter name="Reviewer" value="item.reviewer.label.automatic" enabled="true" />
|
||||
<Filter name="Type" value="item.label.unset" enabled="false" />
|
||||
<Filter name="Severity" value="item.label.unset" enabled="false" />
|
||||
<Filter name="AssignedTo" value="" enabled="false" />
|
||||
<Filter name="Resolution" value="item.label.unset" enabled="false" />
|
||||
<Filter name="Status" value="item.status.label.open" enabled="false" />
|
||||
<Filter name="File" value="" enabled="false" />
|
||||
</Phase>
|
||||
<Phase name="phase.team" enabled="true">
|
||||
<Filter name="Interval" value="7" enabled="false" />
|
||||
<Filter name="Reviewer" value="" enabled="false" />
|
||||
<Filter name="Type" value="item.label.unset" enabled="false" />
|
||||
<Filter name="Severity" value="item.label.unset" enabled="false" />
|
||||
<Filter name="AssignedTo" value="" enabled="false" />
|
||||
<Filter name="Resolution" value="item.label.unset" enabled="true" />
|
||||
<Filter name="Status" value="item.status.label.open" enabled="false" />
|
||||
<Filter name="File" value="" enabled="false" />
|
||||
</Phase>
|
||||
<Phase name="phase.rework" enabled="true">
|
||||
<Filter name="Interval" value="7" enabled="false" />
|
||||
<Filter name="Reviewer" value="" enabled="false" />
|
||||
<Filter name="Type" value="item.label.unset" enabled="false" />
|
||||
<Filter name="Severity" value="item.label.unset" enabled="false" />
|
||||
<Filter name="AssignedTo" value="item.reviewer.label.automatic" enabled="true" />
|
||||
<Filter name="Resolution" value="item.label.unset" enabled="false" />
|
||||
<Filter name="Status" value="item.status.label.open" enabled="true" />
|
||||
<Filter name="File" value="" enabled="false" />
|
||||
</Phase>
|
||||
</Filters>
|
||||
</Review>
|
||||
<Review id="pre-2.5">
|
||||
<Description>Pre 2.5 Review</Description>
|
||||
<Author>brian</Author>
|
||||
<CreationDate format="yyyy-MM-dd :: HH:mm:ss:SSS z">2008-01-04 :: 10:09:23:000 GMT-08:00</CreationDate>
|
||||
<Directory>review</Directory>
|
||||
<Reviewers>
|
||||
<Entry id="brian" name="brian" />
|
||||
<Entry id="ivan" name="ivan" />
|
||||
</Reviewers>
|
||||
<Files>
|
||||
<Entry name="apache2/acmp.c" />
|
||||
<Entry name="apache2/acmp.h" />
|
||||
<Entry name="apache2/apache2.h" />
|
||||
<Entry name="apache2/apache2_config.c" />
|
||||
<Entry name="apache2/apache2_io.c" />
|
||||
<Entry name="apache2/apache2_util.c" />
|
||||
<Entry name="apache2/modsecurity.c" />
|
||||
<Entry name="apache2/modsecurity.h" />
|
||||
<Entry name="apache2/mod_security2.c" />
|
||||
<Entry name="apache2/msc_geo.c" />
|
||||
<Entry name="apache2/msc_geo.h" />
|
||||
<Entry name="apache2/msc_logging.c" />
|
||||
<Entry name="apache2/msc_logging.h" />
|
||||
<Entry name="apache2/msc_lua.c" />
|
||||
<Entry name="apache2/msc_lua.h" />
|
||||
<Entry name="apache2/msc_multipart.c" />
|
||||
<Entry name="apache2/msc_multipart.h" />
|
||||
<Entry name="apache2/msc_parsers.c" />
|
||||
<Entry name="apache2/msc_parsers.h" />
|
||||
<Entry name="apache2/msc_pcre.c" />
|
||||
<Entry name="apache2/msc_pcre.h" />
|
||||
<Entry name="apache2/msc_reqbody.c" />
|
||||
<Entry name="apache2/msc_test.c" />
|
||||
<Entry name="apache2/msc_util.c" />
|
||||
<Entry name="apache2/msc_util.h" />
|
||||
<Entry name="apache2/msc_xml.c" />
|
||||
<Entry name="apache2/msc_xml.h" />
|
||||
<Entry name="apache2/pdf_protect.c" />
|
||||
<Entry name="apache2/pdf_protect.h" />
|
||||
<Entry name="apache2/persist_dbm.c" />
|
||||
<Entry name="apache2/persist_dbm.h" />
|
||||
<Entry name="apache2/re.c" />
|
||||
<Entry name="apache2/re.h" />
|
||||
<Entry name="apache2/re_actions.c" />
|
||||
<Entry name="apache2/re_operators.c" />
|
||||
<Entry name="apache2/re_tfns.c" />
|
||||
<Entry name="apache2/re_variables.c" />
|
||||
<Entry name="apache2/utf8tables.h" />
|
||||
</Files>
|
||||
<FieldItems>
|
||||
<FieldItem id="Type" default="item.type.label.suggestion">
|
||||
<Entry name="item.label.unset" />
|
||||
<Entry name="item.type.label.codingStandards" />
|
||||
<Entry name="item.type.label.programLogic" />
|
||||
<Entry name="item.type.label.optimization" />
|
||||
<Entry name="item.type.label.usability" />
|
||||
<Entry name="item.type.label.clarity" />
|
||||
<Entry name="item.type.label.missing" />
|
||||
<Entry name="item.type.label.irrelevant" />
|
||||
<Entry name="item.type.label.suggestion" />
|
||||
<Entry name="item.type.label.other" />
|
||||
</FieldItem>
|
||||
<FieldItem id="Severity" default="item.severity.label.trivial">
|
||||
<Entry name="item.label.unset" />
|
||||
<Entry name="item.severity.label.critical" />
|
||||
<Entry name="item.severity.label.major" />
|
||||
<Entry name="item.severity.label.normal" />
|
||||
<Entry name="item.severity.label.minor" />
|
||||
<Entry name="item.severity.label.trivial" />
|
||||
</FieldItem>
|
||||
<FieldItem id="Resolution" default="item.resolution.label.validNeedsfixing">
|
||||
<Entry name="item.label.unset" />
|
||||
<Entry name="item.resolution.label.validNeedsfixing" />
|
||||
<Entry name="item.resolution.label.validFixlater" />
|
||||
<Entry name="item.resolution.label.validDuplicate" />
|
||||
<Entry name="item.resolution.label.validWontfix" />
|
||||
<Entry name="item.resolution.label.invalidWontfix" />
|
||||
<Entry name="item.resolution.label.unsureValidity" />
|
||||
</FieldItem>
|
||||
<FieldItem id="Status" default="item.status.label.open">
|
||||
<Entry name="item.status.label.open" />
|
||||
<Entry name="item.status.label.resolved" />
|
||||
<Entry name="item.status.label.closed" />
|
||||
<Entry name="item.status.label.reopened" />
|
||||
</FieldItem>
|
||||
</FieldItems>
|
||||
<Filters>
|
||||
<Phase name="phase.individual" enabled="true">
|
||||
<Filter name="Interval" value="7" enabled="false" />
|
||||
<Filter name="Reviewer" value="item.reviewer.label.automatic" enabled="true" />
|
||||
<Filter name="Type" value="item.label.unset" enabled="false" />
|
||||
<Filter name="Severity" value="item.label.unset" enabled="false" />
|
||||
<Filter name="AssignedTo" value="" enabled="false" />
|
||||
<Filter name="Resolution" value="item.label.unset" enabled="false" />
|
||||
<Filter name="Status" value="item.status.label.open" enabled="false" />
|
||||
<Filter name="File" value="" enabled="false" />
|
||||
</Phase>
|
||||
<Phase name="phase.team" enabled="true">
|
||||
<Filter name="Interval" value="7" enabled="false" />
|
||||
<Filter name="Reviewer" value="" enabled="false" />
|
||||
<Filter name="Type" value="item.label.unset" enabled="false" />
|
||||
<Filter name="Severity" value="item.label.unset" enabled="false" />
|
||||
<Filter name="AssignedTo" value="" enabled="false" />
|
||||
<Filter name="Resolution" value="item.label.unset" enabled="true" />
|
||||
<Filter name="Status" value="item.status.label.open" enabled="false" />
|
||||
<Filter name="File" value="" enabled="false" />
|
||||
</Phase>
|
||||
<Phase name="phase.rework" enabled="true">
|
||||
<Filter name="Interval" value="7" enabled="false" />
|
||||
<Filter name="Reviewer" value="" enabled="false" />
|
||||
<Filter name="Type" value="item.label.unset" enabled="false" />
|
||||
<Filter name="Severity" value="item.label.unset" enabled="false" />
|
||||
<Filter name="AssignedTo" value="item.reviewer.label.automatic" enabled="true" />
|
||||
<Filter name="Resolution" value="item.label.unset" enabled="false" />
|
||||
<Filter name="Status" value="item.status.label.open" enabled="true" />
|
||||
<Filter name="File" value="" enabled="false" />
|
||||
</Phase>
|
||||
</Filters>
|
||||
</Review>
|
||||
</Property>
|
||||
|
||||
85
.project
85
.project
@ -1,85 +0,0 @@
|
||||
<?xml version="1.0" encoding="UTF-8"?>
|
||||
<projectDescription>
|
||||
<name>ModSecurity</name>
|
||||
<comment></comment>
|
||||
<projects>
|
||||
</projects>
|
||||
<buildSpec>
|
||||
<buildCommand>
|
||||
<name>org.eclipse.cdt.make.core.makeBuilder</name>
|
||||
<triggers>clean,full,incremental,</triggers>
|
||||
<arguments>
|
||||
<dictionary>
|
||||
<key>org.eclipse.cdt.make.core.append_environment</key>
|
||||
<value>true</value>
|
||||
</dictionary>
|
||||
<dictionary>
|
||||
<key>org.eclipse.cdt.make.core.enableCleanBuild</key>
|
||||
<value>true</value>
|
||||
</dictionary>
|
||||
<dictionary>
|
||||
<key>org.eclipse.cdt.make.core.build.command</key>
|
||||
<value>make</value>
|
||||
</dictionary>
|
||||
<dictionary>
|
||||
<key>org.eclipse.cdt.make.core.useDefaultBuildCmd</key>
|
||||
<value>true</value>
|
||||
</dictionary>
|
||||
<dictionary>
|
||||
<key>org.eclipse.cdt.make.core.build.target.auto</key>
|
||||
<value>all</value>
|
||||
</dictionary>
|
||||
<dictionary>
|
||||
<key>org.eclipse.cdt.make.core.stopOnError</key>
|
||||
<value>false</value>
|
||||
</dictionary>
|
||||
<dictionary>
|
||||
<key>org.eclipse.cdt.make.core.build.location</key>
|
||||
<value></value>
|
||||
</dictionary>
|
||||
<dictionary>
|
||||
<key>org.eclipse.cdt.make.core.build.target.inc</key>
|
||||
<value>all</value>
|
||||
</dictionary>
|
||||
<dictionary>
|
||||
<key>org.eclipse.cdt.make.core.build.arguments</key>
|
||||
<value></value>
|
||||
</dictionary>
|
||||
<dictionary>
|
||||
<key>org.eclipse.cdt.core.errorOutputParser</key>
|
||||
<value>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;</value>
|
||||
</dictionary>
|
||||
<dictionary>
|
||||
<key>org.eclipse.cdt.make.core.enableAutoBuild</key>
|
||||
<value>false</value>
|
||||
</dictionary>
|
||||
<dictionary>
|
||||
<key>org.eclipse.cdt.make.core.environment</key>
|
||||
<value></value>
|
||||
</dictionary>
|
||||
<dictionary>
|
||||
<key>org.eclipse.cdt.make.core.enabledIncrementalBuild</key>
|
||||
<value>true</value>
|
||||
</dictionary>
|
||||
<dictionary>
|
||||
<key>org.eclipse.cdt.make.core.build.target.clean</key>
|
||||
<value>clean</value>
|
||||
</dictionary>
|
||||
<dictionary>
|
||||
<key>org.eclipse.cdt.make.core.enableFullBuild</key>
|
||||
<value>true</value>
|
||||
</dictionary>
|
||||
</arguments>
|
||||
</buildCommand>
|
||||
<buildCommand>
|
||||
<name>org.eclipse.cdt.make.core.ScannerConfigBuilder</name>
|
||||
<arguments>
|
||||
</arguments>
|
||||
</buildCommand>
|
||||
</buildSpec>
|
||||
<natures>
|
||||
<nature>org.eclipse.cdt.core.cnature</nature>
|
||||
<nature>org.eclipse.cdt.make.core.makeNature</nature>
|
||||
<nature>org.eclipse.cdt.make.core.ScannerConfigNature</nature>
|
||||
</natures>
|
||||
</projectDescription>
|
||||
@ -1,3 +0,0 @@
|
||||
#Fri Jan 04 09:57:26 GMT-08:00 2008
|
||||
eclipse.preferences.version=1
|
||||
indexerId=org.eclipse.cdt.core.fastIndexer
|
||||
7
CHANGES
7
CHANGES
@ -1,12 +1,13 @@
|
||||
07 Oct 2008 - trunk
|
||||
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.
|
||||
|
||||
* Persistent counter updates are now atomic.
|
||||
|
||||
|
||||
24 Sep 2008 - 2.5.7
|
||||
-------------------
|
||||
|
||||
@ -240,7 +240,7 @@ void internal_log(request_rec *r, directory_config *dcfg, modsec_rec *msr,
|
||||
char str1[1024] = "";
|
||||
char str2[1256] = "";
|
||||
|
||||
/* Find the logging FD and look up the logging level in the configuration. */
|
||||
/* Find the logging FD and determine the logging level from configuration. */
|
||||
if (dcfg != NULL) {
|
||||
if ((dcfg->debuglog_fd != NULL)&&(dcfg->debuglog_fd != NOT_SET_P)) {
|
||||
debuglog_fd = dcfg->debuglog_fd;
|
||||
|
||||
@ -1,7 +1,7 @@
|
||||
#!@PERL@
|
||||
#
|
||||
# ModSecurity for Apache 2.x, http://www.modsecurity.org/
|
||||
# Copyright (c) 2004-2009 Breach Security, Inc. (http://www.breach.com/)
|
||||
# Copyright (c) 2004-2008 Breach Security, Inc. (http://www.breach.com/)
|
||||
#
|
||||
# This product is released under the terms of the General Public Licence,
|
||||
# version 2 (GPLv2). Please refer to the file LICENSE (included with this
|
||||
|
||||
@ -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);
|
||||
}
|
||||
|
||||
|
||||
0
apache2/t/action/.empty
Normal file
0
apache2/t/action/.empty
Normal file
0
apache2/t/regression/server_root/data/.empty
Normal file
0
apache2/t/regression/server_root/data/.empty
Normal file
BIN
apache2/t/regression/server_root/htdocs/test.pdf
Normal file
BIN
apache2/t/regression/server_root/htdocs/test.pdf
Normal file
Binary file not shown.
0
apache2/t/regression/server_root/logs/audit/.empty
Normal file
0
apache2/t/regression/server_root/logs/audit/.empty
Normal file
0
apache2/t/regression/server_root/logs/subdir/.empty
Normal file
0
apache2/t/regression/server_root/logs/subdir/.empty
Normal file
0
apache2/t/regression/server_root/tmp/.empty
Normal file
0
apache2/t/regression/server_root/tmp/.empty
Normal file
0
apache2/t/regression/server_root/upload/.empty
Normal file
0
apache2/t/regression/server_root/upload/.empty
Normal file
File diff suppressed because it is too large
Load Diff
@ -4,7 +4,7 @@
|
||||
<article>
|
||||
<title>ModSecurity 2 Data Formats</title>
|
||||
<articleinfo>
|
||||
<releaseinfo>Version 2.6.0-trunk (December 3, 2008)</releaseinfo>
|
||||
<releaseinfo>Version 2.6.0-trunk (March 5, 2009)</releaseinfo>
|
||||
<copyright>
|
||||
<year>2004-2008</year>
|
||||
<holder>Breach Security, Inc. (<ulink url="http://www.breach.com"
|
||||
|
||||
File diff suppressed because it is too large
Load Diff
@ -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';
|
||||
|
||||
|
||||
|
||||
@ -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 <review-xml-file> [ <src-file> ... ]\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";
|
||||
}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user