fix(db): parameterize Kingbase get_tables/get_fields queries (CWE-89) - #1298
Open
sebastionoss wants to merge 1 commit into
Open
fix(db): parameterize Kingbase get_tables/get_fields queries (CWE-89)#1298sebastionoss wants to merge 1 commit into
sebastionoss wants to merge 1 commit into
Conversation
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.
Summary
SQL injection (CWE-89) in the Kingbase branches of
get_tablesandget_fieldsinbackend/apps/db/db.py. The Kingbase code path built its query with Pythonstr.format(...)and passed the fully-interpolated string tocursor.execute(...), unlike the sibling branches for Redshift / StarRocks / Doris in the same function which already use proper%sparameter binding.For
get_fields, the interpolated valuep2is thetable_namepath parameter fromPOST /datasource/getFields/{id}/{table_name}(seebackend/apps/datasource/api/datasource.py:157), so it is directly attacker-controlled and flows unescaped into a SQL statement executed against the workspace's configured Kingbase datasource.Affected code
backend/apps/db/db.py::get_tables— Kingbase branch,cursor.execute(sql.format(sql_param))backend/apps/db/db.py::get_fields— Kingbase branch,cursor.execute(sql.format(p1, p2))wherep2 == table_namebackend/apps/db/db_sql.py::get_table_sql/get_field_sql— the Kingbase templates used'{0}'/'{1}'string placeholdersFix
db_sql.pyto use%splaceholders instead of'{0}'/'{1}'.db.py, pass parameters as a tuple tocursor.execute(cursor.execute(sql, (sql_param,))andcursor.execute(sql, (p1, p2))). This matches the pattern already used for the other PostgreSQL-like backends in the same function.get_fieldshandles the optional-table_name case by only binding(p1,)whensql2is empty, so the placeholder count stays in sync with the SQL.The Kingbase driver (ksycopg2 / psycopg2-compatible) supports
%sparameter binding, so no behavioural change is expected — only the interpolation site changes.Security analysis
Data flow for the higher-severity path:
POST /datasource/getFields/{id}/{table_name}—table_nameis a path parameter (no validation).getFields(...)incrud/datasource.pyforwards it todb.get_fields(ds, table_name=...).table_namereachessql.format(p1, p2)and is embedded in a query executed against the datasource's Kingbase instance.Preconditions:
ws_admin) role in a workspace that has a Kingbase datasource configured.Impact: SQL execution against the customer's Kingbase DB with the datasource's DB credentials. That is a strictly wider capability than the intended "list columns for this table" — cross-schema reads, and depending on driver behaviour, potentially stacked statements.
get_tables'sql_paramis derived fromconf.dbSchema(admin-supplied at datasource creation), so the exposure there is lower but the same fix applies for consistency.Adversarial review
Before submitting we checked whether the workspace-admin gate makes this a non-issue. It does not: workspace admin is an application role, not equivalent to Kingbase DBA, and the datasource credentials are typically distinct from — and potentially more privileged than — what a ws_admin should be able to run arbitrary SQL as. We also confirmed the same function's Redshift / StarRocks / Doris branches already use parameter binding, so this is an inconsistency rather than an intentional design.
Proof of concept
Verified route:
backend/apps/datasource/api/datasource.py:157mountsPOST /datasource/getFields/{id}/{table_name}which callsgetFields(...)->db.get_fields(ds, table_name).With a workspace-admin session cookie and
{id}pointing at a Kingbase datasource:Pre-fix, the
table_namesegment is interpolated into the Kingbase query verbatim via.format(...)and the injected fragment is parsed as SQL by the backend. Post-fix, the same value is bound as a parameter and treated as a literal string comparison againstc.relname, returning an empty result set instead of executing the injected SQL.Testing
git diffon the two files — the parameter tuples match the number of%splaceholders in every branch ((sql_param,),(p1, p2), and(p1,)when the optionalsql2clause is omitted).sql.format(call sites reachingcursor.executeon Kingbase — none remain.Discovered by the Sebastion AI GitHub App.