Merge pull request #3212 from eduar-hte/defensive-intervention

Do not assume ModSecurityIntervention argument to transaction::intervention has been initialized/cleaned

Author: airween

.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`

src/transaction.cc

@@ -1469,31 +1469,40 @@ 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;
 
         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 {
+            it->log = NULL;
         }
         intervention::reset(&m_it);
     }
 
-    return it->disruptive;
+    return disruptive;
 }
 
 
@@ -2260,11 +2269,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,

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();
     }
 }

src/utils/string.h

@@ -68,8 +68,8 @@ std::vector<std::string> ssplit(std::string str, char delimiter);
 std::pair<std::string, std::string> ssplit_pair(const std::string& str, char delimiter);
 std::vector<std::string> 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);