From ab790bcbd93bda35dd293e83432a70bc79d45071 Mon Sep 17 00:00:00 2001 From: wizardchen Date: Thu, 2 Jul 2026 11:41:19 +0800 Subject: [PATCH] fix(security): close SQL validator bypass in agent database tools Harden validateNode with default-deny semantics and PG17 SQL/JSON node recursion so dangerous functions cannot hide inside JSON_SCALAR/JSON_VALUE. Also recurse into FROM subqueries for DuckDB data_analysis and block read_text/read_blob and related file-access functions. --- internal/utils/inject.go | 238 +++++++++++++++++++++++++++++++++- internal/utils/inject_test.go | 84 ++++++++++++ 2 files changed, 319 insertions(+), 3 deletions(-) diff --git a/internal/utils/inject.go b/internal/utils/inject.go index a2bee90dc..5d96a06b4 100644 --- a/internal/utils/inject.go +++ b/internal/utils/inject.go @@ -1432,8 +1432,16 @@ func (v *sqlValidator) validateFromItem(node *pg_query.Node, tables map[string]s } // Handle RangeSubselect (subquery in FROM) - if v.checkSubqueries && node.GetRangeSubselect() != nil { - return fmt.Errorf("subqueries in FROM clause are not allowed") + if rss := node.GetRangeSubselect(); rss != nil { + if v.checkSubqueries { + return fmt.Errorf("subqueries in FROM clause are not allowed") + } + // SECURITY: Even when subqueries are permitted, recurse into the + // subquery so dangerous constructs hidden inside it are still + // validated. Without this, a FROM subquery like + // (SELECT * FROM read_text('/etc/passwd')) + // smuggles a RangeFunction past the check below. + return v.validateSubquery(rss.Subquery, tables, result) } // Handle RangeFunction (function in FROM) @@ -1444,6 +1452,45 @@ func (v *sqlValidator) validateFromItem(node *pg_query.Node, tables map[string]s return nil } +// validateSubquery validates a SELECT statement nested in a FROM subquery. +// It reuses the FROM-item and expression validators so RangeFunction and +// dangerous function checks apply recursively to arbitrarily nested subqueries. +func (v *sqlValidator) validateSubquery(node *pg_query.Node, tables map[string]string, result *SQLValidationResult) error { + if node == nil { + return nil + } + sub := node.GetSelectStmt() + if sub == nil { + return nil + } + for _, fromItem := range sub.FromClause { + if err := v.validateFromItem(fromItem, tables, result); err != nil { + return err + } + } + for _, target := range sub.TargetList { + if err := v.validateNode(target, result); err != nil { + return err + } + } + if sub.WhereClause != nil { + if err := v.validateNode(sub.WhereClause, result); err != nil { + return err + } + } + for _, groupBy := range sub.GroupClause { + if err := v.validateNode(groupBy, result); err != nil { + return err + } + } + if sub.HavingClause != nil { + if err := v.validateNode(sub.HavingClause, result); err != nil { + return err + } + } + return nil +} + // validateNode recursively validates AST nodes // SECURITY: This function uses a COMPREHENSIVE approach to validate ALL node types. // Any node type that contains child expressions MUST be handled to prevent bypass attacks. @@ -1895,6 +1942,70 @@ func (v *sqlValidator) validateNode(node *pg_query.Node, result *SQLValidationRe } } + // JsonScalarExpr (JSON_SCALAR(...)) - PG17 SQL/JSON node. + // Attack: SELECT JSON_SCALAR(pg_read_file('/etc/passwd')) FROM table + if jse := node.GetJsonScalarExpr(); jse != nil { + if err := v.validateNode(jse.Expr, result); err != nil { + return err + } + } + + // JsonFuncExpr (JSON_VALUE / JSON_QUERY / JSON_EXISTS(...)) - PG17 SQL/JSON node. + // Attack: SELECT JSON_VALUE(pg_read_file('/etc/passwd'), '$') FROM table + if jfe := node.GetJsonFuncExpr(); jfe != nil { + if err := v.validateJsonValueExpr(jfe.ContextItem, result); err != nil { + return err + } + if err := v.validateNode(jfe.Pathspec, result); err != nil { + return err + } + for _, arg := range jfe.Passing { + if err := v.validateNode(arg, result); err != nil { + return err + } + } + if err := v.validateJsonBehavior(jfe.OnEmpty, result); err != nil { + return err + } + if err := v.validateJsonBehavior(jfe.OnError, result); err != nil { + return err + } + } + + // JsonParseExpr (JSON(...)) - PG17 SQL/JSON node. + if jpe := node.GetJsonParseExpr(); jpe != nil { + if err := v.validateJsonValueExpr(jpe.Expr, result); err != nil { + return err + } + } + + // JsonSerializeExpr (JSON_SERIALIZE(...)) - PG17 SQL/JSON node. + if jse := node.GetJsonSerializeExpr(); jse != nil { + if err := v.validateJsonValueExpr(jse.Expr, result); err != nil { + return err + } + } + + // JsonTable (JSON_TABLE(...)) - PG17 SQL/JSON node. + if jt := node.GetJsonTable(); jt != nil { + if err := v.validateJsonValueExpr(jt.ContextItem, result); err != nil { + return err + } + for _, arg := range jt.Passing { + if err := v.validateNode(arg, result); err != nil { + return err + } + } + for _, col := range jt.Columns { + if err := v.validateNode(col, result); err != nil { + return err + } + } + if err := v.validateJsonBehavior(jt.OnError, result); err != nil { + return err + } + } + // XmlSerialize if xs := node.GetXmlSerialize(); xs != nil { if err := v.validateNode(xs.Expr, result); err != nil { @@ -1932,7 +2043,108 @@ func (v *sqlValidator) validateNode(node *pg_query.Node, result *SQLValidationRe return fmt.Errorf("AlternativeSubPlan nodes are not allowed") } - return nil + // ============================================================ + // SECURITY: DEFAULT-DENY. + // The recursive branches above validate every expression type we know how + // to inspect. Any node type NOT in the recognized set below is REJECTED, + // because an unhandled type can smuggle a dangerous FuncCall past the + // blacklist (e.g. PG17's JSON_SCALAR wrapping pg_read_file). Whenever a new + // recursive handler is added above, add its Node_* wrapper here as well. + // ============================================================ + switch node.Node.(type) { + case + // Recognized expression types (validated recursively above). + *pg_query.Node_FuncCall, + *pg_query.Node_FuncExpr, + *pg_query.Node_ColumnRef, + *pg_query.Node_TypeCast, + *pg_query.Node_AExpr, + *pg_query.Node_OpExpr, + *pg_query.Node_BoolExpr, + *pg_query.Node_NullTest, + *pg_query.Node_BooleanTest, + *pg_query.Node_CoalesceExpr, + *pg_query.Node_CaseExpr, + *pg_query.Node_CaseWhen, + *pg_query.Node_ResTarget, + *pg_query.Node_SortBy, + *pg_query.Node_List, + *pg_query.Node_AArrayExpr, + *pg_query.Node_RowExpr, + *pg_query.Node_MinMaxExpr, + *pg_query.Node_NullIfExpr, + *pg_query.Node_ScalarArrayOpExpr, + *pg_query.Node_ArrayCoerceExpr, + *pg_query.Node_CoerceViaIo, + *pg_query.Node_CollateExpr, + *pg_query.Node_CollateClause, + *pg_query.Node_SubLink, + *pg_query.Node_DistinctExpr, + *pg_query.Node_XmlExpr, + *pg_query.Node_XmlSerialize, + *pg_query.Node_JsonConstructorExpr, + *pg_query.Node_JsonValueExpr, + *pg_query.Node_JsonExpr, + *pg_query.Node_JsonIsPredicate, + *pg_query.Node_JsonScalarExpr, + *pg_query.Node_JsonFuncExpr, + *pg_query.Node_JsonParseExpr, + *pg_query.Node_JsonSerializeExpr, + *pg_query.Node_JsonTable, + *pg_query.Node_Aggref, + *pg_query.Node_WindowFunc, + *pg_query.Node_WindowDef, + *pg_query.Node_GroupingFunc, + *pg_query.Node_SubscriptingRef, + *pg_query.Node_NamedArgExpr, + *pg_query.Node_FieldSelect, + *pg_query.Node_FieldStore, + *pg_query.Node_RelabelType, + *pg_query.Node_ConvertRowtypeExpr, + *pg_query.Node_RowCompareExpr, + *pg_query.Node_CoerceToDomain, + *pg_query.Node_AIndices, + *pg_query.Node_AIndirection, + // Recognized safe leaf nodes (no child expressions to smuggle through). + *pg_query.Node_AConst, + *pg_query.Node_ParamRef, + *pg_query.Node_SetToDefault, + *pg_query.Node_CurrentOfExpr, + *pg_query.Node_CaseTestExpr, + *pg_query.Node_SqlvalueFunction, + *pg_query.Node_AStar, + *pg_query.Node_Integer, + *pg_query.Node_Float, + *pg_query.Node_Boolean, + *pg_query.Node_String_, + *pg_query.Node_BitString: + return nil + default: + return fmt.Errorf("unsupported SQL expression type %T is not allowed", node.Node) + } +} + +// validateJsonValueExpr validates a JsonValueExpr, which appears as a concrete +// (non-Node) field on several PG17 SQL/JSON expression nodes. Its RawExpr / +// FormattedExpr children can hold arbitrary expressions (including FuncCalls), +// so they must be recursed into. +func (v *sqlValidator) validateJsonValueExpr(jve *pg_query.JsonValueExpr, result *SQLValidationResult) error { + if jve == nil { + return nil + } + if err := v.validateNode(jve.RawExpr, result); err != nil { + return err + } + return v.validateNode(jve.FormattedExpr, result) +} + +// validateJsonBehavior validates the ON EMPTY / ON ERROR behavior of a PG17 +// SQL/JSON function. The DEFAULT branch can carry an arbitrary expression. +func (v *sqlValidator) validateJsonBehavior(jb *pg_query.JsonBehavior, result *SQLValidationResult) error { + if jb == nil { + return nil + } + return v.validateNode(jb.Expr, result) } // validateFuncCall validates a function call @@ -2051,6 +2263,26 @@ func (v *sqlValidator) validateFuncCall(fc *pg_query.FuncCall, result *SQLValida // System catalog modification "pg_catalog": true, "information_schema": true, + + // DuckDB file-access functions (data_analysis tool runs on DuckDB). + // The user's data is already loaded into a session table, so these + // arbitrary-path readers are never legitimately needed and would + // allow reading any file on the app container. + "read_text": true, + "read_blob": true, + "read_csv": true, + "read_csv_auto": true, + "read_parquet": true, + "read_json": true, + "read_json_auto": true, + "read_ndjson": true, + "read_ndjson_auto": true, + "read_json_objects": true, + "read_xlsx": true, + "sniff_csv": true, + "glob": true, + "st_read": true, + "st_read_meta": true, } if dangerousFunctions[funcName] { return fmt.Errorf("function '%s' is not allowed", funcName) diff --git a/internal/utils/inject_test.go b/internal/utils/inject_test.go index f5a515a9e..8bf6437c0 100644 --- a/internal/utils/inject_test.go +++ b/internal/utils/inject_test.go @@ -481,6 +481,90 @@ func TestValidateAndSecureSQL_WithStructuredSearchScopes(t *testing.T) { } } +// TestValidateSQL_JSONNodeBypass verifies that PG17 SQL/JSON expression nodes +// cannot be used to smuggle dangerous functions past the blacklist. These were +// previously accepted because validateNode had no handler for them and fell +// through to a permissive `return nil`. +func TestValidateSQL_JSONNodeBypass(t *testing.T) { + dangerous := []struct { + name string + sql string + }{ + {"JSON_SCALAR + pg_read_file", "SELECT JSON_SCALAR(pg_read_file('/etc/passwd')) AS data FROM knowledge_bases LIMIT 1"}, + {"JSON_VALUE + pg_read_file", "SELECT JSON_VALUE(pg_read_file('/etc/passwd'), '$') AS data FROM knowledge_bases LIMIT 1"}, + {"JSON_QUERY + pg_read_file", "SELECT JSON_QUERY(pg_read_file('/etc/passwd'), '$') AS data FROM knowledge_bases LIMIT 1"}, + {"JSON scalar + lo_export", "SELECT JSON_SCALAR(lo_export(1, '/tmp/x')) FROM knowledge_bases LIMIT 1"}, + {"JSON() parse + pg_read_file", "SELECT JSON(pg_read_file('/etc/passwd')) FROM knowledge_bases LIMIT 1"}, + {"JSON_SERIALIZE + pg_read_file", "SELECT JSON_SERIALIZE(pg_read_file('/etc/passwd')) FROM knowledge_bases LIMIT 1"}, + } + + for _, tt := range dangerous { + t.Run(tt.name, func(t *testing.T) { + _, validation := ValidateSQL(tt.sql, WithSecurityDefaults(10000)) + if validation.Valid { + t.Fatalf("expected SQL to be REJECTED, but it was accepted: %s", tt.sql) + } + }) + } +} + +// TestValidateSQL_UnknownNodeDefaultDeny ensures a legitimate query still +// passes after the default-deny change, guarding against over-blocking. +func TestValidateSQL_DefaultDenyAllowsLegitimate(t *testing.T) { + legit := []string{ + "SELECT id, name FROM knowledge_bases WHERE name LIKE '%test%' ORDER BY created_at DESC LIMIT 10", + "SELECT COUNT(*) AS c FROM knowledges WHERE parse_status = 'completed'", + "SELECT COALESCE(title, 'untitled') FROM knowledges LIMIT 5", + "SELECT CASE WHEN file_size > 100 THEN 'big' ELSE 'small' END FROM knowledges LIMIT 5", + "SELECT kb.name, COUNT(k.id) FROM knowledge_bases kb LEFT JOIN knowledges k ON kb.id = k.knowledge_base_id GROUP BY kb.id, kb.name", + } + for _, sql := range legit { + t.Run(sql, func(t *testing.T) { + _, validation := ValidateSQL(sql, WithSecurityDefaults(10000)) + if !validation.Valid { + t.Fatalf("expected legitimate SQL to pass, got errors: %#v\nSQL: %s", validation.Errors, sql) + } + }) + } +} + +// TestValidateSQL_DuckDBSubqueryFileRead verifies that the data_analysis tool's +// validation options (no WithNoSubqueries) still reject file-reading functions +// hidden inside a FROM subquery, as well as at the top level. +func TestValidateSQL_DuckDBFileRead(t *testing.T) { + // Mirror the exact options used by the data_analysis (DuckDB) tool. + opts := []SQLValidationOption{ + WithAllowedTables("k_data"), + WithSingleStatement(), + WithNoDangerousFunctions(), + } + + dangerous := []struct { + name string + sql string + }{ + {"read_text in FROM subquery", "SELECT * FROM (SELECT * FROM read_text('/etc/passwd')) AS f, k_data AS k LIMIT 1"}, + {"read_text in target list", "SELECT read_text('/etc/passwd') FROM k_data LIMIT 1"}, + {"read_blob in FROM subquery", "SELECT * FROM (SELECT * FROM read_blob('/etc/passwd')) AS f LIMIT 1"}, + {"nested subquery read_csv", "SELECT * FROM (SELECT * FROM (SELECT * FROM read_csv('/etc/passwd')) AS a) AS b LIMIT 1"}, + } + for _, tt := range dangerous { + t.Run(tt.name, func(t *testing.T) { + _, validation := ValidateSQL(tt.sql, opts...) + if validation.Valid { + t.Fatalf("expected SQL to be REJECTED, but it was accepted: %s", tt.sql) + } + }) + } + + t.Run("legitimate table query passes", func(t *testing.T) { + _, validation := ValidateSQL("SELECT product, SUM(amount) FROM k_data GROUP BY product", opts...) + if !validation.Valid { + t.Fatalf("expected legitimate DuckDB query to pass, got: %#v", validation.Errors) + } + }) +} + func BenchmarkInjectAndConditions(b *testing.B) { const sql = "SELECT id, title FROM docs WHERE status = 'active' ORDER BY created_at LIMIT 50" for i := 0; i < b.N; i++ {