diff --git a/packages/safegres/README.md b/packages/safegres/README.md index 7dcb3d52d..19ca9c552 100644 --- a/packages/safegres/README.md +++ b/packages/safegres/README.md @@ -130,6 +130,8 @@ are performance, `P5` is security. | L5 | info | fail-open | An untrusted role reaches an **RLS-off table** via PUBLIC/inheritance † | | L6 | info | neutral | **Unaddressable grant** — an API role holds privileges on a relation its API cannot name ‡ | | L8 | info | fail-open | **DEFINER view bypass** — an untrusted role reads a base relation as the view's owner † | +| L9 | info | fail-open | **DEFINER view write** — an auto-updatable definer view writes a base relation as its owner † | +| L10 | info | fail-open | **Rewrite-rule bypass** — a rule on a view writes a relation as the view's owner, `security_invoker` notwithstanding † | | W1 | medium | — | **No exposure surface configured** — whole database assumed reachable, score capped | † R1/R2/L5 are no-ops until you name the untrusted roles: diff --git a/packages/safegres/__tests__/definer-view.test.ts b/packages/safegres/__tests__/definer-view.test.ts index ba0644542..7abe7559a 100644 --- a/packages/safegres/__tests__/definer-view.test.ts +++ b/packages/safegres/__tests__/definer-view.test.ts @@ -30,6 +30,9 @@ function view(partial: Partial = {}): ViewSnapshot { ownerBypassesRls: false, grants: [grant('anon', 'SELECT')], definition: 'SELECT id, total FROM app.orders', + writable: [], + insteadOfTriggers: false, + rules: [], ...partial }; } diff --git a/packages/safegres/__tests__/view-introspect.test.ts b/packages/safegres/__tests__/view-introspect.test.ts index 883236d5f..6b157cdd8 100644 --- a/packages/safegres/__tests__/view-introspect.test.ts +++ b/packages/safegres/__tests__/view-introspect.test.ts @@ -22,6 +22,19 @@ beforeAll(async () => { CREATE VIEW fx_viewopt.v_bare WITH (security_invoker) AS SELECT id FROM fx_viewopt.t; CREATE VIEW fx_viewopt.v_off WITH (security_invoker = off) AS SELECT id FROM fx_viewopt.t; CREATE VIEW fx_viewopt.v_none AS SELECT id FROM fx_viewopt.t; + + CREATE SCHEMA fx_viewwrite; + CREATE TABLE fx_viewwrite.t (id int); + CREATE TABLE fx_viewwrite.audit (note text); + -- Auto-updatable: a simple view over one relation. + CREATE VIEW fx_viewwrite.v_auto AS SELECT id FROM fx_viewwrite.t; + -- Not updatable: an aggregate has no row to write back. + CREATE VIEW fx_viewwrite.v_agg AS SELECT count(*) AS n FROM fx_viewwrite.t; + -- Updatable only through a rule, which pg_get_viewdef does not show. + CREATE VIEW fx_viewwrite.v_ruled AS SELECT id FROM fx_viewwrite.t; + CREATE RULE v_ruled_ins AS ON INSERT TO fx_viewwrite.v_ruled + DO INSTEAD INSERT INTO fx_viewwrite.audit (note) VALUES ('x'); + CREATE RULE v_ruled_del AS ON DELETE TO fx_viewwrite.v_ruled DO INSTEAD NOTHING; `); }); @@ -45,3 +58,25 @@ describe('introspectViews — security_invoker spellings', () => { }); }); }); + +describe('introspectViews — write paths', () => { + it('reads updatability and the rules pg_get_viewdef does not show', async () => { + const views = await introspectViews(pg.client as never, { schemas: ['fx_viewwrite'] }); + const byName = Object.fromEntries(views.map((v) => [v.name, v])); + + expect(byName.v_auto.writable.sort()).toEqual(['DELETE', 'INSERT', 'UPDATE']); + expect(byName.v_auto.rules).toEqual([]); + expect(byName.v_auto.insteadOfTriggers).toBe(false); + + expect(byName.v_agg.writable).toEqual([]); + + // The bitmask counts rule-conferred updatability too, which is why the + // rules have to be read alongside it rather than inferred from it. + expect(byName.v_ruled.writable).toContain('INSERT'); + expect(byName.v_ruled.rules.map((r) => [r.name, r.event, r.instead]).sort()).toEqual([ + ['v_ruled_del', 'DELETE', true], + ['v_ruled_ins', 'INSERT', true] + ]); + expect(byName.v_ruled.rules[0].definition).toContain('CREATE RULE'); + }); +}); diff --git a/packages/safegres/__tests__/view-writes.test.ts b/packages/safegres/__tests__/view-writes.test.ts new file mode 100644 index 000000000..70aa8ea92 --- /dev/null +++ b/packages/safegres/__tests__/view-writes.test.ts @@ -0,0 +1,338 @@ +import { type RoleGraph } from '../src/checks/lattice'; +import { computeViewWriteReach } from '../src/checks/role-reach'; +import { + analyzeViewWrites, + checkDefinerViewWrite, + checkViewRuleBypass +} from '../src/checks/view-writes'; +import type { RoleAttributes } from '../src/pg/acl'; +import type { ViewRule, ViewSnapshot } from '../src/pg/indexes'; +import type { GrantInfo, TableSnapshot } from '../src/pg/introspect'; + +function table(partial: Partial = {}): TableSnapshot { + return { + schema: 'app', + name: 'submissions', + oid: 1, + rlsEnabled: true, + rlsForced: true, + isPartitioned: false, + owner: 'app_owner', + grants: [], + policies: [], + ...partial + }; +} + +function view(partial: Partial = {}): ViewSnapshot { + return { + schema: 'app', + name: 'inbox', + owner: 'app_owner', + materialized: false, + securityInvoker: false, + ownerBypassesRls: false, + grants: [grant('anon', 'INSERT')], + definition: 'SELECT id, body FROM app.submissions', + writable: ['INSERT', 'UPDATE', 'DELETE'], + insteadOfTriggers: false, + rules: [], + ...partial + }; +} + +function rule(partial: Partial = {}): ViewRule { + return { + name: 'inbox_insert', + event: 'INSERT', + instead: true, + definition: + 'CREATE RULE inbox_insert AS ON INSERT TO app.inbox ' + + 'DO INSTEAD INSERT INTO app.audit_log (note) VALUES (new.body);', + ...partial + }; +} + +function grant(role: string, privilege: GrantInfo['privilege']): GrantInfo { + return { role, privilege, grantable: false, bypassRls: false }; +} + +function role(name: string, partial: Partial = {}): [string, RoleAttributes] { + return [name, { name, bypassRls: false, isSuper: false, inheritsFrom: [], canSetRole: [], ...partial }]; +} + +function graph(...entries: Array<[string, RoleAttributes]>): RoleGraph { + return new Map(entries); +} + +const GRAPH = graph(role('anon'), role('app_owner'), role('member')); +const AUDIT = table({ name: 'audit_log', oid: 2, rlsEnabled: false, rlsForced: false }); + +describe('analyzeViewWrites — auto-updatable views', () => { + it('resolves the base relation a definer view rewrites writes onto', async () => { + const { autoUpdatable } = await analyzeViewWrites([view()], [table()]); + expect(autoUpdatable).toHaveLength(1); + expect(autoUpdatable[0].writeEdges).toEqual([ + { + schema: 'app', + table: 'submissions', + via: 'INSERT', + privilege: 'INSERT', + hops: [{ view: 'app.inbox', owner: 'app_owner' }] + }, + { + schema: 'app', + table: 'submissions', + via: 'UPDATE', + privilege: 'UPDATE', + hops: [{ view: 'app.inbox', owner: 'app_owner' }] + }, + { + schema: 'app', + table: 'submissions', + via: 'DELETE', + privilege: 'DELETE', + hops: [{ view: 'app.inbox', owner: 'app_owner' }] + } + ]); + }); + + it('ignores an invoker view: the rewritten write is checked against the caller', async () => { + const { autoUpdatable } = await analyzeViewWrites([view({ securityInvoker: true })], [table()]); + expect(autoUpdatable).toEqual([]); + }); + + it('ignores a view Postgres will not accept writes on', async () => { + const { autoUpdatable } = await analyzeViewWrites([view({ writable: [] })], [table()]); + expect(autoUpdatable).toEqual([]); + }); + + it('suppresses a view whose INSTEAD OF triggers decide where the write lands', async () => { + const { autoUpdatable, suppressed } = await analyzeViewWrites( + [view({ insteadOfTriggers: true })], + [table()] + ); + expect(autoUpdatable).toEqual([]); + expect(suppressed[0].reason).toContain('INSTEAD OF'); + }); + + it('places no write on a join view: auto-update needs exactly one target', async () => { + const joined = view({ + definition: 'SELECT s.id FROM app.submissions s JOIN app.audit_log a ON a.id = s.id' + }); + const { autoUpdatable } = await analyzeViewWrites([joined], [table(), AUDIT]); + expect(autoUpdatable).toEqual([]); + }); + + it('suppresses a view whose body it cannot read', async () => { + const { autoUpdatable, suppressed } = await analyzeViewWrites( + [view({ definition: 'SELECT ((( FROM nowhere' })], + [table()] + ); + expect(autoUpdatable).toEqual([]); + expect(suppressed).toEqual([{ view: 'app.inbox', reason: 'SQL fragment failed to parse' }]); + }); + + it('follows a view on a view, re-owning the write at each definer hop', async () => { + const inner = view({ name: 'inner', owner: 'inner_owner' }); + const outer = view({ + name: 'outer', + owner: 'outer_owner', + writable: ['INSERT'], + definition: 'SELECT id FROM app.inner' + }); + const { autoUpdatable } = await analyzeViewWrites([outer, inner], [table()]); + const edges = autoUpdatable.find((v) => v.name === 'outer')!.writeEdges; + expect(edges[0].hops).toEqual([ + { view: 'app.outer', owner: 'outer_owner' }, + { view: 'app.inner', owner: 'inner_owner' } + ]); + }); +}); + +describe('analyzeViewWrites — rewrite rules', () => { + it('resolves the relation a rule action writes, which the body never names', async () => { + const { ruleDriven } = await analyzeViewWrites([view({ rules: [rule()] })], [table(), AUDIT]); + expect(ruleDriven).toHaveLength(1); + expect(ruleDriven[0].writeEdges).toEqual([ + { + schema: 'app', + table: 'audit_log', + via: 'INSERT', + privilege: 'INSERT', + hops: [{ view: 'app.inbox', owner: 'app_owner' }], + rule: 'inbox_insert' + } + ]); + }); + + it('reads rules on invoker views too: security_invoker does not govern a rule action', async () => { + const { ruleDriven } = await analyzeViewWrites( + [view({ securityInvoker: true, rules: [rule()] })], + [table(), AUDIT] + ); + expect(ruleDriven[0].writeEdges[0].table).toBe('audit_log'); + }); + + it('keeps the privilege the action exercises, not the one that fired the rule', async () => { + const updating = rule({ + name: 'inbox_delete', + event: 'DELETE', + definition: + 'CREATE RULE inbox_delete AS ON DELETE TO app.inbox ' + + "DO INSTEAD UPDATE app.audit_log SET note = 'deleted' WHERE id = old.id;" + }); + const { ruleDriven } = await analyzeViewWrites([view({ rules: [updating] })], [table(), AUDIT]); + expect(ruleDriven[0].writeEdges[0]).toMatchObject({ via: 'DELETE', privilege: 'UPDATE' }); + }); + + it('reaches nothing through DO INSTEAD NOTHING — a read-only view confers no write', async () => { + const nothing = rule({ + name: 'inbox_no_insert', + definition: 'CREATE RULE inbox_no_insert AS ON INSERT TO app.inbox DO INSTEAD NOTHING;' + }); + const { ruleDriven } = await analyzeViewWrites([view({ rules: [nothing] })], [table(), AUDIT]); + expect(ruleDriven).toEqual([]); + }); + + it('ignores the view its own rule is on: that reference is the trigger, not a target', async () => { + const selfWrite = rule({ + definition: + 'CREATE RULE inbox_insert AS ON INSERT TO app.inbox ' + + 'DO INSTEAD INSERT INTO app.inbox (body) VALUES (new.body);' + }); + const { ruleDriven } = await analyzeViewWrites([view({ rules: [selfWrite] })], [table(), AUDIT]); + expect(ruleDriven).toEqual([]); + }); + + it('suppresses a rule action it cannot read', async () => { + const broken = rule({ definition: 'CREATE RULE ((( AS ON INSERT' }); + const { ruleDriven, suppressed } = await analyzeViewWrites( + [view({ rules: [broken] })], + [table(), AUDIT] + ); + expect(ruleDriven).toEqual([]); + expect(suppressed[0].reason).toContain('cannot follow'); + }); +}); + +describe('computeViewWriteReach', () => { + it('projects the target under the view owner, proven by AST', async () => { + const { autoUpdatable } = await analyzeViewWrites([view({ writable: ['INSERT'] })], [table()]); + const [reach] = computeViewWriteReach(autoUpdatable, GRAPH, ['anon']); + expect(reach.cells).toHaveLength(1); + expect(reach.cells[0]).toMatchObject({ + schema: 'app', + table: 'submissions', + privileges: ['INSERT'], + effectiveRole: 'app_owner', + proof: 'ast' + }); + }); + + it('needs the triggering command on the view: SELECT on it reaches no write', async () => { + const readOnly = view({ writable: ['INSERT'], grants: [grant('anon', 'SELECT')] }); + const { autoUpdatable } = await analyzeViewWrites([readOnly], [table()]); + const [reach] = computeViewWriteReach(autoUpdatable, GRAPH, ['anon']); + expect(reach.cells).toEqual([]); + }); + + it('carries the rule edge on the path', async () => { + const { ruleDriven } = await analyzeViewWrites([view({ rules: [rule()] })], [table(), AUDIT]); + const [reach] = computeViewWriteReach(ruleDriven, GRAPH, ['anon']); + expect(reach.cells[0].path).toEqual([ + { kind: 'grant', via: 'direct', privilege: 'INSERT' }, + { kind: 'view', view: 'app.inbox', owner: 'app_owner' }, + { kind: 'rule', view: 'app.inbox', rule: 'inbox_insert', owner: 'app_owner' } + ]); + }); +}); + +describe('checkDefinerViewWrite (L9)', () => { + async function check(views: ViewSnapshot[], tables: TableSnapshot[], roles: string[]) { + const { autoUpdatable } = await analyzeViewWrites(views, tables); + return checkDefinerViewWrite(autoUpdatable, tables, GRAPH, { roles }); + } + + it('is a no-op with no untrusted roles configured', async () => { + expect(await check([view()], [table()], [])).toEqual([]); + }); + + it('flags a base relation the role writes only as the view owner', async () => { + const findings = await check([view({ writable: ['INSERT'] })], [table()], ['anon']); + expect(findings).toHaveLength(1); + expect(findings[0]).toMatchObject({ + code: 'L9', + severity: 'info', + schema: 'app', + table: 'submissions', + role: 'anon', + privilege: 'INSERT' + }); + expect(findings[0].context).toMatchObject({ + view: 'app.inbox', + effectiveRole: 'app_owner', + proof: 'ast' + }); + }); + + it('never recommends revoking a grant', async () => { + const [finding] = await check([view({ writable: ['INSERT'] })], [table()], ['anon']); + expect(finding.hint).toContain('security_invoker'); + expect(finding.hint).toContain('Do not revoke'); + }); + + it('stays silent when the role can write the base relation anyway', async () => { + const base = table({ grants: [grant('anon', 'INSERT')] }); + expect(await check([view({ writable: ['INSERT'] })], [base], ['anon'])).toEqual([]); + }); + + it('stays silent for a security_invoker view over the same shape', async () => { + const invoker = view({ securityInvoker: true, writable: ['INSERT'] }); + expect(await check([invoker], [table()], ['anon'])).toEqual([]); + }); + + it('says so when the owner is also exempt from the base table policies', async () => { + const base = table({ rlsForced: false, owner: 'app_owner' }); + const [finding] = await check([view({ writable: ['INSERT'] })], [base], ['anon']); + expect(finding.context).toMatchObject({ rlsBypassed: true }); + expect(finding.message).toContain('not subject to its RLS policies'); + }); +}); + +describe('checkViewRuleBypass (L10)', () => { + async function check(views: ViewSnapshot[], tables: TableSnapshot[], roles: string[]) { + const { ruleDriven } = await analyzeViewWrites(views, tables); + return checkViewRuleBypass(ruleDriven, tables, GRAPH, { roles }); + } + + it('flags the relation a rule writes as the view owner', async () => { + const findings = await check([view({ rules: [rule()] })], [table(), AUDIT], ['anon']); + expect(findings).toHaveLength(1); + expect(findings[0]).toMatchObject({ + code: 'L10', + severity: 'info', + schema: 'app', + table: 'audit_log', + role: 'anon', + privilege: 'INSERT' + }); + expect(findings[0].context).toMatchObject({ rule: 'inbox_insert', viewPrivilege: 'INSERT' }); + }); + + it('fires on an invoker view: security_invoker does not govern the rule action', async () => { + const invoker = view({ securityInvoker: true, rules: [rule()] }); + const findings = await check([invoker], [table(), AUDIT], ['anon']); + expect(findings).toHaveLength(1); + }); + + it('stays silent when the role can write the target anyway', async () => { + const audit = table({ ...AUDIT, grants: [grant('anon', 'INSERT')] }); + expect(await check([view({ rules: [rule()] })], [table(), audit], ['anon'])).toEqual([]); + }); + + it('never recommends revoking a grant', async () => { + const [finding] = await check([view({ rules: [rule()] })], [table(), AUDIT], ['anon']); + expect(finding.hint).toContain('Do not revoke'); + }); +}); diff --git a/packages/safegres/corpus/cases/27-definer-view-write/case.json b/packages/safegres/corpus/cases/27-definer-view-write/case.json new file mode 100644 index 000000000..8f08a183d --- /dev/null +++ b/packages/safegres/corpus/cases/27-definer-view-write/case.json @@ -0,0 +1,36 @@ +{ + "title": "Anonymous role writes a locked table through an auto-updatable DEFINER view", + "dimension": "security", + "exposure": { + "schemas": [ + "c_definer_view_write" + ], + "roles": [ + "corpus_anon", + "corpus_user" + ], + "anonRoles": [ + "corpus_anon" + ] + }, + "expect": [ + { + "code": "L9", + "relation": "c_definer_view_write.submissions", + "note": "the view is auto-updatable and not security_invoker, so corpus_anon's INSERT on it inserts into `submissions` as c_view_write_owner — a write no ACL row on `submissions` gives it" + }, + { + "code": "A3", + "relation": "c_definer_view_write.submissions", + "note": "RLS is not FORCEd, which is exactly why the owner's rewritten insert is not filtered by the table's own policy" + } + ], + "forbid": [ + "L8", + "L10", + "L4" + ], + "worstSeverity": "low", + "fix": "Recreate c_definer_view_write.submission_inbox WITH (security_invoker = true) so the insert is checked against the caller, or give the view an owner whose reach matches what it is meant to expose. Revoking corpus_anon's INSERT on the view is not the fix — that grant is what the API serves.", + "id": "27-definer-view-write" +} diff --git a/packages/safegres/corpus/cases/27-definer-view-write/schema.sql b/packages/safegres/corpus/cases/27-definer-view-write/schema.sql new file mode 100644 index 000000000..01be8ef25 --- /dev/null +++ b/packages/safegres/corpus/cases/27-definer-view-write/schema.sql @@ -0,0 +1,40 @@ +DROP SCHEMA IF EXISTS c_definer_view_write CASCADE; +CREATE SCHEMA c_definer_view_write; + +-- The role the view executes as, and the owner of the table behind it. +DO $$ BEGIN + CREATE ROLE c_view_write_owner NOLOGIN; +EXCEPTION WHEN duplicate_object THEN NULL; +END $$; + +GRANT USAGE ON SCHEMA c_definer_view_write TO corpus_anon, corpus_user, c_view_write_owner; + +CREATE TABLE c_definer_view_write.submissions ( + id bigserial PRIMARY KEY, + author text NOT NULL, + body text NOT NULL +); +ALTER TABLE c_definer_view_write.submissions OWNER TO c_view_write_owner; +ALTER TABLE c_definer_view_write.submissions ENABLE ROW LEVEL SECURITY; + +-- Signed-in users write their own rows and read them back. corpus_anon holds +-- no grant on the table at all. +CREATE POLICY submissions_own_rows ON c_definer_view_write.submissions + FOR ALL TO corpus_user + USING (author = (SELECT current_setting('app.author', true))) + WITH CHECK (author = (SELECT current_setting('app.author', true))); +CREATE INDEX submissions_author_idx ON c_definer_view_write.submissions (author); +GRANT SELECT, INSERT ON c_definer_view_write.submissions TO corpus_user; +GRANT USAGE ON SEQUENCE c_definer_view_write.submissions_id_seq TO corpus_user; + +-- The flaw: a simple view over one table is *auto-updatable*, so Postgres +-- rewrites an INSERT on the view into an INSERT on `submissions` — and because +-- the view is not `security_invoker`, that insert is permission-checked +-- against the view's owner, who owns the table. corpus_anon holds nothing but +-- INSERT on the view, and writes rows no policy of `submissions` would have +-- admitted from it. Nothing in the body says so: the write path is +-- `pg_relation_is_updatable`, not `pg_get_viewdef`. +CREATE VIEW c_definer_view_write.submission_inbox AS + SELECT id, author, body FROM c_definer_view_write.submissions; +ALTER VIEW c_definer_view_write.submission_inbox OWNER TO c_view_write_owner; +GRANT INSERT ON c_definer_view_write.submission_inbox TO corpus_anon; diff --git a/packages/safegres/corpus/cases/28-invoker-view-no-write/case.json b/packages/safegres/corpus/cases/28-invoker-view-no-write/case.json new file mode 100644 index 000000000..c699dc580 --- /dev/null +++ b/packages/safegres/corpus/cases/28-invoker-view-no-write/case.json @@ -0,0 +1,31 @@ +{ + "title": "A security_invoker view over the same auto-updatable shape is not a write bypass", + "dimension": "security", + "exposure": { + "schemas": [ + "c_invoker_view_no_write" + ], + "roles": [ + "corpus_anon", + "corpus_user" + ], + "anonRoles": [ + "corpus_anon" + ] + }, + "expect": [ + { + "code": "A3", + "relation": "c_invoker_view_no_write.submissions", + "note": "the only real finding left: RLS is not FORCEd, so the table owner still sees and writes every row" + } + ], + "forbid": [ + "L8", + "L9", + "L10" + ], + "worstSeverity": "low", + "fix": "ALTER TABLE c_invoker_view_no_write.submissions FORCE ROW LEVEL SECURITY. The view needs no fix — `security_invoker` is what the definer-view write path (case 27) is fixed *into*.", + "id": "28-invoker-view-no-write" +} diff --git a/packages/safegres/corpus/cases/28-invoker-view-no-write/schema.sql b/packages/safegres/corpus/cases/28-invoker-view-no-write/schema.sql new file mode 100644 index 000000000..b1f4644a2 --- /dev/null +++ b/packages/safegres/corpus/cases/28-invoker-view-no-write/schema.sql @@ -0,0 +1,37 @@ +DROP SCHEMA IF EXISTS c_invoker_view_no_write CASCADE; +CREATE SCHEMA c_invoker_view_no_write; + +DO $$ BEGIN + CREATE ROLE c_invoker_write_owner NOLOGIN; +EXCEPTION WHEN duplicate_object THEN NULL; +END $$; + +GRANT USAGE ON SCHEMA c_invoker_view_no_write + TO corpus_anon, corpus_user, c_invoker_write_owner; + +CREATE TABLE c_invoker_view_no_write.submissions ( + id bigserial PRIMARY KEY, + author text NOT NULL, + body text NOT NULL +); +ALTER TABLE c_invoker_view_no_write.submissions OWNER TO c_invoker_write_owner; +ALTER TABLE c_invoker_view_no_write.submissions ENABLE ROW LEVEL SECURITY; + +CREATE POLICY submissions_own_rows ON c_invoker_view_no_write.submissions + FOR ALL TO corpus_user + USING (author = (SELECT current_setting('app.author', true))) + WITH CHECK (author = (SELECT current_setting('app.author', true))); +CREATE INDEX submissions_author_idx ON c_invoker_view_no_write.submissions (author); +GRANT SELECT, INSERT ON c_invoker_view_no_write.submissions TO corpus_user; +GRANT USAGE ON SEQUENCE c_invoker_view_no_write.submissions_id_seq TO corpus_user; + +-- The same auto-updatable shape as case 27, with the one difference that +-- matters: `security_invoker` sends the rewritten insert through the caller's +-- own privileges, so corpus_anon's INSERT on the view is denied on +-- `submissions` exactly as a direct insert would be. There is no write edge +-- here, and a rule that reported one would be flagging a correct schema. +CREATE VIEW c_invoker_view_no_write.submission_inbox + WITH (security_invoker = true) AS + SELECT id, author, body FROM c_invoker_view_no_write.submissions; +ALTER VIEW c_invoker_view_no_write.submission_inbox OWNER TO c_invoker_write_owner; +GRANT INSERT ON c_invoker_view_no_write.submission_inbox TO corpus_anon; diff --git a/packages/safegres/corpus/cases/29-rewrite-rule-bypass/case.json b/packages/safegres/corpus/cases/29-rewrite-rule-bypass/case.json new file mode 100644 index 000000000..b6b6b8085 --- /dev/null +++ b/packages/safegres/corpus/cases/29-rewrite-rule-bypass/case.json @@ -0,0 +1,36 @@ +{ + "title": "Anonymous role writes an audit table through a rewrite rule on a view", + "dimension": "security", + "exposure": { + "schemas": [ + "c_rewrite_rule_bypass" + ], + "roles": [ + "corpus_anon", + "corpus_user" + ], + "anonRoles": [ + "corpus_anon" + ] + }, + "expect": [ + { + "code": "L10", + "relation": "c_rewrite_rule_bypass.audit_log", + "note": "the rule's action runs as the view owner c_rule_owner, so corpus_anon's INSERT on the view appends to `audit_log` — and `security_invoker` on the view does not govern it" + }, + { + "code": "A1", + "relation": "c_rewrite_rule_bypass.audit_log", + "note": "the audit table has RLS on and no policies, which is the correct posture for a table only its owner writes — and exactly why the rule's owner-privileged write is worth naming" + } + ], + "forbid": [ + "L8", + "L9", + "L4" + ], + "worstSeverity": "low", + "fix": "Move the audit write into a function corpus_anon must be granted EXECUTE on, or give c_rewrite_rule_bypass.message_inbox an owner whose reach matches what the rule is meant to do. Revoking corpus_anon's INSERT on the view is not the fix — that grant is what the API serves.", + "id": "29-rewrite-rule-bypass" +} diff --git a/packages/safegres/corpus/cases/29-rewrite-rule-bypass/schema.sql b/packages/safegres/corpus/cases/29-rewrite-rule-bypass/schema.sql new file mode 100644 index 000000000..9cedaabe8 --- /dev/null +++ b/packages/safegres/corpus/cases/29-rewrite-rule-bypass/schema.sql @@ -0,0 +1,52 @@ +DROP SCHEMA IF EXISTS c_rewrite_rule_bypass CASCADE; +CREATE SCHEMA c_rewrite_rule_bypass; + +DO $$ BEGIN + CREATE ROLE c_rule_owner NOLOGIN; +EXCEPTION WHEN duplicate_object THEN NULL; +END $$; + +GRANT USAGE ON SCHEMA c_rewrite_rule_bypass TO corpus_anon, corpus_user, c_rule_owner; + +CREATE TABLE c_rewrite_rule_bypass.messages ( + id bigserial PRIMARY KEY, + author text NOT NULL, + body text NOT NULL +); +ALTER TABLE c_rewrite_rule_bypass.messages OWNER TO c_rule_owner; +ALTER TABLE c_rewrite_rule_bypass.messages ENABLE ROW LEVEL SECURITY; +ALTER TABLE c_rewrite_rule_bypass.messages FORCE ROW LEVEL SECURITY; + +CREATE POLICY messages_own_rows ON c_rewrite_rule_bypass.messages + FOR ALL TO corpus_user + USING (author = (SELECT current_setting('app.author', true))) + WITH CHECK (author = (SELECT current_setting('app.author', true))); +CREATE INDEX messages_author_idx ON c_rewrite_rule_bypass.messages (author); +GRANT SELECT, INSERT ON c_rewrite_rule_bypass.messages TO corpus_user; +GRANT USAGE ON SEQUENCE c_rewrite_rule_bypass.messages_id_seq TO corpus_user; + +-- An append-only audit trail nobody but the owner is meant to write. +CREATE TABLE c_rewrite_rule_bypass.audit_log ( + id bigserial PRIMARY KEY, + note text NOT NULL +); +ALTER TABLE c_rewrite_rule_bypass.audit_log OWNER TO c_rule_owner; +ALTER TABLE c_rewrite_rule_bypass.audit_log ENABLE ROW LEVEL SECURITY; +ALTER TABLE c_rewrite_rule_bypass.audit_log FORCE ROW LEVEL SECURITY; + +-- The flaw, and note what it survives: the view *is* `security_invoker`, so +-- its own base relation is read and written as the caller. That setting does +-- not reach the rule. A rewrite rule's actions are permission-checked against +-- the owner of the relation the rule is on, so corpus_anon's INSERT on the +-- view writes `audit_log` as c_rule_owner. `pg_get_viewdef` never names +-- `audit_log`: no reading of the view's definition can find this edge. +CREATE VIEW c_rewrite_rule_bypass.message_inbox + WITH (security_invoker = true) AS + SELECT id, author, body FROM c_rewrite_rule_bypass.messages; +ALTER VIEW c_rewrite_rule_bypass.message_inbox OWNER TO c_rule_owner; + +CREATE RULE message_inbox_insert AS + ON INSERT TO c_rewrite_rule_bypass.message_inbox + DO INSTEAD INSERT INTO c_rewrite_rule_bypass.audit_log (note) VALUES (new.body); + +GRANT INSERT ON c_rewrite_rule_bypass.message_inbox TO corpus_anon; diff --git a/packages/safegres/corpus/cases/30-instead-nothing-rule/case.json b/packages/safegres/corpus/cases/30-instead-nothing-rule/case.json new file mode 100644 index 000000000..80b5f92ea --- /dev/null +++ b/packages/safegres/corpus/cases/30-instead-nothing-rule/case.json @@ -0,0 +1,31 @@ +{ + "title": "A DO INSTEAD NOTHING rule reaches nothing and is not a write bypass", + "dimension": "security", + "exposure": { + "schemas": [ + "c_instead_nothing_rule" + ], + "roles": [ + "corpus_anon", + "corpus_user" + ], + "anonRoles": [ + "corpus_anon" + ] + }, + "expect": [ + { + "code": "A3", + "relation": "c_instead_nothing_rule.messages", + "note": "the only real finding: RLS is not FORCEd, so the table owner still reads every row — the rules themselves confer nothing" + } + ], + "forbid": [ + "L8", + "L9", + "L10" + ], + "worstSeverity": "low", + "fix": "ALTER TABLE c_instead_nothing_rule.messages FORCE ROW LEVEL SECURITY. The view and its rules need no fix — `DO INSTEAD NOTHING` is what a read-only view is *built* from, and this case exists to pin that the write rules stay silent on it.", + "id": "30-instead-nothing-rule" +} diff --git a/packages/safegres/corpus/cases/30-instead-nothing-rule/schema.sql b/packages/safegres/corpus/cases/30-instead-nothing-rule/schema.sql new file mode 100644 index 000000000..66dcdc805 --- /dev/null +++ b/packages/safegres/corpus/cases/30-instead-nothing-rule/schema.sql @@ -0,0 +1,44 @@ +DROP SCHEMA IF EXISTS c_instead_nothing_rule CASCADE; +CREATE SCHEMA c_instead_nothing_rule; + +DO $$ BEGIN + CREATE ROLE c_readonly_view_owner NOLOGIN; +EXCEPTION WHEN duplicate_object THEN NULL; +END $$; + +GRANT USAGE ON SCHEMA c_instead_nothing_rule + TO corpus_anon, corpus_user, c_readonly_view_owner; + +CREATE TABLE c_instead_nothing_rule.messages ( + id bigserial PRIMARY KEY, + author text NOT NULL, + body text NOT NULL +); +ALTER TABLE c_instead_nothing_rule.messages OWNER TO c_readonly_view_owner; +ALTER TABLE c_instead_nothing_rule.messages ENABLE ROW LEVEL SECURITY; + +CREATE POLICY messages_own_rows ON c_instead_nothing_rule.messages + FOR ALL TO corpus_user + USING (author = (SELECT current_setting('app.author', true))) + WITH CHECK (author = (SELECT current_setting('app.author', true))); +CREATE INDEX messages_author_idx ON c_instead_nothing_rule.messages (author); +GRANT SELECT, INSERT ON c_instead_nothing_rule.messages TO corpus_user; +GRANT USAGE ON SEQUENCE c_instead_nothing_rule.messages_id_seq TO corpus_user; + +-- The commonest rule in the wild, and a correct one: `DO INSTEAD NOTHING` +-- makes the view read-only by swallowing the write. It reaches no relation at +-- all, so it confers nothing, and a rule that reported a write edge here would +-- be flagging the very construct that closes the write path. +CREATE VIEW c_instead_nothing_rule.message_feed + WITH (security_invoker = true) AS + SELECT id, author, body FROM c_instead_nothing_rule.messages; +ALTER VIEW c_instead_nothing_rule.message_feed OWNER TO c_readonly_view_owner; + +CREATE RULE message_feed_no_insert AS + ON INSERT TO c_instead_nothing_rule.message_feed DO INSTEAD NOTHING; +CREATE RULE message_feed_no_update AS + ON UPDATE TO c_instead_nothing_rule.message_feed DO INSTEAD NOTHING; +CREATE RULE message_feed_no_delete AS + ON DELETE TO c_instead_nothing_rule.message_feed DO INSTEAD NOTHING; + +GRANT SELECT, INSERT ON c_instead_nothing_rule.message_feed TO corpus_anon; diff --git a/packages/safegres/docs/rules.md b/packages/safegres/docs/rules.md index 3bc890104..2f26f95c1 100644 --- a/packages/safegres/docs/rules.md +++ b/packages/safegres/docs/rules.md @@ -56,9 +56,24 @@ SQL, an unparseable definition, a chain of views deeper than it will follow — entirely, because an unread body is *unknown*, not empty. A reference it cannot pin to exactly one relation resolves to nothing rather than to a plausible candidate. And the remedy is never a revoke: the SELECT on the view is what the API serves, so L8 recommends `security_invoker = true` -or a different owner, and says so explicitly. Only SELECT is modelled — an auto-updatable view can -carry writes the same way, but proving which write reaches which base relation needs more than the -body's relation set. +or a different owner, and says so explicitly. + +L9 and L10 are the write half of the same question, and neither is answerable from the view body +alone. **L9** is auto-update: a simple view over one relation is updatable, so Postgres rewrites an +INSERT/UPDATE/DELETE on the view onto that relation, and on a definer view the rewritten command is +checked against the *owner*. The body says which relation; only `pg_relation_is_updatable` says the +write lands there at all. **L10** is rewrite rules: a rule other than the view's own `_RETURN` rule +is invisible to `pg_get_viewdef`, so `ON INSERT ... DO INSTEAD INSERT INTO audit` reaches a relation +the definition never names. Two properties of L10 are worth stating plainly, both verified against +Postgres 18 rather than inferred: rule actions are permission-checked against the owner of the +relation the rule is on, and `security_invoker` does **not** govern them — it governs the view's own +base relations. An invoker view with such a rule still writes as its owner. + +Both inherit L8's refusals. An `INSTEAD OF` trigger sends the write into a function body whose +target is not proven here, a body that does not resolve to exactly one relation places no write, and +an unreadable rule action is unknown — all three suppress. `DO INSTEAD NOTHING`, the commonest rule +in the wild, reaches no relation and so reports nothing, which is the correct answer for the +read-only views it is used to build. Restrictive-only policies never count as coverage. `BYPASSRLS` and superuser roles are exempt from policy checks — they are not subject to RLS, so a "missing policy" finding for them would be diff --git a/packages/safegres/src/callgraph/extract.ts b/packages/safegres/src/callgraph/extract.ts index a6f519283..e89bd0fce 100644 --- a/packages/safegres/src/callgraph/extract.ts +++ b/packages/safegres/src/callgraph/extract.ts @@ -25,6 +25,22 @@ export interface TableRef extends NameRef { write: boolean; } +/** + * A relation reference with the privilege the reference actually exercises, + * rather than the read/write bit {@link TableRef} carries. `INSERT INTO audit` + * and `UPDATE audit` are both writes, but they are not the same grant, and a + * rule action can mix them in one statement. + */ +export interface RelationAccess extends NameRef { + privilege: 'SELECT' | 'INSERT' | 'UPDATE' | 'DELETE'; +} + +export interface ExtractedAccess { + accesses: RelationAccess[]; + opaque: boolean; + opaqueReason?: string; +} + export interface ExtractedBody { calls: NameRef[]; tables: TableRef[]; @@ -166,6 +182,63 @@ export async function extractQuery(sql: string): Promise { return finalize(out); } +/** + * The same walk as {@link extractQuery}, but resolving each relation + * reference to the privilege it exercises instead of a read/write bit. + * + * Used for statements whose interesting content is *which grant* a reference + * needs — a rewrite rule's actions, where `INSERT INTO audit` means the rule + * needs INSERT on `audit` and nothing else tells you so. + */ +export async function extractAccess(sql: string): Promise { + let ast: unknown; + try { + ast = await parse(sql); + } catch { + return { accesses: [], opaque: true, opaqueReason: 'SQL fragment failed to parse' }; + } + + const byNode = new Map, RelationAccess['privilege']>(); + const commands = [ + ['InsertStmt', 'INSERT'], + ['UpdateStmt', 'UPDATE'], + ['DeleteStmt', 'DELETE'] + ] as const; + for (const [tag, privilege] of commands) { + for (const stmt of findAll(ast, tag)) { + const rel = stmt.relation as Record | undefined; + if (rel) byNode.set(rel, privilege); + } + } + + let opaque = false; + let opaqueReason: string | undefined; + for (const call of findAll(ast, 'FuncCall')) { + const ref = funcNameParts(call); + // A rule action can hide its real target behind a function call; the + // relations that call touches are not in this AST. + if (ref.name === 'query_to_xml' || ref.name === 'dblink' || ref.name === 'dblink_exec') { + opaque = true; + opaqueReason ??= `\`${ref.name}\` executes SQL this analysis cannot see`; + } + } + + const accesses: RelationAccess[] = []; + const seen = new Set(); + for (const rv of findAll(ast, 'RangeVar')) { + const name = typeof rv.relname === 'string' ? rv.relname : undefined; + if (!name) continue; + const schema = typeof rv.schemaname === 'string' ? rv.schemaname : undefined; + const privilege = byNode.get(rv) ?? 'SELECT'; + const key = `${schema ?? ''}.${name}::${privilege}`; + if (seen.has(key)) continue; + seen.add(key); + accesses.push({ ...(schema ? { schema } : {}), name, privilege }); + } + + return { accesses, opaque, ...(opaqueReason ? { opaqueReason } : {}) }; +} + function firstStringArg(call: Record): string | null { const args = call.args; if (!Array.isArray(args) || args.length === 0) return null; diff --git a/packages/safegres/src/checks/definer-view.ts b/packages/safegres/src/checks/definer-view.ts index 21080a38c..876d85f3b 100644 --- a/packages/safegres/src/checks/definer-view.ts +++ b/packages/safegres/src/checks/definer-view.ts @@ -62,12 +62,7 @@ export async function analyzeViewBodies( // A materialized view stores its rows: reading it touches no base relation, // so it is a leaf here, never an edge. const queryable = views.filter((v) => !v.materialized); - const tableKeys = new Set(tables.map((t) => `${t.schema}.${t.name}`)); - const byName = new Map(); - for (const v of queryable) { - byName.set(v.name, [...(byName.get(v.name) ?? []), v]); - } - const viewKeys = new Map(queryable.map((v) => [`${v.schema}.${v.name}`, v])); + const index = buildRelationIndex(queryable, tables); const bodies = new Map>>(); for (const v of queryable) { @@ -98,7 +93,7 @@ export async function analyzeViewBodies( } for (const ref of body.tables) { - const relation = resolve(ref, current.schema, tableKeys, viewKeys, byName); + const relation = resolveRelation(ref, current.schema, index); if (!relation) continue; // a CTE, an alias, or a name we cannot pin down if (relation.kind === 'view') { @@ -139,10 +134,27 @@ export async function analyzeViewBodies( return { views: out, suppressed }; } -type Resolved = +export type Resolved = | { kind: 'table'; schema: string; name: string } | { kind: 'view'; view: ViewSnapshot }; +/** The lookup tables {@link resolveRelation} needs, built once per snapshot. */ +export interface RelationIndex { + tableKeys: Set; + viewKeys: Map; + viewsByName: Map; +} + +export function buildRelationIndex(views: ViewSnapshot[], tables: TableSnapshot[]): RelationIndex { + const viewsByName = new Map(); + for (const v of views) viewsByName.set(v.name, [...(viewsByName.get(v.name) ?? []), v]); + return { + tableKeys: new Set(tables.map((t) => `${t.schema}.${t.name}`)), + viewKeys: new Map(views.map((v) => [`${v.schema}.${v.name}`, v])), + viewsByName + }; +} + /** * Pin a body reference to a relation in the snapshot. * @@ -153,12 +165,10 @@ type Resolved = * table alias, a relation outside the audited schemas) resolves to nothing: * over-approximating here would attribute a read to a relation nobody named. */ -function resolve( +export function resolveRelation( ref: { schema?: string; name: string }, viewSchema: string, - tableKeys: Set, - viewKeys: Map, - viewsByName: Map + { tableKeys, viewKeys, viewsByName }: RelationIndex ): Resolved | null { const candidates = ref.schema ? [ref.schema] : [viewSchema]; for (const schema of candidates) { diff --git a/packages/safegres/src/checks/role-reach.ts b/packages/safegres/src/checks/role-reach.ts index 5632862f0..008411762 100644 --- a/packages/safegres/src/checks/role-reach.ts +++ b/packages/safegres/src/checks/role-reach.ts @@ -34,7 +34,13 @@ export type RoleReachEdge = * The caller read through a view that executes as `owner` — every relation * the body names is read under the owner's privileges, not the caller's. */ - | { kind: 'view'; view: string; owner: string }; + | { kind: 'view'; view: string; owner: string } + /** + * The caller's command fired a rewrite rule on `view`. The rule's actions + * are permission-checked against the rule's table owner, and unlike the + * view edge this is *not* governed by `security_invoker`. + */ + | { kind: 'rule'; view: string; rule: string; owner: string }; /** * How well-founded a reach cell is. Stage 1 is entirely `catalog` — every @@ -154,10 +160,9 @@ export interface ViewReachInput { * that can SELECT a view, one cell per base relation the view body reads, * under the owner the hop executes as. * - * Only SELECT is modelled. An auto-updatable or `INSTEAD OF`-triggered view - * can carry writes the same way, but proving *which* write reaches *which* - * base relation needs more than the body's relation set, and an unproven - * write edge is exactly the kind of guess this model refuses to make. + * Only SELECT is modelled here; the write half is + * {@link computeViewWriteReach}, which needs the catalog's updatability + * answer on top of the body's relation set. * * A view whose body could not be read (dynamic SQL, an unparseable body) must * not appear in `views`: an unreadable body is unknown, not empty. @@ -193,3 +198,79 @@ export function computeViewReach( return { role, cells }; }); } + +/** + * One relation a *write* against a view lands on. + * + * The two privileges are distinct on purpose. `via` is what the caller must + * hold on the view to issue the command; `privilege` is what the rewritten + * command exercises on the target. They coincide for an auto-updatable view + * (an INSERT on the view is an INSERT on its base relation) and routinely + * differ for a rewrite rule, where `ON INSERT ... DO INSTEAD UPDATE other` + * turns one into the other. + */ +export interface ViewWriteEdge { + schema: string; + table: string; + via: PgPrivilege; + privilege: PgPrivilege; + /** View hops, outermost first; the last hop's owner executes the write. */ + hops: Array<{ view: string; owner: string }>; + /** Set when the edge exists because of a rewrite rule, not auto-update. */ + rule?: string; +} + +/** A view, its own ACL, and the relations writes against it reach. */ +export interface ViewWriteInput { + schema: string; + name: string; + owner: string; + grants: GrantInfo[]; + writeEdges: ViewWriteEdge[]; +} + +/** + * Project write edges through views into the reach model: for every role that + * holds the triggering command on the view, one cell per relation that write + * lands on, under the role the landing executes as. + * + * As with {@link computeViewReach}, a view whose write target could not be + * proven must not appear in `views` — an `INSTEAD OF` trigger's body, a + * multi-relation body, or an unreadable rule action is unknown, not empty. + */ +export function computeViewWriteReach( + views: ViewWriteInput[], + graph: RoleGraph, + roles: string[] +): RoleReach[] { + return roles.map((role) => { + const cells: RoleReachCell[] = []; + + for (const view of views) { + const held = effectiveGrants(view, role, graph); + for (const edge of view.writeEdges) { + if (edge.hops.length === 0) continue; + const grant = held.find((g) => g.privilege === edge.via); + if (!grant) continue; + + const last = edge.hops[edge.hops.length - 1]; + cells.push({ + schema: edge.schema, + table: edge.table, + privileges: [edge.privilege], + effectiveRole: last.owner, + path: [ + { kind: 'grant', via: grant.via, privilege: edge.via }, + ...edge.hops.map((h) => ({ kind: 'view' as const, view: h.view, owner: h.owner })), + ...(edge.rule + ? [{ kind: 'rule' as const, view: last.view, rule: edge.rule, owner: last.owner }] + : []) + ], + proof: 'ast' + }); + } + } + + return { role, cells }; + }); +} diff --git a/packages/safegres/src/checks/view-writes.ts b/packages/safegres/src/checks/view-writes.ts new file mode 100644 index 000000000..a6d2ee3d3 --- /dev/null +++ b/packages/safegres/src/checks/view-writes.ts @@ -0,0 +1,364 @@ +/** + * L9 and L10: what a view does *beyond* its SELECT. + * + * L8 models the read edge — a non-`security_invoker` view hands its readers + * the owner's privileges on the relations its body names. Two write paths + * escape that model entirely, and both were verified against PostgreSQL 18 + * rather than inferred: + * + * - **L9, auto-update.** A simple view is updatable: Postgres rewrites an + * INSERT/UPDATE/DELETE on the view onto its single base relation. On a + * definer view that rewrite is permission-checked against the *owner*, so + * INSERT on the view is INSERT on the base table the caller cannot touch. + * `security_invoker = true` closes it (the write is then checked against + * the caller and denied). The body alone cannot prove the write lands: + * `pg_relation_is_updatable` is the catalog half of the proof. + * + * - **L10, rewrite rules.** A rule other than the view's own `_RETURN` rule + * is invisible to `pg_get_viewdef`: `ON INSERT ... DO INSTEAD INSERT INTO + * audit` writes a relation the view body never names. Rule actions are + * checked against the rule's table owner, and — unlike the view's own base + * relations — `security_invoker` does **not** govern them. An invoker view + * with such a rule still writes `audit` as the view owner. + * + * Both keep L8's conservatism. An `INSTEAD OF` trigger sends the write into a + * function body whose target this analysis cannot prove, a multi-relation body + * is not auto-updatable in a way we can pin to one target, and an unreadable + * rule action is unknown — all three suppress rather than guess. And, as in + * L8, the fix is never a revoke: the grant on the view is what the API serves. + */ + +import { extractAccess, extractQuery } from '../callgraph/extract'; +import type { ViewRule, ViewSnapshot } from '../pg/indexes'; +import type { PgPrivilege, TableSnapshot } from '../pg/introspect'; +import type { Finding } from '../types'; +import { + buildRelationIndex, + type RelationIndex, + resolveRelation, + type SuppressedView +} from './definer-view'; +import { effectiveGrants, type LatticeRoleOptions, type RoleGraph } from './lattice'; +import { computeViewWriteReach, type ViewWriteEdge, type ViewWriteInput } from './role-reach'; + +/** How deep a chain of updatable views on views is followed before giving up. */ +const MAX_VIEW_DEPTH = 8; + +export interface ViewWriteAnalysis { + /** Definer views whose writes are auto-rewritten onto a base relation. */ + autoUpdatable: ViewWriteInput[]; + /** Views carrying rewrite rules whose actions reach other relations. */ + ruleDriven: ViewWriteInput[]; + /** Views deliberately left out, with why. */ + suppressed: SuppressedView[]; +} + +/** + * Resolve, for every view in the snapshot, the relations a write against it + * actually lands on — through auto-update rewriting and through rewrite rules. + */ +export async function analyzeViewWrites( + views: ViewSnapshot[], + tables: TableSnapshot[] +): Promise { + const queryable = views.filter((v) => !v.materialized); + const index = buildRelationIndex(queryable, tables); + + const autoUpdatable: ViewWriteInput[] = []; + const ruleDriven: ViewWriteInput[] = []; + const suppressed: SuppressedView[] = []; + + for (const view of queryable) { + const name = `${view.schema}.${view.name}`; + + const auto = await autoUpdateEdges(view, index, suppressed); + if (auto.length > 0) { + autoUpdatable.push({ + schema: view.schema, + name: view.name, + owner: view.owner, + grants: view.grants, + writeEdges: auto + }); + } + + const fromRules: ViewWriteEdge[] = []; + for (const rule of view.rules) { + if (rule.event === 'SELECT') continue; + const edges = await ruleEdges(view, rule, index); + if (edges === null) { + suppressed.push({ + view: name, + reason: `rule ${rule.name} has an action this analysis cannot follow` + }); + continue; + } + fromRules.push(...edges); + } + if (fromRules.length > 0) { + ruleDriven.push({ + schema: view.schema, + name: view.name, + owner: view.owner, + grants: view.grants, + writeEdges: fromRules + }); + } + } + + return { autoUpdatable, ruleDriven, suppressed }; +} + +/** + * The base relation an auto-updatable definer view's writes are rewritten + * onto, if it can be proven. + * + * Auto-update only applies when nothing else has taken over the write path, + * so a view with rules or `INSTEAD OF` triggers is not handled here — the + * catalog's updatability bitmask counts those too, and attributing their + * writes to the body's relation would be a guess. + */ +async function autoUpdateEdges( + view: ViewSnapshot, + index: RelationIndex, + suppressed: SuppressedView[] +): Promise { + const name = `${view.schema}.${view.name}`; + if (view.securityInvoker) return []; // the write is checked against the caller + if (view.writable.length === 0) return []; + if (view.insteadOfTriggers) { + suppressed.push({ + view: name, + reason: 'INSTEAD OF triggers decide where the write lands, in a body this analysis does not follow' + }); + return []; + } + if (view.rules.some((r) => r.event !== 'SELECT')) return []; // rule path, see ruleEdges + + const hops: Array<{ view: string; owner: string }> = [{ view: name, owner: view.owner }]; + let current = view; + + for (;;) { + if (hops.length > MAX_VIEW_DEPTH) { + suppressed.push({ view: name, reason: `view chain deeper than ${MAX_VIEW_DEPTH} hops` }); + return []; + } + + const body = await extractQuery(current.definition); + if (body.opaque) { + suppressed.push({ view: name, reason: body.opaqueReason ?? 'body could not be read' }); + return []; + } + + const resolved = body.tables + .map((ref) => resolveRelation(ref, current.schema, index)) + .filter((r): r is NonNullable => r !== null); + // Auto-update needs exactly one target. Anything else — a join, a body + // whose references we could not pin down — is not a write we can place. + if (resolved.length !== 1) return []; + + const target = resolved[0]; + if (target.kind === 'table') { + return view.writable.map((privilege) => ({ + schema: target.schema, + table: target.name, + via: privilege, + privilege, + hops: [...hops] + })); + } + + const nested = target.view; + if (`${nested.schema}.${nested.name}` === `${current.schema}.${current.name}`) return []; + if (nested.materialized || nested.insteadOfTriggers) return []; + // The inner view re-owns the write unless it defers to the caller, in + // which case whichever owner is already in force stays in force. + const owner = nested.securityInvoker ? hops[hops.length - 1].owner : nested.owner; + hops.push({ view: `${nested.schema}.${nested.name}`, owner }); + current = nested; + } +} + +/** + * The relations a rewrite rule's actions reach, or `null` when the action + * cannot be followed. + * + * The rule's own view is dropped from the result: `pg_get_ruledef` names it in + * the `ON ... TO ` clause, and that reference is the trigger, not a + * target. `DO INSTEAD NOTHING` therefore yields nothing at all, which is the + * correct answer for a read-only view — the commonest rule in the wild. + */ +async function ruleEdges( + view: ViewSnapshot, + rule: ViewRule, + index: RelationIndex +): Promise { + const self = `${view.schema}.${view.name}`; + const { accesses, opaque } = await extractAccess(rule.definition); + if (opaque) return null; + + const edges: ViewWriteEdge[] = []; + for (const access of accesses) { + // Reads inside a rule action are the view's own SELECT path, which L8 + // already models; the escalation a rule adds is the write. + if (access.privilege === 'SELECT') continue; + + const target = resolveRelation(access, view.schema, index); + if (!target) continue; + if (target.kind === 'view') { + // The write recurses into another view's rewrite path; proving where it + // finally lands is more than this analysis can do. + return null; + } + if (`${target.schema}.${target.name}` === self) continue; + + edges.push({ + schema: target.schema, + table: target.name, + via: rule.event, + privilege: access.privilege, + hops: [{ view: self, owner: view.owner }], + rule: rule.name + }); + } + + return edges; +} + +/** + * L9: an untrusted role writes a base relation through an auto-updatable + * definer view. + * + * Fires once per (role, view, base relation, command) where the role holds + * the command on the view, the view executes as someone else, and the role + * holds no such privilege on the relation the write lands on. + */ +export function checkDefinerViewWrite( + views: ViewWriteInput[], + tables: TableSnapshot[], + graph: RoleGraph, + options: LatticeRoleOptions = {} +): Finding[] { + return writeFindings(views, tables, graph, options, 'L9', (ctx) => ({ + message: + `Untrusted role ${ctx.role} can ${ctx.privilege} ${ctx.target} through view ${ctx.view}, ` + + `which executes as its owner ${ctx.owner} — ${ctx.role} holds no ${ctx.privilege} on the ` + + `base relation` + + (ctx.rlsBypassed ? `, and ${ctx.owner} is not subject to its RLS policies` : ''), + hint: + `The view is auto-updatable, so Postgres rewrites the ${ctx.privilege} onto ${ctx.target} and ` + + `checks it against ${ctx.owner}, not ${ctx.role}. Recreate the view ` + + `\`WITH (security_invoker = true)\` so the caller's own grants and policies apply, give it an ` + + `owner whose reach matches what the view is meant to expose, or make it non-updatable. Do not ` + + `revoke the grant on the view — that grant is what the API serves.` + })); +} + +/** + * L10: an untrusted role writes a relation through a rewrite rule on a view. + * + * Unlike L9 this fires on `security_invoker` views too: `security_invoker` + * governs the view's own base relations, not the relations a rule's actions + * name, which are checked against the rule's table owner either way. + */ +export function checkViewRuleBypass( + views: ViewWriteInput[], + tables: TableSnapshot[], + graph: RoleGraph, + options: LatticeRoleOptions = {} +): Finding[] { + return writeFindings(views, tables, graph, options, 'L10', (ctx) => ({ + message: + `Untrusted role ${ctx.role} can ${ctx.privilege} ${ctx.target} through rule ${ctx.rule} on view ` + + `${ctx.view} — the rule's action runs as the view owner ${ctx.owner}, and ${ctx.role} holds no ` + + `${ctx.privilege} on ${ctx.target}` + + (ctx.rlsBypassed ? `, whose RLS policies ${ctx.owner} is not subject to` : ''), + hint: + `Rewrite rules are not shown by \`pg_get_viewdef\` and are not governed by \`security_invoker\`: ` + + `their actions are permission-checked against the owner of the relation the rule is on. Move ` + + `the action into a function the caller must be granted EXECUTE on, or give the view an owner ` + + `whose reach matches what the rule is meant to do. Do not revoke the grant on the view — that ` + + `grant is what the API serves.` + })); +} + +interface WriteContext { + role: string; + view: string; + owner: string; + target: string; + privilege: PgPrivilege; + rule?: string; + rlsBypassed: boolean; +} + +function writeFindings( + views: ViewWriteInput[], + tables: TableSnapshot[], + graph: RoleGraph, + options: LatticeRoleOptions, + code: 'L9' | 'L10', + render: (ctx: WriteContext) => { message: string; hint: string } +): Finding[] { + const untrusted = options.roles ?? []; + if (untrusted.length === 0 || views.length === 0) return []; + + const byKey = new Map(tables.map((t) => [`${t.schema}.${t.name}`, t])); + const out: Finding[] = []; + + for (const { role, cells } of computeViewWriteReach(views, graph, untrusted)) { + for (const cell of cells) { + if (cell.effectiveRole === role) continue; + + const base = byKey.get(`${cell.schema}.${cell.table}`); + if (!base) continue; + const privilege = cell.privileges[0]; + // Already writable in its own right: the view launders nothing. + if (effectiveGrants(base, role, graph).some((g) => g.privilege === privilege)) continue; + + const viewHops = cell.path.filter((e) => e.kind === 'view'); + const ruleEdge = cell.path.find((e) => e.kind === 'rule'); + const grantEdge = cell.path.find((e) => e.kind === 'grant'); + const owner = cell.effectiveRole; + const ownerAttrs = graph.get(owner); + const rlsBypassed = + base.rlsEnabled + && (!!ownerAttrs?.bypassRls || (base.owner === owner && !base.rlsForced)); + + const target = `${base.schema}.${base.name}`; + const { message, hint } = render({ + role, + view: viewHops[0].view, + owner, + target, + privilege, + ...(ruleEdge ? { rule: ruleEdge.rule } : {}), + rlsBypassed + }); + + out.push({ + code, + severity: 'info', + category: 'anti-pattern', + schema: base.schema, + table: base.name, + role, + privilege, + message, + hint, + context: { + view: viewHops[0].view, + effectiveRole: owner, + viaViews: viewHops.map((h) => h.view), + ...(ruleEdge ? { rule: ruleEdge.rule } : {}), + viewPrivilege: grantEdge?.privilege, + baseRlsEnabled: base.rlsEnabled, + rlsBypassed, + proof: cell.proof + } + }); + } + } + + return out; +} diff --git a/packages/safegres/src/commands/audit.ts b/packages/safegres/src/commands/audit.ts index 8ae70d9a2..4890f66b7 100644 --- a/packages/safegres/src/commands/audit.ts +++ b/packages/safegres/src/commands/audit.ts @@ -55,6 +55,7 @@ import { } from '../checks/role-trust'; import { checkSetRoleEscalation } from '../checks/set-role'; import { checkStats, DEFAULT_STATS_THRESHOLDS, type StatsThresholds } from '../checks/stats'; +import { analyzeViewWrites, checkDefinerViewWrite, checkViewRuleBypass } from '../checks/view-writes'; import { configFingerprint } from '../config/fingerprint'; import { allAstRulesDisabled, applyRulesToFindings, matchTablePattern, resolveRules, rulesForTable } from '../config/resolve'; import type { ExposureConfig, SafegresConfig } from '../config/types'; @@ -230,10 +231,23 @@ export async function audit( resolved.rules.get('L8')?.options as LatticeRoleOptions, exposure )?.roles ?? []; + const viewWriteRoles = withExposedRoles( + resolved.rules.get('L9')?.options as LatticeRoleOptions, + exposure + )?.roles ?? []; + const ruleBypassRoles = withExposedRoles( + resolved.rules.get('L10')?.options as LatticeRoleOptions, + exposure + )?.roles ?? []; + const viewWritesEnabled = + !skipAst + && ((viewWriteRoles.length > 0 && resolved.rules.get('L9')?.enabled !== false) + || (ruleBypassRoles.length > 0 && resolved.rules.get('L10')?.enabled !== false)); const needsViews = (perfEnabled && config.perf?.paths?.infer !== false) || resolved.rules.get('L4')?.enabled !== false - || (!skipAst && definerViewRoles.length > 0 && resolved.rules.get('L8')?.enabled !== false); + || (!skipAst && definerViewRoles.length > 0 && resolved.rules.get('L8')?.enabled !== false) + || viewWritesEnabled; const viewSnapshot = needsViews ? await introspectViews(exec, { schemas: options.schemas ?? config.schemas, @@ -337,6 +351,21 @@ export async function audit( ); } + // L9/L10 are the write half of the same question, and share one analysis. + if (viewWritesEnabled) { + const writes = await analyzeViewWrites(viewSnapshot, snapshot); + if (viewWriteRoles.length > 0 && resolved.rules.get('L9')?.enabled !== false) { + findings.push( + ...checkDefinerViewWrite(writes.autoUpdatable, snapshot, roleGraph, { roles: viewWriteRoles }) + ); + } + if (ruleBypassRoles.length > 0 && resolved.rules.get('L10')?.enabled !== false) { + findings.push( + ...checkViewRuleBypass(writes.ruleDriven, snapshot, roleGraph, { roles: ruleBypassRoles }) + ); + } + } + const statsSnapshot: StatsSnapshot | null = statsEnabled ? await introspectStats(exec, { schemas: options.schemas ?? config.schemas, diff --git a/packages/safegres/src/config/presets.ts b/packages/safegres/src/config/presets.ts index b4b768b5a..0ab8fd1c9 100644 --- a/packages/safegres/src/config/presets.ts +++ b/packages/safegres/src/config/presets.ts @@ -26,7 +26,12 @@ export const recommended: SafegresConfig = { // Same posture, same reason: a view that executes as its owner hands its // readers that owner's reach, which is a real bypass, but the rule is new // and body-derived, so it reports at zero weight until validated. - L8: ['info', { rolesFrom: 'anon' }] + L8: ['info', { rolesFrom: 'anon' }], + // The write half of the same story: an auto-updatable definer view, and a + // rewrite rule whose action runs as the view owner. Same posture again — + // new, body-derived, zero weight until validated. + L9: ['info', { rolesFrom: 'anon' }], + L10: ['info', { rolesFrom: 'anon' }] } }; diff --git a/packages/safegres/src/index.ts b/packages/safegres/src/index.ts index f9ff0c06b..31bc709f5 100644 --- a/packages/safegres/src/index.ts +++ b/packages/safegres/src/index.ts @@ -75,6 +75,12 @@ export { checkUnusedIndexes, DEFAULT_STATS_THRESHOLDS } from './checks/stats'; +export type { ViewWriteAnalysis } from './checks/view-writes'; +export { + analyzeViewWrites, + checkDefinerViewWrite, + checkViewRuleBypass +} from './checks/view-writes'; export type { AuditOptions } from './commands/audit'; export { audit } from './commands/audit'; export type { DoctorCheck, DoctorOptions, DoctorReport, DoctorStatus } from './commands/doctor'; diff --git a/packages/safegres/src/pg/indexes.ts b/packages/safegres/src/pg/indexes.ts index 0ff0a64b5..ab859f245 100644 --- a/packages/safegres/src/pg/indexes.ts +++ b/packages/safegres/src/pg/indexes.ts @@ -233,8 +233,48 @@ export interface ViewSnapshot { grants: GrantInfo[]; /** `pg_get_viewdef()` — the body, as SQL text. */ definition: string; + /** + * The write commands Postgres accepts on the view, from + * `pg_relation_is_updatable`. A simple view is *auto-updatable*: the write + * is rewritten onto its single base relation, and on a definer view that + * rewrite runs with the owner's privileges — a write edge the body alone + * cannot prove, which is why it is read from the catalog. + * + * The bitmask also counts updatability conferred by rules and `INSTEAD OF` + * triggers, so it is only auto-updatability when {@link rules} is empty and + * {@link insteadOfTriggers} is false. + */ + writable: Array<'INSERT' | 'UPDATE' | 'DELETE'>; + /** The view has `INSTEAD OF` triggers: writes go wherever their bodies say. */ + insteadOfTriggers: boolean; + /** + * Rewrite rules other than the view's own `_RETURN` SELECT rule. These are + * invisible to `pg_get_viewdef`, and their actions are permission-checked + * against the *rule's table owner* — the view owner — regardless of + * `security_invoker`, which only governs the view's own base relations. + */ + rules: ViewRule[]; +} + +/** A rewrite rule on a view, other than the `_RETURN` rule that defines it. */ +export interface ViewRule { + name: string; + /** The command on the view that fires the rule. */ + event: 'SELECT' | 'INSERT' | 'UPDATE' | 'DELETE'; + /** `DO INSTEAD` — the original command is replaced by the rule's actions. */ + instead: boolean; + /** `pg_get_ruledef()` — the whole `CREATE RULE`, actions included. */ + definition: string; } +/** `pg_rewrite.ev_type` is a char code, not the command name. */ +const RULE_EVENTS: Record = { + 1: 'SELECT', + 2: 'UPDATE', + 3: 'INSERT', + 4: 'DELETE' +}; + /** * Every view and materialized view in scope, with its owner, its * `security_invoker` setting and its own ACL. @@ -262,6 +302,9 @@ export async function introspectViews( owner_bypasses_rls: boolean; grants: Array<{ role: string; privilege: string; grantable: boolean; bypassRls: boolean }>; definition: string; + updatable_bits: number; + instead_of_triggers: boolean; + rules: Array<{ name: string; event: string; instead: boolean; definition: string }>; }>( // Both parameters are referenced (even when only one filters) so Postgres // can infer their types — an unused $N errors out at bind time. @@ -286,7 +329,16 @@ export async function introspectViews( 'false' )::boolean AS security_invoker, c.relacl AS relacl, - pg_get_viewdef(c.oid) AS definition + pg_get_viewdef(c.oid) AS definition, + -- Bitmask over 1 << CMD_*: UPDATE 4, INSERT 8, DELETE 16. The second + -- argument asks the same question the rewriter asks at runtime, so + -- rules and INSTEAD OF triggers count towards it too. + pg_relation_is_updatable(c.oid, true) AS updatable_bits, + -- TRIGGER_TYPE_INSTEAD = 1 << 6. + EXISTS ( + SELECT 1 FROM pg_trigger t + WHERE t.tgrelid = c.oid AND NOT t.tgisinternal AND (t.tgtype & 64) <> 0 + ) AS instead_of_triggers FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace LEFT JOIN pg_roles o ON o.oid = c.relowner @@ -306,6 +358,19 @@ export async function introspectViews( FROM views v, aclexplode(v.relacl) a LEFT JOIN pg_roles rol ON rol.oid = a.grantee WHERE v.relacl IS NOT NULL + ), + rules AS ( + -- _RETURN is the SELECT rule that *is* the view; every other rule is + -- behaviour pg_get_viewdef does not show. + SELECT + r.ev_class AS oid, + r.rulename, + r.ev_type, + r.is_instead, + pg_get_ruledef(r.oid) AS definition + FROM pg_rewrite r + JOIN views v ON v.oid = r.ev_class + WHERE r.rulename <> '_RETURN' ) SELECT v.schema_name, @@ -323,7 +388,18 @@ export async function introspectViews( 'bypassRls', g.bypass_rls )) FROM grants g WHERE g.oid = v.oid), '[]'::jsonb - ) AS grants + ) AS grants, + v.updatable_bits, + v.instead_of_triggers, + COALESCE( + (SELECT jsonb_agg(jsonb_build_object( + 'name', r.rulename, + 'event', r.ev_type, + 'instead', r.is_instead, + 'definition', r.definition + )) FROM rules r WHERE r.oid = v.oid), + '[]'::jsonb + ) AS rules FROM views v ORDER BY v.schema_name, v.view_name`, [options.schemas ?? [], excludes] @@ -342,7 +418,19 @@ export async function introspectViews( grantable: g.grantable, bypassRls: g.bypassRls })), - definition: r.definition + definition: r.definition, + writable: [ + ...(r.updatable_bits & 8 ? ['INSERT' as const] : []), + ...(r.updatable_bits & 4 ? ['UPDATE' as const] : []), + ...(r.updatable_bits & 16 ? ['DELETE' as const] : []) + ], + insteadOfTriggers: r.instead_of_triggers, + rules: r.rules.flatMap((rule) => { + const event = RULE_EVENTS[rule.event]; + return event + ? [{ name: rule.name, event, instead: rule.instead, definition: rule.definition }] + : []; + }) })); } diff --git a/packages/safegres/src/rules/registry.ts b/packages/safegres/src/rules/registry.ts index a8b2c1232..de77981c3 100644 --- a/packages/safegres/src/rules/registry.ts +++ b/packages/safegres/src/rules/registry.ts @@ -230,6 +230,35 @@ export const RULES: RuleMeta[] = [ // policy's, so turning the `P*` rules off does not turn this one off. scope: 'table' }, + { + code: 'L9', + category: 'anti-pattern', + // Ships `info` for the same reason as L8, and understates the same way: a + // definer view that is auto-updatable does not just leak rows, it lets an + // untrusted role *write* a table it holds nothing on, as the owner. On its + // own merits that is `high` — the write is unconditional, and when the + // base table has RLS the owner is exempt from (`context.rlsBypassed`) it + // also writes rows no policy would have admitted. Escalate via + // config/preset once the finding proves itself in the field. + defaultSeverity: 'info', + direction: 'fail-open', + title: 'DEFINER view write — an untrusted role writes a base relation as the view owner (options: { roles: [...] })', + scope: 'table' + }, + { + code: 'L10', + category: 'anti-pattern', + // Ships `info` on the same new-rule posture. Note this one fires on + // `security_invoker` views too: `security_invoker` governs the view's own + // base relations, not the relations a rewrite rule's actions name, which + // are checked against the rule's table owner either way (verified against + // PG 18). A rule is also invisible to `pg_get_viewdef`, so this is reach + // no reading of the view's definition can find. + defaultSeverity: 'info', + direction: 'fail-open', + title: 'Rewrite-rule bypass — a rule on a view writes a relation as the view owner (options: { roles: [...] })', + scope: 'table' + }, { code: 'W1', category: 'meta',