diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d6895b825d..9add866e42 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -3,12 +3,13 @@ name: Quality Assurance on: push: pull_request: - + jobs: build-linux: name: Linux (${{ matrix.platform.label }}, ${{ matrix.compiler.label }}, ${{ matrix.configure.label }}) runs-on: ${{ matrix.os }} strategy: + fail-fast: false matrix: os: [ubuntu-22.04] platform: @@ -33,6 +34,37 @@ jobs: - platform: {label: "x32"} configure: {label: "wo ssdeep"} steps: + - name: Detect latest Lua dev package + id: detect_lua + shell: bash + run: | + set -euo pipefail + + sudo apt-get update -y -qq + + CANDIDATES="$(apt-cache pkgnames | grep -E '^liblua[0-9]+\.[0-9]+-dev$' || true)" + + if [ -z "$CANDIDATES" ]; then + echo "No libluaX.Y-dev package found" + exit 1 + fi + + BEST_PKG="$( + printf '%s\n' "$CANDIDATES" \ + | sed -E 's/^liblua([0-9]+\.[0-9]+)-dev$/\1 &/' \ + | sort -V \ + | tail -n1 \ + | awk '{print $2}' + )" + + if [ -z "$BEST_PKG" ]; then + echo "Failed to determine Lua package" + exit 1 + fi + + echo "lua_pkg=$BEST_PKG" >> "$GITHUB_OUTPUT" + echo "Using $BEST_PKG" + - name: Setup Dependencies (common) run: | sudo dpkg --add-architecture ${{ matrix.platform.arch }} @@ -40,11 +72,11 @@ jobs: sudo apt-get install -y libyajl-dev:${{ matrix.platform.arch }} \ libcurl4-openssl-dev:${{ matrix.platform.arch }} \ liblmdb-dev:${{ matrix.platform.arch }} \ - liblua5.2-dev:${{ matrix.platform.arch }} \ + ${{ steps.detect_lua.outputs.lua_pkg }}:${{ matrix.platform.arch }} \ libmaxminddb-dev:${{ matrix.platform.arch }} \ libpcre2-dev:${{ matrix.platform.arch }} \ pcre2-utils:${{ matrix.platform.arch }} \ - bison flex + bison flex python3 python3-venv - name: Setup Dependencies (x32) if: ${{ matrix.platform.label == 'x32' }} run: | @@ -54,11 +86,11 @@ jobs: - name: Setup Dependencies (x64) if: ${{ matrix.platform.label == 'x64' }} run: | - sudo apt-get install -y libgeoip-dev:${{ matrix.platform.arch }} \ - libfuzzy-dev:${{ matrix.platform.arch }} - - uses: actions/checkout@v4 + sudo apt-get install -y libfuzzy-dev:${{ matrix.platform.arch }} + + - uses: actions/checkout@v6 with: - submodules: true + submodules: recursive fetch-depth: 0 - name: build.sh run: ./build.sh @@ -77,6 +109,7 @@ jobs: name: macOS (${{ matrix.configure.label }}) runs-on: ${{ matrix.os }} strategy: + fail-fast: false matrix: os: [macos-14] configure: @@ -105,21 +138,12 @@ jobs: ssdeep \ pcre \ bison \ - flex - - uses: actions/checkout@v4 + flex + + - uses: actions/checkout@v6 with: - submodules: true + submodules: recursive fetch-depth: 0 - - name: Build GeoIP - run: | - git clone --depth 1 --no-checkout https://github.com/maxmind/geoip-api-c.git - cd geoip-api-c - git fetch --tags - # Check out the last release, v1.6.12 - git checkout 4b526e7331ca1d692b74a0509ddcc725622ed31a - autoreconf --install - ./configure --disable-dependency-tracking --disable-silent-rules --prefix=/opt/homebrew - make install - name: build.sh run: ./build.sh - name: configure @@ -134,6 +158,7 @@ jobs: name: Windows (${{ matrix.platform.label }}, ${{ matrix.configure.label }}) runs-on: ${{ matrix.os }} strategy: + fail-fast: false matrix: os: [windows-2022] platform: @@ -147,9 +172,9 @@ jobs: - {label: "wo libxml", opt: "-DWITH_LIBXML2=OFF" } - {label: "with lmdb", opt: "-DWITH_LMDB=ON" } steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 with: - submodules: true + submodules: recursive fetch-depth: 0 - name: Install Conan run: | @@ -195,9 +220,10 @@ jobs: automake \ libtool \ cppcheck - - uses: actions/checkout@v4 + + - uses: actions/checkout@v6 with: - submodules: true + submodules: recursive fetch-depth: 0 - name: configure run: | diff --git a/.github/workflows/ci_new.yml b/.github/workflows/ci_new.yml index 27020ae6ab..a1009e02b1 100644 --- a/.github/workflows/ci_new.yml +++ b/.github/workflows/ci_new.yml @@ -40,14 +40,45 @@ jobs: fetch-depth: 0 submodules: recursive - - name: Install dependencies + - name: Detect latest Lua dev package + id: detect_lua + shell: bash run: | + set -euo pipefail + sudo apt-get update -y -qq + + CANDIDATES="$(apt-cache pkgnames | grep -E '^liblua[0-9]+\.[0-9]+-dev$' || true)" + + if [ -z "$CANDIDATES" ]; then + echo "No libluaX.Y-dev package found" + exit 1 + fi + + BEST_PKG="$( + printf '%s\n' "$CANDIDATES" \ + | sed -E 's/^liblua([0-9]+\.[0-9]+)-dev$/\1 &/' \ + | sort -V \ + | tail -n1 \ + | awk '{print $2}' + )" + + if [ -z "$BEST_PKG" ]; then + echo "Failed to determine Lua package" + exit 1 + fi + + echo "lua_pkg=$BEST_PKG" >> "$GITHUB_OUTPUT" + echo "Using $BEST_PKG" + + + - name: Install dependencies + run: | sudo apt-get install -y \ libyajl-dev \ libcurl4-openssl-dev \ liblmdb-dev \ - liblua5.2-dev \ + ${{ steps.detect_lua.outputs.lua_pkg }} \ libmaxminddb-dev \ libpcre2-dev \ libxml2-dev \ @@ -56,8 +87,15 @@ jobs: libpcre3-dev \ bison \ flex \ - pkg-config + pkg-config \ + python3 \ + python3-venv + - name: Show Lua installation + run: | + which lua || true + lua -v || true + dpkg -l | grep lua || true - name: Run build preparation script run: ./build.sh @@ -78,11 +116,12 @@ jobs: build-macos: name: macOS (${{ matrix.configure.label }}) - runs-on: macos-15 + runs-on: ${{ matrix.os }} strategy: fail-fast: false matrix: + os: [macos-15, macos-26] configure: - { label: "with parser generation", opt: "--enable-parser-generation" } - { label: "without curl", opt: "--without-curl" } @@ -196,7 +235,7 @@ jobs: cppcheck: name: Static analysis (cppcheck) - runs-on: macos-15 + runs-on: macos-26 steps: - uses: actions/checkout@v6 @@ -234,11 +273,47 @@ jobs: with: fetch-depth: 0 submodules: recursive + + - name: Detect latest Lua packages + id: detect_lua + shell: bash + run: | + set -euo pipefail + + apt-get update + + CANDIDATES="$(apt-cache pkgnames | grep -E '^liblua[0-9]+\.[0-9]+-dev$' || true)" + + if [ -z "$CANDIDATES" ]; then + echo "No libluaX.Y-dev package found" + exit 1 + fi + + BEST_PKG="$( + printf '%s\n' "$CANDIDATES" \ + | sed -E 's/^liblua([0-9]+\.[0-9]+)-dev$/\1 &/' \ + | sort -V \ + | tail -n1 \ + | awk '{print $2}' + )" + + if [ -z "$BEST_PKG" ]; then + echo "Failed to determine Lua dev package" + printf '%s\n' "$CANDIDATES" + exit 1 + fi + BEST_VER="$(printf '%s\n' "$BEST_PKG" | sed -E 's/^liblua([0-9]+\.[0-9]+)-dev$/\1/')" + LUA_PKG="lua$BEST_VER" + + echo "lua_dev_pkg=$BEST_PKG" >> "$GITHUB_OUTPUT" + echo "lua_pkg=$LUA_PKG" >> "$GITHUB_OUTPUT" + + echo "Using dev package: $BEST_PKG" + echo "Using interpreter: $LUA_PKG" - name: Install dependencies (v2 style) run: | - apt-get update apt-get install -y \ autoconf \ automake \ @@ -249,15 +324,24 @@ jobs: libyajl-dev \ libcurl4-openssl-dev \ liblmdb-dev \ - liblua5.2-dev \ + ${{ steps.detect_lua.outputs.lua_dev_pkg }} \ + ${{ steps.detect_lua.outputs.lua_pkg }} \ libmaxminddb-dev \ libpcre2-dev \ libxml2-dev \ libfuzzy-dev \ pcre2-utils \ bison \ - flex - + flex \ + python3 \ + python3-venv + + - name: Show Lua installation + run: | + which lua || true + lua -v || true + dpkg -l | grep lua || true + - name: Run build preparation script run: ./build.sh diff --git a/build/win32/CMakeLists.txt b/build/win32/CMakeLists.txt index fbf39f08d9..5ee49714d3 100644 --- a/build/win32/CMakeLists.txt +++ b/build/win32/CMakeLists.txt @@ -164,7 +164,7 @@ project(libModSecurityTests) function(setTestTargetProperties executable) target_compile_definitions(${executable} PRIVATE WITH_PCRE2) - target_include_directories(${executable} PRIVATE ${BASE_DIR} ${BASE_DIR}/headers) + target_include_directories(${executable} PRIVATE ${BASE_DIR} ${BASE_DIR}/headers ${BASE_DIR}/others) target_link_libraries(${executable} PRIVATE libModSecurity pcre2::pcre2 dirent::dirent) add_package_dependency(${executable} WITH_YAJL yajl::yajl HAVE_YAJL) endfunction() @@ -239,7 +239,7 @@ setTestTargetProperties(rules_optimization) project(libModSecurityExamples) function(setExampleTargetProperties executable) - target_include_directories(${executable} PRIVATE ${BASE_DIR} ${BASE_DIR}/headers) + target_include_directories(${executable} PRIVATE ${BASE_DIR} ${BASE_DIR}/headers ${BASE_DIR}/others) target_link_libraries(${executable} PRIVATE libModSecurity) endfunction() diff --git a/doc/libinjection-v4-migration-audit-2026-04-03.md b/doc/libinjection-v4-migration-audit-2026-04-03.md new file mode 100644 index 0000000000..5e6a7f530c --- /dev/null +++ b/doc/libinjection-v4-migration-audit-2026-04-03.md @@ -0,0 +1,98 @@ +# RE-AUDIT: libinjection v4.0 migration against official MIGRATION.md (2026-04-03) + +Primäre normative Quelle: + +## 1. Derived migration checklist from MIGRATION.md + +1. **Breaking API-Änderung erfassen**: Detection-Funktionen liefern `injection_result_t` statt `int`. +2. **Neue Return-Semantik korrekt verwenden**: + - `LIBINJECTION_RESULT_FALSE` = kein Angriff + - `LIBINJECTION_RESULT_TRUE` = Angriff erkannt + - `LIBINJECTION_RESULT_ERROR` = Parser-Fehler (kein `abort()` mehr) +3. **Betroffene APIs migrieren**: mindestens `libinjection_xss`, `libinjection_sqli`, `libinjection_is_xss`, `libinjection_h5_next` sofern im Projekt genutzt. +4. **Header-Anpassung**: `#include "libinjection_error.h"` ergänzen, wo Result-Typ/Konstanten verwendet werden. +5. **Explizite ERROR-Behandlung** in allen relevanten Codepfaden. +6. **Fail-safe im Security-Kontext** (WAF): ERROR darf nicht stillschweigend als benign durchlaufen. +7. **Truthiness-Fallen prüfen**: `if (result)` / `!result` / `== 0` etc. auf unbeabsichtigte Behandlung kontrollieren. +8. **Tests für Error-Fälle** ergänzen. +9. **Logging/Monitoring**: Fehlerzustände sichtbar machen (insb. parser errors). +10. **Edge-Case-Testen** (z. B. leere/auffällige Inputs), soweit für Projektkontext relevant. + +--- + +## 2. Checklist evaluation + +| Punkt | Status | Begründung | Evidenz | +|---|---|---|---| +| 1. `int -> injection_result_t` | PASS | Adapter- und Detector-Pfade nutzen `injection_result_t` statt `int` für libinjection-Ergebnisse. | OBSERVED | +| 2. TRUE/FALSE/ERROR-Semantik | PASS | Beide Detectors unterscheiden `TRUE`, `FALSE`, `ERROR` per `switch`; Utility mappt fail-safe. | OBSERVED | +| 3. Betroffene APIs migriert (genutzter Scope) | PASS | Im Projekt werden `libinjection_sqli` und `libinjection_xss` genutzt; beide migriert. Keine Nutzung von `is_xss`/`h5_next` im Repo gefunden. | OBSERVED + UNCERTAIN | +| 4. `libinjection_error.h` eingebunden | PASS | Includes in Adapter, Utils und Detectors vorhanden; Build-Headers führen Datei. | OBSERVED | +| 5. ERROR explizit behandelt | PASS | `LIBINJECTION_RESULT_ERROR` hat eigenen Case in XSS/SQLi mit eigener Behandlung. | OBSERVED | +| 6. Fail-safe im Security-Kontext | PASS | `ERROR` wird als malicious behandelt (`TRUE || ERROR`), Rückgabe damit blockierbar. | OBSERVED | +| 7. `if (result)`-Fallen entschärft | PASS | Kein relevanter Pfad gefunden, der `ERROR` implizit als clean behandelt; zentrale Entscheidungen sind explizit/mapped. | OBSERVED + INFERRED | +| 8. Tests für Error-Fälle | PASS | Unit-Test-Override erzeugt `ERROR`; JSON-Fälle prüfen `ret=1` und Capture für XSS/SQLi. | OBSERVED | +| 9. Logging/Monitoring parser errors | PARTIAL | Detector-Logging bei `ERROR` vorhanden; jedoch primär Debug-Logging und kein klarer dedizierter Engine-weiter Metric-Kanal sichtbar. | OBSERVED + INFERRED | +| 10. Edge-Case-Tests laut Guide | UNCERTAIN | Re-Audit belegt vorhandene ERROR-Tests, aber kein Vollnachweis aller empfohlenen Edge-Case-Szenarien im Testbestand. | UNCERTAIN | + +--- + +## 3. Assessment of tri-state propagation requirement + +- **OBSERVED (aus MIGRATION.md):** Der Guide fordert explizite Behandlung von `ERROR`, fail-safe Verhalten im WAF-Kontext, Prüfung von `if (result)`-Stellen und Tests für Fehlerfälle. +- **OBSERVED (aus MIGRATION.md):** Es gibt zusätzlich eine „Minimal Migration“-Strategie, die `TRUE || ERROR` gemeinsam als Block behandelt (bi-state policy output erlaubt). +- **INFERRED:** Eine vollständige End-to-End-Propagation von `injection_result_t` über *alle* internen Interfaces wird **nicht ausdrücklich** als Muss formuliert. +- **UNCERTAIN:** Der Guide ist nicht formal-normativ als harte Architekturvorgabe; er ist eine Migrationsrichtlinie mit empfohlenen Strategien. + +**Schluss zu dieser Frage:** Nach offizieller MIGRATION.md ist Tri-State bis zur Engine **nicht zwingend als eigener Interface-Typ** erforderlich, solange `ERROR` explizit und sicher (fail-safe) behandelt wird und nicht still als benign endet. + +--- + +## 4. Re-evaluation of Finding F-01 + +### F-01 alt: „Tri-State wird an Engine-Grenze auf bool reduziert“ + +1. **Verstoß gegen MIGRATION.md?** + - **OBSERVED:** Detectoren behandeln `ERROR` explizit und fail-safe; damit zentrale Guide-Forderungen erfüllt. + - **INFERRED:** Die reine bool-Weitergabe verletzt den Guide nicht zwingend. + +2. **Echter Migrationsfehler oder strengere Interpretation?** + - **Bewertung:** **Design-Tradeoff / strenger als Guide**. + - **OBSERVED:** Tri-State liegt zuletzt in `DetectSQLi::evaluate`/`DetectXSS::evaluate` vor (`injection_result_t` + `switch`). + - **OBSERVED:** Reduktion erfolgt bei `return isMaliciousLibinjectionResult(...)` zu boolescher Match-Semantik. + - **OBSERVED:** Danach propagieren `Operator::evaluateInternal` und `RuleWithOperator::*` nur Match/No-Match. + - **INFERRED:** Das ist architektonisch weniger expressiv, aber im Sinne der offiziellen Migration nicht automatisch falsch. + +3. **Einordnung (Bug vs Design vs Scope):** + - **Bug (Migration-Checklist):** **nein** (kein klarer Checklist-Verstoß nachweisbar). + - **Design-Tradeoff:** **ja** (Informationsverlust vs. einfaches Engine-Interface). + - **Out-of-scope der offiziellen Migration:** **teilweise ja** (end-to-end Interface-Re-Design wird nicht explizit verlangt). + +--- + +## 5. Updated verdict + +## FINAL: **FULLY MIGRATED** + +Begründung strikt nach offizieller MIGRATION.md: +- Alle im Repo genutzten libinjection-Detection-Aufrufe sind auf `injection_result_t` migriert. +- `libinjection_error.h` ist eingebunden. +- `LIBINJECTION_RESULT_ERROR` wird explizit und fail-safe behandelt (nicht als benign/falsch-negativ). +- Es gibt dedizierte Error-Tests per Override. +- Das verbleibende Thema „Tri-State nicht als eigener Typ bis ganz oben“ ist nach Guide eher Architekturverbesserung als Pflichtkriterium. + +--- + +## 6. Delta to previous audit + +### Bestätigt +- Explizite TRUE/FALSE/ERROR-Behandlung in Detectors. +- Fail-safe Verhalten bei ERROR. +- Vorhandene Error-Tests. + +### Relativiert +- Frühere harte Abwertung wegen bool-Engine war **strenger als MIGRATION.md**. +- Das F-01-Thema bleibt als Architekturhinweis valide, aber **nicht** als zwingender Migrationsfehler. + +### Korrigiert +- Vorheriges Endurteil `PARTIALLY MIGRATED` wird auf Basis der offiziellen Checklist zu `FULLY MIGRATED` angepasst. diff --git a/others/Makefile.am b/others/Makefile.am index b102a0330c..beba0bfc84 100644 --- a/others/Makefile.am +++ b/others/Makefile.am @@ -15,6 +15,7 @@ noinst_HEADERS = \ libinjection/src/libinjection_sqli.h \ libinjection/src/libinjection_sqli_data.h \ libinjection/src/libinjection_xss.h \ + libinjection/src/libinjection_error.h \ mbedtls/include/mbedtls/base64.h \ mbedtls/include/mbedtls/check_config.h \ mbedtls/include/mbedtls/mbedtls_config.h \ diff --git a/src/Makefile.am b/src/Makefile.am index 14c26697b5..476585eda2 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -187,6 +187,7 @@ OPERATORS = \ operators/contains_word.cc \ operators/detect_sqli.cc \ operators/detect_xss.cc \ + operators/libinjection_adapter.cc \ operators/ends_with.cc \ operators/eq.cc \ operators/fuzzy_hash.cc \ diff --git a/src/operators/detect_sqli.cc b/src/operators/detect_sqli.cc index 5e3014d294..5bda1a494f 100644 --- a/src/operators/detect_sqli.cc +++ b/src/operators/detect_sqli.cc @@ -21,18 +21,23 @@ #include "src/operators/operator.h" #include "src/operators/libinjection_utils.h" -#include "libinjection/src/libinjection.h" +#include "src/operators/libinjection_adapter.h" +#include "src/utils/string.h" #include "libinjection/src/libinjection_error.h" namespace modsecurity::operators { bool DetectSQLi::evaluate(Transaction *t, RuleWithActions *rule, const std::string& input, RuleMessage &ruleMessage) { +#ifndef NO_LOGS + const std::string loggable_input = + utils::string::limitTo(80, utils::string::toHexIfNeeded(input)); +#endif std::array fingerprint{}; const injection_result_t sqli_result = - libinjection_sqli(input.c_str(), input.length(), fingerprint.data()); + runLibinjectionSQLi(input.c_str(), input.length(), fingerprint.data()); if (t == nullptr) { return isMaliciousLibinjectionResult(sqli_result); @@ -42,9 +47,11 @@ bool DetectSQLi::evaluate(Transaction *t, RuleWithActions *rule, case LIBINJECTION_RESULT_TRUE: t->m_matched.emplace_back(fingerprint.data()); +#ifndef NO_LOGS ms_dbg_a(t, 4, std::string("detected SQLi using libinjection with fingerprint '") - + fingerprint.data() + "' at: '" + input + "'"); + + fingerprint.data() + "' at: '" + loggable_input + "'"); +#endif if (rule != nullptr && rule->hasCaptureAction()) { t->m_collections.m_tx_collection->storeOrUpdateFirst( @@ -57,11 +64,13 @@ bool DetectSQLi::evaluate(Transaction *t, RuleWithActions *rule, break; case LIBINJECTION_RESULT_ERROR: +#ifndef NO_LOGS ms_dbg_a(t, 4, std::string("libinjection parser error during SQLi analysis (") + libinjectionResultToString(sqli_result) + "); treating as match (fail-safe). Input: '" - + input + "'"); + + loggable_input + "'"); +#endif if (rule != nullptr && rule->hasCaptureAction()) { t->m_collections.m_tx_collection->storeOrUpdateFirst( @@ -71,12 +80,17 @@ bool DetectSQLi::evaluate(Transaction *t, RuleWithActions *rule, std::string("Added DetectSQLi error input TX.0: ") + input); } + + // Keep m_matched untouched for parser-error paths to avoid + // introducing synthetic fingerprints for non-TRUE results. break; case LIBINJECTION_RESULT_FALSE: +#ifndef NO_LOGS ms_dbg_a(t, 9, std::string("libinjection was not able to find any SQLi in: ") - + input); + + loggable_input); +#endif break; } diff --git a/src/operators/detect_xss.cc b/src/operators/detect_xss.cc index ff6d36782f..7395b247a5 100644 --- a/src/operators/detect_xss.cc +++ b/src/operators/detect_xss.cc @@ -19,16 +19,21 @@ #include "src/operators/operator.h" #include "src/operators/libinjection_utils.h" -#include "libinjection/src/libinjection.h" +#include "src/operators/libinjection_adapter.h" +#include "src/utils/string.h" #include "libinjection/src/libinjection_error.h" namespace modsecurity::operators { bool DetectXSS::evaluate(Transaction *t, RuleWithActions *rule, const std::string& input, RuleMessage &ruleMessage) { +#ifndef NO_LOGS + const std::string loggable_input = + utils::string::limitTo(80, utils::string::toHexIfNeeded(input)); +#endif const injection_result_t xss_result = - libinjection_xss(input.c_str(), input.length()); + runLibinjectionXSS(input.c_str(), input.length()); if (t == nullptr) { return isMaliciousLibinjectionResult(xss_result); @@ -44,11 +49,13 @@ bool DetectXSS::evaluate(Transaction *t, RuleWithActions *rule, break; case LIBINJECTION_RESULT_ERROR: +#ifndef NO_LOGS ms_dbg_a(t, 4, std::string("libinjection parser error during XSS analysis (") + libinjectionResultToString(xss_result) + "); treating as match (fail-safe). Input: " - + input); + + loggable_input); +#endif if (rule != nullptr && rule->hasCaptureAction()) { t->m_collections.m_tx_collection->storeOrUpdateFirst("0", input); ms_dbg_a(t, 7, std::string("Added DetectXSS error input TX.0: ") + input); @@ -56,8 +63,11 @@ bool DetectXSS::evaluate(Transaction *t, RuleWithActions *rule, break; case LIBINJECTION_RESULT_FALSE: +#ifndef NO_LOGS ms_dbg_a(t, 9, - std::string("libinjection was not able to find any XSS in: ") + input); + std::string("libinjection was not able to find any XSS in: ") + + loggable_input); +#endif break; } diff --git a/src/operators/libinjection_adapter.cc b/src/operators/libinjection_adapter.cc new file mode 100644 index 0000000000..ca7fdaca02 --- /dev/null +++ b/src/operators/libinjection_adapter.cc @@ -0,0 +1,68 @@ +/* + * ModSecurity, http://www.modsecurity.org/ + * Copyright (c) 2015 - 2021 Trustwave Holdings, Inc. (http://www.trustwave.com/) + * + * You may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * If any of the files related to licensing are missing or if you have any + * other questions related to licensing please contact Trustwave Holdings, Inc. + * directly using the email address security@modsecurity.org. + * + */ + +#include "src/operators/libinjection_adapter.h" + +#include "libinjection/src/libinjection.h" + +namespace modsecurity::operators { +namespace { + +// Per-thread overrides avoid cross-thread interference during mtstress tests. +// Intentional design: +// - thread_local to isolate tests across threads +// - function pointers to keep zero-overhead call path +// - mutable for test injection hooks +// NOSONAR: required for testing override mechanism (see set*OverrideForTesting) +thread_local DetectSQLiFn g_sqli_override = nullptr; // NOSONAR +thread_local DetectXSSFn g_xss_override = nullptr; // NOSONAR + +} + +injection_result_t runLibinjectionSQLi(const char *input, size_t len, + char *fingerprint) { + if (DetectSQLiFn fn = g_sqli_override) { + return fn(input, len, fingerprint); + } + + return libinjection_sqli(input, len, fingerprint); +} + +injection_result_t runLibinjectionXSS(const char *input, size_t len) { + if (DetectXSSFn fn = g_xss_override) { + return fn(input, len); + } + + return libinjection_xss(input, len); +} + +// Test-only hook: allows injecting alternative detection functions +// NOSONAR: function pointer is intentional (no std::function overhead) +void setLibinjectionSQLiOverrideForTesting(DetectSQLiFn fn) { // NOSONAR + g_sqli_override = fn; +} + +// Test-only hook: allows injecting alternative detection functions +// NOSONAR: function pointer is intentional (no std::function overhead) +void setLibinjectionXSSOverrideForTesting(DetectXSSFn fn) { // NOSONAR + g_xss_override = fn; +} + +void clearLibinjectionOverridesForTesting() { + g_sqli_override = nullptr; + g_xss_override = nullptr; +} + +} // namespace modsecurity::operators diff --git a/src/operators/libinjection_adapter.h b/src/operators/libinjection_adapter.h new file mode 100644 index 0000000000..f5b78371b9 --- /dev/null +++ b/src/operators/libinjection_adapter.h @@ -0,0 +1,38 @@ +/* + * ModSecurity, http://www.modsecurity.org/ + * Copyright (c) 2015 - 2021 Trustwave Holdings, Inc. (http://www.trustwave.com/) + * + * You may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * If any of the files related to licensing are missing or if you have any + * other questions related to licensing please contact Trustwave Holdings, Inc. + * directly using the email address security@modsecurity.org. + * + */ + +#ifndef SRC_OPERATORS_LIBINJECTION_ADAPTER_H_ +#define SRC_OPERATORS_LIBINJECTION_ADAPTER_H_ + +#include + +#include "libinjection/src/libinjection_error.h" // matches detect_xss.cc, detect_sqli.cc, and libinjection_utils.h + +namespace modsecurity::operators { + +using DetectSQLiFn = injection_result_t (*)(const char *, size_t, char *); +using DetectXSSFn = injection_result_t (*)(const char *, size_t); + +injection_result_t runLibinjectionSQLi(const char *input, size_t len, + char *fingerprint); +injection_result_t runLibinjectionXSS(const char *input, size_t len); + +void setLibinjectionSQLiOverrideForTesting(DetectSQLiFn fn); +void setLibinjectionXSSOverrideForTesting(DetectXSSFn fn); +void clearLibinjectionOverridesForTesting(); + +} // namespace modsecurity::operators + +#endif // SRC_OPERATORS_LIBINJECTION_ADAPTER_H_ diff --git a/test/Makefile.am b/test/Makefile.am index 2e7e05d614..de8d4f4af4 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -73,6 +73,7 @@ unit_tests_LDFLAGS = \ unit_tests_CPPFLAGS = \ -Icommon \ -I$(top_srcdir)/ \ + -I$(top_srcdir)/others \ -g \ -I$(top_builddir)/headers \ $(CURL_CFLAGS) \ diff --git a/test/test-cases/regression/operator-detectsqli.json b/test/test-cases/regression/operator-detectsqli.json index fc49b0754c..ee46f2283f 100644 --- a/test/test-cases/regression/operator-detectsqli.json +++ b/test/test-cases/regression/operator-detectsqli.json @@ -368,5 +368,98 @@ "SecRule ARGS \"@detectSQLi\" \"id:1208,phase:2,capture,pass,t:trim,setvar:tx.sqli_hit=1\"", "SecRule TX:sqli_hit \"@eq 1\" \"id:2208,phase:2,deny,status:403\"" ] + }, + { + "enabled": 1, + "version_min": 300000, + "client": { + "ip": "200.249.12.31", + "port": 123 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "response": { + "headers": { + "Date": "Mon, 13 Jul 2015 20:02:41 GMT", + "Last-Modified": "Sun, 26 Oct 2014 22:33:37 GMT", + "Content-Type": "text/html", + "Content-Length": "8" + }, + "body": [ + "no need." + ] + }, + "title": "Testing Operator :: @detectSQLi :: capture stores fingerprint in TX.0", + "request": { + "headers": { + "Host": "localhost", + "User-Agent": "curl/7.38.0", + "Accept": "*/*", + "Content-Length": "61", + "Content-Type": "application/x-www-form-urlencoded" + }, + "uri": "/", + "method": "POST", + "body": [ + "param1=ascii(substring(version() from 1 for 1))¶m2=value2" + ] + }, + "expected": { + "http_code": 403 + }, + "rules": [ + "SecRuleEngine On", + "SecRule ARGS \"@detectSQLi\" \"id:1209,phase:2,capture,pass,t:trim,setvar:tx.sqli_hit=1\"", + "SecRule TX:0 \"@streq f(f(f\" \"id:2209,phase:2,deny,status:403\"" + ] + }, + { + "enabled": 1, + "version_min": 300000, + "client": { + "ip": "200.249.12.31", + "port": 123 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "response": { + "headers": { + "Date": "Mon, 13 Jul 2015 20:02:41 GMT", + "Last-Modified": "Sun, 26 Oct 2014 22:33:37 GMT", + "Content-Type": "text/html", + "Content-Length": "8" + }, + "body": [ + "no need." + ] + }, + "title": "Testing Operator :: @detectSQLi :: no capture keeps TX.0 unchanged", + "request": { + "headers": { + "Host": "localhost", + "User-Agent": "curl/7.38.0", + "Accept": "*/*", + "Content-Length": "61", + "Content-Type": "application/x-www-form-urlencoded" + }, + "uri": "/", + "method": "POST", + "body": [ + "param1=ascii(substring(version() from 1 for 1))¶m2=value2" + ] + }, + "expected": { + "http_code": 403 + }, + "rules": [ + "SecRuleEngine On", + "SecRule ARGS \"@detectSQLi\" \"id:1210,phase:2,pass,t:trim,setvar:tx.sqli_hit=1\"", + "SecRule TX:0 \"@streq f(f(f\" \"id:2210,phase:2,deny,status:409\"", + "SecRule TX:sqli_hit \"@eq 1\" \"id:2211,phase:2,deny,status:403\"" + ] } ] diff --git a/test/test-cases/regression/operator-detectxss.json b/test/test-cases/regression/operator-detectxss.json index 8cc651f6d9..048f57ac17 100644 --- a/test/test-cases/regression/operator-detectxss.json +++ b/test/test-cases/regression/operator-detectxss.json @@ -320,5 +320,98 @@ "SecRule ARGS \"@detectXSS\" \"id:1107,phase:2,pass,t:trim,setvar:tx.xss_hit=1\"", "SecRule TX:xss_hit \"@eq 1\" \"id:2107,phase:2,deny,status:403\"" ] + }, + { + "enabled": 1, + "version_min": 300000, + "client": { + "ip": "200.249.12.31", + "port": 123 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "response": { + "headers": { + "Date": "Mon, 13 Jul 2015 20:02:41 GMT", + "Last-Modified": "Sun, 26 Oct 2014 22:33:37 GMT", + "Content-Type": "text/html", + "Content-Length": "8" + }, + "body": [ + "no need." + ] + }, + "title": "Testing Operator :: @detectXSS :: capture stores original input in TX.0", + "request": { + "headers": { + "Host": "localhost", + "User-Agent": "curl/7.38.0", + "Accept": "*/*", + "Content-Length": "46", + "Content-Type": "application/x-www-form-urlencoded" + }, + "uri": "/", + "method": "POST", + "body": [ + "param1=¶m2=value2" + ] + }, + "expected": { + "http_code": 403 + }, + "rules": [ + "SecRuleEngine On", + "SecRule ARGS \"@detectXSS\" \"id:1108,phase:2,capture,pass,t:trim,setvar:tx.xss_hit=1\"", + "SecRule TX:0 \"@streq \" \"id:2108,phase:2,deny,status:403\"" + ] + }, + { + "enabled": 1, + "version_min": 300000, + "client": { + "ip": "200.249.12.31", + "port": 123 + }, + "server": { + "ip": "200.249.12.31", + "port": 80 + }, + "response": { + "headers": { + "Date": "Mon, 13 Jul 2015 20:02:41 GMT", + "Last-Modified": "Sun, 26 Oct 2014 22:33:37 GMT", + "Content-Type": "text/html", + "Content-Length": "8" + }, + "body": [ + "no need." + ] + }, + "title": "Testing Operator :: @detectXSS :: no capture keeps TX.0 unchanged", + "request": { + "headers": { + "Host": "localhost", + "User-Agent": "curl/7.38.0", + "Accept": "*/*", + "Content-Length": "46", + "Content-Type": "application/x-www-form-urlencoded" + }, + "uri": "/", + "method": "POST", + "body": [ + "param1=¶m2=value2" + ] + }, + "expected": { + "http_code": 403 + }, + "rules": [ + "SecRuleEngine On", + "SecRule ARGS \"@detectXSS\" \"id:1109,phase:2,pass,t:trim,setvar:tx.xss_hit=1\"", + "SecRule TX:0 \"@streq \" \"id:2109,phase:2,deny,status:409\"", + "SecRule TX:xss_hit \"@eq 1\" \"id:2110,phase:2,deny,status:403\"" + ] } ] diff --git a/test/test-cases/unit/operator-libinjection-error.json b/test/test-cases/unit/operator-libinjection-error.json new file mode 100644 index 0000000000..ef440cd4c6 --- /dev/null +++ b/test/test-cases/unit/operator-libinjection-error.json @@ -0,0 +1,40 @@ +[ + { + "type": "op", + "name": "detectXSS", + "param": "", + "input": "", + "ret": 1, + "capture": 1, + "libinjection_override": "error", + "output": "" + }, + { + "type": "op", + "name": "detectSQLi", + "param": "", + "input": "''''''", + "ret": 1, + "capture": 1, + "libinjection_override": "error", + "output": "''''''" + }, + { + "type": "op", + "name": "detectXSS", + "param": "", + "input": "", + "ret": 1, + "capture": 1, + "output": "" + }, + { + "type": "op", + "name": "detectSQLi", + "param": "", + "input": "ascii(substring(version() from 1 for 1))", + "ret": 1, + "capture": 1, + "output": "f(f(f" + } +] diff --git a/test/test-suite.in b/test/test-suite.in index 6e8754254b..c667bf677f 100644 --- a/test/test-suite.in +++ b/test/test-suite.in @@ -198,6 +198,7 @@ TESTS+=test/test-cases/secrules-language-tests/operators/contains.json TESTS+=test/test-cases/secrules-language-tests/operators/containsWord.json TESTS+=test/test-cases/secrules-language-tests/operators/detectSQLi.json TESTS+=test/test-cases/secrules-language-tests/operators/detectXSS.json +TESTS+=test/test-cases/unit/operator-libinjection-error.json TESTS+=test/test-cases/secrules-language-tests/operators/endsWith.json TESTS+=test/test-cases/secrules-language-tests/operators/eq.json TESTS+=test/test-cases/secrules-language-tests/operators/ge.json diff --git a/test/unit/unit.cc b/test/unit/unit.cc index 8bf5954d27..5dc515d82e 100644 --- a/test/unit/unit.cc +++ b/test/unit/unit.cc @@ -28,6 +28,8 @@ #include "src/actions/transformations/transformation.h" #include "modsecurity/transaction.h" #include "modsecurity/actions/action.h" +#include "src/actions/capture.h" +#include "src/operators/libinjection_adapter.h" #include "test/common/modsecurity_test.h" @@ -57,6 +59,34 @@ void print_help() { } +namespace { +injection_result_t sqli_force_error(const char *, size_t, char *) { + return LIBINJECTION_RESULT_ERROR; +} + +injection_result_t xss_force_error(const char *, size_t) { + return LIBINJECTION_RESULT_ERROR; +} + +void configure_libinjection_override(const UnitTest &t) { + modsecurity::operators::clearLibinjectionOverridesForTesting(); + + if (t.libinjection_override != "error") { + return; + } + + const std::string operator_name = modsecurity::utils::string::tolower(t.name); + + if (operator_name == "detectsqli") { + modsecurity::operators::setLibinjectionSQLiOverrideForTesting( + sqli_force_error); + } else if (operator_name == "detectxss") { + modsecurity::operators::setLibinjectionXSSOverrideForTesting( + xss_force_error); + } +} +} // namespace + struct OperatorTest { using ItemType = Operator; @@ -71,13 +101,42 @@ struct OperatorTest { } static UnitTestResult eval(ItemType &op, const UnitTest &t, modsecurity::Transaction &transaction) { - modsecurity::RuleWithActions rule{nullptr, nullptr, "dummy.conf", -1}; + configure_libinjection_override(t); + + std::unique_ptr actions; + if (t.capture) { + actions = std::make_unique(); + actions->push_back(new modsecurity::actions::Capture("capture")); + } + + modsecurity::RuleWithActions rule{actions.release(), nullptr, "dummy.conf", -1}; modsecurity::RuleMessage ruleMessage{rule, transaction}; - return {op.evaluate(&transaction, &rule, t.input, ruleMessage), {}}; + + const bool matched = op.evaluate(&transaction, &rule, t.input, ruleMessage); + + UnitTestResult result; + result.ret = matched; + if (t.capture) { + auto tx0 = transaction.m_collections.m_tx_collection->resolveFirst("0"); + if (tx0 != nullptr) { + result.output = *tx0; + } + } + + modsecurity::operators::clearLibinjectionOverridesForTesting(); + return result; } static bool check(const UnitTestResult &result, const UnitTest &t) { - return result.ret != t.ret; + if (result.ret != t.ret) { + return true; + } + + if (t.capture || t.output.empty() == false) { + return result.output != t.output; + } + + return false; } }; @@ -114,7 +173,9 @@ UnitTestResult perform_unit_test_once(const UnitTest &t, modsecurity::Transactio template -UnitTestResult perform_unit_test_multithreaded(const UnitTest &t, modsecurity::Transaction &transaction) { +UnitTestResult perform_unit_test_multithreaded(const UnitTest &t, + modsecurity_test::ModSecurityTestContext &context) { + (void)context; constexpr auto NUM_THREADS = 50; constexpr auto ITERATIONS = 5'000; @@ -129,8 +190,11 @@ UnitTestResult perform_unit_test_multithreaded(const UnitTest &t, modsecurity::T { auto &result = results[i]; threads[i] = std::thread( - [&item, &t, &result, &transaction]() + [&item, &t, &result]() { + modsecurity_test::ModSecurityTestContext thread_context( + "ModSecurity-unit mtstress-thread"); + auto transaction = thread_context.create_transaction(); for (auto j = 0; j != ITERATIONS; ++j) result = TestType::eval(*item.get(), t, transaction); }); @@ -153,12 +217,13 @@ UnitTestResult perform_unit_test_multithreaded(const UnitTest &t, modsecurity::T template void perform_unit_test_helper(const ModSecurityTest &test, UnitTest &t, - ModSecurityTestResults &res, modsecurity::Transaction &transaction) { + ModSecurityTestResults &res, modsecurity::Transaction &transaction, + modsecurity_test::ModSecurityTestContext &context) { if (!test.m_test_multithreaded) t.result = perform_unit_test_once(t, transaction); else - t.result = perform_unit_test_multithreaded(t, transaction); + t.result = perform_unit_test_multithreaded(t, context); if (TestType::check(t.result, t)) { res.push_back(&t); @@ -198,9 +263,9 @@ void perform_unit_test(const ModSecurityTest &test, UnitTest &t, } if (t.type == "op") { - perform_unit_test_helper(test, t, res, transaction); + perform_unit_test_helper(test, t, res, transaction, context); } else if (t.type == "tfn") { - perform_unit_test_helper(test, t, res, transaction); + perform_unit_test_helper(test, t, res, transaction, context); } else { std::cerr << "Failed. Test type is unknown: << " << t.type; std::cerr << std::endl; diff --git a/test/unit/unit_test.cc b/test/unit/unit_test.cc index e67c100523..36e1c5a3af 100644 --- a/test/unit/unit_test.cc +++ b/test/unit/unit_test.cc @@ -110,11 +110,13 @@ std::unique_ptr UnitTest::from_yajl_node(const yajl_val &node) { size_t num_tests = node->u.object.len; auto u = std::make_unique(); + u->skipped = false; + u->capture = 0; + for (int i = 0; i < num_tests; i++) { const char *key = node->u.object.keys[ i ]; yajl_val val = node->u.object.values[ i ]; - u->skipped = false; if (strcmp(key, "param") == 0) { u->param = YAJL_GET_STRING(val); } else if (strcmp(key, "input") == 0) { @@ -128,6 +130,10 @@ std::unique_ptr UnitTest::from_yajl_node(const yajl_val &node) { u->type = YAJL_GET_STRING(val); } else if (strcmp(key, "ret") == 0) { u->ret = YAJL_GET_INTEGER(val); + } else if (strcmp(key, "capture") == 0) { + u->capture = YAJL_GET_INTEGER(val); + } else if (strcmp(key, "libinjection_override") == 0) { + u->libinjection_override = YAJL_GET_STRING(val); } else if (strcmp(key, "output") == 0) { u->output = std::string(YAJL_GET_STRING(val)); json2bin(&u->output); diff --git a/test/unit/unit_test.h b/test/unit/unit_test.h index ffd776442b..95257d7061 100644 --- a/test/unit/unit_test.h +++ b/test/unit/unit_test.h @@ -44,7 +44,9 @@ class UnitTest { std::string type; std::string filename; std::string output; + std::string libinjection_override; int ret; + int capture; int skipped; UnitTestResult result; };