diff --git a/CHANGES b/CHANGES index bd03641530..3adb24743e 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,9 @@ +DD mmm YYYY - 2.9.x (to be released) +------------------- + * Multipart names/filenames may include single quote if double-quote enclosed + [Issue #2352 - @martinhsv] + + 21 Jun 2021 - 2.9.4 ------------------- diff --git a/apache2/msc_multipart.c b/apache2/msc_multipart.c index 880b13c579..d087c863e1 100644 --- a/apache2/msc_multipart.c +++ b/apache2/msc_multipart.c @@ -20,7 +20,7 @@ #include "msc_util.h" #include "msc_parsers.h" -void validate_quotes(modsec_rec *msr, char *data) { +void validate_quotes(modsec_rec *msr, char *data, char quote) { int i, len; if(msr == NULL) @@ -32,6 +32,12 @@ void validate_quotes(modsec_rec *msr, char *data) { if(data == NULL) return; + // if the value was enclosed in double quotes, then we don't care about + // a single quote character within the name. + if (quote == '"') { + return; + } + len = strlen(data); for(i = 0; i < len; i++) { @@ -123,9 +129,10 @@ static int multipart_parse_content_disposition(modsec_rec *msr, char *c_d_value) * set so the user can deal with it in the rules if they so wish. */ + char quote = '\0'; if ((*p == '"') || (*p == '\'')) { /* quoted */ - char quote = *p; + quote = *p; // remember which quote character was used for the value if (quote == '\'') { msr->mpd->flag_invalid_quoting = 1; @@ -182,7 +189,7 @@ static int multipart_parse_content_disposition(modsec_rec *msr, char *c_d_value) if (strcmp(name, "name") == 0) { - validate_quotes(msr, value); + validate_quotes(msr, value, quote); msr->multipart_name = apr_pstrdup(msr->mp, value); @@ -201,7 +208,7 @@ static int multipart_parse_content_disposition(modsec_rec *msr, char *c_d_value) else if (strcmp(name, "filename") == 0) { - validate_quotes(msr, value); + validate_quotes(msr, value, quote); msr->multipart_filename = apr_pstrdup(msr->mp, value); diff --git a/tests/regression/misc/00-multipart-parser.t b/tests/regression/misc/00-multipart-parser.t index de39bf0864..ebdadee76d 100644 --- a/tests/regression/misc/00-multipart-parser.t +++ b/tests/regression/misc/00-multipart-parser.t @@ -736,6 +736,45 @@ ), }, +# Single quote within double quotes is ok +{ + type => "misc", + comment => "multipart parser (C-D uses single quote within double quotes)", + conf => qq( + SecRuleEngine On + SecDebugLog $ENV{DEBUG_LOG} + SecDebugLogLevel 9 + SecRequestBodyAccess On + SecRule MULTIPART_INVALID_QUOTING "!\@eq 0" "phase:2,deny,id:500169" + ), + match_log => { + debug => [ qr/Adding request argument \(BODY\): name "a'b/s, 1 ], + -debug => [ qr/Adding request argument \(BODY\): name "b/s, 1 ], + }, + match_response => { + status => qr/^200$/, + }, + request => new HTTP::Request( + POST => "http://$ENV{SERVER_NAME}:$ENV{SERVER_PORT}/test.txt", + [ + "Content-Type" => "multipart/form-data; boundary=---------------------------69343412719991675451336310646", + ], + normalize_raw_request_data( + q( + -----------------------------69343412719991675451336310646 + Content-Disposition: form-data; name="a'b" + + 1 + -----------------------------69343412719991675451336310646 + Content-Disposition: form-data; name="aaa"; filename="d'ummy" + + 2 + -----------------------------69343412719991675451336310646-- + ), + ), + ), +}, + # Invalid boundary separators { type => "misc",