From 80bcbbf022880c5177f8130118778ab22914b0bf Mon Sep 17 00:00:00 2001 From: Praneet Tenkila <67573804+praneet390@users.noreply.github.com> Date: Fri, 6 Feb 2026 23:18:43 +0530 Subject: [PATCH 1/4] Hardening note: avoid shell-based popen in InspectFile operator --- src/operators/inspect_file.cc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/operators/inspect_file.cc b/src/operators/inspect_file.cc index 28c9c072a..063e6bd14 100644 --- a/src/operators/inspect_file.cc +++ b/src/operators/inspect_file.cc @@ -63,6 +63,12 @@ bool InspectFile::evaluate(Transaction *transaction, const std::string &str) { std::string res; std::string openstr; + // SECURITY HARDENING NOTE: + // popen() executes via shell with concatenated arguments. + // Current inputs are engine-controlled, but replacing this + // with argv-based exec/spawn would remove shell parsing risk. + + openstr.append(m_param); openstr.append(" "); openstr.append(str); From ffc4b6722b5babf20f1354932187000c45d91598 Mon Sep 17 00:00:00 2001 From: Praneet Tenkila <67573804+praneet390@users.noreply.github.com> Date: Mon, 23 Feb 2026 18:28:57 +0530 Subject: [PATCH 2/4] Hardening: replace shell-based popen() with execvp() in InspectFile operator (v3) --- src/operators/inspect_file.cc | 130 +++++++++++++++++++++++++--------- 1 file changed, 95 insertions(+), 35 deletions(-) diff --git a/src/operators/inspect_file.cc b/src/operators/inspect_file.cc index 063e6bd14..192a53618 100644 --- a/src/operators/inspect_file.cc +++ b/src/operators/inspect_file.cc @@ -16,15 +16,20 @@ #include "src/operators/inspect_file.h" #include - #include #include +#include #include "src/operators/operator.h" #include "src/utils/system.h" #ifdef WIN32 #include "src/compat/msvc.h" +#else +#include +#include +#include +#include #endif namespace modsecurity { @@ -52,46 +57,101 @@ bool InspectFile::init(const std::string ¶m2, std::string *error) { return true; } - bool InspectFile::evaluate(Transaction *transaction, const std::string &str) { if (m_isScript) { return m_lua.run(transaction, str); - } else { - FILE *in; - char buff[512]; - std::stringstream s; - std::string res; - std::string openstr; - - // SECURITY HARDENING NOTE: - // popen() executes via shell with concatenated arguments. - // Current inputs are engine-controlled, but replacing this - // with argv-based exec/spawn would remove shell parsing risk. - - - openstr.append(m_param); - openstr.append(" "); - openstr.append(str); - if (!(in = popen(openstr.c_str(), "r"))) { - return false; - } - - while (fgets(buff, sizeof(buff), in) != NULL) { - s << buff; - } - - pclose(in); - - res.append(s.str()); - if (res.size() > 1 && res[0] != '1') { - return true; /* match */ - } - - /* no match */ + } + +#ifndef WIN32 + /* + * SECURITY HARDENING: + * Replace shell-based popen() execution with fork()+execvp() + * to avoid shell interpretation while preserving behavior. + */ + + int pipefd[2]; + if (pipe(pipefd) == -1) { return false; } -} + pid_t pid = fork(); + if (pid == -1) { + close(pipefd[0]); + close(pipefd[1]); + return false; + } + + if (pid == 0) { + // Child process + close(pipefd[0]); // Close read end + dup2(pipefd[1], STDOUT_FILENO); // Redirect stdout + close(pipefd[1]); + + std::vector argv; + argv.push_back(const_cast(m_param.c_str())); + argv.push_back(const_cast(str.c_str())); + argv.push_back(nullptr); + + execvp(argv[0], argv.data()); + + // execvp failed + _exit(1); + } + + // Parent process + close(pipefd[1]); // Close write end + + char buff[512]; + std::stringstream s; + ssize_t count; + + while ((count = read(pipefd[0], buff, sizeof(buff))) > 0) { + s.write(buff, count); + } + + close(pipefd[0]); + waitpid(pid, nullptr, 0); + + std::string res = s.str(); + + if (res.size() > 1 && res[0] != '1') { + return true; /* match */ + } + + return false; + +#else + /* + * Windows fallback: preserve existing behavior + */ + FILE *in; + char buff[512]; + std::stringstream s; + std::string res; + std::string openstr; + + openstr.append(m_param); + openstr.append(" "); + openstr.append(str); + + if (!(in = popen(openstr.c_str(), "r"))) { + return false; + } + + while (fgets(buff, sizeof(buff), in) != NULL) { + s << buff; + } + + pclose(in); + + res.append(s.str()); + if (res.size() > 1 && res[0] != '1') { + return true; /* match */ + } + + return false; +#endif +} } // namespace operators } // namespace modsecurity From 0c1209d60bc0be3956035a4696cc34b8e30f16d0 Mon Sep 17 00:00:00 2001 From: Praneet Tenkila <67573804+praneet390@users.noreply.github.com> Date: Mon, 23 Feb 2026 19:02:13 +0530 Subject: [PATCH 3/4] Refactor inspect_file to use std::array and std::vector --- src/operators/inspect_file.cc | 45 ++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/src/operators/inspect_file.cc b/src/operators/inspect_file.cc index 192a53618..d53c0c8ef 100644 --- a/src/operators/inspect_file.cc +++ b/src/operators/inspect_file.cc @@ -13,12 +13,21 @@ * */ +/* + * ModSecurity, http://www.modsecurity.org/ + * Copyright (c) 2015 - 2021 Trustwave Holdings, Inc. + * + * Licensed under the Apache License, Version 2.0 + */ + #include "src/operators/inspect_file.h" #include #include #include #include +#include +#include #include "src/operators/operator.h" #include "src/utils/system.h" @@ -29,7 +38,6 @@ #include #include #include -#include #endif namespace modsecurity { @@ -69,8 +77,8 @@ bool InspectFile::evaluate(Transaction *transaction, const std::string &str) { * to avoid shell interpretation while preserving behavior. */ - int pipefd[2]; - if (pipe(pipefd) == -1) { + std::array pipefd{}; + if (pipe(pipefd.data()) == -1) { return false; } @@ -83,36 +91,39 @@ bool InspectFile::evaluate(Transaction *transaction, const std::string &str) { if (pid == 0) { // Child process - close(pipefd[0]); // Close read end - dup2(pipefd[1], STDOUT_FILENO); // Redirect stdout + close(pipefd[0]); + dup2(pipefd[1], STDOUT_FILENO); close(pipefd[1]); + // Create mutable copies (avoid const_cast) + std::string param_copy = m_param; + std::string str_copy = str; + std::vector argv; - argv.push_back(const_cast(m_param.c_str())); - argv.push_back(const_cast(str.c_str())); + argv.push_back(param_copy.data()); + argv.push_back(str_copy.data()); argv.push_back(nullptr); execvp(argv[0], argv.data()); - // execvp failed - _exit(1); + _exit(1); // exec failed } // Parent process - close(pipefd[1]); // Close write end + close(pipefd[1]); - char buff[512]; std::stringstream s; + std::array buff{}; ssize_t count; - while ((count = read(pipefd[0], buff, sizeof(buff))) > 0) { - s.write(buff, count); + while ((count = read(pipefd[0], buff.data(), buff.size())) > 0) { + s.write(buff.data(), count); } close(pipefd[0]); waitpid(pid, nullptr, 0); - std::string res = s.str(); + const std::string res = s.str(); if (res.size() > 1 && res[0] != '1') { return true; /* match */ @@ -125,7 +136,7 @@ bool InspectFile::evaluate(Transaction *transaction, const std::string &str) { * Windows fallback: preserve existing behavior */ FILE *in; - char buff[512]; + std::array buff{}; std::stringstream s; std::string res; std::string openstr; @@ -138,8 +149,8 @@ bool InspectFile::evaluate(Transaction *transaction, const std::string &str) { return false; } - while (fgets(buff, sizeof(buff), in) != NULL) { - s << buff; + while (fgets(buff.data(), buff.size(), in) != NULL) { + s << buff.data(); } pclose(in); From 35ad0d04a9b95d81ae2c53aef7d90c36b7b1ee3e Mon Sep 17 00:00:00 2001 From: Praneet Tenkila <67573804+praneet390@users.noreply.github.com> Date: Mon, 23 Feb 2026 19:10:00 +0530 Subject: [PATCH 4/4] Refactor inspect_file.cc to use inline variable declaration --- src/operators/inspect_file.cc | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/operators/inspect_file.cc b/src/operators/inspect_file.cc index d53c0c8ef..f47deec01 100644 --- a/src/operators/inspect_file.cc +++ b/src/operators/inspect_file.cc @@ -123,13 +123,12 @@ bool InspectFile::evaluate(Transaction *transaction, const std::string &str) { close(pipefd[0]); waitpid(pid, nullptr, 0); - const std::string res = s.str(); - - if (res.size() > 1 && res[0] != '1') { - return true; /* match */ - } + if (const std::string res = s.str(); + res.size() > 1 && res[0] != '1') { + return true; +} - return false; +return false; #else /* @@ -155,10 +154,10 @@ bool InspectFile::evaluate(Transaction *transaction, const std::string &str) { pclose(in); - res.append(s.str()); - if (res.size() > 1 && res[0] != '1') { - return true; /* match */ - } + if (const std::string res = s.str(); + res.size() > 1 && res[0] != '1') { + return true; +} return false; #endif