From 33811156a04cdcdac548728a1422485ecc4023fa Mon Sep 17 00:00:00 2001 From: martinhsv <55407942+martinhsv@users.noreply.github.com> Date: Thu, 5 Dec 2019 12:51:22 -0800 Subject: [PATCH] Multipart Content-Disposition should allow filename* field --- CHANGES | 2 + src/request_body_processor/multipart.cc | 104 ++++++-- test/test-cases/regression/issue-1825.json | 291 +++++++++++++++++++++ 3 files changed, 374 insertions(+), 23 deletions(-) create mode 100644 test/test-cases/regression/issue-1825.json diff --git a/CHANGES b/CHANGES index 000f843841..0134e749fd 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,8 @@ v3.0.4 - YYYY-MMM-DD (to be released) ------------------------------------- + - Multipart Content-Dispostion should allow field: filename*= + [@martinhsv] - Fix: ModSecurity 3.x inspectFile operator does not pass FILES_TMPNAMES parameter to lua engine [Issue #2204, #2205 - @kadirerdogan] diff --git a/src/request_body_processor/multipart.cc b/src/request_body_processor/multipart.cc index b38d098644..f72637ae5f 100644 --- a/src/request_body_processor/multipart.cc +++ b/src/request_body_processor/multipart.cc @@ -35,6 +35,8 @@ namespace modsecurity { namespace RequestBodyProcessor { +static const char* mime_charset_special = "!#$%&+-^_`{}~"; +static const char* attr_char_special = "!#$&+-.^_`~"; Multipart::Multipart(std:: string header, Transaction *transaction) : m_reqbody_no_files_length(0), @@ -208,6 +210,7 @@ void Multipart::validate_quotes(const char *data) { int Multipart::parse_content_disposition(const char *c_d_value, int offset) { const char *p = NULL; + std::string filenameStar; /* accept only what we understand */ if (strncmp(c_d_value, "form-data", 9) != 0) { @@ -276,13 +279,45 @@ int Multipart::parse_content_disposition(const char *c_d_value, int offset) { return -6; } - /* Accept both quotes as some backends will accept them, but - * technically "'" is invalid and so flag_invalid_quoting is - * set so the user can deal with it in the rules if they so wish. - */ + if (name == "filename*") { + /* filename*=charset'[optional-language]'filename */ + /* Read beyond the charset and the optional language*/ + const char* start_of_charset = p; + while ((*p != '\0') && (isalnum(*p) || (strchr(mime_charset_special, *p)))) { + p++; + } + if ((*p != '\'') || (p == start_of_charset)) { + return -16; // Must be at least one legit char before ' for start of language + } + p++; + while ((*p != '\0') && (*p != '\'')) { + p++; + } + if (*p != '\'') { + return -17; // Single quote for end-of-language not found + } + p++; - if ((*p == '"') || (*p == '\'')) { - /* quoted */ + /* Now read what should be the actual filename */ + const char* start_of_filename = p; + while ((*p != '\0') && (*p != ';')) { + if (*p == '%') { + if ((*(p+1) == '\0') || (!isxdigit(*(p+1))) || (!isxdigit(*p+2))) { + return -18; + } + p += 3; + } else if (isalnum(*p) || strchr(attr_char_special, *p)) { + p++; + } else { + return -19; + } + } + value.assign(start_of_filename, p - start_of_filename); + } else if ((*p == '"') || (*p == '\'')) { + /* Accept both quotes as some backends will accept them, but + * technically "'" is invalid and so flag_invalid_quoting is + * set so the user can deal with it in the rules if they so wish. + */ char quote = *p; if (quote == '\'') { @@ -364,8 +399,17 @@ int Multipart::parse_content_disposition(const char *c_d_value, int offset) { m_mpp->m_filenameOffset = offset + ((p - c_d_value) - value.size()); ms_dbg_a(m_transaction, 9, - "Multipart: Content-Disposition filename: " \ - + value + "."); + "Multipart: Content-Disposition filename: " + value + "."); + } else if (name == "filename*") { + if (!filenameStar.empty()) { + ms_dbg_a(m_transaction, 4, + "Multipart: Warning: Duplicate Content-Disposition " \ + "filename*: " + value + "."); + return -20; + } + filenameStar.assign(value); + ms_dbg_a(m_transaction, 9, + "Multipart: Content-Disposition filename*: " + value + "."); } else { return -11; } @@ -377,26 +421,34 @@ int Multipart::parse_content_disposition(const char *c_d_value, int offset) { /* the next character must be a zero or a semi-colon */ if (*p == '\0') { - return 1; /* this is OK */ - } - if (*p != ';') { - p--; - if (*p == '\'' || *p == '\"') { - ms_dbg_a(m_transaction, 9, - "Multipart: Invalid quoting detected: " \ - + std::string(p) + " length " \ - + std::to_string(strlen(p)) + " bytes"); - m_flag_invalid_quoting = 1; + // Just let the 'while' condition end the loop + } else { + if (*p != ';') { + p--; + if (*p == '\'' || *p == '\"') { + ms_dbg_a(m_transaction, 9, + "Multipart: Invalid quoting detected: " \ + + std::string(p) + " length " \ + + std::to_string(strlen(p)) + " bytes"); + m_flag_invalid_quoting = 1; + } + p++; + return -12; } - p++; - return -12; + p++; /* move over the semi-colon */ } - p++; /* move over the semi-colon */ } /* loop will stop when (*p == '\0') */ } + if (!filenameStar.empty() && m_mpp->m_filename.empty()) { + ms_dbg_a(m_transaction, 4, + "Multipart: Warning: no filename= but filename*:" \ + + filenameStar + "."); + return -21; + } + return 1; } @@ -675,8 +727,14 @@ int Multipart::process_part_header(std::string *error, int offset) { } header_value = m_mpp->m_headers.at("Content-Disposition").second; - rc = parse_content_disposition(header_value.c_str(), - m_mpp->m_headers.at("Content-Disposition").first); + try { + rc = parse_content_disposition(header_value.c_str(), + m_mpp->m_headers.at("Content-Disposition").first); + } catch (...) { + ms_dbg_a(m_transaction, 1, + "Multipart: Unexpected error parsing Content-Disposition header."); + rc = -99; + } if (rc < 0) { ms_dbg_a(m_transaction, 1, "Multipart: Invalid Content-Disposition header (" diff --git a/test/test-cases/regression/issue-1825.json b/test/test-cases/regression/issue-1825.json new file mode 100644 index 0000000000..6bc0898b63 --- /dev/null +++ b/test/test-cases/regression/issue-1825.json @@ -0,0 +1,291 @@ +[ + { + "enabled":1, + "version_min":300000, + "title":"multipart Content-Disposition should allow filename* field (1/6)", + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*", + "Content-Length":"330", + "Content-Type":"multipart/form-data; boundary=--------------------------756b6d74fa1a8ee2", + "Expect":"100-continue" + }, + "uri":"/", + "method":"POST", + "body":[ + "----------------------------756b6d74fa1a8ee2", + "Content-Disposition: form-data; name=\"name\"", + "", + "test", + "----------------------------756b6d74fa1a8ee2", + "Content-Disposition: form-data; name=\"filedata\"; filename=\"03CB1664.txt\"; filename*=utf-8''03CB1664.txt", + "Content-Type: text/plain", + "", + "This is a very small test file..", + "----------------------------756b6d74fa1a8ee2--" + ] + }, + "response":{ + "headers":"", + "body":"" + }, + "expected":{ + "debug_log":"Target value: \"03CB1664.txt\" \\(Variable: MULTIPART_FILENAME" + }, + "rules":[ + "SecRuleEngine On", + "SecRule MULTIPART_FILENAME \"@contains 0\" \"id:1,phase:2,pass,t:trim\"" + ] + }, + { + "enabled":1, + "version_min":300000, + "title":"multipart Content-Disposition should allow filename* field (2/6)", + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*", + "Content-Length":"330", + "Content-Type":"multipart/form-data; boundary=--------------------------756b6d74fa1a8ee2", + "Expect":"100-continue" + }, + "uri":"/", + "method":"POST", + "body":[ + "----------------------------756b6d74fa1a8ee2", + "Content-Disposition: form-data; name=\"name\"", + "", + "test", + "----------------------------756b6d74fa1a8ee2", + "Content-Disposition: form-data; name=\"filedata\"; filename*= ISO-8859-1''ab0-_xy.txt; filename=\"ab0-_xy.txt\"", + "Content-Type: text/plain", + "", + "This is a very small test file..", + "----------------------------756b6d74fa1a8ee2--" + ] + }, + "response":{ + "headers":"", + "body":"" + }, + "expected":{ + "debug_log":"Target value: \"ab0-_xy.txt\" \\(Variable: MULTIPART_FILENAME" + }, + "rules":[ + "SecRuleEngine On", + "SecRule MULTIPART_FILENAME \"@contains 0\" \"id:1,phase:2,pass,t:trim\"" + ] + }, + { + "enabled":1, + "version_min":300000, + "title":"multipart Content-Disposition should allow filename* field (3/6)", + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*", + "Content-Length":"330", + "Content-Type":"multipart/form-data; boundary=--------------------------756b6d74fa1a8ee2", + "Expect":"100-continue" + }, + "uri":"/", + "method":"POST", + "body":[ + "----------------------------756b6d74fa1a8ee2", + "Content-Disposition: form-data; name=\"name\"", + "", + "test", + "----------------------------756b6d74fa1a8ee2", + "Content-Disposition: form-data; name=\"filedata\"; filename*=utf-8''03CB1664.txt", + "Content-Type: text/plain", + "", + "This is a very small test file..", + "----------------------------756b6d74fa1a8ee2--\r" + ] + }, + "response":{ + "headers":"", + "body":"" + }, + "expected":{ + "debug_log":"Warning: no filename= but filename*" + }, + "rules":[ + "SecRuleEngine On", + "SecRule MULTIPART_FILENAME \"@contains 0\" \"id:1,phase:2,pass,t:trim\"" + ] + }, + { + "enabled":1, + "version_min":300000, + "title":"multipart Content-Disposition should allow filename* field (4/6)", + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*", + "Content-Length":"330", + "Content-Type":"multipart/form-data; boundary=--------------------------756b6d74fa1a8ee2", + "Expect":"100-continue" + }, + "uri":"/", + "method":"POST", + "body":[ + "----------------------------756b6d74fa1a8ee2", + "Content-Disposition: form-data; name=\"name\"", + "", + "test", + "----------------------------756b6d74fa1a8ee2", + "Content-Disposition: form-data; name=\"filedata\"; filename=\"03CB1664.txt\"; filename*=''03CB1664.txt", + "Content-Type: text/plain", + "", + "This is a very small test file..", + "----------------------------756b6d74fa1a8ee2--" + ] + }, + "response":{ + "headers":"", + "body":"" + }, + "expected":{ + "debug_log":"Multipart: Invalid Content-Disposition header \\(-16" + }, + "rules":[ + "SecRuleEngine On", + "SecRule MULTIPART_FILENAME \"@contains 0\" \"id:1,phase:2,pass,t:trim\"" + ] + }, + { + "enabled":1, + "version_min":300000, + "title":"multipart Content-Disposition should allow filename* field (5/6)", + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*", + "Content-Length":"330", + "Content-Type":"multipart/form-data; boundary=--------------------------756b6d74fa1a8ee2", + "Expect":"100-continue" + }, + "uri":"/", + "method":"POST", + "body":[ + "----------------------------756b6d74fa1a8ee2", + "Content-Disposition: form-data; name=\"name\"", + "", + "test", + "----------------------------756b6d74fa1a8ee2", + "Content-Disposition: form-data; name=\"filedata\"; filename=\"03CB1664.txt\"; filename*=UTF-8'03CB1664.txt", + "Content-Type: text/plain", + "", + "This is a very small test file..", + "----------------------------756b6d74fa1a8ee2--" + ] + }, + "response":{ + "headers":"", + "body":"" + }, + "expected":{ + "debug_log":"Multipart: Invalid Content-Disposition header \\(-17" + }, + "rules":[ + "SecRuleEngine On", + "SecRule MULTIPART_FILENAME \"@contains 0\" \"id:1,phase:2,pass,t:trim\"" + ] + }, + { + "enabled":1, + "version_min":300000, + "title":"multipart Content-Disposition should allow filename* field (6/6)", + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*", + "Content-Length":"330", + "Content-Type":"multipart/form-data; boundary=--------------------------756b6d74fa1a8ee2", + "Expect":"100-continue" + }, + "uri":"/", + "method":"POST", + "body":[ + "----------------------------756b6d74fa1a8ee2", + "Content-Disposition: form-data; name=\"name\"", + "", + "test", + "----------------------------756b6d74fa1a8ee2", + "Content-Disposition: form-data; name=\"filedata\"; filename=\"03CB1664.txt\"; filename*=utf-8''%61%4G.txt", + "Content-Type: text/plain", + "", + "This is a very small test file..", + "----------------------------756b6d74fa1a8ee2--" + ] + }, + "response":{ + "headers":"", + "body":"" + }, + "expected":{ + "debug_log":"Multipart: Invalid Content-Disposition header \\(-18" + }, + "rules":[ + "SecRuleEngine On", + "SecRule MULTIPART_FILENAME \"@contains 0\" \"id:1,phase:2,pass,t:trim\"" + ] + } +] +