diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 72b131b9..fe246aea 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -214,7 +214,7 @@ jobs: echo $AIKIDO_VERSION echo "AIKIDO_VERSION=$AIKIDO_VERSION" >> $GITHUB_ENV echo "AIKIDO_LIBZEN=libzen_internals_${{ env.ARCH }}-unknown-linux-gnu.so" >> $GITHUB_ENV - echo "AIKIDO_LIBZEN_VERSION=0.1.60" >> $GITHUB_ENV + echo "AIKIDO_LIBZEN_VERSION=0.1.61" >> $GITHUB_ENV - name: Download artifacts (NTS) uses: actions/download-artifact@fa0a91b85d4f404e444e00e005971372dc801d16 # v4 diff --git a/docs/invalid-sql-queries.md b/docs/invalid-sql-queries.md new file mode 100644 index 00000000..5be743f6 --- /dev/null +++ b/docs/invalid-sql-queries.md @@ -0,0 +1,7 @@ +# Blocking invalid SQL queries + +Zen can block SQL queries that it can't tokenize when they contain user input. This prevents attackers from bypassing SQL injection detection with malformed queries. For example, ClickHouse ignores invalid SQL after `;`, and SQLite runs queries before an unclosed `/*` comment. + +This is NOT on by default, but it can be enabled by setting the `AIKIDO_BLOCK_INVALID_SQL` environment variable to `true`. +If enabled, in blocking mode, these queries are blocked. In detection-only mode, they are reported but still executed. + diff --git a/lib/php-extension/Environment.cpp b/lib/php-extension/Environment.cpp index bc2903f8..a9e6cebe 100644 --- a/lib/php-extension/Environment.cpp +++ b/lib/php-extension/Environment.cpp @@ -24,7 +24,7 @@ std::string GetPhpEnvVariable(const std::string& env_key) { std::string GetSystemEnvVariable(const std::string& env_key) { const char* env_value = getenv(env_key.c_str()); if (!env_value) return ""; - + if (env_key == "AIKIDO_TOKEN") { AIKIDO_LOG_DEBUG("sys_env[%s] = %s\n", env_key.c_str(), AnonymizeToken(env_value).c_str()); } else { @@ -105,16 +105,16 @@ std::string GetFrankenEnvVariable(const std::string& env_key) { if (std::string(sapi_module.name) != "frankenphp") { return ""; } - + // Force $_SERVER autoglobal to be initialized (it's lazily loaded in PHP) // This is CRITICAL in ZTS mode to ensure each thread gets request-specific $_SERVER values zend_is_auto_global_str(ZEND_STRL("_SERVER")); - + if (Z_TYPE(PG(http_globals)[TRACK_VARS_SERVER]) != IS_ARRAY) { AIKIDO_LOG_DEBUG("franken_env[%s] = (empty - $_SERVER not an array)\n", env_key.c_str()); return ""; } - + std::string env_value = AIKIDO_GLOBAL(server).GetVar(env_key.c_str()); if (!env_value.empty()) { if (env_key == "AIKIDO_TOKEN") { @@ -143,9 +143,9 @@ std::string GetLaravelEnvVariable(const std::string& env_key) { Load env variables from the following sources (priority order): - System environment variables - FrankenPHP environment variables ($_SERVER - request-specific, thread-safe) - - PHP environment variables + - PHP environment variables - Laravel environment variables - + Order is critical: In multithreaded environments (FrankenPHP worker/classic, ZTS), getenv() returns cached process-level values that may belong to a different request. $_SERVER must be checked first to get fresh, request-specific environment data. @@ -228,6 +228,7 @@ void LoadEnvironmentFromGetters(const std::vector& envGetters) { AIKIDO_GLOBAL(collect_api_schema) = GetEnvBool(envGetters,"AIKIDO_FEATURE_COLLECT_API_SCHEMA", true); AIKIDO_GLOBAL(localhost_allowed_by_default) = GetEnvBool(envGetters, "AIKIDO_LOCALHOST_ALLOWED_BY_DEFAULT", true); AIKIDO_GLOBAL(trust_proxy) = GetEnvBool(envGetters, "AIKIDO_TRUST_PROXY", true); + AIKIDO_GLOBAL(block_invalid_sql) = GetEnvBool(envGetters, "AIKIDO_BLOCK_INVALID_SQL", false); AIKIDO_GLOBAL(disk_logs) = GetEnvBool(envGetters, "AIKIDO_DISK_LOGS", false); AIKIDO_GLOBAL(sapi_name) = sapi_module.name; AIKIDO_GLOBAL(token) = GetEnvString(envGetters, "AIKIDO_TOKEN", ""); diff --git a/lib/php-extension/RequestProcessor.cpp b/lib/php-extension/RequestProcessor.cpp index c728363f..166c70fe 100644 --- a/lib/php-extension/RequestProcessor.cpp +++ b/lib/php-extension/RequestProcessor.cpp @@ -96,6 +96,7 @@ std::string RequestProcessor::GetInitData(const std::string& userProvidedToken) {"disk_logs", AIKIDO_GLOBAL(disk_logs)}, {"localhost_allowed_by_default", AIKIDO_GLOBAL(localhost_allowed_by_default)}, {"collect_api_schema", AIKIDO_GLOBAL(collect_api_schema)}, + {"block_invalid_sql", AIKIDO_GLOBAL(block_invalid_sql)}, {"packages", packages}}; return NormalizeAndDumpJson(initData); } diff --git a/lib/php-extension/include/php_aikido.h b/lib/php-extension/include/php_aikido.h index 51cadbe1..fc07546a 100644 --- a/lib/php-extension/include/php_aikido.h +++ b/lib/php-extension/include/php_aikido.h @@ -34,6 +34,7 @@ bool disk_logs; // When enabled, it writes logs to disk instead of stdout. It's bool collect_api_schema; bool trust_proxy; bool localhost_allowed_by_default; +bool block_invalid_sql; // Block SQL queries that fail tokenization when user input is present (AIKIDO_BLOCK_INVALID_SQL, default: false) bool uses_symfony_http_foundation; // If true, method override is supported using X-HTTP-METHOD-OVERRIDE or _method query param unsigned int report_stats_interval_to_agent; // Report once every X requests the collected stats to Agent std::chrono::high_resolution_clock::time_point currentRequestStart; diff --git a/lib/request-processor/aikido_types/config.go b/lib/request-processor/aikido_types/config.go index 42d2cedf..74ff6be6 100644 --- a/lib/request-processor/aikido_types/config.go +++ b/lib/request-processor/aikido_types/config.go @@ -23,6 +23,7 @@ type AikidoConfigData struct { TrustProxy bool `json:"trust_proxy"` // default: true LocalhostAllowedByDefault bool `json:"localhost_allowed_by_default"` // default: true CollectApiSchema bool `json:"collect_api_schema"` // default: true + BlockInvalidSql bool `json:"block_invalid_sql"` // default: false DiskLogs bool `json:"disk_logs"` // default: false Packages map[string]string `json:"packages"` // default: {} } diff --git a/lib/request-processor/vulnerabilities/sql-injection/checkContextForSqlInjection.go b/lib/request-processor/vulnerabilities/sql-injection/checkContextForSqlInjection.go index c97ba1ba..2b948e6a 100644 --- a/lib/request-processor/vulnerabilities/sql-injection/checkContextForSqlInjection.go +++ b/lib/request-processor/vulnerabilities/sql-injection/checkContextForSqlInjection.go @@ -5,6 +5,7 @@ import ( "main/helpers" "main/instance" "main/utils" + zen_internals "main/vulnerabilities/zen-internals" ) /** @@ -15,26 +16,37 @@ func CheckContextForSqlInjection(instance *instance.RequestProcessorInstance, sq trimmedSql := helpers.TrimInvisible(sql) dialectId := utils.GetSqlDialectFromString(dialect) + blockInvalidSql := false + if server := instance.GetCurrentServer(); server != nil { + blockInvalidSql = server.AikidoConfig.BlockInvalidSql + } + for _, source := range context.SOURCES { mapss := source.CacheGet(instance) for str, path := range mapss { trimmedInputString := helpers.TrimInvisible(str) - if detectSQLInjection(trimmedSql, trimmedInputString, dialectId) { + result := detectSQLInjection(trimmedSql, trimmedInputString, dialectId) + + if (result == zen_internals.SQLInjectionDetected) || + (result == zen_internals.SQLInjectionTokenizeFailed && blockInvalidSql) { + metadata := map[string]string{ + "sql": sql, + "dialect": dialect, + } + if result == zen_internals.SQLInjectionTokenizeFailed { + metadata["failedToTokenize"] = "true" + } return &utils.InterceptorResult{ Operation: operation, Kind: utils.Sql_injection, Source: source.Name, PathToPayload: path, - Metadata: map[string]string{ - "sql": sql, - "dialect": dialect, - }, - Payload: str, + Metadata: metadata, + Payload: str, } } } } return nil - } diff --git a/lib/request-processor/vulnerabilities/sql-injection/detectSqlInjection.go b/lib/request-processor/vulnerabilities/sql-injection/detectSqlInjection.go index 4f0900e1..9b2b6485 100644 --- a/lib/request-processor/vulnerabilities/sql-injection/detectSqlInjection.go +++ b/lib/request-processor/vulnerabilities/sql-injection/detectSqlInjection.go @@ -34,12 +34,11 @@ func shouldReturnEarly(query, userInput string) bool { return match } -func detectSQLInjection(query string, userInput string, dialect int) bool { +func detectSQLInjection(query string, userInput string, dialect int) int { if shouldReturnEarly(query, userInput) { - return false + return zen_internals.SqlInjectionClean } // Executing our final check with zen_internals - return zen_internals.DetectSQLInjection(query, userInput, dialect) == 1 - + return zen_internals.DetectSQLInjection(query, userInput, dialect) } diff --git a/lib/request-processor/vulnerabilities/zen-internals/zen_internals.go b/lib/request-processor/vulnerabilities/zen-internals/zen_internals.go index 4676ad3f..160c981f 100644 --- a/lib/request-processor/vulnerabilities/zen-internals/zen_internals.go +++ b/lib/request-processor/vulnerabilities/zen-internals/zen_internals.go @@ -33,6 +33,13 @@ type ZenInternalsLibrary struct { detectSqlInjection C.detect_sql_injection_func } +const ( + SqlInjectionClean = 0 + SQLInjectionDetected = 1 + SQLInjectionError = 2 + SQLInjectionTokenizeFailed = 3 +) + var zenLib = &ZenInternalsLibrary{} func Init() bool { @@ -75,7 +82,7 @@ func DetectSQLInjection(query string, user_input string, dialect int) int { detectFn := zenLib.detectSqlInjection if detectFn == nil { - return 0 + return SqlInjectionClean } // Convert strings to C strings diff --git a/tests/server/test_sql_injection_invalid_sql_blocked/env.json b/tests/server/test_sql_injection_invalid_sql_blocked/env.json new file mode 100644 index 00000000..8d65cd26 --- /dev/null +++ b/tests/server/test_sql_injection_invalid_sql_blocked/env.json @@ -0,0 +1,6 @@ +{ + "AIKIDO_BLOCK": "1", + "AIKIDO_BLOCK_INVALID_SQL": "1", + "AIKIDO_LOCALHOST_ALLOWED_BY_DEFAULT": "0", + "AIKIDO_FEATURE_COLLECT_API_SCHEMA": "1" +} diff --git a/tests/server/test_sql_injection_invalid_sql_blocked/expect_detection_blocked.json b/tests/server/test_sql_injection_invalid_sql_blocked/expect_detection_blocked.json new file mode 100644 index 00000000..a06f2b85 --- /dev/null +++ b/tests/server/test_sql_injection_invalid_sql_blocked/expect_detection_blocked.json @@ -0,0 +1,31 @@ +{ + "type": "detected_attack", + "request": { + "method": "GET", + "source": "php", + "route": "/testDetection" + }, + "attack": { + "kind": "sql_injection", + "operation": "pdo->query", + "module": "PDO", + "blocked": true, + "source": "query", + "path": ".id", + "stack": "tests/server/test_sql_injection_invalid_sql_blocked/index.php(18): PDO->query()", + "payload": "1 /*", + "metadata": { + "dialect": "sqlite", + "sql": "SELECT * FROM cats WHERE id = 1 /*", + "failedToTokenize": "true" + }, + "user": { + "id": "12345", + "name": "Test User" + } + }, + "agent": { + "dryMode": false, + "library": "firewall-php" + } +} diff --git a/tests/server/test_sql_injection_invalid_sql_blocked/index.php b/tests/server/test_sql_injection_invalid_sql_blocked/index.php new file mode 100644 index 00000000..e17d0ea3 --- /dev/null +++ b/tests/server/test_sql_injection_invalid_sql_blocked/index.php @@ -0,0 +1,24 @@ +setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); + + $pdo->exec("CREATE TABLE IF NOT EXISTS cats ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + name TEXT NOT NULL, + age INTEGER NOT NULL + )"); + + $pdo->exec("INSERT INTO cats (name, age) VALUES ('n', 1)"); + + $id = $_GET['id']; + $pdo->query("SELECT * FROM cats WHERE id = $id"); + + echo "Query executed!"; + +} catch (PDOException $e) { + echo "Error: " . $e->getMessage(); +} diff --git a/tests/server/test_sql_injection_invalid_sql_blocked/start_config.json b/tests/server/test_sql_injection_invalid_sql_blocked/start_config.json new file mode 100644 index 00000000..430d3112 --- /dev/null +++ b/tests/server/test_sql_injection_invalid_sql_blocked/start_config.json @@ -0,0 +1,10 @@ +{ + "success": true, + "serviceId": 1, + "heartbeatIntervalInMS": 600000, + "endpoints": [], + "blockedUserIds": [], + "allowedIPAddresses": [], + "receivedAnyStats": true, + "block": true +} diff --git a/tests/server/test_sql_injection_invalid_sql_blocked/test.py b/tests/server/test_sql_injection_invalid_sql_blocked/test.py new file mode 100644 index 00000000..2c09276d --- /dev/null +++ b/tests/server/test_sql_injection_invalid_sql_blocked/test.py @@ -0,0 +1,24 @@ +from testlib import * + +""" +Invalid SQL (failed tokenization) with user input is blocked when AIKIDO_BLOCK=1 and +AIKIDO_BLOCK_INVALID_SQL=1. Uses PDO SQLite and an unclosed block comment in the query. +""" + + +def run_test(): + response = php_server_get("/testDetection?id=1+%2F*") + assert_response_code_is(response, 500) + assert_response_body_contains(response, "") + + mock_server_wait_for_new_events(5) + + events = mock_server_get_events() + assert_events_length_is(events, 2) + assert_started_event_is_valid(events[0]) + assert_event_contains_subset_file(events[1], "expect_detection_blocked.json") + + +if __name__ == "__main__": + load_test_args() + run_test() diff --git a/tests/server/test_sql_injection_invalid_sql_not_blocked/env.json b/tests/server/test_sql_injection_invalid_sql_not_blocked/env.json new file mode 100644 index 00000000..7d87cafe --- /dev/null +++ b/tests/server/test_sql_injection_invalid_sql_not_blocked/env.json @@ -0,0 +1,6 @@ +{ + "AIKIDO_BLOCK": "1", + "AIKIDO_BLOCK_INVALID_SQL": "0", + "AIKIDO_LOCALHOST_ALLOWED_BY_DEFAULT": "0", + "AIKIDO_FEATURE_COLLECT_API_SCHEMA": "1" +} diff --git a/tests/server/test_sql_injection_invalid_sql_not_blocked/index.php b/tests/server/test_sql_injection_invalid_sql_not_blocked/index.php new file mode 100644 index 00000000..e17d0ea3 --- /dev/null +++ b/tests/server/test_sql_injection_invalid_sql_not_blocked/index.php @@ -0,0 +1,24 @@ +setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); + + $pdo->exec("CREATE TABLE IF NOT EXISTS cats ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + name TEXT NOT NULL, + age INTEGER NOT NULL + )"); + + $pdo->exec("INSERT INTO cats (name, age) VALUES ('n', 1)"); + + $id = $_GET['id']; + $pdo->query("SELECT * FROM cats WHERE id = $id"); + + echo "Query executed!"; + +} catch (PDOException $e) { + echo "Error: " . $e->getMessage(); +} diff --git a/tests/server/test_sql_injection_invalid_sql_not_blocked/start_config.json b/tests/server/test_sql_injection_invalid_sql_not_blocked/start_config.json new file mode 100644 index 00000000..430d3112 --- /dev/null +++ b/tests/server/test_sql_injection_invalid_sql_not_blocked/start_config.json @@ -0,0 +1,10 @@ +{ + "success": true, + "serviceId": 1, + "heartbeatIntervalInMS": 600000, + "endpoints": [], + "blockedUserIds": [], + "allowedIPAddresses": [], + "receivedAnyStats": true, + "block": true +} diff --git a/tests/server/test_sql_injection_invalid_sql_not_blocked/test.py b/tests/server/test_sql_injection_invalid_sql_not_blocked/test.py new file mode 100644 index 00000000..72ee423f --- /dev/null +++ b/tests/server/test_sql_injection_invalid_sql_not_blocked/test.py @@ -0,0 +1,19 @@ +import time +from testlib import * + +""" +With AIKIDO_BLOCK=1 but AIKIDO_BLOCK_INVALID_SQL=0, queries that only fail tokenization +(result 3) are not blocked and no attack event is reported. +""" + + +def run_test(): + response = php_server_get("/testDetection?id=1+%2F*") + assert_response_code_is(response, 200) + assert_response_body_contains(response, "Error: SQLSTATE[HY000]") + assert_events_length_is(mock_server_get_events(), 1) # No attack event is reported. + assert_started_event_is_valid(mock_server_get_events()[0]) + +if __name__ == "__main__": + load_test_args() + run_test()