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/6] 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/6] 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/6] 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/6] 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 From f3b96472a7701a01574209b685680769a30d1a70 Mon Sep 17 00:00:00 2001 From: Praneet Tenkila <67573804+praneet390@users.noreply.github.com> Date: Fri, 13 Mar 2026 11:57:28 +0530 Subject: [PATCH 5/6] Refactor InspectFile for security and clarity --- src/operators/inspect_file.cc | 99 ++++++++++++++++++++--------------- 1 file changed, 57 insertions(+), 42 deletions(-) diff --git a/src/operators/inspect_file.cc b/src/operators/inspect_file.cc index f47deec01..5f5d4456b 100644 --- a/src/operators/inspect_file.cc +++ b/src/operators/inspect_file.cc @@ -13,21 +13,12 @@ * */ -/* - * 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" @@ -38,11 +29,14 @@ #include #include #include +#include +#include #endif namespace modsecurity { namespace operators { + bool InspectFile::init(const std::string ¶m2, std::string *error) { std::ifstream *iss; std::string err; @@ -50,7 +44,6 @@ bool InspectFile::init(const std::string ¶m2, std::string *error) { m_file = utils::find_resource(m_param, param2, &err); iss = new std::ifstream(m_file, std::ios::in); - if (iss->is_open() == false) { error->assign("Failed to open file: " + m_param + ". " + err); delete iss; @@ -65,6 +58,7 @@ 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); @@ -72,13 +66,11 @@ bool InspectFile::evaluate(Transaction *transaction, const std::string &str) { #ifndef WIN32 /* - * SECURITY HARDENING: - * Replace shell-based popen() execution with fork()+execvp() - * to avoid shell interpretation while preserving behavior. + * Use fork()+execv() to avoid shell interpretation and PATH ambiguity. + * Execute the resolved m_file path directly instead of m_param. */ - - std::array pipefd{}; - if (pipe(pipefd.data()) == -1) { + int pipefd[2]; + if (pipe(pipefd) == -1) { return false; } @@ -91,55 +83,77 @@ bool InspectFile::evaluate(Transaction *transaction, const std::string &str) { if (pid == 0) { // Child process - close(pipefd[0]); - dup2(pipefd[1], STDOUT_FILENO); + close(pipefd[0]); // Close read end + dup2(pipefd[1], STDOUT_FILENO); // Redirect stdout to pipe close(pipefd[1]); - // Create mutable copies (avoid const_cast) - std::string param_copy = m_param; + // Create mutable copies for execv() argument array + std::string file_copy = m_file; std::string str_copy = str; - std::vector argv; - argv.push_back(param_copy.data()); + std::vector argv; + argv.push_back(file_copy.data()); argv.push_back(str_copy.data()); argv.push_back(nullptr); - execvp(argv[0], argv.data()); + // Use execv() with the resolved path — avoids PATH lookup ambiguity + execv(file_copy.data(), argv.data()); - _exit(1); // exec failed + // execv() failed: exit child immediately + _exit(1); } // Parent process - close(pipefd[1]); + close(pipefd[1]); // Close write end - std::stringstream s; std::array buff{}; + std::stringstream s; ssize_t count; - while ((count = read(pipefd[0], buff.data(), buff.size())) > 0) { - s.write(buff.data(), count); + // Retry on EINTR so a signal does not silently truncate output + while (true) { + count = read(pipefd[0], buff.data(), buff.size()); + if (count > 0) { + s.write(buff.data(), count); + } else if (count == 0) { + // EOF + break; + } else { + if (errno == EINTR) { + continue; + } + // Unrecoverable read error + break; + } } close(pipefd[0]); - waitpid(pid, nullptr, 0); - if (const std::string res = s.str(); - res.size() > 1 && res[0] != '1') { - return true; -} + // Check child exit status; treat non-zero exit as no match + int wstatus = 0; + if (waitpid(pid, &wstatus, 0) == -1) { + return false; + } + if (!WIFEXITED(wstatus) || WEXITSTATUS(wstatus) != 0) { + return false; + } -return false; + const std::string res = s.str(); + if (res.size() > 1 && res[0] != '1') { + return true; + } + + return false; #else /* - * Windows fallback: preserve existing behavior + * Windows fallback: use popen() to invoke the script. */ FILE *in; std::array buff{}; std::stringstream s; - std::string res; - std::string openstr; + std::string openstr; openstr.append(m_param); openstr.append(" "); openstr.append(str); @@ -148,20 +162,21 @@ return false; return false; } - while (fgets(buff.data(), buff.size(), in) != NULL) { + while (fgets(buff.data(), static_cast(buff.size()), in) != NULL) { s << buff.data(); } pclose(in); - if (const std::string res = s.str(); - res.size() > 1 && res[0] != '1') { - return true; -} + const std::string res = s.str(); + if (res.size() > 1 && res[0] != '1') { + return true; + } return false; #endif } + } // namespace operators } // namespace modsecurity From 69b774e89d900dc13e68d28ad836d5b7bfbd8ff9 Mon Sep 17 00:00:00 2001 From: Praneet Tenkila <67573804+praneet390@users.noreply.github.com> Date: Fri, 13 Mar 2026 12:05:17 +0530 Subject: [PATCH 6/6] Refactor inspect_file.cc for better pipe management --- src/operators/inspect_file.cc | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/src/operators/inspect_file.cc b/src/operators/inspect_file.cc index 5f5d4456b..3f44f85d1 100644 --- a/src/operators/inspect_file.cc +++ b/src/operators/inspect_file.cc @@ -69,8 +69,8 @@ bool InspectFile::evaluate(Transaction *transaction, const std::string &str) { * Use fork()+execv() to avoid shell interpretation and PATH ambiguity. * Execute the resolved m_file path directly instead of m_param. */ - int pipefd[2]; - if (pipe(pipefd) == -1) { + std::array pipefd{}; + if (pipe(pipefd.data()) == -1) { return false; } @@ -108,24 +108,17 @@ bool InspectFile::evaluate(Transaction *transaction, const std::string &str) { std::array buff{}; std::stringstream s; - ssize_t count; // Retry on EINTR so a signal does not silently truncate output - while (true) { + ssize_t count = 0; + do { count = read(pipefd[0], buff.data(), buff.size()); if (count > 0) { s.write(buff.data(), count); - } else if (count == 0) { - // EOF - break; - } else { - if (errno == EINTR) { - continue; - } - // Unrecoverable read error - break; + } else if (count < 0 && errno == EINTR) { + count = 1; // sentinel: keep looping } - } + } while (count > 0); close(pipefd[0]); @@ -138,8 +131,7 @@ bool InspectFile::evaluate(Transaction *transaction, const std::string &str) { return false; } - const std::string res = s.str(); - if (res.size() > 1 && res[0] != '1') { + if (const std::string res = s.str(); res.size() > 1 && res[0] != '1') { return true; } @@ -168,8 +160,7 @@ bool InspectFile::evaluate(Transaction *transaction, const std::string &str) { pclose(in); - const std::string res = s.str(); - if (res.size() > 1 && res[0] != '1') { + if (const std::string res = s.str(); res.size() > 1 && res[0] != '1') { return true; }