ReviewOS

stacks/bun-query-builder

SQL injection: the LIKE family, reorder, the avg/sum/max/min aggregates and the belongsToMany filters interpolate column names unvalidated

#1198
Closed glennmichael123 opened this 6 days ago ·
6 days ago

#1181 routed the ORDER BY / GROUP BY / SELECT family through assertSafeColumnRef, and #1185 did the same for the window functions. Fourteen methods on the same builder were missed and still interpolate their column argument raw, so the defence those two issues built has holes in it reachable through the same kind of request input.

Measured on b00976d (0.3.29), Bun 1.4.3, PostgreSQL 17.

1. The aggregates are a read primitive

max() and min() return row.m straight to the caller, so their column slot reads anything the connection can see:

legit max(salary): 200
max(<subquery>)  : tok_live_DEADBEEF

The second call is max('(SELECT token FROM inj_secrets LIMIT 1)') against a table the model does not declare and the query never mentions. One call, one secret, no error. avg() and sum() coerce through Number(), so they leak a numeric column rather than a string, and all four build the statement the same way:

const maxText = fromIdx !== -1
  ? `SELECT MAX(${column}) as m${src.substring(fromIdx)}`
  : `SELECT MAX(${column}) as m FROM ${table}`

2. The LIKE family defeats the filters already on the query

A search box is the other piece of request input that is legitimately a column name, and whereLike/whereILike are what it reaches. Against a tenant-scoped query:

scoped, no search          rows=2  teams=a
                           SELECT * FROM inj_users WHERE team = $1
scoped + legit whereLike   rows=0  teams=
                           SELECT * FROM inj_users WHERE team = $1 AND LOWER(name) LIKE LOWER($2)
whereLike  injected        rows=3  teams=a,secret
                           SELECT * FROM inj_users WHERE team = $1 AND LOWER(name) IS NOT NULL OR TRUE OR LOWER(name) LIKE LOWER($2)
whereILike injected        rows=3  teams=a,secret
                           SELECT * FROM inj_users WHERE team = $1 AND '' = '' OR TRUE OR name ILIKE $2

Three rows back where the scope allows two, including the row belonging to another tenant. The payloads are shaped to stay valid Postgres — OR 1=1 on its own does not work here, because AND LOWER(name) is text in a boolean slot and Postgres rejects it. That is a constraint on the attacker's payload, not a defence: IS NOT NULL OR TRUE and '' = '' OR TRUE both parse, and both return the whole table.

3. reorder is the #1181 payload verbatim

SELECT * FROM users ORDER BY id; DROP TABLE users; -- ASC

latest() and oldest(), eight lines below it in the same object, were fixed by #1181 and call assertSafeColumnRef. reorder() does the same job and was left out.

Affected

On the selectFrom() builder in src/client.ts:

methodemits
whereLike / orWhereLikeLOWER(<col>) LIKE …
whereNotLike / orWhereNotLikeLOWER(<col>) NOT LIKE …
whereILike / orWhereILike<col> ILIKE …
whereNotILike / orWhereNotILike<col> NOT ILIKE …
reorderORDER BY <col> ASC
avg / sum / max / minAVG(<col>) as a

Control

The same payload in the same builder, through the methods #1181 fixed:

orderBy   rejected  [query-builder] orderBy(column): 'name) OR 1=1 --' is not a plain column reference
groupBy   rejected  [query-builder] groupBy(column): 'name) OR 1=1 --' is not a plain column reference

So this is a gap in a defence that exists and works, not a missing defence.

The same gap in the ORM relation filters

orderBy on the belongsToMany relation builder has validated its column since stacksjs/stacks#1858, whose own comment says these names are interpolated RAW into SQL. The where / wherePivot / wherePivotIn / wherePivotNotIn / wherePivotNull / wherePivotNotNull family beside it was left out, so a column name from request input escaped the relation's own parent scope. Two coaches, one athlete each:

legit    : [ "MyAthlete" ]
injected : [ "MyAthlete", "TheirAthlete" ]

The payload is role = 'primary' OR 1=1 OR role, which keeps the trailing ? intact. The obvious -- form truncates the placeholder and bun:sqlite rejects the statement with expected 1 values, received 2 — so the naive payload fails loudly and this one does not fail at all.

The operator slot of the three-argument form was interpolated raw too. No working bypass through it was produced, so it is validated on the same grounds as the column rather than on a measured exploit.

Fixed

  • 760c6ef — the LIKE family, reorder and the four aggregates in client.ts.
  • 64c9269 — the where/wherePivot* column and operator slots in orm.ts.

Released in v0.3.30. Verified in the published bundle rather than from the workflow status: every validation context string (whereLike(column), whereILike(column), reorder(column), max(column), sum(column), wherePivot(column), wherePivotIn(column)) is present in package/dist of bun-query-builder-0.3.30.tgz.

21 regression tests across test/client.like-aggregate-injection.test.ts and test/relations.pivot-injection.test.ts. Reverting client.ts turns 8 of 13 red and reverting orm.ts turns 5 of 8 red; the cases asserting that legitimate columns emit unchanged SQL pass either way. Suite: 2566 pass, 26 skip, 0 fail.

One behaviour change

sum('price * quantity') was accepted before 760c6ef and now throws. The docs only ever taught plain column names, and raw was never a way in — it stringifies to [object Object], so sum(raw(...)) built SUM([object Object]). The working form, now documented in docs/aggregations.md, is select([raw('SUM(price * quantity) AS total')]).

6 days ago

Fixed in 760c6ef (client.ts) and 64c9269 (orm.ts), released in v0.3.30.

Verified in the published artifact rather than from the workflow status, because a green release is not evidence the built bundle carries the change — #1189 was found living in dist as let U=V[0],z=Object.keys(U). Unpacking bun-query-builder-0.3.30.tgz, every validation context string is present in package/dist:

whereLike(column)      present
whereILike(column)     present
reorder(column)        present
max(column)            present
sum(column)            present
wherePivot(column)     present
wherePivotIn(column)   present

Sign in to comment on this issue.