-
Notifications
You must be signed in to change notification settings - Fork 13
IDOR protection #391
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
IDOR protection #391
Changes from all commits
7a0298a
d1413ed
ef22c29
55c6871
43cacbf
975ac92
d8a19d4
d8031c8
c222de1
25630b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| # IDOR Protection | ||
|
|
||
| IDOR stands for Insecure Direct Object Reference β it's when one account can access another account's data because a query doesn't properly filter by account. | ||
|
|
||
| If your SaaS has accounts (or organizations, workspaces, teams, ...) and uses a column like `tenant_id` to keep each account's data separate, IDOR protection ensures every SQL query filters on the correct tenant. Zen analyzes queries at runtime and throws an error if a query is missing that filter or uses the wrong tenant ID, catching mistakes like: | ||
|
|
||
| - A `SELECT` that forgets the tenant filter, letting one account read another's orders | ||
| - An `UPDATE` or `DELETE` without a tenant filter, letting one account modify another's data | ||
| - An `INSERT` that omits the tenant column, creating orphaned or misassigned rows | ||
|
|
||
| Zen catches these at runtime so they surface during development and testing, not in production. See [IDOR vulnerability explained](https://www.aikido.dev/blog/idor-vulnerability-explained) for more background. | ||
|
|
||
| > [!IMPORTANT] | ||
| > IDOR protection always throws an exception on violations regardless of block/detect mode. A missing filter is a developer bug, not an external attack. | ||
|
|
||
| ## Setup | ||
|
|
||
| ### 1. Enable IDOR protection at startup | ||
|
|
||
| ```php | ||
| \aikido\enable_idor_protection("tenant_id", ["users"]); | ||
| ``` | ||
|
|
||
| - `tenant_column_name` β the column name that identifies the tenant in your database tables (e.g. `account_id`, `organization_id`, `team_id`). | ||
| - `excluded_tables` β tables that Zen should skip IDOR checks for, because rows aren't scoped to a single tenant (e.g. a shared `users` table that stores users across all tenants). | ||
|
|
||
| ### 2. Set the tenant ID per request | ||
|
|
||
| Every request must have a tenant ID when IDOR protection is enabled. Call `set_tenant_id` early in your request handler (e.g. in middleware after authentication): | ||
|
|
||
| ```php | ||
| \aikido\set_tenant_id($user->organizationId); | ||
| ``` | ||
|
|
||
| > [!IMPORTANT] | ||
| > If `set_tenant_id` is not called for a request, Zen will throw an exception when a SQL query is executed. | ||
|
|
||
| ### 3. Bypass for specific queries (optional) | ||
|
|
||
| Some queries don't need tenant filtering (e.g. aggregations across all tenants for an admin dashboard). Use `without_idor_protection` to bypass the check for a specific callback: | ||
|
|
||
| ```php | ||
| $result = \aikido\without_idor_protection(function() use ($pdo) { | ||
| return $pdo->query("SELECT count(*) FROM agents WHERE status = 'running'"); | ||
| }); | ||
| ``` | ||
|
|
||
| ## Troubleshooting | ||
|
|
||
| <details> | ||
| <summary>Missing tenant filter</summary> | ||
|
|
||
| ``` | ||
| Zen IDOR protection: query on table 'orders' is missing a filter on column 'tenant_id' | ||
| ``` | ||
|
|
||
| This means you have a query like `SELECT * FROM orders WHERE status = 'active'` that doesn't filter on `tenant_id`. The same check applies to `UPDATE` and `DELETE` queries. | ||
|
|
||
| </details> | ||
|
|
||
| <details> | ||
| <summary>Wrong tenant ID value</summary> | ||
|
|
||
| ``` | ||
| Zen IDOR protection: query on table 'orders' filters 'tenant_id' with value '456' but tenant ID is '123' | ||
| ``` | ||
|
|
||
| This means the query filters on `tenant_id`, but the value doesn't match the tenant ID set via `set_tenant_id`. | ||
|
|
||
| </details> | ||
|
|
||
| <details> | ||
| <summary>Missing tenant column in INSERT</summary> | ||
|
|
||
| ``` | ||
| Zen IDOR protection: INSERT on table 'orders' is missing column 'tenant_id' | ||
| ``` | ||
|
|
||
| This means an `INSERT` statement doesn't include the tenant column. Every INSERT must include the tenant column with the correct tenant ID value. | ||
|
|
||
| </details> | ||
|
|
||
| <details> | ||
| <summary>Wrong tenant ID in INSERT</summary> | ||
|
|
||
| ``` | ||
| Zen IDOR protection: INSERT on table 'orders' sets 'tenant_id' to '456' but tenant ID is '123' | ||
| ``` | ||
|
|
||
| This means the INSERT includes the tenant column, but the value doesn't match the tenant ID set via `set_tenant_id`. | ||
|
|
||
| </details> | ||
|
|
||
| <details> | ||
| <summary>Missing set_tenant_id call</summary> | ||
|
|
||
| ``` | ||
| Zen IDOR protection: setTenantId() was not called for this request. Every request must have a tenant ID when IDOR protection is enabled. | ||
| ``` | ||
|
|
||
| </details> | ||
|
|
||
| ## Supported databases | ||
|
|
||
| - MySQL (via `mysqli` and PDO) | ||
| - PostgreSQL (via PDO) | ||
| - SQLite (via PDO) | ||
|
|
||
| Any ORM or query builder that uses PDO or mysqli under the hood is supported (e.g. Eloquent, Doctrine DBAL). | ||
|
|
||
| ## Prepared statements | ||
|
|
||
| Zen supports placeholder resolution for prepared statements executed via `PDOStatement::execute()`: | ||
|
|
||
| ```php | ||
| // Positional placeholders β values are resolved | ||
| $stmt = $pdo->prepare("SELECT * FROM orders WHERE tenant_id = ?"); | ||
| $stmt->execute([$tenantId]); | ||
|
|
||
| // Named placeholders β values are resolved | ||
| $stmt = $pdo->prepare("SELECT * FROM orders WHERE tenant_id = :tid"); | ||
| $stmt->execute([':tid' => $tenantId]); | ||
| ``` | ||
|
|
||
| Parameters bound with `bindValue()` or `bindParam()` before calling `execute()` without arguments are also resolved: | ||
|
|
||
| ```php | ||
| $stmt = $pdo->prepare("SELECT * FROM orders WHERE tenant_id = :tid"); | ||
| $stmt->bindValue(':tid', $tenantId); | ||
| $stmt->execute(); | ||
| ``` | ||
|
|
||
| ## Statements that are always allowed | ||
|
|
||
| Zen only checks statements that read or modify row data (`SELECT`, `INSERT`, `UPDATE`, `DELETE`). The following statement types are also recognized and never trigger an IDOR error: | ||
|
|
||
| - DDL β `CREATE TABLE`, `ALTER TABLE`, `DROP TABLE`, ... | ||
| - Session commands β `SET`, `SHOW`, ... | ||
| - Transactions β `BEGIN`, `COMMIT`, `ROLLBACK`, ... |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| #include "Includes.h" | ||
|
|
||
| ZEND_FUNCTION(enable_idor_protection) { | ||
| ScopedTimer scopedTimer("enable_idor_protection", "aikido_op"); | ||
| auto& requestCache = AIKIDO_GLOBAL(requestCache); | ||
|
|
||
| if (IsAikidoDisabledOrBypassed()) { | ||
| RETURN_BOOL(false); | ||
| } | ||
|
|
||
| char *tenantColumnName = nullptr; | ||
| size_t tenantColumnNameLength = 0; | ||
| zval *excludedTablesZval = nullptr; | ||
|
|
||
| ZEND_PARSE_PARAMETERS_START(1, 2) | ||
| Z_PARAM_STRING(tenantColumnName, tenantColumnNameLength) | ||
| Z_PARAM_OPTIONAL | ||
| Z_PARAM_ARRAY(excludedTablesZval) | ||
| ZEND_PARSE_PARAMETERS_END(); | ||
|
|
||
| if (!tenantColumnName || tenantColumnNameLength == 0) { | ||
| AIKIDO_LOG_ERROR("enable_idor_protection: tenant_column_name is null or empty!\n"); | ||
| RETURN_BOOL(false); | ||
| } | ||
|
|
||
| json excludedTablesJson = json::array(); | ||
| if (excludedTablesZval) { | ||
| HashTable *ht = Z_ARRVAL_P(excludedTablesZval); | ||
| zval *entry; | ||
| ZEND_HASH_FOREACH_VAL(ht, entry) { | ||
| if (Z_TYPE_P(entry) == IS_STRING) { | ||
| excludedTablesJson.push_back(std::string(Z_STRVAL_P(entry), Z_STRLEN_P(entry))); | ||
| } | ||
| } ZEND_HASH_FOREACH_END(); | ||
| } | ||
|
|
||
| json idorConfig = { | ||
| {"column_name", std::string(tenantColumnName, tenantColumnNameLength)}, | ||
| {"excluded_tables", excludedTablesJson} | ||
| }; | ||
| requestCache.idorConfigJson = idorConfig.dump(); | ||
|
|
||
| AIKIDO_LOG_INFO("Enabled IDOR protection with tenant column '%s'\n", tenantColumnName); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Logging tenantColumnName with %s may read past the buffer if it's not NUL-terminated. Use a length-limited format (e.g. "%.*s", (int)tenantColumnNameLength, tenantColumnName) to avoid out-of-bounds reads. Detailsβ¨ AI Reasoning π§ How do I fix it? Reply |
||
| RETURN_BOOL(true); | ||
| } | ||
|
|
||
| ZEND_FUNCTION(set_tenant_id) { | ||
| auto& requestCache = AIKIDO_GLOBAL(requestCache); | ||
|
|
||
| if (IsAikidoDisabledOrBypassed()) { | ||
| return; | ||
| } | ||
|
|
||
| char *id = nullptr; | ||
| size_t idLength = 0; | ||
|
|
||
| ZEND_PARSE_PARAMETERS_START(1, 1) | ||
| Z_PARAM_STRING(id, idLength) | ||
| ZEND_PARSE_PARAMETERS_END(); | ||
|
|
||
| if (!id || idLength == 0) { | ||
| AIKIDO_LOG_ERROR("set_tenant_id: id is null or empty!\n"); | ||
| return; | ||
| } | ||
|
|
||
| requestCache.tenantId = std::string(id, idLength); | ||
| AIKIDO_LOG_DEBUG("Set tenant ID to %s\n", requestCache.tenantId.c_str()); | ||
| } | ||
|
|
||
| ZEND_FUNCTION(without_idor_protection) { | ||
| auto& requestCache = AIKIDO_GLOBAL(requestCache); | ||
| zend_fcall_info fci; | ||
| zend_fcall_info_cache fci_cache; | ||
|
|
||
| ZEND_PARSE_PARAMETERS_START(1, 1) | ||
| Z_PARAM_FUNC(fci, fci_cache) | ||
| ZEND_PARSE_PARAMETERS_END(); | ||
|
|
||
| requestCache.idorDisabled = true; | ||
|
|
||
| zval retval; | ||
| ZVAL_UNDEF(&retval); | ||
| fci.retval = &retval; | ||
|
|
||
| zend_result result = (zend_result)zend_call_function(&fci, &fci_cache); | ||
|
|
||
| requestCache.idorDisabled = false; | ||
|
|
||
| if (result == SUCCESS && !EG(exception)) { | ||
| if (!Z_ISUNDEF(retval)) { | ||
| ZVAL_COPY_VALUE(return_value, &retval); | ||
| } | ||
| } else { | ||
| zval_ptr_dtor(&retval); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsIdorViolation parses the event JSON, and action.Execute later reparses the same string. Avoid double JSON parsing by parsing once and reusing the result or by exposing a parsed-event check.
Show fix
Details
β¨ AI Reasoning
βThe code path now calls IsIdorViolation(outputEvent) early and then later calls action.Execute(outputEvent) which parses the same event string again. JSON parsing is non-trivial and can be frequent for events; parsing the same string twice doubles CPU and allocations. Consolidating parsing (parse once and reuse the parsed object or have Execute expose an API that allows checking the already-parsed event) would remove this wasted work. The regression was introduced by adding the IDOR pre-check before the existing Execute call.
Reply
@AikidoSec feedback: [FEEDBACK]to get better review comments in the future.Reply
@AikidoSec ignore: [REASON]to ignore this issue.More info