github_mirrors/ModSecurity
3
0
Fork 1
mirror of https://github.com/owasp-modsecurity/ModSecurity.git synced 2025-08-17 22:56:18 +03:00
Code Issues Packages Projects Releases Wiki Activity
ModSecurity/review/pre-2.5-brian.txt
brectanus 800dacd743 Add text version of review.
2008-01-10 22:04:36 +00:00

1899 lines
46 KiB
Plaintext
Raw Blame History

File: CHANGES
===================================================================
[brian] Suggestion @ 227
Remove TODO.
TODO: more to come
File: apache2/acmp.c
===================================================================
[brian] Clarity @ 181
Add parens for clarity.
*ucs_chars++ = *c++;
[brian] Suggestion @ 278
Remove comment.
//return acmp_child_for_code(node, letter)
[brian] Suggestion @ 316
Change to #idef DEBUG_ACMP or similar.
/* printf("%c ->left %c \n", node->node->letter, node->left->node->letter); */
[brian] Suggestion @ 323
Change to #idef DEBUG_ACMP or similar.
/* printf("%c ->right %c \n", node->node->letter, node->right->node->letter); */
[brian] Suggestion @ 385
Change to #idef DEBUG_ACMP or similar.
/* printf("fail direction: *%s* => *%s*\n", child->text, child->fail->text); */
[brian] Suggestion @ 396
Change to #idef DEBUG_ACMP or similar.
/* printf("fail direction: *%s* => *%s*\n", node->text, node->fail->text); */
File: apache2/apache2_config.c
===================================================================
[brian] Suggestion @ 586
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.");
}
[brian] Suggestion @ 678
Probably should check we were able to allocate.
msre_rule *phrule = apr_palloc(rule->ruleset->mp, sizeof(msre_rule));
[brian] Suggestion @ 702
Comment or remove.
TODO
[brian] Suggestion @ 937
ENH instead of TODO
TODO
[brian] Suggestion @ 1010
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).");
}
[brian] Suggestion @ 1149
Fix TODO or make ENH
// TODO Validate encoding
// return apr_psprintf(cmd->pool, "ModSecurity: Invalid value for SecRequestBodyAccess: %s", p1);
[brian] Suggestion @ 1205
Fix TODO or change to ENH.
// TODO check whether the parameter is a valid MIME type of "null"
[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 @ 1384
Fix or TODO->ENH
// TODO enforce format (letters, digits, ., _, -)
[brian] Suggestion @ 1464
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;
}
[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);
}
[brian] Suggestion @ 1567
Fix TODO or change to ENH.
AP_INIT_TAKE1 (
"SecAction",
cmd_action,
NULL,
CMD_SCOPE_ANY,
"" // TODO
),
[brian] Suggestion @ 1679
Fix TODO or change to ENH.
AP_INIT_TAKE1 (
"SecDataDir",
cmd_data_dir,
NULL,
CMD_SCOPE_MAIN,
"" // TODO
),
[brian] Suggestion @ 1704
Fix TODO or change to ENH.
AP_INIT_TAKE1 (
"SecDefaultAction",
cmd_default_action,
NULL,
CMD_SCOPE_ANY,
"" // TODO
),
[brian] Suggestion @ 1832
Fix TODO or change to ENH.
AP_INIT_TAKE1 (
"SecResponseBodyLimit",
cmd_response_body_limit,
NULL,
CMD_SCOPE_ANY,
"" // TODO
),
[brian] Suggestion @ 1840
Fix TODO or change to ENH.
AP_INIT_TAKE1 (
"SecResponseBodyLimitAction",
cmd_response_body_limit_action,
NULL,
CMD_SCOPE_ANY,
"" // TODO
),
[brian] Suggestion @ 1864
Fix TODO or change to ENH.
AP_INIT_TAKE23 (
"SecRule",
cmd_rule,
NULL,
CMD_SCOPE_ANY,
"" // TODO
),
[brian] Suggestion @ 1888
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
),
[brian] Suggestion @ 1920
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
),
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/apache2_util.c
===================================================================
[brian] Suggestion @ 95
Portable way to format sizeof()?
msr_log(msr, 1, "Exec: Unable to allocate %lu bytes.", (unsigned long)sizeof(*procnew));
File: apache2/mod_security2.c
===================================================================
[brian] Suggestion @ 685
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);
[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] Suggestion @ 50
Spelling.
throught
[brian] ProgramLogic @ 235
This allows an empty string as a valid part. This misvalidates "ctl:auditLogParts=+", etc.
is_valid_parts_specification
[brian] Suggestion @ 406
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;
[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);
[brian] Suggestion @ 876
Change TODO to ENH.
/* AUDITLOG_PART_UPLOADS */
// TODO: Implement
File: apache2/msc_lua.c
===================================================================
[brian] Missing @ 155
Log an error.
} else {
// TODO Error
return NULL;
}
[brian] Suggestion @ 363
Change C++ to C style comment.
// Get the response from the script.
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] Clarity @ 127
Add parens for clarity.
*t++ = *p++;
[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 @ 199
len is always assumed to be at least 1. What is preventing a len of 0?
if (len > 1) {
[brian] Suggestion @ 286
Use MULTIPART_BUF_SIZE for the constant.
if (strlen(new_value) > 4096) {
[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. */
[brian] Suggestion @ 861
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 */
[brian] Suggestion @ 876
Use '\n' vs 0x0a as is done elsewhere.
if ((c == 0x0a)||(msr->mpd->bufleft == 0)||(process_buffer)) {
File: apache2/msc_parsers.c
===================================================================
[brian] Suggestion @ 12
Headers are not used. I think they were added for iconv support, but Ivan removed it.
#include "iconv.h"
#include <errno.h>
File: apache2/msc_reqbody.c
===================================================================
[brian] Suggestion @ 199
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));
[brian] Suggestion @ 465
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));
[brian] Suggestion @ 474
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));
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 @ 835
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 */
[brian] Optimization @ 1175
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;
}
}
[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 @ 200
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;
}
[brian] Suggestion @ 297
Should replace with isvarnamechar() if possible.
while((*p != '\0')&&(*p != '|')&&(*p != ':')&&(*p != ',')&&(!isspace(*p))) p++; /* ENH replace with isvarnamechar() */
[brian] Suggestion @ 356
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);
[brian] Suggestion @ 364
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);
[brian] Clarity @ 371
Add parens for clarity.
*d++ = *p++;
[brian] Clarity @ 379
Add parens for clarity.
*d++ = *p++;
[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] Clarity @ 908
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) {
...
}
[brian] Missing @ 1096
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;
}
[brian] Optimization @ 1119
Should move this to a static global for performance.
static const char *const severities[] = {
"EMERGENCY",
"ALERT",
"CRITICAL",
"ERROR",
"WARNING",
"NOTICE",
"INFO",
"DEBUG",
NULL,
};
[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] Suggestion @ 1233
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));
}
}
[brian] Suggestion @ 1341
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));
}
[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);
}
[brian] Irrelevant @ 1716
These do not appear to be needed.
tfnspath = NULL;
tfnskey = NULL;
[brian] ProgramLogic @ 1728
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;
}
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] Suggestion @ 202
No.
/* ENH Do we want to support %{DIGIT} as well? */
[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] Irrelevant @ 530
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.
[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 @ 726
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
[brian] Optimization @ 774
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
[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] Suggestion @ 812
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';
[brian] Clarity @ 814
Add parens for clarity.
*d++ = *s++;
[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] Missing @ 1232
Missing error log needs implemented.
} else {
/* ENH Log warning detected variable name but no collection. */
return 0;
}
[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 @ 1296
Missing error log needs implemented.
} else {
/* ENH Log warning detected variable name but no collection. */
return 0;
}
[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] Clarity @ 265
Add parens for clarity.
*next++ = '\0';
[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 @ 1109
@geoLookup should set error_msg on success to something like "Successful geograpical lookup of \"%s\" at %s."
msre_op_geoLookup_execute
[brian] Missing @ 1230
@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. */
}
[brian] Suggestion @ 1259
Remove the ifdef as lua is required?
#ifdef WITH_LUA
[brian] Suggestion @ 1259
Need to remove the LUA #ifdef's
#ifdef WITH_LUA
[brian] Suggestion @ 1260
Change from TODO to ENH.
// TODO Write & use string_ends(s, e).
[brian] Suggestion @ 1275
Change from TODO to ENH.
[brian] Suggestion @ 1324
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;
}
[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 @ 1403
@validateByteRange does not output the VAR name on match. Need to append " at %s."
msre_op_validateByteRange_execute
[brian] Missing @ 1477
@validateurlEncoding does not output VAR name nor offset in error_msg on match.
msre_op_validateUrlEncoding_execute
[brian] Missing @ 1659
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
[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';
File: apache2/re_variables.c
===================================================================
[brian] Suggestion @ 841
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);
[brian] Suggestion @ 853
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);
[brian] Suggestion @ 2204
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
);
[brian] Suggestion @ 2270
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
);
[brian] Suggestion @ 2281
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
);
[brian] Suggestion @ 2303
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
);
[brian] Suggestion @ 2534
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
);
[brian] Suggestion @ 2908
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
);
Reference in New Issue View Git Blame Copy Permalink