Fix: Only delete Multipart tmp files after rules have run

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

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

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<std::shared_ptr<RequestBodyProcessor::MultipartPartTmpFile>> m_multipartPartTmpFiles;
+
  private:
     /**
      * Pointer to the callback function that will be called to fill

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<RequestBodyProcessor::MultipartPartTmpFile>(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(

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<RequestBodyProcessor::MultipartPartTmpFile> m_tmp_file;
     std::pair<size_t, size_t> 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;

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

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

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\""
+    ]
+  }
+]