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.
2026-08-31 00:50:02 +08:00 · 2026-07-02 11:41:19 +08:00
parent 0060c0d397
commit ab790bcbd9
2 changed files with 319 additions and 3 deletions
@@ -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)
@@ -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++ {