2008-01-04 :: 10:55:40:361 GMT-08:00
2008-01-04 :: 10:57:29:687 GMT-08:00
brian
brian
apache2/re_variables.c
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.open
2008-01-04 :: 10:57:54:279 GMT-08:00
2008-01-04 :: 11:16:11:570 GMT-08:00
brian
brian
apache2/re_variables.c
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.open
2008-01-04 :: 11:02:30:450 GMT-08:00
2008-01-04 :: 11:03:43:300 GMT-08:00
brian
brian
apache2/re_variables.c
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.open
2008-01-04 :: 11:06:50:690 GMT-08:00
2008-01-04 :: 11:07:26:033 GMT-08:00
brian
brian
apache2/re_variables.c
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.open
2008-01-04 :: 11:08:36:733 GMT-08:00
2008-01-04 :: 11:09:06:681 GMT-08:00
brian
brian
apache2/re_variables.c
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.open
2008-01-04 :: 11:14:32:043 GMT-08:00
2008-01-04 :: 14:52:02:794 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.open
2008-01-04 :: 11:25:25:879 GMT-08:00
2008-01-04 :: 11:26:10:856 GMT-08:00
brian
brian
apache2/apache2_util.c
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.open
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-04 :: 13:24:14:701 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.open
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-04 :: 15:45:51:571 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.open
2008-01-04 :: 15:47:09:574 GMT-08:00
2008-01-04 :: 15:48:00:456 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.open
2008-01-04 :: 15:49:05:878 GMT-08:00
2008-01-04 :: 15:51:13:090 GMT-08:00
brian
brian
apache2/re_operators.c
item.type.label.suggestion
item.severity.label.major
Need to resolve the TODOs introduced by Lua processing.
item.resolution.label.validNeedsfixing
item.status.label.open
2008-01-04 :: 15:54:24:055 GMT-08:00
2008-01-04 :: 15:55:22:775 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.open
2008-01-04 :: 15:56:23:415 GMT-08:00
2008-01-04 :: 15:57:17:579 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.open
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-04 :: 16:07:33:188 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.validNeedsfixing
item.status.label.open
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-04 :: 16:06:13:415 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.open
2008-01-04 :: 16:12:30:943 GMT-08:00
2008-01-04 :: 16:12:47:358 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.open
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-04 :: 16:25:17:586 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.open
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-04 :: 17:08:12:017 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.open
2008-01-04 :: 17:05:44:757 GMT-08:00
2008-01-07 :: 23:10:38:787 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. 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-04 :: 17:11:59:825 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.open
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-07 :: 11:43:36:608 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.open
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-07 :: 11:54:26:858 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.open
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-07 :: 16:09:41:123 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.validNeedsfixing
item.status.label.open
2008-01-07 :: 16:17:58:895 GMT-08:00
2008-01-07 :: 16:18:20:235 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.validNeedsfixing
item.status.label.open
2008-01-07 :: 16:22:43:295 GMT-08:00
2008-01-07 :: 16:32:24:969 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.open
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-07 :: 16:39:30:027 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.open
2008-01-07 :: 16:39:47:318 GMT-08:00
2008-01-07 :: 16:39:57:589 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.open
2008-01-07 :: 16:40:15:634 GMT-08:00
2008-01-07 :: 16:40:35:216 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.open
2008-01-07 :: 16:40:43:464 GMT-08:00
2008-01-07 :: 16:41:41:536 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.open
2008-01-07 :: 20:46:50:832 GMT-08:00
2008-01-07 :: 20:48:20:448 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.open
2008-01-07 :: 20:47:54:093 GMT-08:00
2008-01-07 :: 20:48:06:880 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.open
2008-01-07 :: 20:48:42:400 GMT-08:00
2008-01-07 :: 20:50:00:027 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.open
2008-01-07 :: 21:28:41:095 GMT-08:00
2008-01-07 :: 21:40:15:511 GMT-08:00
brian
brian
apache2/msc_reqbody.c
item.type.label.programLogic
item.severity.label.major
Potential memory leak if modsecurity_request_body_store_disk() fails. Returning here causes modsecurity_request_body_end() to never be called and never free chunk data. See also notes in read_request_body() in apache_io.c.
/* Write the data we keep in memory */
chunks = (msc_data_chunk **)msr->msc_reqbody_chunks->elts;
for(i = 0; i < msr->msc_reqbody_chunks->nelts; i++) {
disklen += chunks[i]->length;
if (modsecurity_request_body_store_disk(msr, chunks[i]->data, chunks[i]->length, error_msg) < 0) {
return -1;
}
free(chunks[i]->data);
chunks[i]->data = NULL;
}
item.resolution.label.validNeedsfixing
item.status.label.open
2008-01-07 :: 21:42:51:192 GMT-08:00
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-07 :: 22:00:34:198 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.open
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-07 :: 22:24:44:929 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.open
2008-01-07 :: 22:30:42:215 GMT-08:00
2008-01-07 :: 22:45:42:260 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.open
2008-01-07 :: 22:46:51:423 GMT-08:00
2008-01-07 :: 22:47:22:231 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.open
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-08 :: 12:16:40:432 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.open
2008-01-08 :: 12:20:29:491 GMT-08:00
2008-01-08 :: 12:28:11:109 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.open