diff --git a/apache2/msc_lua.c b/apache2/msc_lua.c index 44faa0ec5..b54c2bcac 100644 --- a/apache2/msc_lua.c +++ b/apache2/msc_lua.c @@ -376,7 +376,7 @@ static int l_setvar(lua_State *L) { if(nargs != 2) { msr_log(msr, 8, "m.setvar: Failed m.setvar funtion must has 2 arguments"); - return -1; + return 0; } var_value = luaL_checkstring (L, 2); var_name = luaL_checkstring (L, 1); @@ -384,13 +384,13 @@ static int l_setvar(lua_State *L) { lua_pop(L,2); if(var_value == NULL || var_name == NULL) - return -1; + return 0; chr = strchr((char *)var_name,0x2e); if(chr == NULL) { msr_log(msr, 8, "m.setvar: Must specify a collection using dot character - ie m.setvar(tx.myvar,mydata)"); - return -1; + return 0; } return msre_action_setvar_execute(msr,msr->msc_rule_mptmp,rule,(char *)var_name,(char *)var_value); diff --git a/apache2/msc_multipart.c b/apache2/msc_multipart.c index c8c677391..4d35a7cf5 100644 --- a/apache2/msc_multipart.c +++ b/apache2/msc_multipart.c @@ -84,6 +84,80 @@ static char *multipart_construct_filename(modsec_rec *msr) { /** * */ +/* + * Error codes of multipart_parse_content_disposition(). The numeric values + * are part of the REQBODY_ERROR_MSG text, so keep them stable; add new codes + * at the end. Use multipart_cd_error_message() for the description. + */ +enum ContentDispositionError { + CD_ERR_NOT_FORM_DATA = -1, + CD_ERR_SEMICOLON_EXPECTED = -2, + CD_ERR_PARAMETER_MISSING = -3, + CD_ERR_VALUE_MISSING = -4, + CD_ERR_EQUALS_MISSING = -5, + CD_ERR_VALUE_EMPTY = -6, + CD_ERR_QUOTE_EMPTY_UNTERMINATED = -7, + CD_ERR_ESCAPE_INCOMPLETE = -8, + /* -9 is reserved: improper escaping, tolerated because of IE */ + CD_ERR_QUOTE_UNTERMINATED = -10, + CD_ERR_UNKNOWN_PARAMETER = -11, + CD_ERR_SEMICOLON_EXPECTED_AFTER_VALUE = -12, + CD_ERR_EQUALS_EXPECTED = -13, + CD_ERR_DUPLICATE_NAME = -14, + /* -15 was shared by both duplicate filename cases, see -20 and -21 */ + CD_ERR_FILENAME_STAR_CHARSET = -16, + CD_ERR_FILENAME_STAR_LANGUAGE = -17, + CD_ERR_FILENAME_STAR_PCT_ENCODING = -18, + CD_ERR_FILENAME_STAR_CHARACTER = -19, + CD_ERR_DUPLICATE_FILENAME = -20, + CD_ERR_DUPLICATE_FILENAME_STAR = -21, +}; + +static const char *multipart_cd_error_message(int rc) { + switch (rc) { + case CD_ERR_NOT_FORM_DATA: + return "disposition type is not form-data"; + case CD_ERR_SEMICOLON_EXPECTED: + return "expected ';' after form-data"; + case CD_ERR_PARAMETER_MISSING: + return "missing parameter after ';'"; + case CD_ERR_VALUE_MISSING: + return "parameter has no value"; + case CD_ERR_EQUALS_MISSING: + return "missing '=' after parameter name"; + case CD_ERR_VALUE_EMPTY: + return "parameter value is empty"; + case CD_ERR_QUOTE_EMPTY_UNTERMINATED: + return "end of header after opening quote"; + case CD_ERR_ESCAPE_INCOMPLETE: + return "backslash at end of quoted value"; + case CD_ERR_UNKNOWN_PARAMETER: + return "unknown parameter"; + case CD_ERR_SEMICOLON_EXPECTED_AFTER_VALUE: + return "expected ';' after parameter value"; + case CD_ERR_EQUALS_EXPECTED: + return "expected '=' after parameter name"; + case CD_ERR_DUPLICATE_NAME: + return "duplicate name parameter"; + case CD_ERR_QUOTE_UNTERMINATED: + return "closing quote not found"; + case CD_ERR_FILENAME_STAR_CHARSET: + return "filename*: charset missing"; + case CD_ERR_FILENAME_STAR_LANGUAGE: + return "filename*: closing quote of language not found"; + case CD_ERR_FILENAME_STAR_PCT_ENCODING: + return "filename*: invalid percent encoding"; + case CD_ERR_FILENAME_STAR_CHARACTER: + return "filename*: invalid character"; + case CD_ERR_DUPLICATE_FILENAME: + return "duplicate filename parameter"; + case CD_ERR_DUPLICATE_FILENAME_STAR: + return "duplicate filename* parameter"; + default: + return "unknown error"; + } +} + static int multipart_parse_content_disposition(modsec_rec *msr, char *c_d_value) { assert(msr != NULL); assert(c_d_value != NULL); @@ -259,7 +333,7 @@ static int multipart_parse_content_disposition(modsec_rec *msr, char *c_d_value) msr->mpd->flag_duplicate_part_header = 1; msr_log(msr, 4, "Multipart: Warning: Duplicate Content-Disposition filename: %s", log_escape_nq(msr->mp, value)); - return -15; + return CD_ERR_DUPLICATE_FILENAME; } else { validate_quotes(msr, value, quote); @@ -277,7 +351,7 @@ static int multipart_parse_content_disposition(modsec_rec *msr, char *c_d_value) msr->mpd->flag_duplicate_part_header = 1; msr_log(msr, 4, "Multipart: Warning: Duplicate Content-Disposition filename*: %s.", log_escape_nq(msr->mp, value)); - return -15; + return CD_ERR_DUPLICATE_FILENAME_STAR; } int invalid_count = 0; int changed = 0; @@ -368,8 +442,8 @@ static int multipart_process_part_header(modsec_rec *msr, char **error_msg) { rc = multipart_parse_content_disposition(msr, header_value); if (rc < 0) { - *error_msg = apr_psprintf(msr->mp, "Multipart: Invalid Content-Disposition header (%d): %s.", - rc, log_escape_nq(msr->mp, header_value)); + *error_msg = apr_psprintf(msr->mp, "Multipart: Invalid Content-Disposition header (%d: %s): %s.", + rc, multipart_cd_error_message(rc), log_escape_nq(msr->mp, header_value)); return -1; } diff --git a/tests/regression/misc/70-multipart-cd-errors.t b/tests/regression/misc/70-multipart-cd-errors.t new file mode 100644 index 000000000..98c752fa5 --- /dev/null +++ b/tests/regression/misc/70-multipart-cd-errors.t @@ -0,0 +1,64 @@ +### Content-Disposition error codes are named in the error message (#3655) + +{ + type => "misc", + comment => "multipart duplicate filename reports -20 with its description", + conf => q( + SecRuleEngine On + SecRequestBodyAccess On + SecRule REQBODY_ERROR "!@eq 0" "id:500655,phase:2,deny,status:403,msg:'%{REQBODY_ERROR_MSG}'" + ), + match_log => { + error => [ qr/Invalid Content-Disposition header \(-20: duplicate filename parameter\)/, 1 ], + }, + match_response => { + status => qr/^403$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ + "Content-Type" => "multipart/form-data; boundary=0000", + ], + normalize_raw_request_data( + q( + --0000 + Content-Disposition: form-data; name="file"; filename="safe.jpg"; filename="safe.php" + Content-Type: text/plain + + hello + --0000-- + ), + ), + ), +}, +{ + type => "misc", + comment => "multipart duplicate filename* reports -21 with its description", + conf => q( + SecRuleEngine On + SecRequestBodyAccess On + SecRule REQBODY_ERROR "!@eq 0" "id:500656,phase:2,deny,status:403,msg:'%{REQBODY_ERROR_MSG}'" + ), + match_log => { + error => [ qr/Invalid Content-Disposition header \(-21: duplicate filename\* parameter\)/, 1 ], + }, + match_response => { + status => qr/^403$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ + "Content-Type" => "multipart/form-data; boundary=0000", + ], + normalize_raw_request_data( + q( + --0000 + Content-Disposition: form-data; name="file"; filename*=UTF8''safe.jpg; filename*=UTF8''safe.php + Content-Type: text/plain + + hello + --0000-- + ), + ), + ), +}, diff --git a/tests/regression/target/10-multipart_filename.t b/tests/regression/target/10-multipart_filename.t index c743eb93d..b2d38aa9b 100644 --- a/tests/regression/target/10-multipart_filename.t +++ b/tests/regression/target/10-multipart_filename.t @@ -291,7 +291,7 @@ SecRule MULTIPART_FILENAME:file "\@contains Math+physics v1.pdf" "id:1,phase:2,pass,t:trim" ), match_log => { - debug => [ qr/Invalid Content-Disposition header \(-18\): form-data; name="file"; filename\*=UTF-8''Math\+physics%20v1\.pdf%ZZ/s, 1 ], + debug => [ qr/Invalid Content-Disposition header \(-18: filename\*: invalid percent encoding\): form-data; name="file"; filename\*=UTF-8''Math\+physics%20v1\.pdf%ZZ/s, 1 ], }, match_response => { status => qr/^200$/, @@ -335,7 +335,7 @@ SecRule MULTIPART_FILENAME:file "\@contains Math+physics v1.pdf" "id:1,phase:2,pass,t:trim" ), match_log => { - debug => [ qr/Invalid Content-Disposition header \(-16\): form-data; name="file"; filename\*="UTF-8''Math\+physics%20v1\.pdf"/s, 1 ], + debug => [ qr/Invalid Content-Disposition header \(-16: filename\*: charset missing\): form-data; name="file"; filename\*="UTF-8''Math\+physics%20v1\.pdf"/s, 1 ], }, match_response => { status => qr/^200$/, diff --git a/tests/regression/target/10-variable-multipart_strict_error.t b/tests/regression/target/10-variable-multipart_strict_error.t index bf32f11e6..0857720c3 100644 --- a/tests/regression/target/10-variable-multipart_strict_error.t +++ b/tests/regression/target/10-variable-multipart_strict_error.t @@ -515,7 +515,7 @@ SecRule MULTIPART_STRICT_ERROR "!\@eq 0" "id:990102,phase:2,deny,status:400,log,t:none" ), match_log => { - debug => [ qr/Multipart: Invalid Content-Disposition header \(\-15\): form-data; name="file"; filename="safe.jpg"; filename="safe.php"/s, 1 ], + debug => [ qr/Multipart: Invalid Content-Disposition header \(-20: duplicate filename parameter\): form-data; name="file"; filename="safe.jpg"; filename="safe.php"/s, 1 ], }, match_response => { status => qr/^400$/, @@ -561,7 +561,7 @@ SecRule MULTIPART_STRICT_ERROR "!\@eq 0" "id:990102,phase:2,deny,status:400,log,t:none" ), match_log => { - debug => [ qr/Multipart: Invalid Content-Disposition header \(\-15\): form-data; name="file"; filename\*=UTF8''safe.jpg; filename\*=UTF8''safe.php/s, 1 ], + debug => [ qr/Multipart: Invalid Content-Disposition header \(-21: duplicate filename\* parameter\): form-data; name="file"; filename\*=UTF8''safe.jpg; filename\*=UTF8''safe.php/s, 1 ], }, match_response => { status => qr/^400$/, @@ -607,7 +607,7 @@ SecRule MULTIPART_STRICT_ERROR "!\@eq 0" "id:990102,phase:2,deny,status:400,log,t:none" ), match_log => { - debug => [ qr/Multipart: Invalid Content-Disposition header \(\-15\): form-data; name="file"; filename\*=UTF8''safe.jpg; filename="safe.jpg"; filename="safe.php"/s, 1 ], + debug => [ qr/Multipart: Invalid Content-Disposition header \(-20: duplicate filename parameter\): form-data; name="file"; filename\*=UTF8''safe.jpg; filename="safe.jpg"; filename="safe.php"/s, 1 ], }, match_response => { status => qr/^400$/, @@ -651,7 +651,7 @@ SecRule MULTIPART_STRICT_ERROR "!\@eq 0" "id:990102,phase:2,deny,status:400,log,t:none" ), match_log => { - debug => [ qr/Invalid Content-Disposition header \(-18\): form-data; name="file"; filename\*=UTF-8''Math\+physics%20v1\.pdf%ZZ/s, 1 ], + debug => [ qr/Invalid Content-Disposition header \(-18: filename\*: invalid percent encoding\): form-data; name="file"; filename\*=UTF-8''Math\+physics%20v1\.pdf%ZZ/s, 1 ], }, match_response => { status => qr/^400$/, @@ -698,7 +698,7 @@ SecRule MULTIPART_STRICT_ERROR "!\@eq 0" "id:990102,phase:2,deny,status:400,log,t:none" ), match_log => { - debug => [ qr/Invalid Content-Disposition header \(-16\): form-data; name="file"; filename\*="UTF-8''Math\+physics%20v1\.pdf"/s, 1 ], + debug => [ qr/Invalid Content-Disposition header \(-16: filename\*: charset missing\): form-data; name="file"; filename\*="UTF-8''Math\+physics%20v1\.pdf"/s, 1 ], }, match_response => { status => qr/^400$/,