From 513c87ee459b92e0545a6371e4f7c48b7796b426 Mon Sep 17 00:00:00 2001 From: b1v1r Date: Fri, 5 Feb 2010 18:15:31 +0000 Subject: [PATCH] Added SecUploadFileLimit (MODSEC-116). --- CHANGES | 5 +- apache2/apache2_config.c | 29 +++++++ apache2/modsecurity.h | 2 + apache2/msc_multipart.c | 24 +++++- apache2/msc_multipart.h | 4 + apache2/re_variables.c | 24 ++++++ .../t/regression/misc/00-multipart-parser.t | 77 +++++++++++++++++++ doc/modsecurity2-apache-reference.xml | 42 +++++++++- modsecurity.conf-minimal | 4 +- 9 files changed, 205 insertions(+), 6 deletions(-) diff --git a/CHANGES b/CHANGES index c79a4351..c30a3534 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,9 @@ -25 Jan 2010 - 2.5.12 +28 Jan 2010 - 2.5.12 -------------------- + * Added SecUploadFileLimit to limit the number of uploaded file parts that + will be processed in a multipart POST. The default is 100. + * Fixed path normalization to better handle backreferences that extend above root directories. Reported by Sogeti/ESEC R&D. diff --git a/apache2/apache2_config.c b/apache2/apache2_config.c index 7ef3996b..83cb996e 100644 --- a/apache2/apache2_config.c +++ b/apache2/apache2_config.c @@ -89,6 +89,7 @@ void *create_directory_config(apr_pool_t *mp, char *path) dcfg->upload_keep_files = NOT_SET; dcfg->upload_validates_files = NOT_SET; dcfg->upload_filemode = NOT_SET; + dcfg->upload_file_limit = NOT_SET; /* These are only used during the configuration process. */ dcfg->tmp_chain_starter = NULL; @@ -432,6 +433,8 @@ void *merge_directory_configs(apr_pool_t *mp, void *_parent, void *_child) ? parent->upload_validates_files : child->upload_validates_files); merged->upload_filemode = (child->upload_filemode == NOT_SET ? parent->upload_filemode : child->upload_filemode); + merged->upload_file_limit = (child->upload_file_limit == NOT_SET + ? parent->upload_file_limit : child->upload_file_limit); /* Misc */ merged->data_dir = (child->data_dir == NOT_SET_P @@ -541,6 +544,7 @@ void init_directory_config(directory_config *dcfg) if (dcfg->upload_keep_files == NOT_SET) dcfg->upload_keep_files = KEEP_FILES_OFF; if (dcfg->upload_validates_files == NOT_SET) dcfg->upload_validates_files = 0; if (dcfg->upload_filemode == NOT_SET) dcfg->upload_filemode = mode2fileperms(0600); + if (dcfg->upload_file_limit == NOT_SET) dcfg->upload_file_limit = 100; /* Misc */ if (dcfg->data_dir == NOT_SET_P) dcfg->data_dir = NULL; @@ -1622,6 +1626,23 @@ static const char *cmd_upload_dir(cmd_parms *cmd, void *_dcfg, const char *p1) return NULL; } +static const char *cmd_upload_file_limit(cmd_parms *cmd, void *_dcfg, + const char *p1) +{ + directory_config *dcfg = (directory_config *)_dcfg; + + if (dcfg == NULL) return NULL; + + if (strcasecmp(p1, "default") == 0) { + dcfg->upload_file_limit = NOT_SET; + } + else { + dcfg->upload_file_limit = atoi(p1); + } + + return NULL; +} + static const char *cmd_upload_filemode(cmd_parms *cmd, void *_dcfg, const char *p1) { @@ -2333,6 +2354,14 @@ const command_rec module_directives[] = { "path to the file upload area" ), + AP_INIT_TAKE1 ( + "SecUploadFileLimit", + cmd_upload_file_limit, + NULL, + CMD_SCOPE_ANY, + "limit the number of uploaded files processed" + ), + AP_INIT_TAKE1 ( "SecUploadFileMode", cmd_upload_filemode, diff --git a/apache2/modsecurity.h b/apache2/modsecurity.h index 096461e8..8aa0b5f5 100644 --- a/apache2/modsecurity.h +++ b/apache2/modsecurity.h @@ -331,6 +331,7 @@ struct modsec_rec { /* upload */ int upload_extract_files; int upload_remove_files; + int upload_files_count; /* other */ apr_table_t *collections_original; @@ -437,6 +438,7 @@ struct directory_config { int upload_keep_files; int upload_validates_files; int upload_filemode; /* int only so NOT_SET works */ + int upload_file_limit; /* Used only in the configuration phase. */ msre_rule *tmp_chain_starter; diff --git a/apache2/msc_multipart.c b/apache2/msc_multipart.c index 89b152dd..c520ca84 100644 --- a/apache2/msc_multipart.c +++ b/apache2/msc_multipart.c @@ -413,16 +413,32 @@ static int multipart_process_part_data(modsec_rec *msr, char **error_msg) { /* add data to the part we are building */ if (msr->mpd->mpp->type == MULTIPART_FILE) { + int extract = msr->upload_extract_files; /* remember where we started */ if (msr->mpd->mpp->length == 0) { msr->mpd->mpp->offset = msr->mpd->buf_offset; } + /* check if the file limit has been reached */ + if (extract && (msr->mpd->nfiles >= msr->txcfg->upload_file_limit)) { + if (msr->mpd->flag_file_limit_exceeded == 0) { + *error_msg = apr_psprintf(msr->mp, + "Multipart: Upload file limit exceeded " + "SecUploadFileLimit %d.", + msr->txcfg->upload_file_limit); + msr_log(msr, 3, "%s", *error_msg); + + msr->mpd->flag_file_limit_exceeded = 1; + } + + extract = 0; + } + /* only store individual files on disk if we are going * to keep them or if we need to have them approved later */ - if (msr->upload_extract_files) { + if (extract) { /* first create a temporary file if we don't have it already */ if (msr->mpd->mpp->tmp_file_fd == 0) { /* construct temporary file name */ @@ -437,8 +453,12 @@ static int multipart_process_part_data(modsec_rec *msr, char **error_msg) { return -1; } + /* keep track of the files count */ + msr->mpd->nfiles++; + if (msr->txcfg->debuglog_level >= 4) { - msr_log(msr, 4, "Multipart: Created temporary file: %s", + msr_log(msr, 4, "Multipart: Created temporary file %d: %s", + msr->mpd->nfiles, log_escape_nq(msr->mp, msr->mpd->mpp->tmp_file_name)); } } diff --git a/apache2/msc_multipart.h b/apache2/msc_multipart.h index 1ad9d2f9..5fff0fce 100644 --- a/apache2/msc_multipart.h +++ b/apache2/msc_multipart.h @@ -68,6 +68,9 @@ struct multipart_data { /* this array keeps parts */ apr_array_header_t *parts; + /* Number of parts that are files */ + int nfiles; + /* mime boundary used to detect when * parts end and begin */ @@ -119,6 +122,7 @@ struct multipart_data { int flag_missing_semicolon; int flag_invalid_quoting; int flag_invalid_header_folding; + int flag_file_limit_exceeded; }; diff --git a/apache2/re_variables.c b/apache2/re_variables.c index 2868a376..ccc56d28 100644 --- a/apache2/re_variables.c +++ b/apache2/re_variables.c @@ -1390,6 +1390,18 @@ static int var_multipart_invalid_header_folding_generate(modsec_rec *msr, msre_v } } +/* MULTIPART_FILE_LIMIT_EXCEEDED */ + +static int var_multipart_file_limit_exceeded_generate(modsec_rec *msr, msre_var *var, msre_rule *rule, + apr_table_t *vartab, apr_pool_t *mptmp) +{ + if ((msr->mpd != NULL)&&(msr->mpd->flag_file_limit_exceeded != 0)) { + return var_simple_generate(var, vartab, mptmp, "1"); + } else { + return var_simple_generate(var, vartab, mptmp, "0"); + } +} + /* MULTIPART_STRICT_ERROR */ static int var_multipart_strict_error_generate(modsec_rec *msr, msre_var *var, msre_rule *rule, @@ -1407,6 +1419,7 @@ static int var_multipart_strict_error_generate(modsec_rec *msr, msre_var *var, m ||(msr->mpd->flag_missing_semicolon != 0) ||(msr->mpd->flag_invalid_quoting != 0) ||(msr->mpd->flag_invalid_header_folding != 0) + ||(msr->mpd->flag_file_limit_exceeded != 0) ) { return var_simple_generate(var, vartab, mptmp, "1"); } @@ -2502,6 +2515,17 @@ void msre_engine_register_default_variables(msre_engine *engine) { PHASE_REQUEST_BODY ); + /* MULTIPART_FILE_LIMIT_EXCEEDED */ + msre_engine_variable_register(engine, + "MULTIPART_FILE_LIMIT_EXCEEDED", + VAR_SIMPLE, + 0, 0, + NULL, + var_multipart_file_limit_exceeded_generate, + VAR_DONT_CACHE, /* flag */ + PHASE_REQUEST_BODY + ); + /* MULTIPART_STRICT_ERROR */ msre_engine_variable_register(engine, "MULTIPART_STRICT_ERROR", diff --git a/apache2/t/regression/misc/00-multipart-parser.t b/apache2/t/regression/misc/00-multipart-parser.t index da1a3eee..818dd23a 100644 --- a/apache2/t/regression/misc/00-multipart-parser.t +++ b/apache2/t/regression/misc/00-multipart-parser.t @@ -1737,3 +1737,80 @@ ), ), }, + +# File limits +{ + type => "misc", + comment => "multipart parser (normal)", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRequestBodyAccess On + SecTmpDir "$ENV{TEMP_DIR}" + SecUploadDir "$ENV{UPLOAD_DIR}" + SecUploadKeepFiles On + SecUploadFileLimit 2 + + # These should be set + SecRule MULTIPART_STRICT_ERROR "!\@eq 1" "phase:2,deny" + SecRule MULTIPART_FILE_LIMIT_EXCEEDED "!\@eq 1" "phase:2,deny" + + # This should not be set + SecRule REQBODY_PROCESSOR_ERROR "\@eq 1" "phase:2,deny" + + # Theses should still be accurate + SecRule &FILES "!\@eq 3" "phase:2,deny" + SecRule &FILES_NAMES "!\@eq 3" "phase:2,deny" + SecRule &FILES_SIZES "!\@eq 3" "phase:2,deny" + SecRule FILES_SIZES:/^image/ "\@eq 0" "phase:2,deny" + + # This should be the SecUploadFileLimit + SecRule &FILES_TMPNAMES "!\@eq 2" "phase:2,deny" + ), + match_log => { + debug => [ qr/Multipart: Upload file limit exceeded.*name: test.*variable: This is test data/s, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ + "Content-Type" => q(multipart/form-data; boundary=0000), + ], + normalize_raw_request_data( + q( + --0000 + Content-Disposition: form-data; name="name" + + Brian Rectanus + --0000 + Content-Disposition: form-data; name="email" + + brian.rectanus@breach.com + --0000 + Content-Disposition: form-data; name="image1"; filename="image1.jpg" + Content-Type: image/jpeg + + BINARYDATA1 + --0000 + Content-Disposition: form-data; name="image2"; filename="image2.jpg" + Content-Type: image/jpeg + + BINARYDATA2 + --0000 + Content-Disposition: form-data; name="image3"; filename="image3.jpg" + Content-Type: image/jpeg + + BINARYDATA3 + --0000 + Content-Disposition: form-data; name="test" + + This is test data. + --0000-- + ), + ), + ), +}, + diff --git a/doc/modsecurity2-apache-reference.xml b/doc/modsecurity2-apache-reference.xml index 902e30a4..835bd8b8 100644 --- a/doc/modsecurity2-apache-reference.xml +++ b/doc/modsecurity2-apache-reference.xml @@ -2530,6 +2530,42 @@ SecRuleUpdateActionById 12345 "t:compressWhitespace,deny,status:403,msg:'A new m SecUploadKeepFiles. +
+ <literal>SecUploadFileLimit</literal> + + Description: Configures the maximum number of + file uploads processed in a multipart POST. + + Syntax: SecUploadFileLimit number + + Example Usage: SecUploadFileLimit 10 + + Processing Phase: N/A + + Scope: Any + + Version: 2.5.12 + + Dependencies/Notes: The default is set to 100 + files, but you are encouraged to reduce this value. Any file over the + limit will not be extracted and the MULTIPART_FILE_LIMIT_EXCEEDED and MULTIPART_STRICT_ERROR flags will be set. To + prevent bypassing any file checks, you must check for one of these + flags. + + + If the limit is exceeded, the part name and file name will still + be recorded in FILES_NAME and + FILES, the file size will be + recorded in FILES_SIZES, but there + will be no record in FILES_TMPNAMES + as a temporary file was not created. + +
+
<literal>SecUploadFileMode</literal> @@ -3174,7 +3210,8 @@ SecRule ARGS "@pm some key words" id:12345,deny,status:500 MULTIPART_LF_LINE, MULTIPART_SEMICOLON_MISSING MULTIPART_INVALID_QUOTING - MULTIPART_INVALID_HEADER_FOLDING. Each of these + MULTIPART_INVALID_HEADER_FOLDING + MULTIPART_FILE_LIMIT_EXCEEDED. Each of these variables covers one unusual (although sometimes legal) aspect of the request body in multipart/form-data format. Your policies should always contain a rule to check @@ -3198,7 +3235,8 @@ HF %{MULTIPART_HEADER_FOLDING}, \ LF %{MULTIPART_LF_LINE}, \ SM %{MULTIPART_SEMICOLON_MISSING}, \ IQ %{MULTIPART_INVALID_QUOTING}, \ -IQ %{MULTIPART_INVALID_HEADER_FOLDING}'" +IQ %{MULTIPART_INVALID_HEADER_FOLDING}, \ +FE %{MULTIPART_FILE_LIMIT_EXCEEDED}'" The multipart/form-data parser was upgraded in ModSecurity v2.1.3 to actively look for signs of evasion. Many variables diff --git a/modsecurity.conf-minimal b/modsecurity.conf-minimal index 584cac19..7afce654 100644 --- a/modsecurity.conf-minimal +++ b/modsecurity.conf-minimal @@ -12,6 +12,7 @@ SecPcreMatchLimitRecursion 1000 # TODO Choose a folder private to Apache. # SecUploadDir /opt/apache-frontend/tmp/ SecUploadKeepFiles Off +SecUploadFileLimit 10 # Debug log SecDebugLog logs/modsec_debug.log @@ -58,7 +59,8 @@ HF %{MULTIPART_HEADER_FOLDING}, \ LF %{MULTIPART_LF_LINE}, \ SM %{MULTIPART_SEMICOLON_MISSING}, \ IQ %{MULTIPART_INVALID_QUOTING}, \ -IH %{MULTIPART_INVALID_HEADER_FOLDING}'" +IH %{MULTIPART_INVALID_HEADER_FOLDING}, \ +IH %{MULTIPART_FILE_LIMIT_EXCEEDED}'" # Did we see anything that might be a boundary? SecRule MULTIPART_UNMATCHED_BOUNDARY "!@eq 0" \