diff --git a/CHANGES b/CHANGES index 9e81c616..53361edd 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,8 @@ v3.x.y - YYYY-MMM-DD (to be released) ------------------------------------- + - Fix: Only delete Multipart tmp files after rules have run + [Issue #2427 - @martinhsv] - Fixed MatchedVar on chained rules [Issue #2423, #2435, #2436 - @michaelgranzow-avi] - Add support for new operator rxGlobal diff --git a/Makefile.am b/Makefile.am index ba7173e3..387e7c83 100644 --- a/Makefile.am +++ b/Makefile.am @@ -159,6 +159,7 @@ TESTS+=test/test-cases/regression/issue-2000.json TESTS+=test/test-cases/regression/issue-2111.json TESTS+=test/test-cases/regression/issue-2196.json TESTS+=test/test-cases/regression/issue-2423-msg-in-chain.json +TESTS+=test/test-cases/regression/issue-2427.json TESTS+=test/test-cases/regression/issue-394.json TESTS+=test/test-cases/regression/issue-849.json TESTS+=test/test-cases/regression/issue-960.json diff --git a/headers/modsecurity/transaction.h b/headers/modsecurity/transaction.h index 2225c532..7d6c3b8a 100644 --- a/headers/modsecurity/transaction.h +++ b/headers/modsecurity/transaction.h @@ -111,6 +111,7 @@ enum AllowType : int; namespace RequestBodyProcessor { class XML; class JSON; +class MultipartPartTmpFile; } namespace operators { class Operator; @@ -612,6 +613,8 @@ class Transaction : public TransactionAnchoredVariables, public TransactionSecMa std::string m_variableTimeWDay; std::string m_variableTimeYear; + std::vector> m_multipartPartTmpFiles; + private: /** * Pointer to the callback function that will be called to fill diff --git a/src/request_body_processor/multipart.cc b/src/request_body_processor/multipart.cc index 032cfd3c..ad83db80 100644 --- a/src/request_body_processor/multipart.cc +++ b/src/request_body_processor/multipart.cc @@ -38,6 +38,61 @@ namespace RequestBodyProcessor { static const char* mime_charset_special = "!#$%&+-^_`{}~"; static const char* attr_char_special = "!#$&+-.^_`~"; +MultipartPartTmpFile::~MultipartPartTmpFile() { + if (!m_tmp_file_name.empty() && m_delete) { + /* make sure it is closed first */ + if (m_tmp_file_fd > 0) { + Close(); + } + + const int unlink_rc = unlink(m_tmp_file_name.c_str()); + if (unlink_rc < 0) { + ms_dbg_a(m_transaction, 1, "Multipart: Failed to delete file (part) \"" \ + + m_tmp_file_name + "\" because " \ + + std::to_string(errno) + "(" \ + + strerror(errno) + ")"); + } else { + ms_dbg_a(m_transaction, 4, "Multipart: file deleted successfully (part) \"" \ + + m_tmp_file_name + "\""); + } + + } +} + +void MultipartPartTmpFile::Open() { + struct tm timeinfo; + char tstr[300]; + time_t tt = time(NULL); + + localtime_r(&tt, &timeinfo); + + memset(tstr, '\0', 300); + strftime(tstr, 299, "/%Y%m%d-%H%M%S", &timeinfo); + + std::string path = m_transaction->m_rules->m_uploadDirectory.m_value; + path = path + tstr + "-" + *m_transaction->m_id.get(); + path += "-file-XXXXXX"; + + char* tmp = strdup(path.c_str()); + m_tmp_file_fd = mkstemp(tmp); + m_tmp_file_name.assign(tmp); + free(tmp); + ms_dbg_a(m_transaction, 4, "MultipartPartTmpFile: Create filename= " + m_tmp_file_name); + + int mode = m_transaction->m_rules->m_uploadFileMode.m_value; + if ((m_tmp_file_fd != -1) && (mode != 0)) { + if (fchmod(m_tmp_file_fd, mode) == -1) { + m_tmp_file_fd = -1; + } + } +} + +void MultipartPartTmpFile::Close() { + close(m_tmp_file_fd); + m_tmp_file_fd = -1; +} + + Multipart::Multipart(const std::string &header, Transaction *transaction) : m_reqbody_no_files_length(0), m_nfiles(0), @@ -80,30 +135,14 @@ Multipart::~Multipart() { if (m_transaction->m_rules->m_uploadKeepFiles != RulesSetProperties::TrueConfigBoolean) { for (MultipartPart *m : m_parts) { - if (m->m_type == MULTIPART_FILE) { - if (!m->m_tmp_file_name.empty()) { - /* make sure it is closed first */ - if (m->m_tmp_file_fd > 0) { - close(m->m_tmp_file_fd); - m->m_tmp_file_fd = -1; - } - const int unlink_rc = - unlink(m->m_tmp_file_name.c_str()); - - if (unlink_rc < 0) { - ms_dbg_a(m_transaction, 1, - "Multipart: Failed to delete file (part) \"" \ - + m->m_tmp_file_name + "\" because " \ - + std::to_string(errno) + "(" \ - + strerror(errno) + ")"); - } else { - ms_dbg_a(m_transaction, 4, - "Multipart: file deleted successfully (part) \"" \ - + m->m_tmp_file_name + "\""); - } - - } + if ((m->m_type == MULTIPART_FILE) && (m->m_tmp_file)) { + // only mark for deletion for now; the file should stay on disk until + // the transaction is complete + ms_dbg_a(m_transaction, 9, "Multipart: Marking temporary file for deletion: " \ + + m->m_tmp_file->getFilename()); + m->m_tmp_file->setDelete(); } + } } @@ -432,7 +471,7 @@ int Multipart::parse_content_disposition(const char *c_d_value, int offset) { + std::to_string(strlen(p)) + " bytes"); m_flag_invalid_quoting = 1; } - p++; + /* p++; */ return -12; } p++; /* move over the semi-colon */ @@ -453,40 +492,6 @@ int Multipart::parse_content_disposition(const char *c_d_value, int offset) { } -int Multipart::tmp_file_name(std::string *filename) const { - std::string path; - struct tm timeinfo; - char tstr[300]; - char *tmp; - int fd; - int mode; - time_t tt = time(NULL); - - localtime_r(&tt, &timeinfo); - - path = m_transaction->m_rules->m_uploadDirectory.m_value; - mode = m_transaction->m_rules->m_uploadFileMode.m_value; - - memset(tstr, '\0', 300); - strftime(tstr, 299, "/%Y%m%d-%H%M%S", &timeinfo); - path = path + tstr + "-" + *m_transaction->m_id.get(); - path = path + "-file-XXXXXX"; - - tmp = strdup(path.c_str()); - - fd = mkstemp(tmp); - filename->assign(tmp); - free(tmp); - if ((fd != -1) && (mode != 0)) { - if (fchmod(fd, mode) == -1) { - return -1; - } - } - - return fd; -} - - int Multipart::process_part_data(std::string *error, size_t offset) { char *p = m_buf + (MULTIPART_BUF_SIZE - m_bufleft); char localreserve[2] = { '\0', '\0' }; /* initialized to quiet warning */ @@ -546,20 +551,18 @@ int Multipart::process_part_data(std::string *error, size_t offset) { */ if (extract) { /* first create a temporary file if we don't have it already */ - if (m_mpp->m_tmp_file_fd == 0) { - std::string path; - m_mpp->m_tmp_file_fd = tmp_file_name(&path); - - /* construct temporary file name */ - m_mpp->m_tmp_file_name = path; + if (!m_mpp->m_tmp_file || !m_mpp->m_tmp_file->isValid()) { + m_mpp->m_tmp_file = std::make_shared(m_transaction); + m_transaction->m_multipartPartTmpFiles.push_back(m_mpp->m_tmp_file); + m_mpp->m_tmp_file->Open(); /* do we have an opened file? */ - if (m_mpp->m_tmp_file_fd < 0) { + if (!m_mpp->m_tmp_file || m_mpp->m_tmp_file->getFd() < 0) { ms_dbg_a(m_transaction, 1, "Multipart: Failed to create file: " \ - + m_mpp->m_tmp_file_name); + + m_mpp->m_tmp_file->getFilename()); error->assign("Multipart: Failed to create file: " \ - + m_mpp->m_tmp_file_name); + + m_mpp->m_tmp_file->getFilename()); return -1; } /* keep track of the files count */ @@ -569,18 +572,18 @@ int Multipart::process_part_data(std::string *error, size_t offset) { ms_dbg_a(m_transaction, 4, "Multipart: Created temporary file " \ + std::to_string(m_nfiles) + " (mode o" + std::to_string(m_transaction->m_rules->m_uploadFileMode.m_value) + "): " \ - + m_mpp->m_tmp_file_name); + + m_mpp->m_tmp_file->getFilename()); } /* write the reserve first */ if (m_reserve[0] != 0) { - if (write(m_mpp->m_tmp_file_fd, &m_reserve[1], m_reserve[0]) + if (write(m_mpp->m_tmp_file->getFd(), &m_reserve[1], m_reserve[0]) != m_reserve[0]) { ms_dbg_a(m_transaction, 1, "Multipart: writing to \"" \ - + m_mpp->m_tmp_file_name + "\" failed"); + + m_mpp->m_tmp_file->getFilename() + "\" failed"); error->assign("Multipart: writing to \"" \ - + m_mpp->m_tmp_file_name + "\" failed"); + + m_mpp->m_tmp_file->getFilename() + "\" failed"); return -1; } @@ -594,14 +597,14 @@ int Multipart::process_part_data(std::string *error, size_t offset) { /* write data to the file */ - if (write(m_mpp->m_tmp_file_fd, m_buf, + if (write(m_mpp->m_tmp_file->getFd(), m_buf, MULTIPART_BUF_SIZE - m_bufleft) != (MULTIPART_BUF_SIZE - m_bufleft)) { ms_dbg_a(m_transaction, 1, "Multipart: writing to \"" \ - + m_mpp->m_tmp_file_name + "\" failed"); + + m_mpp->m_tmp_file->getFilename() + "\" failed"); error->assign("Multipart: writing to \"" \ - + m_mpp->m_tmp_file_name + "\" failed"); + + m_mpp->m_tmp_file->getFilename() + "\" failed"); return -1; } @@ -911,11 +914,9 @@ int Multipart::process_boundary(int last_part) { /* if there was a part being built finish it */ if (m_mpp != NULL) { /* close the temp file */ - if ((m_mpp->m_type == MULTIPART_FILE) - && (!m_mpp->m_tmp_file_name.empty()) - && (m_mpp->m_tmp_file_fd != 0)) { - close(m_mpp->m_tmp_file_fd); - m_mpp->m_tmp_file_fd = -1; + if ((m_mpp->m_type == MULTIPART_FILE) && (m_mpp->m_tmp_file) + && (m_mpp->m_tmp_file->isValid())) { + m_mpp->m_tmp_file->Close(); } if (m_mpp->m_type != MULTIPART_FILE) { @@ -1128,8 +1129,10 @@ int Multipart::multipart_complete(std::string *error) { if (m->m_type == MULTIPART_FILE) { std::string tmp_name; std::string name; - if (!m->m_tmp_file_name.empty()) { - tmp_name.assign(m->m_tmp_file_name); + if (m->m_tmp_file && !m->m_tmp_file->getFilename().empty()) { + tmp_name.assign(m->m_tmp_file->getFilename()); + m_transaction->m_variableFilesTmpNames.set(m->m_tmp_file->getFilename(), + m->m_tmp_file->getFilename(), m->m_filenameOffset); } if (!m->m_filename.empty()) { name.assign(m->m_filename); @@ -1149,9 +1152,6 @@ int Multipart::multipart_complete(std::string *error) { m_transaction->m_variableFilesTmpContent.set(m->m_filename, m->m_value, m->m_valueOffset); - m_transaction->m_variableFilesTmpNames.set(m->m_tmp_file_name, - m->m_tmp_file_name, m->m_filenameOffset); - file_combined_size = file_combined_size + m->m_tmp_file_size.first; m_transaction->m_variableFilesCombinedSize.set( diff --git a/src/request_body_processor/multipart.h b/src/request_body_processor/multipart.h index 674025d7..1a3d73cd 100644 --- a/src/request_body_processor/multipart.h +++ b/src/request_body_processor/multipart.h @@ -54,6 +54,38 @@ struct MyEqual { }; +class MultipartPartTmpFile { + public: + explicit MultipartPartTmpFile(Transaction *transaction) + : m_transaction(transaction), + m_tmp_file_fd(0), + m_delete(false) + { } + + ~MultipartPartTmpFile(); + + // forbid copying + MultipartPartTmpFile(const MultipartPartTmpFile&) = delete; + MultipartPartTmpFile& operator=(const MultipartPartTmpFile&) = delete; + + int getFd() const {return m_tmp_file_fd;} + void setFd(int fd) {m_tmp_file_fd = fd;} + const std::string& getFilename() const {return m_tmp_file_name;} + void setDelete() {m_delete = true;} + + bool isValid() const {return ((m_tmp_file_fd != 0) && (!m_tmp_file_name.empty()));} + + void Open(); + void Close(); + + private: + Transaction *m_transaction; + int m_tmp_file_fd; + std::string m_tmp_file_name; + bool m_delete; // whether to delete when transaction is done +}; + + class MultipartPart { public: MultipartPart() @@ -63,8 +95,6 @@ class MultipartPart { m_value(""), m_valueOffset(0), m_value_parts(), - m_tmp_file_name(""), - m_tmp_file_fd(0), m_tmp_file_size(), m_filename(""), m_filenameOffset(0), @@ -98,8 +128,7 @@ class MultipartPart { /* std::string m_content_type; */ /* files only, the name of the temporary file holding data */ - std::string m_tmp_file_name; - int m_tmp_file_fd; + std::shared_ptr m_tmp_file; std::pair m_tmp_file_size; /* files only, filename as supplied by the browser */ @@ -133,8 +162,6 @@ class Multipart { int process_part_header(std::string *error, int offset); int process_part_data(std::string *error, size_t offset); - int tmp_file_name(std::string *filename) const; - void validate_quotes(const char *data); size_t m_reqbody_no_files_length; diff --git a/test/cppcheck_suppressions.txt b/test/cppcheck_suppressions.txt index 8d3a5874..8962ce74 100644 --- a/test/cppcheck_suppressions.txt +++ b/test/cppcheck_suppressions.txt @@ -35,7 +35,7 @@ invalidScanfArgType_int:src/rules_set_properties.cc:102 unmatchedSuppression:src/utils/geo_lookup.cc:82 useInitializationList:src/utils/shared_files.h:87 unmatchedSuppression:src/utils/msc_tree.cc -functionStatic:headers/modsecurity/transaction.h:404 +functionStatic:headers/modsecurity/transaction.h:405 duplicateBranch:src/audit_log/audit_log.cc:223 unreadVariable:src/request_body_processor/multipart.cc:435 stlcstrParam:src/audit_log/writer/parallel.cc:145 diff --git a/test/test-cases/data/inspectFile-abcdef.lua b/test/test-cases/data/inspectFile-abcdef.lua new file mode 100644 index 00000000..13164633 --- /dev/null +++ b/test/test-cases/data/inspectFile-abcdef.lua @@ -0,0 +1,10 @@ +#!/usr/bin/lua + +function main(filename) + local file = io.open(filename, 'r') + local chunk = file:read(1024) + local ret = string.match(chunk, 'abcdef') + io.close(file) + + return ret +end diff --git a/test/test-cases/regression/issue-2427.json b/test/test-cases/regression/issue-2427.json new file mode 100644 index 00000000..02f7b16f --- /dev/null +++ b/test/test-cases/regression/issue-2427.json @@ -0,0 +1,121 @@ +[ + { + "enabled":1, + "version_min":300000, + "title":"Tmp file retained for @inspectFile (1/2)", + "resource":"lua", + "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":"/wheee/f%20i%20l%20e%20", + "method":"POST", + "body":[ + "----------------------------756b6d74fa1a8ee2", + "Content-Disposition: form-data; name=\"name\"", + "", + "test", + "----------------------------756b6d74fa1a8ee2", + "Content-Disposition: form-data; name=\"name2\"", + "", + "test2", + "----------------------------756b6d74fa1a8ee2", + "Content-Disposition: form-data; name=\"filedata\"; filename=\"small_text_file1.txt\"", + "Content-Type: text/plain", + "", + "This is a very small test file..", + "----------------------------756b6d74fa1a8ee2", + "Content-Disposition: form-data; filename=\"small_text_file2.txt\"; name=\"small2.txt\" ", + "Content-Type: text/plain", + "", + "This is another very small test file that contains the search content abcdef..", + "----------------------------756b6d74fa1a8ee2--" + ] + }, + "expected":{ + "debug_log":"Returning from lua script: abcdef.*Rule returned 1", + "http_code":403 + }, + "rules":[ + "SecRuleEngine On", + "SecRequestBodyAccess On", + "SecTmpSaveUploadedFiles On", + "SecUploadKeepFiles Off", + "SecUploadDir /tmp", + "SecTmpDir /tmp", + "SecRule FILES_TMPNAMES \"@inspectFile test-cases/data/inspectFile-abcdef.lua\" \"id:1,phase:2,deny,status:403\"" + ] + }, + { + "enabled":1, + "version_min":300000, + "title":"Tmp file retained for @inspectFile (2/2)", + "resource":"lua", + "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":"/wheee/f%20i%20l%20e%20", + "method":"POST", + "body":[ + "----------------------------756b6d74fa1a8ee2", + "Content-Disposition: form-data; name=\"name\"", + "", + "test", + "----------------------------756b6d74fa1a8ee2", + "Content-Disposition: form-data; name=\"name2\"", + "", + "test2", + "----------------------------756b6d74fa1a8ee2", + "Content-Disposition: form-data; name=\"filedata\"; filename=\"small_text_file1.txt\"", + "Content-Type: text/plain", + "", + "This is a very small test file..", + "----------------------------756b6d74fa1a8ee2", + "Content-Disposition: form-data; filename=\"small_text_file2.txt\"; name=\"small2.txt\" ", + "Content-Type: text/plain", + "", + "This is another very small test file that does not contain the search content.", + "----------------------------756b6d74fa1a8ee2--" + ] + }, + "expected":{ + "http_code":200 + }, + "rules":[ + "SecRuleEngine On", + "SecRequestBodyAccess On", + "SecTmpSaveUploadedFiles On", + "SecUploadKeepFiles Off", + "SecUploadDir /tmp", + "SecTmpDir /tmp", + "SecRule FILES_TMPNAMES \"@inspectFile test-cases/data/inspectFile-abcdef.lua\" \"id:1,phase:2,deny,status:403\"" + ] + } +]