Repository navigation
fix(query-postgres): block function-call SQLi bypass in data-explorer WHERE clauses - #328
Merged
Merged
Conversation
The data-browser WHERE-clause validator relied on a keyword denylist applied after string literals are stripped (security review finding #4). Data-returning functions such as query_to_xml/database_to_xml/table_to_xml take their SQL payload as a string-literal argument, so the payload was stripped before the denylist ran and the function names themselves were not denylisted. A clause like: 1=1 AND query_to_xml('select ... from users', true, false, '') IS NOT NULL therefore passed validation and exfiltrated arbitrary tables. Enumerating dangerous functions can never be complete, so add a structural rule that rejects function-call syntax outright: any identifier immediately preceding '(' is treated as a function call and rejected. Only grouping parens and IN/AND/OR/NOT (...) are permitted. With subqueries, UNION, and function calls all blocked, a WHERE clause can reference only the current table's columns and constants, closing the cross-table exfiltration class. Regression tests cover the XML-function bypass and arbitrary function calls, and confirm legitimate grouping and IN-lists still validate.
📓 Changelog previewThis is what your commits will add to the generated ## [Unreleased]
### Fixed
- **query-postgres:** Block function calls in data-browser WHERE clauses
- **query-postgres:** Close quoted function call bypasses
- **query-postgres:** Reject alternate subquery forms
- **query-postgres:** Parse SQL literals safely
- **query-postgres:** Handle high-bit dollar tags
### Testing
- **query-postgres:** Execute attacks against seeded table |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security review finding #4 — SQL injection via denylist bypass in the Postgres data-explorer (High)
Location: `crates/temps-query-postgres/src/lib.rs` (`validate_sql`)
User WHERE clauses are concatenated into `SELECT * FROM … WHERE `, guarded only by a keyword denylist applied after string literals are stripped. Data-returning functions like `query_to_xml`/`database_to_xml`/`table_to_xml` aren't on the denylist, and their SQL payload lives inside a quoted string that gets stripped before the checks run — so:
```sql
1=1 AND query_to_xml('select ... from users', true, false, '') IS NOT NULL
```
passes validation and exfiltrates other tables.
Fix
A keyword denylist can never be complete. Add a structural rule that rejects function-call syntax outright: any identifier immediately preceding `(` is treated as a function call and rejected. Only grouping parens and `IN`/`AND`/`OR`/`NOT (...)` are permitted.
With subqueries (already blocked), `UNION` (already blocked), and now function calls all rejected, a WHERE clause can reference only the current table's columns and constants — closing the cross-table exfiltration class rather than playing denylist whack-a-mole.
Verification (regression tests only)
`cargo test -p temps-query-postgres --lib` — 35 pass, including:
`cargo clippy -p temps-query-postgres --all-targets -- -D warnings` clean.