From c947f5e40dd3428024a3a8b9c9174ee9c85fd150 Mon Sep 17 00:00:00 2001 From: Eduardo Arias Date: Mon, 5 Aug 2024 11:18:35 -0700 Subject: [PATCH 1/3] Do not assume ModSecurityIntervention argument to transaction::intervention has been initialized/cleaned - Keep m_it->disruptive value and use it as return value to guarantee that the value is correct. - If m_it->disruptive is false and the 'it' argument has not been initialized/cleaned, the function may incorrectly return a non-zero value. - When a disruptive intervention is being reported by the function, defensively initialize log & url to NULL if there's no such data to provide to the caller. - If the caller has not initialized/cleaned those fields in the 'it' argument, after returning from transaction::intervention, the user can safely read the log & url fields and in all scenarios they'll have valid values. --- src/transaction.cc | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/transaction.cc b/src/transaction.cc index 5826c264..a19ce88b 100644 --- a/src/transaction.cc +++ b/src/transaction.cc @@ -1469,16 +1469,24 @@ int Transaction::processLogging() { * @brief Check if ModSecurity has anything to ask to the server. * * Intervention can generate a log event and/or perform a disruptive action. + * + * If the return value is true, all fields in the ModSecurityIntervention + * parameter have been initialized and are safe to access. + * If the return value is false, no changes to the ModSecurityIntervention + * parameter have been made. * - * @param Pointer ModSecurityIntervention structure + * @param it Pointer to ModSecurityIntervention structure * @retval true A intervention should be made. * @retval false Nothing to be done. * */ bool Transaction::intervention(ModSecurityIntervention *it) { + const auto disruptive = m_it.disruptive; if (m_it.disruptive) { if (m_it.url) { it->url = strdup(m_it.url); + } else { + it->url = NULL; } it->disruptive = m_it.disruptive; it->status = m_it.status; @@ -1489,11 +1497,13 @@ bool Transaction::intervention(ModSecurityIntervention *it) { utils::string::replaceAll(&log, std::string("%d"), std::to_string(it->status)); it->log = strdup(log.c_str()); + } else { + it->log = NULL; } intervention::reset(&m_it); } - return it->disruptive; + return disruptive; } @@ -2260,11 +2270,16 @@ extern "C" void msc_transaction_cleanup(Transaction *transaction) { * * Intervention can generate a log event and/or perform a disruptive action. * - * @param transaction ModSecurity transaction. + * If the return value is not zero, all fields in the ModSecurityIntervention + * parameter have been initialized and are safe to access. + * If the return value is zero, no changes to the ModSecurityIntervention + * parameter have been made. * - * @return Pointer to ModSecurityIntervention structure - * @retval >0 A intervention should be made. - * @retval NULL Nothing to be done. + * @param transaction ModSecurity transaction. + * @param it Pointer to ModSecurityIntervention structure + * @returns If an intervention should be made + * @retval >0 A intervention should be made. + * @retval 0 Nothing to be done. * */ extern "C" int msc_intervention(Transaction *transaction, From 0b5493d4e73f29a5e4b08ec8488922d29b1450a3 Mon Sep 17 00:00:00 2001 From: Eduardo Arias Date: Tue, 6 Aug 2024 06:32:52 -0700 Subject: [PATCH 2/3] Minor performance improvements setting up intervention's log - Initialize `log` temporary value on construction instead of doing default initialization and then calling `append`. - Leverage `std::string_view` to replace `const std::string&` parameters in `utils::string::replaceAll` to avoid creating a `std::string` object (and associated allocation and copy) for the string literal`%d` --- src/transaction.cc | 5 ++--- src/utils/string.cc | 8 ++++---- src/utils/string.h | 4 ++-- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/src/transaction.cc b/src/transaction.cc index a19ce88b..7dc8ba24 100644 --- a/src/transaction.cc +++ b/src/transaction.cc @@ -1492,9 +1492,8 @@ bool Transaction::intervention(ModSecurityIntervention *it) { it->status = m_it.status; if (m_it.log != NULL) { - std::string log(""); - log.append(m_it.log); - utils::string::replaceAll(&log, std::string("%d"), + std::string log(m_it.log); + utils::string::replaceAll(log, "%d", std::to_string(it->status)); it->log = strdup(log.c_str()); } else { diff --git a/src/utils/string.cc b/src/utils/string.cc index 8d9e08ff..276fd31f 100644 --- a/src/utils/string.cc +++ b/src/utils/string.cc @@ -254,11 +254,11 @@ unsigned char *c2x(unsigned what, unsigned char *where) { } -void replaceAll(std::string *str, const std::string& from, - const std::string& to) { +void replaceAll(std::string &str, std::string_view from, + std::string_view to) { size_t start_pos = 0; - while ((start_pos = str->find(from, start_pos)) != std::string::npos) { - str->replace(start_pos, from.length(), to); + while ((start_pos = str.find(from, start_pos)) != std::string::npos) { + str.replace(start_pos, from.length(), to); start_pos += to.length(); } } diff --git a/src/utils/string.h b/src/utils/string.h index 55ebc0bd..1cd63296 100644 --- a/src/utils/string.h +++ b/src/utils/string.h @@ -68,8 +68,8 @@ std::vector ssplit(std::string str, char delimiter); std::pair ssplit_pair(const std::string& str, char delimiter); std::vector split(std::string str, char delimiter); void chomp(std::string *str); -void replaceAll(std::string *str, const std::string& from, - const std::string& to); +void replaceAll(std::string &str, std::string_view from, + std::string_view to); std::string removeWhiteSpacesIfNeeded(std::string a); std::string parserSanitizer(std::string a); From cf643d6072b61e7c5ea2c1c14e9ca75a775a0f2c Mon Sep 17 00:00:00 2001 From: Eduardo Arias Date: Thu, 8 Aug 2024 08:16:14 -0700 Subject: [PATCH 3/3] Avoid duplicate definition of --enable-assertions=yes configure flag on Unix builds - This configuration flag was introduced in commit d47185d in the context of PR #3207. - Moved to the configure step's 'run' command in order to be shared across configurations. - For the sake of reference, matrix.platform.configure should be used for configuration flags that are needed for a specific platform/architecture (which was the reason it was introduced in commit d9255d8, PR #3144). --- .github/workflows/ci.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7e44db5a..10ac5144 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -12,8 +12,8 @@ jobs: matrix: os: [ubuntu-22.04] platform: - - {label: "x64", arch: "amd64", configure: "--enable-assertions=yes"} - - {label: "x32", arch: "i386", configure: "PKG_CONFIG_PATH=/usr/lib/i386-linux-gnu/pkgconfig CFLAGS=-m32 CXXFLAGS=-m32 LDFLAGS=-m32 --enable-assertions=yes"} + - {label: "x64", arch: "amd64", configure: ""} + - {label: "x32", arch: "i386", configure: "PKG_CONFIG_PATH=/usr/lib/i386-linux-gnu/pkgconfig CFLAGS=-m32 CXXFLAGS=-m32 LDFLAGS=-m32"} compiler: - {label: "gcc", cc: "gcc", cxx: "g++"} - {label: "clang", cc: "clang", cxx: "clang++"} @@ -66,7 +66,7 @@ jobs: env: CC: ${{ matrix.compiler.cc }} CXX: ${{ matrix.compiler.cxx }} - run: ./configure ${{ matrix.platform.configure }} ${{ matrix.configure.opt }} + run: ./configure ${{ matrix.platform.configure }} ${{ matrix.configure.opt }} --enable-assertions=yes - uses: ammaraskar/gcc-problem-matcher@master - name: make run: make -j `nproc`