From 9a7618ca059057b8270331c4131b2c4fa0de1589 Mon Sep 17 00:00:00 2001 From: Brandon Payton Date: Thu, 30 Apr 2020 17:57:30 -0700 Subject: [PATCH 1/2] Support reopening audit logs --- headers/modsecurity/audit_log.h | 1 + headers/modsecurity/rules_set.h | 1 + src/audit_log/audit_log.cc | 7 ++++ src/audit_log/writer/https.h | 3 ++ src/audit_log/writer/parallel.cc | 32 ++++++++++++++++++ src/audit_log/writer/parallel.h | 1 + src/audit_log/writer/serial.cc | 15 +++++++++ src/audit_log/writer/serial.h | 1 + src/audit_log/writer/writer.h | 1 + src/rules_set.cc | 21 ++++++++++++ src/utils/shared_files.cc | 58 ++++++++++++++++++++++++++++++++ src/utils/shared_files.h | 1 + 12 files changed, 142 insertions(+) diff --git a/headers/modsecurity/audit_log.h b/headers/modsecurity/audit_log.h index b0bc923efc..9f266bc00c 100644 --- a/headers/modsecurity/audit_log.h +++ b/headers/modsecurity/audit_log.h @@ -168,6 +168,7 @@ class AuditLog { bool setType(AuditLogType audit_type); bool init(std::string *error); + bool reopen(std::string *error); virtual bool close(); bool saveIfRelevant(Transaction *transaction); diff --git a/headers/modsecurity/rules_set.h b/headers/modsecurity/rules_set.h index 66d9eecde8..00a4d4c0d9 100644 --- a/headers/modsecurity/rules_set.h +++ b/headers/modsecurity/rules_set.h @@ -104,6 +104,7 @@ int msc_rules_add_remote(RulesSet *rules, const char *key, const char *uri, int msc_rules_add_file(RulesSet *rules, const char *file, const char **error); int msc_rules_add(RulesSet *rules, const char *plain_rules, const char **error); int msc_rules_cleanup(RulesSet *rules); +int msc_rules_reopen_audit_log(RulesSet *rules, const char **error); #ifdef __cplusplus } diff --git a/src/audit_log/audit_log.cc b/src/audit_log/audit_log.cc index aa63aad0c9..db1d3472dd 100644 --- a/src/audit_log/audit_log.cc +++ b/src/audit_log/audit_log.cc @@ -372,6 +372,13 @@ bool AuditLog::merge(AuditLog *from, std::string *error) { return init(error); } +bool AuditLog::reopen(std::string *error) { + if (m_writer != NULL) { + return m_writer->reopen(error); + } + return true; +} + } // namespace audit_log } // namespace modsecurity diff --git a/src/audit_log/writer/https.h b/src/audit_log/writer/https.h index 086232800b..c0d92bab39 100644 --- a/src/audit_log/writer/https.h +++ b/src/audit_log/writer/https.h @@ -42,6 +42,9 @@ class Https : public Writer { bool init(std::string *error) override; bool write(Transaction *transaction, int parts, std::string *error) override; + bool reopen(std::string *error) override { + return true; + } }; } // namespace writer diff --git a/src/audit_log/writer/parallel.cc b/src/audit_log/writer/parallel.cc index 44d4fe5b7d..878d784853 100644 --- a/src/audit_log/writer/parallel.cc +++ b/src/audit_log/writer/parallel.cc @@ -191,6 +191,38 @@ bool Parallel::write(Transaction *transaction, int parts, std::string *error) { return true; } + +bool Parallel::reopen(std::string *error) { + bool success1 = true; + bool success2 = true; + std::string error1; + std::string error2; + + if (!m_audit->m_path1.empty()) { + success1 = utils::SharedFiles::getInstance().reopen(m_audit->m_path1, &error1); + } + if (!m_audit->m_path2.empty()) { + success2 = utils::SharedFiles::getInstance().reopen(m_audit->m_path2, &error2); + } + + std::stringstream errorStream; + if (!success1 || !success2) { + errorStream << "There was an error reopening parallel audit logs."; + + if (!success1 && !error1.empty()) { + errorStream << " " << error1; + } + if (!success2 && !error2.empty()) { + errorStream << " " << error2; + } + + *error = errorStream.str(); + } + + return success1 && success2; +} + + } // namespace writer } // namespace audit_log } // namespace modsecurity diff --git a/src/audit_log/writer/parallel.h b/src/audit_log/writer/parallel.h index 46b7590d9d..15dc753549 100644 --- a/src/audit_log/writer/parallel.h +++ b/src/audit_log/writer/parallel.h @@ -40,6 +40,7 @@ class Parallel : public Writer { bool init(std::string *error) override; bool write(Transaction *transaction, int parts, std::string *error) override; + bool reopen(std::string *error) override; /** diff --git a/src/audit_log/writer/serial.cc b/src/audit_log/writer/serial.cc index c6b1d5e603..17bc53a5cf 100644 --- a/src/audit_log/writer/serial.cc +++ b/src/audit_log/writer/serial.cc @@ -49,6 +49,21 @@ bool Serial::write(Transaction *transaction, int parts, std::string *error) { error); } + +bool Serial::reopen(std::string *error) { + bool success; + + std::string errorDetail; + success = utils::SharedFiles::getInstance().reopen(m_audit->m_path1, &errorDetail); + if (!success) { + *error = "There was an error reopening the serial audit log. "; + error->append(errorDetail); + } + + return success; +} + + } // namespace writer } // namespace audit_log } // namespace modsecurity diff --git a/src/audit_log/writer/serial.h b/src/audit_log/writer/serial.h index b855574bba..698cced611 100644 --- a/src/audit_log/writer/serial.h +++ b/src/audit_log/writer/serial.h @@ -47,6 +47,7 @@ class Serial : public Writer { bool init(std::string *error) override; bool write(Transaction *transaction, int parts, std::string *error) override; + bool reopen(std::string *error) override; }; } // namespace writer diff --git a/src/audit_log/writer/writer.h b/src/audit_log/writer/writer.h index 5dd1c194c5..cdf4d0671d 100644 --- a/src/audit_log/writer/writer.h +++ b/src/audit_log/writer/writer.h @@ -50,6 +50,7 @@ class Writer { virtual bool init(std::string *error) = 0; virtual bool write(Transaction *transaction, int parts, std::string *error) = 0; + virtual bool reopen(std::string *error) = 0; static void generateBoundary(std::string *boundary); diff --git a/src/rules_set.cc b/src/rules_set.cc index 91383d3f0d..f12de9b266 100644 --- a/src/rules_set.cc +++ b/src/rules_set.cc @@ -318,5 +318,26 @@ extern "C" int msc_rules_cleanup(RulesSet *rules) { } +extern "C" int msc_rules_reopen_audit_log(RulesSet *rules, const char **error) { + bool succeeded = true; + std::string errorStr; + + if (rules->m_auditLog != NULL) { + succeeded = rules->m_auditLog->reopen(&errorStr); + } + + if (!succeeded) { + if (!errorStr.empty()) { + *error = strdup(errorStr.c_str()); + } else { + // Guarantee an error message is always assigned in the event of a failure + *error = strdup("Unknown error reopening audit log"); + } + } + + return succeeded ? 0 : -1; +} + + } // namespace modsecurity diff --git a/src/utils/shared_files.cc b/src/utils/shared_files.cc index d3a529785f..460585d15c 100644 --- a/src/utils/shared_files.cc +++ b/src/utils/shared_files.cc @@ -155,6 +155,64 @@ bool SharedFiles::open(const std::string& fileName, std::string *error) { } +bool SharedFiles::reopen(const std::string& fileName, std::string *error) { + std::pair *a = NULL; + bool ret = true; + FILE *fp; + struct stat target_stat; + struct stat current_stat; + +#if MODSEC_USE_GENERAL_LOCK + pthread_mutex_lock(m_generalLock); +#endif + + for (auto &i : m_handlers) { + if (i.first == fileName) { + a = &i.second; + break; + } + } + + if (a == NULL || a->first == NULL || a->second == NULL) { + error->assign("Cannot find open file to reopen: " + fileName); + ret = false; + goto out; + } + + if ( + stat(fileName.c_str(), &target_stat) == 0 && + fstat(fileno(a->second), ¤t_stat) == 0 && + current_stat.st_dev == target_stat.st_dev && + current_stat.st_ino == target_stat.st_ino + ) { + // We already have a file open at the right path. Let's skip reopen. + ret = true; + goto out; + } + + // Use fopen() and fclose() instead of freopen() because freopen() closes + // the current file before proving it can open the new file. + // If the open fails, it's better to continue writing to the old file than + // to fail outright. + fp = fopen(fileName.c_str(), "a"); + if (fp == NULL) { + error->assign("Failed to open file: " + fileName); + ret = false; + goto out; + } + + fclose(a->second); + a->second = fp; + +out: +#if MODSEC_USE_GENERAL_LOCK + pthread_mutex_unlock(m_generalLock); +#endif + + return ret; +} + + void SharedFiles::close(const std::string& fileName) { std::pair a; /* int ret; */ diff --git a/src/utils/shared_files.h b/src/utils/shared_files.h index 92c8d25727..b81352ca20 100644 --- a/src/utils/shared_files.h +++ b/src/utils/shared_files.h @@ -57,6 +57,7 @@ typedef struct msc_file_handler { class SharedFiles { public: bool open(const std::string& fileName, std::string *error); + bool reopen(const std::string& filename, std::string *error); void close(const std::string& fileName); bool write(const std::string& fileName, const std::string &msg, std::string *error); From 4b4a4238279caa51e79f9476d53d4b9b2fb09d0c Mon Sep 17 00:00:00 2001 From: Brandon Payton Date: Fri, 1 May 2020 11:05:40 -0700 Subject: [PATCH 2/2] Update cppcheck suppressions --- test/cppcheck_suppressions.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/cppcheck_suppressions.txt b/test/cppcheck_suppressions.txt index 2d97548f5f..a6a0aaba5f 100644 --- a/test/cppcheck_suppressions.txt +++ b/test/cppcheck_suppressions.txt @@ -33,7 +33,8 @@ invalidScanfArgType_int:src/rules_set_properties.cc:102 // ModSecurity v3 code... // unmatchedSuppression:src/utils/geo_lookup.cc:82 -useInitializationList:src/utils/shared_files.h:87 +functionConst:src/utils/shared_files.h:60 +useInitializationList:src/utils/shared_files.h:88 unmatchedSuppression:src/utils/msc_tree.cc functionStatic:headers/modsecurity/transaction.h:373 duplicateBranch:src/audit_log/audit_log.cc:224