Add IDOR protection by hansott · Pull Request #455 · AikidoSec/firewall-php

hansott · 2026-08-20T11:32:02Z

No description provided.

aikido-pr-checks · 2026-08-20T11:32:42Z

+	defer C.call_free_string(freeFn, resultPtr)
+
+	result := C.GoString(resultPtr)
+	log.Debugf(nil, "IdorAnalyzeSql(\"%s\", %d) -> %s", query, dialect, result)


IdorAnalyzeSql logs the full query, which may contain personal data or other user-submitted values.

Details

✨ AI Reasoning
The new diagnostic log includes the complete query argument. SQL submitted by an application can contain literal names, email addresses, identifiers, or other personal values, so logging the full query exposes potentially sensitive user input.

🔧 How do I fix it?
Keep sensitive data such as emails, passwords, and tokens out of logs. When logging values tied to a user, prefer a safe identifier like a user ID over the raw input, and strip line breaks from any user-provided text you do log.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

aikido-pr-checks · 2026-08-20T11:32:43Z

                break;
+            case SQL_PARAMS:
+                ctx = "SQL_PARAMS";
+                ret = GetEventCacheField(&EventCache::sqlParams);


SQL_PARAMS exposes raw bound parameter values to callback logging, potentially recording personal data submitted through PDO.

Details

✨ AI Reasoning
The new SQL_PARAMS callback returns serialized bound parameter values. The callback wrapper logs every non-empty returned value, so adding this case causes raw PDO parameters to be written to logs, including values such as names, emails, or account identifiers.

🔧 How do I fix it?
Keep sensitive data such as emails, passwords, and tokens out of logs. When logging values tied to a user, prefer a safe identifier like a user ID over the raw input, and strip line breaks from any user-provided text you do log.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

aikido-pr-checks · 2026-08-20T11:32:43Z

+	return "Zen IDOR protection: set_tenant_id() was not called. A tenant ID is required for every query."
+}
+
+func checkWhereFilters(result SqlQueryResult, tenantColumn string, tenantId string, excludedTables map[string]bool, params map[string]string) string {


The function checkWhereFilters uses an ambiguous 'check' prefix, obscuring that it validates tenant filters and returns a violation message.

Details

✨ AI Reasoning
The function evaluates whether tenant filters are present and valid, returning an error message when validation fails. The prefix does not clarify whether the function returns data, a boolean, or performs side effects.

🔧 How do I fix it?
Replace 'check' with more descriptive verbs that indicate the function's action or purpose. Use 'validate' for validation, 'get' for retrieval, or 'is' for boolean checks. Ensure the name clearly communicates the function's intent and return type.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

aikido-pr-checks · 2026-08-20T11:32:44Z

+	return ""
+}
+
+func checkInsert(result SqlQueryResult, tenantColumn string, tenantId string, excludedTables map[string]bool, params map[string]string) string {


The function checkInsert uses an ambiguous 'check' prefix, obscuring that it validates tenant columns in INSERT statements and returns a violation message.

Details

✨ AI Reasoning
The function validates tenant ownership for inserted rows and returns a descriptive violation message. Its name leaves the function's result and purpose underspecified.

🔧 How do I fix it?
Replace 'check' with more descriptive verbs that indicate the function's action or purpose. Use 'validate' for validation, 'get' for retrieval, or 'is' for boolean checks. Ensure the name clearly communicates the function's intent and return type.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

aikido-pr-checks · 2026-08-20T11:32:45Z

+}
+
+// CheckContextForIdor returns a violation message, or "" if the query is allowed.
+func CheckContextForIdor(instance *instance.RequestProcessorInstance, query string, dialect string) string {


The function CheckContextForIdor uses an ambiguous 'check' prefix, obscuring that it validates a SQL query and returns an IDOR violation message.

Details

✨ AI Reasoning
The function orchestrates IDOR validation for SQL queries and returns a violation string when the query is not allowed. The name does not communicate this validation result or operation clearly.

🔧 How do I fix it?
Replace 'check' with more descriptive verbs that indicate the function's action or purpose. Use 'validate' for validation, 'get' for retrieval, or 'is' for boolean checks. Ensure the name clearly communicates the function's intent and return type.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

aikido-pr-checks · 2026-08-20T11:35:19Z

+	dialectId := utils.GetSqlDialectFromString(dialect)
+	results, ok := analyzeSql(query, dialectId)
+	if !ok {
+		return ""


🟠 High - PostgreSQL PDO queries bypass the new IDOR check because the dialect is misidentified

The new IDOR protection claims to support PostgreSQL PDO, but the PHP hook forwards PDO::ATTR_DRIVER_NAME verbatim (pgsql) while the Go dialect mapper only recognizes postgres. That causes PostgreSQL statements to be analyzed as the generic dialect, and when the parser cannot analyze PostgreSQL-specific SQL the new CheckContextForIdor path returns an empty result and allows the query. As a result, tenants using PDO PostgreSQL can run statements without the intended tenant-filter enforcement, weakening the access-control protection this PR adds.

Show fix

Normalize the PDO PostgreSQL driver name before analysis, e.g. map pgsql to the PostgreSQL dialect in the PHP or Go layer, and add PostgreSQL regression tests that exercise both allowed and blocked IDOR cases with real PDO pgsql queries so parser failures cannot silently disable enforcement.

_{More info - Reply on this comment to give feedback or ignore the issue.}

aikido-pr-checks · 2026-08-20T11:37:03Z

+	// Check SQL injection first: block malicious queries before spending time on IDOR analysis.
 	res := sql_injection.CheckContextForSqlInjection(instance, query, operation, dialect)
 	if res != nil {
 		return attack.ReportAttackDetected(res, instance)
 	}


🟠 High - SQL injection detections suppress IDOR enforcement when AIKIDO_BLOCK is off

This PR returns immediately after any SQL-injection match, so the new IDOR validator is never reached for those queries. The returned action is an ordinary attack throw, and the PHP layer explicitly skips ordinary actions in detection-only mode while only force-executing idorViolation actions. As a result, on deployments with AIKIDO_BLOCK=0, a query that both triggers SQLi detection and omits or weakens the tenant filter can still execute, bypassing the PR's promised always-enforced IDOR protection.

Show fix

Do not short-circuit IDOR evaluation on SQL-injection findings. Evaluate IDOR before returning, or merge the results so an IDOR violation still produces an idorViolation throw even when SQL injection is also detected; add a regression test with AIKIDO_BLOCK=0 covering a SQLi-detected query that should still be blocked by IDOR.

_{More info - Reply on this comment to give feedback or ignore the issue.}

aikido-pr-checks · 2026-08-20T11:38:59Z

+    auto& requestCache = AIKIDO_GLOBAL(requestCache);
+    requestCache.idorIgnoredDepth++;
+
+    zval retval;
+    ZVAL_UNDEF(&retval);
+    fci.retval = &retval;
+    fci.param_count = 0;
+    fci.params = nullptr;
+
+    zend_call_function(&fci, &fcc);
+
+    requestCache.idorIgnoredDepth--;


🟡 Medium - without_idor_protection disables IDOR checks for other fibers in the same request

The new bypass helper stores its ignore state in the request-global RequestCache::idorIgnoredDepth before invoking the callback and only clears it after zend_call_function returns. If the callback suspends a PHP Fiber or otherwise yields control, another Fiber in the same request can run while that global depth is still non-zero, and the Go-side IDOR checker will skip enforcement for that unrelated query. In async PHP applications that use Fibers, one code path opting out of IDOR can therefore accidentally or maliciously disable tenant checks for other concurrent database operations in the same request.

Show fix

Scope the IDOR-ignore flag to the current execution context rather than the whole request, for example by tying it to the current fiber/coroutine or by implementing the bypass around a synchronous, non-yielding critical section. At minimum, document and enforce that without_idor_protection() callbacks must not suspend, and add a regression test with two Fibers proving unrelated queries remain protected.

_{More info - Reply on this comment to give feedback or ignore the issue.}

aikido-pr-checks · 2026-08-20T13:12:33Z

 		return attack.ReportAttackDetected(res, instance)
 	}
+
+	if idorMessage := idor.CheckContextForIdor(instance, query, dialect); idorMessage != "" {


idor.CheckContextForIdor starts with Check, which leaves the function's intent and side effects unclear.

Details

✨ AI Reasoning
The new IDOR detection call uses a function named CheckContextForIdor. A Check prefix does not communicate whether the function only returns a result or also performs detection-related side effects, making its behavior less clear to callers.

🔧 How do I fix it?
Replace 'check' with more descriptive verbs that indicate the function's action or purpose. Use 'validate' for validation, 'get' for retrieval, or 'is' for boolean checks. Ensure the name clearly communicates the function's intent and return type.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

aikido-pr-checks · 2026-08-20T13:12:33Z

+    requestCache.tenantId = tenantIdValue;
+    requestCache.tenantIdSet = true;
+
+    AIKIDO_LOG_DEBUG("Set tenant id to %s\n", requestCache.tenantId.c_str());


The AIKIDO_LOG_DEBUG call logs requestCache.tenantId, which may identify a person and exposes personal data in application logs.

Details

✨ AI Reasoning
The new debug log includes request-derived tenant identity data through requestCache.tenantId. This value is supplied by the caller and may identify an individual, so writing it to logs exposes personal data.

🔧 How do I fix it?
Keep sensitive data such as emails, passwords, and tokens out of logs. When logging values tied to a user, prefer a safe identifier like a user ID over the raw input, and strip line breaks from any user-provided text you do log.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

aikido-pr-checks · 2026-08-20T13:12:34Z

+        zval* entry;
+        ZEND_HASH_FOREACH_VAL(Z_ARRVAL_P(excludedTablesArr), entry) {
+            if (!IsScalarZval(entry)) {
+                AIKIDO_LOG_WARN("enable_idor_protection: skipping non-string entry in excludedTables!\n");


enable_idor_protection warns “skipping non-string entry”, but it only skips non-scalars. Numeric/boolean entries are accepted, so the warning contradicts the actual branch condition.

Suggested change

AIKIDO_LOG_WARN("enable_idor_protection: skipping non-string entry in excludedTables!\n");

AIKIDO_LOG_WARN("enable_idor_protection: skipping non-scalar entry in excludedTables!\n");

Details

✨ AI Reasoning
The warning describes a narrower condition than the code actually checks. The loop skips only non-scalars, not all non-strings. This mismatch between condition and message can mislead operators about what data was rejected.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

aikido-pr-checks · 2026-08-20T13:15:18Z

+            }
+            case CONTEXT_TENANT_ID:
+                ctx = "TENANT_ID";
+                ret = requestCache.tenantIdSet ? requestCache.tenantId : "";


🟡 Medium - Embedded NUL tenant IDs are truncated before IDOR enforcement compares them

set_tenant_id() stores the full PHP string, including embedded \0 bytes, but the callback path that hands the tenant ID to Go returns it as a plain NUL-terminated C string. That truncates values such as "victim\0attacker" to "victim" before CheckContextForIdor compares them, so the firewall can validate queries against the wrong tenant prefix. If an application derives tenant IDs from attacker-controlled or binary-bearing data, a query filtered on the truncated prefix can pass the new IDOR check even though it does not match the real tenant ID set in PHP.

Show fix

Reject tenant IDs containing embedded NUL bytes in set_tenant_id(), or change the PHP<->Go callback contract to pass explicit lengths instead of NUL-terminated strings for tenant IDs. Add regression coverage for a tenant ID containing \0 to ensure Go receives the exact value and mismatched prefixes are blocked.

_{More info - Reply on this comment to give feedback or ignore the issue.}

aikido-pr-checks · 2026-08-20T13:17:29Z

+		case "select", "update", "delete":
+			if msg := checkWhereFilters(result, tenantColumn, tenantId, excludedTables, params); msg != "" {
+				return msg
+			}
+		case "insert":
+			if msg := checkInsert(result, tenantColumn, tenantId, excludedTables, params); msg != "" {


🟠 High - IDOR checks ignore tenant reassignment in UPDATE and upsert assignment clauses

The new validator only verifies tenant filters for UPDATE statements and only verifies inserted column values for INSERT statements, but it never inspects assignments that modify the tenant column itself. A query such as UPDATE cats SET tenant_id='org_456' WHERE id=1 AND tenant_id='org_123' therefore passes because the WHERE clause matches the current tenant, and MySQL INSERT ... ON DUPLICATE KEY UPDATE tenant_id=... can similarly reassign an existing row to another tenant. This creates a direct cross-tenant isolation bypass: code running as one tenant can move rows into a different tenant while still satisfying the new IDOR check.

Show fix

Extend the SQL analysis contract and Go-side validation to inspect tenant-column assignments in UPDATE statements and conflict/update clauses such as ON DUPLICATE KEY UPDATE, and reject any assignment whose resolved value differs from the current tenant. Add regression tests that block UPDATE ... SET tenant_id = ? WHERE tenant_id = ? and duplicate-key upserts that change tenant_id on the update path.

_{More info - Reply on this comment to give feedback or ignore the issue.}

aikido-pr-checks · 2026-08-20T13:17:58Z

+    requestCache.idorTenantColumnName = tenantColumnNameStr;
+    requestCache.idorExcludedTables = excludedTables;
+    requestCache.idorProtectionEnabled = true;


🟠 High - IDOR protection configuration is lost after each worker request

The new IDOR settings are stored only in the per-request RequestCache, but request initialization unconditionally resets that cache before every request. In worker deployments such as FrankenPHP, applications are commonly bootstrapped once and then call worker_rinit() for each request, so a one-time enable_idor_protection() call made during bootstrap is silently cleared on the next request. That leaves subsequent SQL queries running with IDOR protection disabled even though the application enabled it at startup.

Show fix

Persist IDOR configuration in process- or app-level state rather than the per-request cache, or require and document that enable_idor_protection() must run on every request before any queries execute. Add a worker-mode regression test that enables IDOR during bootstrap, handles two requests, and verifies the second request is still protected.

_{More info - Reply on this comment to give feedback or ignore the issue.}

aikido-pr-checks · 2026-08-21T12:13:02Z

+				continue
+			}
+			if f.Table != nil {
+				if *f.Table == table.Name || (table.Alias != nil && *f.Table == *table.Alias) {


The nested table/filter traversal reaches five meaningful control-flow levels around the qualified-table match, making checkWhereFilters difficult to follow. Extract the filter-selection logic into a helper.

Details

✨ AI Reasoning
The table and filter traversal combines several checks inside the same loop chain, requiring readers to follow multiple branches before understanding how a matching tenant filter is selected. Extracting the filter-matching logic would make the surrounding validation easier to read and maintain.

🔧 How do I fix it?
Keep nesting levels under 4. Extract complex logic into separate functions when indentation exceeds 4 levels.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.}
_{Reply @AikidoSec ignore: [REASON] to ignore this issue.}
_{More info}

aikido-pr-checks Bot reviewed Aug 20, 2026

View reviewed changes

hansott force-pushed the idor2 branch from 0a8552b to ceb2536 Compare August 20, 2026 13:11

aikido-pr-checks Bot reviewed Aug 20, 2026

View reviewed changes

Comment thread lib/php-extension/HandleIdorProtection.cpp Outdated

aikido-pr-checks Bot reviewed Aug 20, 2026

View reviewed changes

hansott added 2 commits August 21, 2026 13:58

Add IDOR protection

54ba5f1

Fix complication on PHP 7.x

5268712

hansott force-pushed the idor2 branch from ceb2536 to 5268712 Compare August 21, 2026 12:12

aikido-pr-checks Bot reviewed Aug 21, 2026

View reviewed changes

Fix compilation on PHP 7.x

e654831

	AIKIDO_LOG_WARN("enable_idor_protection: skipping non-string entry in excludedTables!\n");
	AIKIDO_LOG_WARN("enable_idor_protection: skipping non-scalar entry in excludedTables!\n");

Uh oh!

Conversation

hansott commented Aug 20, 2026

Uh oh!

aikido-pr-checks Bot Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aikido-pr-checks Bot Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Aug 20, 2026

Choose a reason for hiding this comment

Uh oh!

aikido-pr-checks Bot Aug 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

aikido-pr-checks Bot Aug 20, 2026 •

edited

Loading