From 83d3900580a04400547e28b89725fb120d02f3f1 Mon Sep 17 00:00:00 2001 From: Tom Sommer Date: Sat, 19 Sep 2026 16:11:52 +0200 Subject: [PATCH] Fix fd and temporary file leak on truncated multipart payloads When SecUploadKeepFiles or SecTmpSaveUploadedFiles is enabled, each MULTIPART_FILE part is extracted to a temporary file in SecUploadDir. The file descriptor is closed and the file is marked for deletion in Multipart::process_boundary(), which is only reached when the boundary that terminates the part is seen. If the request body ends without the final boundary, the part that was still being built is left in Multipart::m_mpp: it is never pushed to Multipart::m_parts and process_boundary() is never called for it. The destructor of Multipart only marked the parts in m_parts for deletion, so the temporary file of the dangling part was neither closed nor unlinked. Since the shared_ptr to the MultipartPartTmpFile has been stored in Transaction::m_multipartPartTmpFiles, it survived until ~Transaction, where ~MultipartPartTmpFile closed the descriptor only inside the m_delete branch. The net effect was one leaked file descriptor and one orphaned file in SecUploadDir per request, which is trivially triggerable by a remote client sending a truncated multipart body. Three changes address this: - ~MultipartPartTmpFile() now always closes the descriptor when one is open, instead of doing it only when the file is also marked for deletion. - ~Multipart() applies the same mark-for-deletion treatment to m_mpp as it already does for the parts in m_parts. Multipart is a stack object in Transaction::processRequestBody(), so it is destroyed before m_multipartPartTmpFiles, and the mark is honoured when the shared_ptr is released. - MultipartPartTmpFile::Open() closes the descriptor before invalidating it when fchmod()/_chmod() fails, instead of just overwriting it with -1. - The descriptor is initialised to -1 and MultipartPartTmpFile::isValid() accepts any descriptor >= 0, so -1 is the only invalid value. Before, 0 was used as the unset marker and a descriptor 0 returned by mkstemp() would have been treated as not open. A regression test with a multipart body containing a file part and no final boundary is added; under valgrind --track-fds=yes the unfixed code reports an open descriptor for the temporary file at exit and leaves the file in SecUploadDir. --- src/request_body_processor/multipart.cc | 20 +++++-- src/request_body_processor/multipart.h | 4 +- ...quest-body-parser-multipart-truncated.json | 55 +++++++++++++++++++ test/test-suite.in | 1 + 4 files changed, 72 insertions(+), 8 deletions(-) create mode 100644 test/test-cases/regression/request-body-parser-multipart-truncated.json diff --git a/src/request_body_processor/multipart.cc b/src/request_body_processor/multipart.cc index c8d78c7e00..9ba7016869 100644 --- a/src/request_body_processor/multipart.cc +++ b/src/request_body_processor/multipart.cc @@ -44,12 +44,12 @@ 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(); - } + /* make sure it is closed first */ + if (m_tmp_file_fd >= 0) { + Close(); + } + if (!m_tmp_file_name.empty() && m_delete) { 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) \"" \ @@ -93,7 +93,7 @@ void MultipartPartTmpFile::Open() { #else if (_chmod(m_tmp_file_name.c_str(), mode) == -1) { #endif - m_tmp_file_fd = -1; + Close(); } } } @@ -158,6 +158,14 @@ Multipart::~Multipart() { } } + + /* the part being built when the payload ended is not in m_parts */ + if ((m_mpp != nullptr) && (m_mpp->m_type == MULTIPART_FILE) + && (m_mpp->m_tmp_file)) { + ms_dbg_a(m_transaction, 9, "Multipart: Marking temporary file for deletion: " \ + + m_mpp->m_tmp_file->getFilename()); + m_mpp->m_tmp_file->setDelete(); + } } while (m_parts.empty() == false) { diff --git a/src/request_body_processor/multipart.h b/src/request_body_processor/multipart.h index 08d4ffe920..c7a1be7732 100644 --- a/src/request_body_processor/multipart.h +++ b/src/request_body_processor/multipart.h @@ -58,7 +58,7 @@ class MultipartPartTmpFile { public: explicit MultipartPartTmpFile(Transaction *transaction) : m_transaction(transaction), - m_tmp_file_fd(0), + m_tmp_file_fd(-1), m_delete(false) { } @@ -73,7 +73,7 @@ class MultipartPartTmpFile { 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()));} + bool isValid() const {return ((m_tmp_file_fd >= 0) && (!m_tmp_file_name.empty()));} void Open(); void Close(); diff --git a/test/test-cases/regression/request-body-parser-multipart-truncated.json b/test/test-cases/regression/request-body-parser-multipart-truncated.json new file mode 100644 index 0000000000..f5b3b4f535 --- /dev/null +++ b/test/test-cases/regression/request-body-parser-multipart-truncated.json @@ -0,0 +1,55 @@ +[ + { + "enabled": 1, + "version_min": 300000, + "title": "multipart parser (truncated body, file part without final boundary)", + "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": "117", + "Content-Type": "multipart/form-data; boundary=0000" + }, + "uri": "/", + "method": "POST", + "body": [ + "--0000\r\n", + "Content-Disposition: form-data; name=\"file1\"; filename=\"upload.txt\"\r\n", + "Content-Type: text/plain\r\n", + "\r\n", + "UPLOADDATA\r\n" + ] + }, + "response": { + "headers": { + "Date": "Mon, 13 Jul 2015 20:02:41 GMT", + "Last-Modified": "Sun, 26 Oct 2014 22:33:37 GMT", + "Content-Type": "text/html", + "Content-Length": "8" + }, + "body": [ + "no need." + ] + }, + "expected": { + "debug_log": "Multipart: Final boundary missing\\.", + "http_code": 200 + }, + "rules": [ + "SecRuleEngine On", + "SecRequestBodyAccess On", + "SecTmpSaveUploadedFiles On", + "SecUploadDir /tmp", + "SecRule REQBODY_ERROR \"!@eq 0\" \"id:500058,phase:2,pass,log\"" + ] + } +] diff --git a/test/test-suite.in b/test/test-suite.in index ebda49fb81..19f5971648 100644 --- a/test/test-suite.in +++ b/test/test-suite.in @@ -98,6 +98,7 @@ TESTS+=test/test-cases/regression/operator-verifysvnr.json TESTS+=test/test-cases/regression/request-body-parser-json.json TESTS+=test/test-cases/regression/request-body-parser-multipart-crlf.json TESTS+=test/test-cases/regression/request-body-parser-multipart.json +TESTS+=test/test-cases/regression/request-body-parser-multipart-truncated.json TESTS+=test/test-cases/regression/request-body-parser-xml.json TESTS+=test/test-cases/regression/request-body-parser-xml-validade-dtd.json TESTS+=test/test-cases/regression/rule-920120.json