diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dee0ae05d..11ee06636 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -28,11 +28,16 @@ jobs: - {label: "wo ssdeep", opt: "--without-ssdeep" } - {label: "with lmdb", opt: "--with-lmdb" } - {label: "with pcre", opt: "--with-pcre" } + - {label: "with log message escape", opt: "--enable-log-message-escape" } exclude: - platform: {label: "x32"} configure: {label: "wo geoip"} - platform: {label: "x32"} configure: {label: "wo ssdeep"} + - platform: {label: "x32"} + configure: {label: "with log message escape"} + - compiler: {label: "clang"} + configure: {label: "with log message escape"} steps: - name: Detect latest Lua dev package id: detect_lua @@ -99,7 +104,7 @@ jobs: CC: ${{ matrix.compiler.cc }} CXX: ${{ matrix.compiler.cxx }} run: ./configure ${{ matrix.platform.configure }} ${{ matrix.configure.opt }} --enable-assertions=yes - - uses: ammaraskar/gcc-problem-matcher@master + - uses: ammaraskar/gcc-problem-matcher@cb2e3f949c41b7818628f35f8484a9b0acf90cf9 - name: make run: make -j `nproc` - name: check @@ -122,6 +127,7 @@ jobs: - {label: "wo maxmind", opt: "-DWITH_MAXMIND=OFF" } - {label: "wo libxml", opt: "-DWITH_LIBXML2=OFF" } - {label: "with lmdb", opt: "-DWITH_LMDB=ON" } + - {label: "with log message escape", opt: "-DLOG_MESSAGE_ESCAPE=ON" } steps: - uses: actions/checkout@v6 with: @@ -131,7 +137,7 @@ jobs: run: | pip3 install conan --upgrade conan profile detect - - uses: ammaraskar/msvc-problem-matcher@master + - uses: ammaraskar/msvc-problem-matcher@47189a176388253d16c6f0b79eee6274512b761a - name: Build ${{ matrix.configuration }} ${{ matrix.platform.arch }} ${{ matrix.configure.label }} shell: cmd run: vcbuild.bat ${{ matrix.configuration }} ${{ matrix.platform.arch }} NO_ASAN "${{ matrix.configure.opt }}" diff --git a/.github/workflows/ci_new.yml b/.github/workflows/ci_new.yml index fd0845243..0a6c81a02 100644 --- a/.github/workflows/ci_new.yml +++ b/.github/workflows/ci_new.yml @@ -33,6 +33,11 @@ jobs: - { label: "with lmdb", opt: "--with-lmdb" } - { label: "with pcre2 (default)", opt: "" } - { label: "with pcre", opt: "--with-pcre" } + - { label: "with log message escape", opt: "--enable-log-message-escape" } + + exclude: + - compiler: { label: "clang" } + configure: { label: "with log message escape" } steps: - uses: actions/checkout@v6 @@ -106,7 +111,7 @@ jobs: CXX: ${{ matrix.compiler.cxx }} run: ./configure ${{ matrix.platform.configure }} ${{ matrix.configure.opt }} --enable-assertions=yes - - uses: ammaraskar/gcc-problem-matcher@master + - uses: ammaraskar/gcc-problem-matcher@cb2e3f949c41b7818628f35f8484a9b0acf90cf9 - name: Compile run: make -j "$(nproc)" @@ -162,7 +167,7 @@ jobs: - name: Configure run: ./configure ${{ matrix.configure.opt }} --enable-assertions=yes - - uses: ammaraskar/gcc-problem-matcher@master + - uses: ammaraskar/gcc-problem-matcher@cb2e3f949c41b7818628f35f8484a9b0acf90cf9 - name: Compile run: make -j "$(sysctl -n hw.logicalcpu)" @@ -187,6 +192,7 @@ jobs: - { label: "without maxmind", opt: "-DWITH_MAXMIND=OFF" } - { label: "without libxml", opt: "-DWITH_LIBXML2=OFF" } - { label: "with lmdb", opt: "-DWITH_LMDB=ON" } + - { label: "with log message escape", opt: "-DLOG_MESSAGE_ESCAPE=ON" } steps: - uses: actions/checkout@v6 @@ -199,7 +205,7 @@ jobs: pip3 install conan --upgrade conan profile detect - - uses: ammaraskar/msvc-problem-matcher@master + - uses: ammaraskar/msvc-problem-matcher@47189a176388253d16c6f0b79eee6274512b761a - name: Build project shell: cmd diff --git a/build/win32/CMakeLists.txt b/build/win32/CMakeLists.txt index 399d4727c..fad7c38c1 100644 --- a/build/win32/CMakeLists.txt +++ b/build/win32/CMakeLists.txt @@ -7,6 +7,8 @@ option(WITH_LUA "Include LUA support" ON) option(WITH_LIBXML2 "Include LibXML2 support" ON) option(WITH_MAXMIND "Include MaxMind support" ON) option(WITH_CURL "Include CURL support" ON) +option(LOG_MESSAGE_ESCAPE + "Escape quotes and backslashes in textual log message fields" OFF) option(USE_ASAN "Build with Address Sanitizer" OFF) @@ -126,6 +128,7 @@ enable_feature(HAVE_LUA ${WITH_LUA}) enable_feature(HAVE_LIBXML2 ${WITH_LIBXML2}) enable_feature(HAVE_MAXMIND ${WITH_MAXMIND}) enable_feature(HAVE_CURL ${WITH_CURL}) +enable_feature(MSC_LOG_MESSAGE_ESCAPE ${LOG_MESSAGE_ESCAPE}) include(${CMAKE_CURRENT_LIST_DIR}/ConfigureChecks.cmake) @@ -218,7 +221,33 @@ enable_testing() file(READ ${BASE_DIR}/test/test-suite.in TEST_FILES_RAW) string(REPLACE "\n" ";" TEST_FILES ${TEST_FILES_RAW}) +set(SKIP_CONDITIONAL_TEST FALSE) foreach(TEST_FILE ${TEST_FILES}) + string(STRIP "${TEST_FILE}" TEST_FILE) + + # Select the appropriate branch of the Automake conditional shared by + # the Autotools and CMake test suites. + if(TEST_FILE STREQUAL "if LOG_MESSAGE_ESCAPE") + if(LOG_MESSAGE_ESCAPE) + set(SKIP_CONDITIONAL_TEST FALSE) + else() + set(SKIP_CONDITIONAL_TEST TRUE) + endif() + continue() + elseif(TEST_FILE STREQUAL "else") + if(SKIP_CONDITIONAL_TEST) + set(SKIP_CONDITIONAL_TEST FALSE) + else() + set(SKIP_CONDITIONAL_TEST TRUE) + endif() + continue() + elseif(TEST_FILE STREQUAL "endif") + set(SKIP_CONDITIONAL_TEST FALSE) + continue() + elseif(SKIP_CONDITIONAL_TEST) + continue() + endif() + # ignore comment lines string(FIND ${TEST_FILE} "#" is_comment) if(NOT is_comment EQUAL 0) diff --git a/build/win32/config.h.cmake b/build/win32/config.h.cmake index 2f6a73085..53d802f54 100644 --- a/build/win32/config.h.cmake +++ b/build/win32/config.h.cmake @@ -63,6 +63,9 @@ /* Define if libcurl is available */ #cmakedefine HAVE_CURL +/* Define to escape quotes and backslashes in textual log message fields */ +#cmakedefine MSC_LOG_MESSAGE_ESCAPE 1 + /* Name of package */ #define PACKAGE "@PACKAGE_NAME@" @@ -89,4 +92,4 @@ #cmakedefine STDC_HEADERS #endif -#endif // ndef MODSECURITY_CONFIG_H \ No newline at end of file +#endif // ndef MODSECURITY_CONFIG_H diff --git a/configure.ac b/configure.ac index 92f8438a7..555891476 100644 --- a/configure.ac +++ b/configure.ac @@ -276,11 +276,19 @@ AC_SUBST([MSC_GIT_VERSION]) MSC_ARG_ENABLE_BOOL([assertions], [Turn on assertions feature: undefine NDEBUG], [false], [assertions]) MSC_ARG_ENABLE_BOOL([debug-logs], [Turn off the SecDebugLog feature], [true], [debugLogs]) +MSC_ARG_ENABLE_BOOL([log-message-escape], + [Escape quotes and backslashes in textual log message fields], + [false], [logMessageEscape]) if test "$debugLogs" != "true"; then MODSEC_NO_LOGS="-DNO_LOGS=1" AC_SUBST(MODSEC_NO_LOGS) fi +if test "$logMessageEscape" = "true"; then + AC_DEFINE([MSC_LOG_MESSAGE_ESCAPE], [1], + [Define to escape quotes and backslashes in textual log message fields]) +fi + MSC_ARG_ENABLE_BOOL([afl-fuzz], [Turn on the afl fuzzer compilation utilities], [false], [aflFuzzer]) MSC_ARG_ENABLE_BOOL([examples], [Turn on the examples compilation (default option)], [true], [buildExamples]) MSC_ARG_ENABLE_BOOL([parser-generation], [Enables parser generation during the build], [false], [buildParser]) @@ -333,6 +341,7 @@ fi AM_CONDITIONAL([EXAMPLES], [test $buildExamples = true]) AM_CONDITIONAL([BUILD_PARSER], [test $buildParser = true]) AM_CONDITIONAL([USE_MUTEX_ON_PM], [test $mutexPm = true]) +AM_CONDITIONAL([LOG_MESSAGE_ESCAPE], [test $logMessageEscape = true]) # General link options @@ -497,6 +506,12 @@ else echo " + SecDebugLog ....disabled" fi +if test "$logMessageEscape" = "true"; then + echo " + Log message escape ....enabled" +else + echo " + Log message escape ....disabled" +fi + if test "$aflFuzzer" = "true"; then echo " + afl fuzzer ....enabled" echo " ($FUZZ_CPPCFLAGS)" diff --git a/src/operators/operator.cc b/src/operators/operator.cc index 935add621..4162ca1de 100644 --- a/src/operators/operator.cc +++ b/src/operators/operator.cc @@ -104,6 +104,17 @@ bool Operator::evaluateInternal(Transaction *transaction, } +/** + * Returns the operator's custom match message, or builds the legacy message. + * Keep this representation independent of textual log escaping: it is also + * serialized as the JSON audit log's match value. RuleMessage::log applies + * optional quote and backslash escaping when producing textual output. + * + * @param t Transaction used to evaluate macro-expanded operator parameters. + * @param key Matched variable name, including its collection. + * @param value Matched variable value before transformations. + * @return Custom or generated match description in the legacy representation. + */ std::string Operator::resolveMatchMessage(Transaction *t, std::string key, std::string value) { std::string ret = m_match_message; diff --git a/src/rule_message.cc b/src/rule_message.cc index 7c021d17e..d1591c572 100644 --- a/src/rule_message.cc +++ b/src/rule_message.cc @@ -18,11 +18,27 @@ #include "modsecurity/rules_set.h" #include "modsecurity/modsecurity.h" #include "modsecurity/transaction.h" +#include "src/config.h" #include "src/utils/string.h" +#ifdef MSC_LOG_MESSAGE_ESCAPE +constexpr bool kEscapeLogMessage = true; +#else +constexpr bool kEscapeLogMessage = false; +#endif + namespace modsecurity { +/** + * Builds the bracketed rule and transaction fields for a textual log message. + * With MSC_LOG_MESSAGE_ESCAPE, request-derived values escape quotes and + * backslashes before insertion so the field delimiters remain unchanged. + * URI and data limits are applied before escaping their values. + * + * @param rm Rule match and transaction whose details are formatted. + * @return Textual fields, including the leading space before the first field. + */ std::string RuleMessage::_details(const RuleMessage &rm) { std::string msg; @@ -30,7 +46,9 @@ std::string RuleMessage::_details(const RuleMessage &rm) { msg.append(" [line \"" + std::to_string(rm.m_rule.getLineNumber()) + "\"]"); msg.append(" [id \"" + std::to_string(rm.m_rule.m_ruleId) + "\"]"); msg.append(" [rev \"" + utils::string::toHexIfNeeded(rm.m_rule.m_rev, true) + "\"]"); - msg.append(" [msg \"" + rm.m_message + "\"]"); + msg.append(" [msg \"" + + utils::string::toHexIfNeeded( + rm.m_message, kEscapeLogMessage) + "\"]"); msg.append(" [data \"" + utils::string::toHexIfNeeded(utils::string::limitTo(200, rm.m_data), true) + "\"]"); msg.append(" [severity \"" + std::to_string(rm.m_severity) + "\"]"); @@ -42,27 +60,56 @@ std::string RuleMessage::_details(const RuleMessage &rm) { msg.append(" [tag \"" + utils::string::toHexIfNeeded(a, true) + "\"]"); } - msg.append(" [hostname \"" + rm.m_transaction.m_requestHostName \ - + "\"]"); - msg.append(" [uri \"" + utils::string::limitTo(200, rm.m_transaction.m_uri_no_query_string_decoded) + "\"]"); - msg.append(" [unique_id \"" + rm.m_transaction.m_id + "\"]"); + msg.append(" [hostname \"" + utils::string::toHexIfNeeded( + rm.m_transaction.m_requestHostName, kEscapeLogMessage) + "\"]"); + msg.append(" [uri \"" + utils::string::toHexIfNeeded( + utils::string::limitTo(200, + rm.m_transaction.m_uri_no_query_string_decoded), + kEscapeLogMessage) + "\"]"); + msg.append(" [unique_id \"" + utils::string::toHexIfNeeded( + rm.m_transaction.m_id, kEscapeLogMessage) + "\"]"); msg.append(" [ref \"" + utils::string::limitTo(200, rm.m_reference) + "\"]"); return msg; } +/** + * Builds the server-address, URI and transaction-ID fields for an error log. + * MSC_LOG_MESSAGE_ESCAPE enables quote and backslash escaping within values; + * the URI is limited before escaping to avoid cutting an escape sequence. + * + * @param rm Rule match providing the transaction details. + * @return Bracketed error-log tail with no leading space. + */ std::string RuleMessage::_errorLogTail(const RuleMessage &rm) { std::string msg; - msg.append("[hostname \"" + rm.m_transaction.m_serverIpAddress + "\"]"); - msg.append(" [uri \"" + utils::string::limitTo(200, rm.m_transaction.m_uri_no_query_string_decoded) + "\"]"); - msg.append(" [unique_id \"" + rm.m_transaction.m_id + "\"]"); + msg.append("[hostname \"" + utils::string::toHexIfNeeded( + rm.m_transaction.m_serverIpAddress, kEscapeLogMessage) + "\"]"); + msg.append(" [uri \"" + utils::string::toHexIfNeeded( + utils::string::limitTo(200, + rm.m_transaction.m_uri_no_query_string_decoded), + kEscapeLogMessage) + "\"]"); + msg.append(" [unique_id \"" + utils::string::toHexIfNeeded( + rm.m_transaction.m_id, kEscapeLogMessage) + "\"]"); return msg; } +/** + * Formats a rule match for textual logs without changing its stored contents. + * With MSC_LOG_MESSAGE_ESCAPE, the legacy match description is escaped as a + * whole, including backslashes in existing control-character representations. + * No limit is applied after escaping, so generated escapes remain complete. + * Bracketed fields are formatted separately to preserve their delimiters. + * + * @param rm Rule match to format; its contents remain available for JSON logs. + * @param props LogMessageInfo flags selecting optional client and tail fields. + * @param code Response status, or -1 to leave a status placeholder. + * @return Textual log entry with non-printable characters escaped. + */ std::string RuleMessage::log(const RuleMessage &rm, int props, int code) { std::string msg(""); msg.reserve(2048); @@ -84,7 +131,13 @@ std::string RuleMessage::log(const RuleMessage &rm, int props, int code) { msg.append("ModSecurity: Warning. "); } - msg.append(rm.m_match); + // Escape the stored match only for textual output. Its legacy contents + // must remain unchanged for JSON audit logs and connector callbacks. + if (kEscapeLogMessage) { + msg.append(utils::string::toHexIfNeeded(rm.m_match, true)); + } else { + msg.append(rm.m_match); + } msg.append(_details(rm)); if (props & ErrorLogTailLogMessageInfo) { diff --git a/test/test-cases/regression/issue-1528.json b/test/test-cases/regression/issue-1528.json index 53c8daa22..aea06c160 100644 --- a/test/test-cases/regression/issue-1528.json +++ b/test/test-cases/regression/issue-1528.json @@ -34,7 +34,7 @@ }, "expected": { "debug_log": "Rule returned 1", - "error_log": "Matched \"Operator `Rx' with parameter `\\^attack\\$'", + "error_log": "Operator `Rx' with parameter `\\^attack\\$'", "http_code": 200 }, "rules": [ diff --git a/test/test-cases/regression/issue-3601-legacy.json b/test/test-cases/regression/issue-3601-legacy.json new file mode 100644 index 000000000..b77906d6a --- /dev/null +++ b/test/test-cases/regression/issue-3601-legacy.json @@ -0,0 +1,334 @@ +[ + { + "enabled": 1, + "version_min": 300000, + "title": "RuleMessage text log preserves normal field delimiters with legacy escaping", + "github_issue": 3601, + "client": { + "ip": "192.0.2.10", + "port": 12345 + }, + "server": { + "ip": "192.0.2.20", + "port": 80, + "hostname": "normal.example" + }, + "request": { + "headers": { + "Host": "normal.example", + "X-Log": "ordinary", + "Content-Length": "0" + }, + "uri": "/normal", + "method": "GET", + "http_version": 1.1, + "body": [ + "" + ] + }, + "response": { + "headers": { + "Content-Length": "0" + }, + "body": [ + "" + ] + }, + "expected": { + "error_log": "\\Q[msg \"request=ordinary\"] [data \"\"] [severity \"0\"] [ver \"\"] [maturity \"0\"] [accuracy \"0\"] [hostname \"normal.example\"] [uri \"/normal\"]\\E", + "http_code": 200 + }, + "rules": [ + "SecRuleEngine On", + "SecRule REQUEST_URI \"@contains /normal\" \"id:3601001,phase:1,pass,log,msg:'request=%{REQUEST_HEADERS:X-Log}'\"" + ] + }, + { + "enabled": 1, + "version_min": 300000, + "title": "RuleMessage text log retains legacy special-character escaping", + "github_issue": 3601, + "client": { + "ip": "192.0.2.10", + "port": 12345 + }, + "server": { + "ip": "192.0.2.20", + "port": 80, + "hostname": "host.example\" ] [uri \"forged\\literal\\x22\r\nend" + }, + "request": { + "headers": { + "Host": "request.example", + "X-Log": "message\" ] [hostname \"forged\\literal\\x22\r\nend", + "Content-Length": "0" + }, + "uri": "/safe\" ] [unique_id \"forged\\literal\\x22\r\nend", + "method": "GET", + "http_version": 1.1, + "body": [ + "" + ] + }, + "response": { + "headers": { + "Content-Length": "0" + }, + "body": [ + "" + ] + }, + "expected": { + "error_log": "(?=.*\\Q(Value: `/safe\" ] [unique_id \"forged\\literal\\x22\\x0d\\x0aend' )\\E)(?=.*\\Q[msg \"request=message\" ] [hostname \"forged\\literal\\x22\\x0d\\x0aend\"] [data \"\"] [severity \"0\"] [ver \"\"] [maturity \"0\"] [accuracy \"0\"] [hostname \"host.example\" ] [uri \"forged\\literal\\x22\\x0d\\x0aend\"] [uri \"/safe\" ] [unique_id \"forged\\literal\\x22\\x0d\\x0aend\"]\\E)", + "http_code": 200 + }, + "rules": [ + "SecRuleEngine On", + "SecRule REQUEST_URI \"@contains /safe\" \"id:3601002,phase:1,pass,log,msg:'request=%{REQUEST_HEADERS:X-Log}'\"" + ] + }, + { + "enabled": 1, + "version_min": 300000, + "title": "msg (unquoted action value): escaped double quote immediately after a %{macro} does not truncate the action value", + "url": "https://github.com/owasp-modsecurity/ModSecurity/pull/3641#pullrequestreview", + "client": { + "ip": "200.249.12.31", + "port": 2313 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "request": { + "headers": { + "Host": "localhost", + "User-Agent": "curl/7.38.0", + "Accept": "*/*", + "Content-Length": "0" + }, + "uri": "/test", + "method": "GET", + "http_version": 1.1, + "body": [ + "" + ] + }, + "response": { + "headers": { + "Content-Length": "0" + }, + "body": [ + "" + ] + }, + "expected": { + "error_log": "\\Q[msg \"prefixGET\\\"suffix\"]\\E", + "http_code": 200 + }, + "rules": [ + "SecRuleEngine On", + "SecRule REQUEST_METHOD \"@unconditionalMatch\" \"phase:1,id:10007,log,pass,msg:prefix%{REQUEST_METHOD}\\\"suffix\"" + ] + }, + { + "enabled": 1, + "version_min": 300000, + "title": "Text and JSON match retains legacy output with fixed parameter", + "github_issue": 3601, + "client": { + "ip": "192.0.2.10", + "port": 12345 + }, + "server": { + "ip": "192.0.2.20", + "port": 80, + "hostname": "normal.example" + }, + "request": { + "headers": { + "Host": "normal.example", + "X-Log": "alpha\"beta\\literal\\x22\r\nend", + "Content-Length": "0" + }, + "uri": "/normal", + "method": "GET", + "http_version": 1.1, + "body": [ + "" + ] + }, + "response": { + "headers": { + "Content-Length": "0" + }, + "body": [ + "" + ] + }, + "expected": { + "audit_log": "\\Q\"match\":\"Matched \\\"Operator `Contains' with parameter `alpha' against variable `REQUEST_HEADERS:X-Log' (Value: `alpha\\\"beta\\\\literal\\\\x22\\\\x0d\\\\x0aend' )\",\\E", + "http_code": 200, + "error_log": "\\QModSecurity: Warning. Matched \"Operator `Contains' with parameter `alpha' against variable `REQUEST_HEADERS:X-Log' (Value: `alpha\"beta\\literal\\x22\\x0d\\x0aend' ) [file \"\\E" + }, + "rules": [ + "SecRuleEngine On", + "SecRule REQUEST_HEADERS:X-Log \"@contains alpha\" \"id:3601100,phase:1,pass,log,auditlog,msg:'JSON match compatibility'\"", + "SecAuditEngine On", + "SecAuditLogFormat JSON", + "SecAuditLogType Serial", + "SecAuditLogParts ABHZ", + "SecAuditLog /tmp/issue-3601-legacy-match.log" + ] + }, + { + "enabled": 1, + "version_min": 300000, + "title": "Text and JSON match retains legacy output with macro-expanded parameter", + "github_issue": 3601, + "client": { + "ip": "192.0.2.10", + "port": 12345 + }, + "server": { + "ip": "192.0.2.20", + "port": 80, + "hostname": "normal.example" + }, + "request": { + "headers": { + "Host": "normal.example", + "X-Log": "alpha\"beta\\literal\\x22\r\nend", + "Content-Length": "0" + }, + "uri": "/normal", + "method": "GET", + "http_version": 1.1, + "body": [ + "" + ] + }, + "response": { + "headers": { + "Content-Length": "0" + }, + "body": [ + "" + ] + }, + "expected": { + "audit_log": "\\Q\"match\":\"Matched \\\"Operator `EndsWith' with parameter `alpha\\\"beta\\\\literal\\\\x22\\r\\nend' against variable `REQUEST_HEADERS:X-Log' (Value: `alpha\\\"beta\\\\literal\\\\x22\\\\x0d\\\\x0aend' )\",\\E", + "http_code": 200, + "error_log": "\\QModSecurity: Warning. Matched \"Operator `EndsWith' with parameter `alpha\"beta\\literal\\x22\\x0d\\x0aend' against variable `REQUEST_HEADERS:X-Log' (Value: `alpha\"beta\\literal\\x22\\x0d\\x0aend' ) [file \"\\E" + }, + "rules": [ + "SecRuleEngine On", + "SecRule REQUEST_HEADERS:X-Log \"@endsWith %{REQUEST_HEADERS:X-Log}\" \"id:3601101,phase:1,pass,log,auditlog,msg:'JSON match compatibility'\"", + "SecAuditEngine On", + "SecAuditLogFormat JSON", + "SecAuditLogType Serial", + "SecAuditLogParts ABHZ", + "SecAuditLog /tmp/issue-3601-legacy-match.log" + ] + }, + { + "enabled": 1, + "version_min": 300000, + "title": "Text and JSON match retains legacy output with value truncation boundary", + "github_issue": 3601, + "client": { + "ip": "192.0.2.10", + "port": 12345 + }, + "server": { + "ip": "192.0.2.20", + "port": 80, + "hostname": "normal.example" + }, + "request": { + "headers": { + "Host": "normal.example", + "X-Log": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\"\\tail", + "Content-Length": "0" + }, + "uri": "/normal", + "method": "GET", + "http_version": 1.1, + "body": [ + "" + ] + }, + "response": { + "headers": { + "Content-Length": "0" + }, + "body": [ + "" + ] + }, + "expected": { + "audit_log": "\\Q\"match\":\"Matched \\\"Operator `Contains' with parameter `aaa' against variable `REQUEST_HEADERS:X-Log' (Value: `aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\\\"\\\\ (4 characters omitted)' )\",\\E", + "http_code": 200, + "error_log": "\\QModSecurity: Warning. Matched \"Operator `Contains' with parameter `aaa' against variable `REQUEST_HEADERS:X-Log' (Value: `aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\"\\ (4 characters omitted)' ) [file \"\\E" + }, + "rules": [ + "SecRuleEngine On", + "SecRule REQUEST_HEADERS:X-Log \"@contains aaa\" \"id:3601102,phase:1,pass,log,auditlog,msg:'JSON match compatibility'\"", + "SecAuditEngine On", + "SecAuditLogFormat JSON", + "SecAuditLogType Serial", + "SecAuditLogParts ABHZ", + "SecAuditLog /tmp/issue-3601-legacy-match.log" + ] + }, + { + "enabled": 1, + "version_min": 300000, + "title": "Text and JSON match retains legacy output with fixed backslash parameter", + "github_issue": 3601, + "client": { + "ip": "192.0.2.10", + "port": 12345 + }, + "server": { + "ip": "192.0.2.20", + "port": 80, + "hostname": "normal.example" + }, + "request": { + "headers": { + "Host": "normal.example", + "X-Log": "alpha\"beta\\literal\\x22\r\nend", + "Content-Length": "0" + }, + "uri": "/normal", + "method": "GET", + "http_version": 1.1, + "body": [ + "" + ] + }, + "response": { + "headers": { + "Content-Length": "0" + }, + "body": [ + "" + ] + }, + "expected": { + "audit_log": "\\Q\"match\":\"Matched \\\"Operator `Contains' with parameter `\\\\x22' against variable `REQUEST_HEADERS:X-Log' (Value: `alpha\\\"beta\\\\literal\\\\x22\\\\x0d\\\\x0aend' )\",\\E", + "http_code": 200, + "error_log": "\\QModSecurity: Warning. Matched \"Operator `Contains' with parameter `\\x22' against variable `REQUEST_HEADERS:X-Log' (Value: `alpha\"beta\\literal\\x22\\x0d\\x0aend' ) [file \"\\E" + }, + "rules": [ + "SecRuleEngine On", + "SecRule REQUEST_HEADERS:X-Log \"@contains \\x22\" \"id:3601103,phase:1,pass,log,auditlog,msg:'JSON match compatibility'\"", + "SecAuditEngine On", + "SecAuditLogFormat JSON", + "SecAuditLogType Serial", + "SecAuditLogParts ABHZ", + "SecAuditLog /tmp/issue-3601-legacy-match.log" + ] + } +] diff --git a/test/test-cases/regression/issue-3601.json b/test/test-cases/regression/issue-3601.json new file mode 100644 index 000000000..6b8c7043e --- /dev/null +++ b/test/test-cases/regression/issue-3601.json @@ -0,0 +1,334 @@ +[ + { + "enabled": 1, + "version_min": 300000, + "title": "RuleMessage text log preserves normal field delimiters", + "github_issue": 3601, + "client": { + "ip": "192.0.2.10", + "port": 12345 + }, + "server": { + "ip": "192.0.2.20", + "port": 80, + "hostname": "normal.example" + }, + "request": { + "headers": { + "Host": "normal.example", + "X-Log": "ordinary", + "Content-Length": "0" + }, + "uri": "/normal", + "method": "GET", + "http_version": 1.1, + "body": [ + "" + ] + }, + "response": { + "headers": { + "Content-Length": "0" + }, + "body": [ + "" + ] + }, + "expected": { + "error_log": "\\Q[msg \"request=ordinary\"] [data \"\"] [severity \"0\"] [ver \"\"] [maturity \"0\"] [accuracy \"0\"] [hostname \"normal.example\"] [uri \"/normal\"]\\E", + "http_code": 200 + }, + "rules": [ + "SecRuleEngine On", + "SecRule REQUEST_URI \"@contains /normal\" \"id:3601001,phase:1,pass,log,msg:'request=%{REQUEST_HEADERS:X-Log}'\"" + ] + }, + { + "enabled": 1, + "version_min": 300000, + "title": "RuleMessage text log escapes request-derived field delimiters", + "github_issue": 3601, + "client": { + "ip": "192.0.2.10", + "port": 12345 + }, + "server": { + "ip": "192.0.2.20", + "port": 80, + "hostname": "host.example\" ] [uri \"forged\\literal\\x22\r\nend" + }, + "request": { + "headers": { + "Host": "request.example", + "X-Log": "message\" ] [hostname \"forged\\literal\\x22\r\nend", + "Content-Length": "0" + }, + "uri": "/safe\" ] [unique_id \"forged\\literal\\x22\r\nend", + "method": "GET", + "http_version": 1.1, + "body": [ + "" + ] + }, + "response": { + "headers": { + "Content-Length": "0" + }, + "body": [ + "" + ] + }, + "expected": { + "error_log": "(?=.*\\Q(Value: `/safe\\x22 ] [unique_id \\x22forged\\x5cliteral\\x5cx22\\x5cx0d\\x5cx0aend' )\\E)(?=.*\\Q[msg \"request=message\\x22 ] [hostname \\x22forged\\x5cliteral\\x5cx22\\x0d\\x0aend\"] [data \"\"] [severity \"0\"] [ver \"\"] [maturity \"0\"] [accuracy \"0\"] [hostname \"host.example\\x22 ] [uri \\x22forged\\x5cliteral\\x5cx22\\x0d\\x0aend\"] [uri \"/safe\\x22 ] [unique_id \\x22forged\\x5cliteral\\x5cx22\\x0d\\x0aend\"]\\E)", + "http_code": 200 + }, + "rules": [ + "SecRuleEngine On", + "SecRule REQUEST_URI \"@contains /safe\" \"id:3601002,phase:1,pass,log,msg:'request=%{REQUEST_HEADERS:X-Log}'\"" + ] + }, + { + "enabled": 1, + "version_min": 300000, + "title": "msg (unquoted action value): escaped double quote immediately after a %{macro} does not truncate the action value", + "url": "https://github.com/owasp-modsecurity/ModSecurity/pull/3641#pullrequestreview", + "client": { + "ip": "200.249.12.31", + "port": 2313 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "request": { + "headers": { + "Host": "localhost", + "User-Agent": "curl/7.38.0", + "Accept": "*/*", + "Content-Length": "0" + }, + "uri": "/test", + "method": "GET", + "http_version": 1.1, + "body": [ + "" + ] + }, + "response": { + "headers": { + "Content-Length": "0" + }, + "body": [ + "" + ] + }, + "expected": { + "error_log": "\\Q[msg \"prefixGET\\x5c\\x22suffix\"]\\E", + "http_code": 200 + }, + "rules": [ + "SecRuleEngine On", + "SecRule REQUEST_METHOD \"@unconditionalMatch\" \"phase:1,id:10007,log,pass,msg:prefix%{REQUEST_METHOD}\\\"suffix\"" + ] + }, + { + "enabled": 1, + "version_min": 300000, + "title": "Text and JSON match retains legacy output with fixed parameter", + "github_issue": 3601, + "client": { + "ip": "192.0.2.10", + "port": 12345 + }, + "server": { + "ip": "192.0.2.20", + "port": 80, + "hostname": "normal.example" + }, + "request": { + "headers": { + "Host": "normal.example", + "X-Log": "alpha\"beta\\literal\\x22\r\nend", + "Content-Length": "0" + }, + "uri": "/normal", + "method": "GET", + "http_version": 1.1, + "body": [ + "" + ] + }, + "response": { + "headers": { + "Content-Length": "0" + }, + "body": [ + "" + ] + }, + "expected": { + "audit_log": "\\Q\"match\":\"Matched \\\"Operator `Contains' with parameter `alpha' against variable `REQUEST_HEADERS:X-Log' (Value: `alpha\\\"beta\\\\literal\\\\x22\\\\x0d\\\\x0aend' )\",\\E", + "http_code": 200, + "error_log": "\\QModSecurity: Warning. Matched \\x22Operator `Contains' with parameter `alpha' against variable `REQUEST_HEADERS:X-Log' (Value: `alpha\\x22beta\\x5cliteral\\x5cx22\\x5cx0d\\x5cx0aend' ) [file \"\\E" + }, + "rules": [ + "SecRuleEngine On", + "SecRule REQUEST_HEADERS:X-Log \"@contains alpha\" \"id:3601100,phase:1,pass,log,auditlog,msg:'JSON match compatibility'\"", + "SecAuditEngine On", + "SecAuditLogFormat JSON", + "SecAuditLogType Serial", + "SecAuditLogParts ABHZ", + "SecAuditLog /tmp/issue-3601-match.log" + ] + }, + { + "enabled": 1, + "version_min": 300000, + "title": "Text and JSON match retains legacy output with macro-expanded parameter", + "github_issue": 3601, + "client": { + "ip": "192.0.2.10", + "port": 12345 + }, + "server": { + "ip": "192.0.2.20", + "port": 80, + "hostname": "normal.example" + }, + "request": { + "headers": { + "Host": "normal.example", + "X-Log": "alpha\"beta\\literal\\x22\r\nend", + "Content-Length": "0" + }, + "uri": "/normal", + "method": "GET", + "http_version": 1.1, + "body": [ + "" + ] + }, + "response": { + "headers": { + "Content-Length": "0" + }, + "body": [ + "" + ] + }, + "expected": { + "audit_log": "\\Q\"match\":\"Matched \\\"Operator `EndsWith' with parameter `alpha\\\"beta\\\\literal\\\\x22\\r\\nend' against variable `REQUEST_HEADERS:X-Log' (Value: `alpha\\\"beta\\\\literal\\\\x22\\\\x0d\\\\x0aend' )\",\\E", + "http_code": 200, + "error_log": "\\QModSecurity: Warning. Matched \\x22Operator `EndsWith' with parameter `alpha\\x22beta\\x5cliteral\\x5cx22\\x0d\\x0aend' against variable `REQUEST_HEADERS:X-Log' (Value: `alpha\\x22beta\\x5cliteral\\x5cx22\\x5cx0d\\x5cx0aend' ) [file \"\\E" + }, + "rules": [ + "SecRuleEngine On", + "SecRule REQUEST_HEADERS:X-Log \"@endsWith %{REQUEST_HEADERS:X-Log}\" \"id:3601101,phase:1,pass,log,auditlog,msg:'JSON match compatibility'\"", + "SecAuditEngine On", + "SecAuditLogFormat JSON", + "SecAuditLogType Serial", + "SecAuditLogParts ABHZ", + "SecAuditLog /tmp/issue-3601-match.log" + ] + }, + { + "enabled": 1, + "version_min": 300000, + "title": "Text and JSON match retains legacy output with value truncation boundary", + "github_issue": 3601, + "client": { + "ip": "192.0.2.10", + "port": 12345 + }, + "server": { + "ip": "192.0.2.20", + "port": 80, + "hostname": "normal.example" + }, + "request": { + "headers": { + "Host": "normal.example", + "X-Log": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\"\\tail", + "Content-Length": "0" + }, + "uri": "/normal", + "method": "GET", + "http_version": 1.1, + "body": [ + "" + ] + }, + "response": { + "headers": { + "Content-Length": "0" + }, + "body": [ + "" + ] + }, + "expected": { + "audit_log": "\\Q\"match\":\"Matched \\\"Operator `Contains' with parameter `aaa' against variable `REQUEST_HEADERS:X-Log' (Value: `aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\\\"\\\\ (4 characters omitted)' )\",\\E", + "http_code": 200, + "error_log": "\\QModSecurity: Warning. Matched \\x22Operator `Contains' with parameter `aaa' against variable `REQUEST_HEADERS:X-Log' (Value: `aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\\x22\\x5c (4 characters omitted)' ) [file \"\\E" + }, + "rules": [ + "SecRuleEngine On", + "SecRule REQUEST_HEADERS:X-Log \"@contains aaa\" \"id:3601102,phase:1,pass,log,auditlog,msg:'JSON match compatibility'\"", + "SecAuditEngine On", + "SecAuditLogFormat JSON", + "SecAuditLogType Serial", + "SecAuditLogParts ABHZ", + "SecAuditLog /tmp/issue-3601-match.log" + ] + }, + { + "enabled": 1, + "version_min": 300000, + "title": "Text and JSON match retains legacy output with fixed backslash parameter", + "github_issue": 3601, + "client": { + "ip": "192.0.2.10", + "port": 12345 + }, + "server": { + "ip": "192.0.2.20", + "port": 80, + "hostname": "normal.example" + }, + "request": { + "headers": { + "Host": "normal.example", + "X-Log": "alpha\"beta\\literal\\x22\r\nend", + "Content-Length": "0" + }, + "uri": "/normal", + "method": "GET", + "http_version": 1.1, + "body": [ + "" + ] + }, + "response": { + "headers": { + "Content-Length": "0" + }, + "body": [ + "" + ] + }, + "expected": { + "audit_log": "\\Q\"match\":\"Matched \\\"Operator `Contains' with parameter `\\\\x22' against variable `REQUEST_HEADERS:X-Log' (Value: `alpha\\\"beta\\\\literal\\\\x22\\\\x0d\\\\x0aend' )\",\\E", + "http_code": 200, + "error_log": "\\QModSecurity: Warning. Matched \\x22Operator `Contains' with parameter `\\x5cx22' against variable `REQUEST_HEADERS:X-Log' (Value: `alpha\\x22beta\\x5cliteral\\x5cx22\\x5cx0d\\x5cx0aend' ) [file \"\\E" + }, + "rules": [ + "SecRuleEngine On", + "SecRule REQUEST_HEADERS:X-Log \"@contains \\x22\" \"id:3601103,phase:1,pass,log,auditlog,msg:'JSON match compatibility'\"", + "SecAuditEngine On", + "SecAuditLogFormat JSON", + "SecAuditLogType Serial", + "SecAuditLogParts ABHZ", + "SecAuditLog /tmp/issue-3601-match.log" + ] + } +] diff --git a/test/test-cases/regression/misc-escaped-quote-after-macro-expansion.json b/test/test-cases/regression/misc-escaped-quote-after-macro-expansion.json index 21bdfa90d..c3210c90e 100644 --- a/test/test-cases/regression/misc-escaped-quote-after-macro-expansion.json +++ b/test/test-cases/regression/misc-escaped-quote-after-macro-expansion.json @@ -174,49 +174,5 @@ "SecRuleEngine On", "SecRule REQUEST_METHOD \"@unconditionalMatch\" \"phase:1,id:10006,log,pass,logdata:\"value=%{REQUEST_METHOD}\\\\\\\"end\"\"" ] - }, - { - "enabled": 1, - "version_min": 300000, - "title": "msg (unquoted action value): escaped double quote immediately after a %{macro} does not truncate the action value", - "url": "https://github.com/owasp-modsecurity/ModSecurity/pull/3641#pullrequestreview", - "client": { - "ip": "200.249.12.31", - "port": 2313 - }, - "server": { - "ip": "200.249.12.31", - "port": 80 - }, - "request": { - "headers": { - "Host": "localhost", - "User-Agent": "curl/7.38.0", - "Accept": "*/*", - "Content-Length": "0" - }, - "uri": "/test", - "method": "GET", - "http_version": 1.1, - "body": [ - "" - ] - }, - "response": { - "headers": { - "Content-Length": "0" - }, - "body": [ - "" - ] - }, - "expected": { - "error_log": "prefixGET\\\\\"suffix", - "http_code": 200 - }, - "rules": [ - "SecRuleEngine On", - "SecRule REQUEST_METHOD \"@unconditionalMatch\" \"phase:1,id:10007,log,pass,msg:prefix%{REQUEST_METHOD}\\\"suffix\"" - ] } ] diff --git a/test/test-cases/regression/operator-rx.json b/test/test-cases/regression/operator-rx.json index 030881921..d33328fb7 100644 --- a/test/test-cases/regression/operator-rx.json +++ b/test/test-cases/regression/operator-rx.json @@ -84,7 +84,7 @@ }, "expected": { "debug_log": "Executing operator \"Rx\" with param \"\\^0\\$\"", - "error_log": "Matched \"Operator `Rx' with parameter `\\^0\\$'", + "error_log": "Operator `Rx' with parameter `\\^0\\$'", "http_code": 200 }, "rules": [ @@ -177,7 +177,7 @@ }, "expected": { "debug_log": "rx: regex error 'MATCH_LIMIT' for pattern", - "error_log": "Matched \"Operator `StrEq' with parameter `1' against variable `MSC_PCRE_ERROR'", + "error_log": "Operator `StrEq' with parameter `1' against variable `MSC_PCRE_ERROR'", "http_code": 200 }, "rules": [ @@ -226,7 +226,7 @@ }, "expected": { "debug_log": "rx: regex error 'MATCH_LIMIT' for pattern", - "error_log": "Matched \"Operator `StrEq' with parameter `1' against variable `MSC_PCRE_LIMITS_EXCEEDED'", + "error_log": "Operator `StrEq' with parameter `1' against variable `MSC_PCRE_LIMITS_EXCEEDED'", "http_code": 200 }, "rules": [ @@ -275,7 +275,7 @@ }, "expected": { "debug_log": "rx: regex error 'MATCH_LIMIT' for pattern", - "error_log": "Matched \"Operator `StrEq' with parameter `1' against variable `TX:MSC_PCRE_LIMITS_EXCEEDED'", + "error_log": "Operator `StrEq' with parameter `1' against variable `TX:MSC_PCRE_LIMITS_EXCEEDED'", "http_code": 200 }, "rules": [ diff --git a/test/test-suite.in b/test/test-suite.in index d89e994a4..46adf2f77 100644 --- a/test/test-suite.in +++ b/test/test-suite.in @@ -76,6 +76,11 @@ TESTS+=test/test-cases/regression/issue-2423-msg-in-chain.json TESTS+=test/test-cases/regression/issue-2427.json TESTS+=test/test-cases/regression/issue-2296.json TESTS+=test/test-cases/regression/issue-3340.json +if LOG_MESSAGE_ESCAPE +TESTS+=test/test-cases/regression/issue-3601.json +else +TESTS+=test/test-cases/regression/issue-3601-legacy.json +endif TESTS+=test/test-cases/regression/issue-394.json TESTS+=test/test-cases/regression/issue-849.json TESTS+=test/test-cases/regression/issue-960.json