fix(0002): constrain pg_depend classid/refclassid to avoid cross-catalog oid collisions - #177
Open
karpovantonme wants to merge 1 commit into
Conversation
…log oid collisions
`0002_auth_users_exposed` joined pg_depend to auth.users on refobjid alone:
join pg_catalog.pg_depend d
on d.refobjid = auth_users_pg_class.oid
pg_depend.objid/refobjid are only unique within a (classid, refclassid), so a
dependency row that points at an object in a different catalog whose oid happens
to equal auth.users' pg_class oid matches as well. In the reported case a view
called a SECURITY DEFINER function whose pg_proc oid equalled auth.users'
pg_class oid (16499); the view selects only from public tables, yet it was
reported as CRITICAL auth_users_exposed and triggered a security advisory email.
Add `d.refclassid = 'pg_catalog.pg_class'::regclass` and
`d.classid = 'pg_catalog.pg_rewrite'::regclass`, the same filters the other
lints already carry after supabase#166.
Also drop the redundant `pg_class pg_class_auth_users` join. Once refclassid is
constrained it always resolves to the same row as `auth_users_pg_class`, which
is already restricted to auth.users.
The regression test forces the collision by moving a function's pg_proc oid onto
auth.users' pg_class oid inside the test transaction, then asserts the view that
calls it is not reported.
splinter.sql regenerated via bin/compile.py.
Closes supabase#171
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.
Fixes #171.
Problem
0002_auth_users_exposedjoinspg_dependtoauth.usersonrefobjidalone:pg_depend.objid/refobjidare only unique within a(classid, refclassid)pair. Without those predicates, a dependency row that points at an object in a different catalog whose oid happens to equalauth.users'pg_classoid is matched too.In the reported case a public view called a SECURITY DEFINER helper function whose
pg_procoid equalledauth.users'pg_classoid (16499). The view selects only from public tables and has no relation dependency onauth.users, yet it was reported as CRITICALauth_users_exposedand triggered an "action required: security vulnerabilities" email.Fix
Constrain both sides of the join:
This matches the filters the other lints already carry after #166 ("objid is only unique within a classid"), which added
classidpredicates to 0001, 0004-0011, 0016 and 0017 but did not touch this join in 0002.Also drops the redundant
join pg_catalog.pg_class pg_class_auth_users on d.refobjid = pg_class_auth_users.oid. Oncerefclassidis constrained it always resolves to the same row asauth_users_pg_class, which is already restricted toauth.users, sorelrowsecurityis read from that alias instead.No change to the lint's true positives: a view or materialized view whose rewrite really depends on
auth.usersstill carriesclassid = pg_rewriteandrefclassid = pg_class.Test
An oid collision cannot be produced by waiting for one, so the regression case forces it inside the test transaction: a function is created, its
pg_procoid is moved ontoauth.users'pg_classoid, and a view that calls the function (and never referencesauth.users) is created. The lint must return 0 rows. Onmainthat view is reported; with this change it is not. Everything runs inside the existingbegin/rollback.splinter.sqlregenerated viabin/compile.py.Verification
docker compose -f dockerfiles/docker-compose.yml run --rm testonsupabase/postgres:15.1.1.13— all 28 tests pass.pre-commit run --all-files— all hooks pass.