diff --git a/examples/multithread/Makefile.am b/examples/multithread/Makefile.am index 0871efa1e1..bb5cf67982 100644 --- a/examples/multithread/Makefile.am +++ b/examples/multithread/Makefile.am @@ -54,4 +54,8 @@ multithread_CPPFLAGS = \ MAINTAINERCLEANFILES = \ Makefile.in +EXTRA_DIST = \ + basic_rules.conf \ + inspectfile_rules.conf \ + inspectfile_helper.sh diff --git a/examples/multithread/inspectfile_helper.sh b/examples/multithread/inspectfile_helper.sh new file mode 100755 index 0000000000..50c5dd2444 --- /dev/null +++ b/examples/multithread/inspectfile_helper.sh @@ -0,0 +1,10 @@ +#!/bin/sh + +# deterministic helper for @inspectFile stress runs +if [ "$1" = "herewego" ]; then + echo 0 + exit 0 +fi + +echo 1 +exit 0 diff --git a/examples/multithread/inspectfile_rules.conf b/examples/multithread/inspectfile_rules.conf new file mode 100644 index 0000000000..8cc8956b27 --- /dev/null +++ b/examples/multithread/inspectfile_rules.conf @@ -0,0 +1,7 @@ +SecDebugLog debug.log +SecDebugLogLevel 3 + +SecRuleEngine On + +# ensure each transaction executes @inspectFile +SecRule ARGS:foo "@inspectFile ./inspectfile_helper.sh" "id:101,phase:2,pass,nolog,noauditlog,t:none" diff --git a/examples/multithread/multithread.cc b/examples/multithread/multithread.cc index b679e40623..56ab34fa01 100644 --- a/examples/multithread/multithread.cc +++ b/examples/multithread/multithread.cc @@ -1,15 +1,24 @@ #include #include -#include +#include +#include +#include +#include + +#ifndef WIN32 +#include +#endif #include #include #include -static void process_request(modsecurity::ModSecurity *modsec, modsecurity::RulesSet *rules, int tid) { +static void process_request(modsecurity::ModSecurity *modsec, + modsecurity::RulesSet *rules, int tid, int iterations, + std::atomic *completed_threads) { std::cout << "Hello World! It's me, thread #" << tid << std::endl; - for(int i = 0; i != 1'000; i++) { + for (int i = 0; i != iterations; i++) { auto modsecTransaction = std::make_unique(modsec, rules, nullptr); modsecTransaction->processConnection("127.0.0.1", 12345, "127.0.0.1", 80); @@ -32,15 +41,52 @@ static void process_request(modsecurity::ModSecurity *modsec, modsecurity::Rules std::this_thread::sleep_for(std::chrono::microseconds(100)); } + completed_threads->fetch_add(1); std::cout << "Thread #" << tid << " exits" << std::endl; } -int main (int argc, char *argv[]) { +#ifndef WIN32 +static int count_open_fds(void) { + int count = 0; + DIR *dir = opendir("/proc/self/fd"); + if (dir == nullptr) { + return -1; + } + + while (readdir(dir) != nullptr) { + count++; + } + closedir(dir); + return count; +} +#endif + +int main (int argc, const char *argv[]) { + auto modsec = std::make_unique(); modsec->setConnectorInformation("ModSecurity-test v0.0.1-alpha (Simple " \ "example on how to use ModSecurity API"); - const char main_rule_uri[] = "basic_rules.conf"; + const char *main_rule_uri = "basic_rules.conf"; + if (argc >= 2) { + main_rule_uri = argv[1]; + } + + int thread_count = 100; + if (argc >= 3) { + thread_count = std::atoi(argv[2]); + } + + int iterations = 1000; + if (argc >= 4) { + iterations = std::atoi(argv[3]); + } + + if (thread_count <= 0 || iterations <= 0) { + std::cerr << "Usage: ./multithread [rules.conf] [threads] [iterations]" << std::endl; + return 2; + } + auto rules = std::make_unique(); if (rules->loadFromUri(main_rule_uri) < 0) { std::cerr << "Problems loading the rules..." << std::endl; @@ -48,13 +94,21 @@ int main (int argc, char *argv[]) { return -1; } - constexpr auto NUM_THREADS = 100; - std::array threads; + std::vector threads; + threads.resize(thread_count); + std::atomic completed_threads{0}; + +#ifndef WIN32 + const int open_fds_before = count_open_fds(); + std::cout << "open_fds_before=" << open_fds_before << std::endl; +#endif + auto start = std::chrono::steady_clock::now(); for (auto i = 0; i != threads.size(); ++i) { threads[i] = std::thread( - [&modsec, &rules, i]() { - process_request(modsec.get(), rules.get(), i); + [&modsec, &rules, i, iterations, &completed_threads]() { + process_request(modsec.get(), rules.get(), static_cast(i), + iterations, &completed_threads); }); } @@ -64,5 +118,16 @@ int main (int argc, char *argv[]) { threads[i].join(); } + auto elapsed = std::chrono::steady_clock::now() - start; + std::cout << "completed_threads=" << completed_threads.load() << std::endl; + std::cout << "elapsed_ms=" + << std::chrono::duration_cast(elapsed).count() + << std::endl; + +#ifndef WIN32 + const int open_fds_after = count_open_fds(); + std::cout << "open_fds_after=" << open_fds_after << std::endl; +#endif + return 0; -} \ No newline at end of file +} diff --git a/src/operators/inspect_file.cc b/src/operators/inspect_file.cc index 28c9c072ad..f0ab06cafd 100644 --- a/src/operators/inspect_file.cc +++ b/src/operators/inspect_file.cc @@ -16,20 +16,111 @@ #include "src/operators/inspect_file.h" #include +#include -#include #include +#include +#include +#include +#include #include "src/operators/operator.h" #include "src/utils/system.h" #ifdef WIN32 #include "src/compat/msvc.h" +#include +#else +#include +#include +#include +#include + +extern char **environ; #endif namespace modsecurity { namespace operators { +#ifdef WIN32 +namespace { +bool stringToWide(const std::string &input, std::wstring *output) { + if (output == nullptr) { + return false; + } + + if (input.empty()) { + output->clear(); + return true; + } + + int needed = MultiByteToWideChar(CP_UTF8, 0, input.c_str(), -1, nullptr, 0); + if (needed <= 0) { + return false; + } + + output->resize(needed); + int written = MultiByteToWideChar(CP_UTF8, 0, input.c_str(), -1, &(*output)[0], needed); + if (written != needed) { + return false; + } + + output->pop_back(); + return true; +} + +std::wstring quoteWindowsArg(const std::wstring &arg) { + if (arg.empty()) { + return L"\"\""; + } + + bool requires_quotes = false; + for (wchar_t c : arg) { + if (c == L' ' || c == L'\t' || c == L'\"') { + requires_quotes = true; + break; + } + } + + if (requires_quotes == false) { + return arg; + } + + std::wstring out; + out.push_back(L'"'); + + size_t backslashes = 0; + for (wchar_t c : arg) { + if (c == L'\\') { + backslashes++; + continue; + } + + if (c == L'"') { + out.append(backslashes * 2 + 1, L'\\'); + out.push_back(L'"'); + backslashes = 0; + continue; + } + + if (backslashes > 0) { + out.append(backslashes, L'\\'); + backslashes = 0; + } + + out.push_back(c); + } + + if (backslashes > 0) { + out.append(backslashes * 2, L'\\'); + } + + out.push_back(L'"'); + return out; +} +} // namespace +#endif + bool InspectFile::init(const std::string ¶m2, std::string *error) { std::ifstream *iss; std::string err; @@ -57,32 +148,200 @@ bool InspectFile::evaluate(Transaction *transaction, const std::string &str) { if (m_isScript) { return m_lua.run(transaction, str); } else { - FILE *in; - char buff[512]; +#ifndef WIN32 + std::array pipefd = {{-1, -1}}; + if (pipe(pipefd.data()) == -1) { + return false; + } + + posix_spawn_file_actions_t actions; + if (posix_spawn_file_actions_init(&actions) != 0) { + close(pipefd[0]); + close(pipefd[1]); + return false; + } + + bool action_error = false; + if (posix_spawn_file_actions_addclose(&actions, pipefd[0]) != 0) { + action_error = true; + } + if (!action_error + && posix_spawn_file_actions_adddup2(&actions, pipefd[1], STDOUT_FILENO) != 0) { + action_error = true; + } + if (!action_error + && posix_spawn_file_actions_addclose(&actions, pipefd[1]) != 0) { + action_error = true; + } + + if (action_error) { + posix_spawn_file_actions_destroy(&actions); + close(pipefd[0]); + close(pipefd[1]); + return false; + } + + std::vector argv_storage; + argv_storage.emplace_back(m_file); + argv_storage.emplace_back(str); + + std::vector argv; + argv.reserve(argv_storage.size() + 1); + // cppcheck-suppress constVariableReference + for (std::string &arg : argv_storage) { + argv.push_back(arg.data()); + } + argv.push_back(nullptr); + + pid_t pid = 0; + int spawn_rc = posix_spawn(&pid, m_file.c_str(), &actions, nullptr, argv.data(), + environ); + + posix_spawn_file_actions_destroy(&actions); + + close(pipefd[1]); + + if (spawn_rc != 0) { + close(pipefd[0]); + return false; + } + + std::array buff = {}; std::stringstream s; - std::string res; - std::string openstr; + bool read_error = false; + bool done_reading = false; + while (!done_reading) { + ssize_t count = read(pipefd[0], buff.data(), buff.size()); + if (count > 0) { + s.write(buff.data(), count); + } else if (count == 0) { + done_reading = true; + } else if (errno != EINTR) { + read_error = true; + done_reading = true; + } + } + + close(pipefd[0]); - openstr.append(m_param); - openstr.append(" "); - openstr.append(str); - if (!(in = popen(openstr.c_str(), "r"))) { + int wstatus = 0; + while (waitpid(pid, &wstatus, 0) == -1) { + if (errno != EINTR) { + return false; + } + } + + if (read_error || WIFEXITED(wstatus) == 0 || WEXITSTATUS(wstatus) != 0) { return false; } - while (fgets(buff, sizeof(buff), in) != NULL) { - s << buff; + std::string res = s.str(); + if (res.size() > 1 && res[0] != '1') { + return true; /* match */ } - pclose(in); + /* no match */ + return false; +#else + std::wstring executable_path; + std::wstring argument; + if (stringToWide(m_file, &executable_path) == false + || stringToWide(str, &argument) == false) { + return false; + } + + SECURITY_ATTRIBUTES security_attributes; + memset(&security_attributes, 0, sizeof(security_attributes)); + security_attributes.nLength = sizeof(security_attributes); + security_attributes.bInheritHandle = TRUE; + + HANDLE child_stdout_read = nullptr; + HANDLE child_stdout_write = nullptr; + if (CreatePipe(&child_stdout_read, &child_stdout_write, + &security_attributes, 0) == FALSE) { + return false; + } - res.append(s.str()); + if (SetHandleInformation(child_stdout_read, HANDLE_FLAG_INHERIT, 0) == FALSE) { + CloseHandle(child_stdout_read); + CloseHandle(child_stdout_write); + return false; + } + + STARTUPINFOW startup_info; + PROCESS_INFORMATION process_info; + memset(&startup_info, 0, sizeof(startup_info)); + memset(&process_info, 0, sizeof(process_info)); + startup_info.cb = sizeof(startup_info); + startup_info.dwFlags = STARTF_USESTDHANDLES; + startup_info.hStdInput = GetStdHandle(STD_INPUT_HANDLE); + startup_info.hStdOutput = child_stdout_write; + startup_info.hStdError = GetStdHandle(STD_ERROR_HANDLE); + + std::wstring command_line = quoteWindowsArg(executable_path); + command_line.append(L" "); + command_line.append(quoteWindowsArg(argument)); + + if (CreateProcessW(executable_path.c_str(), &command_line[0], nullptr, + nullptr, TRUE, 0, nullptr, nullptr, &startup_info, + &process_info) == FALSE) { + CloseHandle(child_stdout_read); + CloseHandle(child_stdout_write); + return false; + } + + CloseHandle(child_stdout_write); + + char buff[512]; + std::stringstream s; + bool read_error = false; + + while (true) { + DWORD bytes_read = 0; + BOOL read_ok = ReadFile(child_stdout_read, buff, sizeof(buff), &bytes_read, nullptr); + if (read_ok == FALSE) { + DWORD err = GetLastError(); + if (err == ERROR_BROKEN_PIPE) { + break; + } + read_error = true; + break; + } + + if (bytes_read == 0) { + break; + } + + s.write(buff, bytes_read); + } + + CloseHandle(child_stdout_read); + + DWORD wait_rc = WaitForSingleObject(process_info.hProcess, INFINITE); + if (wait_rc != WAIT_OBJECT_0) { + CloseHandle(process_info.hThread); + CloseHandle(process_info.hProcess); + return false; + } + + DWORD exit_code = 1; + BOOL got_exit_code = GetExitCodeProcess(process_info.hProcess, &exit_code); + + CloseHandle(process_info.hThread); + CloseHandle(process_info.hProcess); + + if (read_error || got_exit_code == FALSE || exit_code != 0) { + return false; + } + + std::string res = s.str(); if (res.size() > 1 && res[0] != '1') { return true; /* match */ } /* no match */ return false; +#endif } } diff --git a/test/stress/inspectfile_multithread_stress.sh b/test/stress/inspectfile_multithread_stress.sh new file mode 100755 index 0000000000..646a96c02a --- /dev/null +++ b/test/stress/inspectfile_multithread_stress.sh @@ -0,0 +1,53 @@ +#!/bin/sh +set -eu + +SCRIPT_DIR=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd) +REPO_ROOT=$(CDPATH= cd -- "$SCRIPT_DIR/../.." && pwd) +EXAMPLE_DIR="$REPO_ROOT/examples/multithread" +BIN="$EXAMPLE_DIR/multithread" +RULES="$EXAMPLE_DIR/inspectfile_rules.conf" +THREADS="${1:-60}" +ITERATIONS="${2:-120}" +TIMEOUT_SECS="${3:-90}" + +if [ ! -x "$BIN" ]; then + echo "stress-test: missing binary $BIN" >&2 + echo "build it first (e.g. make -C examples/multithread multithread)" >&2 + exit 2 +fi + +TMP_OUT=$(mktemp) +trap 'rm -f "$TMP_OUT"' EXIT + +cd "$EXAMPLE_DIR" +set +e +if command -v timeout >/dev/null 2>&1; then + timeout "$TIMEOUT_SECS" "$BIN" "$RULES" "$THREADS" "$ITERATIONS" >"$TMP_OUT" 2>&1 +else + echo "stress-test: warning: 'timeout' command not found, running without timeout" >&2 + "$BIN" "$RULES" "$THREADS" "$ITERATIONS" >"$TMP_OUT" 2>&1 +fi +STATUS=$? +set -e +if [ "$STATUS" -ne 0 ]; then + echo "stress-test: process exit status=$STATUS" >&2 + cat "$TMP_OUT" >&2 + exit "$STATUS" +fi + +EXPECTED="completed_threads=$THREADS" +if ! grep -q "$EXPECTED" "$TMP_OUT"; then + echo "stress-test: missing expected marker: $EXPECTED" >&2 + cat "$TMP_OUT" >&2 + exit 3 +fi + +if grep -q "open_fds_before=" "$TMP_OUT" && grep -q "open_fds_after=" "$TMP_OUT"; then + BEFORE=$(grep 'open_fds_before=' "$TMP_OUT" | tail -n1 | cut -d= -f2) + AFTER=$(grep 'open_fds_after=' "$TMP_OUT" | tail -n1 | cut -d= -f2) + DELTA=$((AFTER - BEFORE)) + echo "fd_delta=$DELTA" +fi + +echo "stress-test: ok" +cat "$TMP_OUT" diff --git a/test/test-cases/data/inspectfile-not-executable.txt b/test/test-cases/data/inspectfile-not-executable.txt new file mode 100644 index 0000000000..47fc7a5288 --- /dev/null +++ b/test/test-cases/data/inspectfile-not-executable.txt @@ -0,0 +1 @@ +this file is intentionally not executable diff --git a/test/test-cases/data/inspectfile-posix-helper.sh b/test/test-cases/data/inspectfile-posix-helper.sh new file mode 100755 index 0000000000..7e3c02cc5f --- /dev/null +++ b/test/test-cases/data/inspectfile-posix-helper.sh @@ -0,0 +1,18 @@ +#!/bin/sh + +if [ "$1" = "match" ]; then + echo 0 + exit 0 +fi + +if [ "$1" = "nomatch" ]; then + echo 1 + exit 0 +fi + +if [ "$1" = "stderr-only" ]; then + echo 0 1>&2 + exit 0 +fi + +exit 7 diff --git a/test/test-cases/regression/issue-3489-inspectfile-posix.json b/test/test-cases/regression/issue-3489-inspectfile-posix.json new file mode 100644 index 0000000000..7ed8d58aa5 --- /dev/null +++ b/test/test-cases/regression/issue-3489-inspectfile-posix.json @@ -0,0 +1,131 @@ +[ + { + "enabled": 1, + "version_min": 300000, + "title": "Issue 3489: @inspectFile executes external helper without shell (match)", + "client": { + "ip": "200.249.12.31", + "port": 123 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "request": { + "headers": { + "Host": "localhost", + "User-Agent": "curl/7.38.0", + "Accept": "*/*", + "Content-Length": "0", + "Content-Type": "application/x-www-form-urlencoded" + }, + "uri": "/whee?res=match", + "method": "GET", + "body": [ + "" + ] + }, + "response": { + "headers": { + "Content-Length": "8" + }, + "body": [ + "no need." + ] + }, + "expected": { + "debug_log": "Rule returned 1.", + "http_code": 200 + }, + "rules": [ + "SecRuleEngine On", + "SecRule ARGS:res \"@inspectFile test-cases/data/inspectfile-posix-helper.sh\" \"id:348901,phase:2,pass,t:none\"" + ] + }, + { + "enabled": 1, + "version_min": 300000, + "title": "Issue 3489: @inspectFile executes external helper without shell (no match)", + "client": { + "ip": "200.249.12.31", + "port": 123 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "request": { + "headers": { + "Host": "localhost", + "User-Agent": "curl/7.38.0", + "Accept": "*/*", + "Content-Length": "0", + "Content-Type": "application/x-www-form-urlencoded" + }, + "uri": "/whee?res=nomatch", + "method": "GET", + "body": [ + "" + ] + }, + "response": { + "headers": { + "Content-Length": "8" + }, + "body": [ + "no need." + ] + }, + "expected": { + "debug_log": "Rule returned 0.", + "http_code": 200 + }, + "rules": [ + "SecRuleEngine On", + "SecRule ARGS:res \"@inspectFile test-cases/data/inspectfile-posix-helper.sh\" \"id:348902,phase:2,pass,t:none\"" + ] + }, + { + "enabled": 1, + "version_min": 300000, + "title": "Issue 3489: @inspectFile handles exec failure as no match", + "client": { + "ip": "200.249.12.31", + "port": 123 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "request": { + "headers": { + "Host": "localhost", + "User-Agent": "curl/7.38.0", + "Accept": "*/*", + "Content-Length": "0", + "Content-Type": "application/x-www-form-urlencoded" + }, + "uri": "/whee?res=match", + "method": "GET", + "body": [ + "" + ] + }, + "response": { + "headers": { + "Content-Length": "8" + }, + "body": [ + "no need." + ] + }, + "expected": { + "debug_log": "Rule returned 0.", + "http_code": 200 + }, + "rules": [ + "SecRuleEngine On", + "SecRule ARGS:res \"@inspectFile test-cases/data/inspectfile-not-executable.txt\" \"id:348903,phase:2,pass,t:none\"" + ] + } +]