Skip to content

Commit 09fa3b9

Browse files
NikolaySNikolayS
committed
Merge branch 'security/remove-explain-generic' into 'main'
fix(security): remove explain_generic SECURITY DEFINER helper (RCE) See merge request postgres-ai/postgresai!335
2 parents 1494db2 + c65175c commit 09fa3b9

4 files changed

Lines changed: 9 additions & 227 deletions

File tree

‎cli/lib/init.ts‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -924,14 +924,6 @@ export async function verifyInitSetup(params: {
924924
}
925925

926926
// Check for helper functions
927-
const explainFnRes = await params.client.query(
928-
"select has_function_privilege($1, 'postgres_ai.explain_generic(text, text, text)', 'EXECUTE') as ok",
929-
[role]
930-
);
931-
if (!explainFnRes.rows?.[0]?.ok) {
932-
missingRequired.push("EXECUTE on postgres_ai.explain_generic(text, text, text)");
933-
}
934-
935927
const tableDescribeFnRes = await params.client.query(
936928
"select has_function_privilege($1, 'postgres_ai.table_describe(text)', 'EXECUTE') as ok",
937929
[role]

‎cli/lib/supabase.ts‎

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -733,24 +733,6 @@ export async function verifyInitSetupViaSupabase(params: {
733733
}
734734

735735
// Check helper functions - first verify they exist to avoid has_function_privilege errors
736-
const explainFnExistsRes = await params.client.query(
737-
"SELECT oid FROM pg_proc WHERE proname = 'explain_generic' AND pronamespace = (SELECT oid FROM pg_namespace WHERE nspname = 'postgres_ai')",
738-
true
739-
);
740-
if (explainFnExistsRes.rowCount === 0) {
741-
missingRequired.push("function postgres_ai.explain_generic exists");
742-
} else {
743-
const explainFnRes = await params.client.query(
744-
`SELECT has_function_privilege('${escapeLiteral(role)}', 'postgres_ai.explain_generic(text, text, text)', 'EXECUTE') as ok`,
745-
true
746-
);
747-
if (!explainFnRes.rows?.[0]?.ok) {
748-
missingRequired.push(
749-
"EXECUTE on postgres_ai.explain_generic(text, text, text)"
750-
);
751-
}
752-
}
753-
754736
const tableDescribeFnExistsRes = await params.client.query(
755737
"SELECT oid FROM pg_proc WHERE proname = 'table_describe' AND pronamespace = (SELECT oid FROM pg_namespace WHERE nspname = 'postgres_ai')",
756738
true

‎cli/sql/06.helpers.sql‎

Lines changed: 0 additions & 122 deletions
Original file line numberDiff line numberDiff line change
@@ -2,126 +2,6 @@
22
-- These functions use SECURITY DEFINER to allow the monitoring user to perform
33
-- operations they don't have direct permissions for.
44

5-
/*
6-
* explain_generic
7-
*
8-
* Function to get generic explain plans with optional HypoPG index testing.
9-
* Requires: PostgreSQL 16+ (for generic_plan option), HypoPG extension (optional).
10-
*
11-
* Security notes:
12-
* - EXPLAIN without ANALYZE is read-only (plans but doesn't execute the query)
13-
* - PostgreSQL's EXPLAIN only accepts a single statement (primary protection)
14-
* - Input validation uses a simple heuristic to detect multiple statements
15-
* (Note: may reject valid queries containing semicolons in string literals)
16-
*
17-
* Usage examples:
18-
* -- Basic generic plan
19-
* select postgres_ai.explain_generic('select * from users where id = $1');
20-
*
21-
* -- JSON format
22-
* select postgres_ai.explain_generic('select * from users where id = $1', 'json');
23-
*
24-
* -- Test a hypothetical index
25-
* select postgres_ai.explain_generic(
26-
* 'select * from users where email = $1',
27-
* 'text',
28-
* 'create index on users (email)'
29-
* );
30-
*/
31-
create or replace function postgres_ai.explain_generic(
32-
in query text,
33-
in format text default 'text',
34-
in hypopg_index text default null,
35-
out result text
36-
)
37-
language plpgsql
38-
security definer
39-
set search_path = pg_catalog, public
40-
as $$
41-
declare
42-
v_line record;
43-
v_lines text[] := '{}';
44-
v_explain_query text;
45-
v_hypo_result record;
46-
v_version int;
47-
v_hypopg_available boolean;
48-
v_clean_query text;
49-
begin
50-
-- Check PostgreSQL version (generic_plan requires 16+)
51-
select current_setting('server_version_num')::int into v_version;
52-
53-
if v_version < 160000 then
54-
raise exception 'generic_plan requires PostgreSQL 16+, current version: %',
55-
current_setting('server_version');
56-
end if;
57-
58-
-- Input validation: reject empty queries
59-
if query is null or trim(query) = '' then
60-
raise exception 'query cannot be empty';
61-
end if;
62-
63-
-- Input validation: detect multiple statements (defense-in-depth)
64-
-- Note: This is a simple heuristic - EXPLAIN itself only accepts single statements
65-
-- Limitation: Queries with semicolons inside string literals will be rejected
66-
v_clean_query := trim(query);
67-
if v_clean_query like '%;%' then
68-
-- Strip trailing semicolon if present (common user convenience)
69-
v_clean_query := regexp_replace(v_clean_query, ';\s*$', '');
70-
-- If there's still a semicolon, reject (likely multiple statements or semicolon in string)
71-
if v_clean_query like '%;%' then
72-
raise exception 'query contains semicolon (multiple statements not allowed; note: semicolons in string literals are also not supported)';
73-
end if;
74-
end if;
75-
76-
-- Check if HypoPG extension is available
77-
if hypopg_index is not null then
78-
select exists(
79-
select 1 from pg_extension where extname = 'hypopg'
80-
) into v_hypopg_available;
81-
82-
if not v_hypopg_available then
83-
raise exception 'HypoPG extension is required for hypothetical index testing but is not installed';
84-
end if;
85-
86-
-- Create hypothetical index
87-
select * into v_hypo_result from hypopg_create_index(hypopg_index);
88-
raise notice 'Created hypothetical index: % (oid: %)',
89-
v_hypo_result.indexname, v_hypo_result.indexrelid;
90-
end if;
91-
92-
-- Build and execute EXPLAIN query
93-
-- Note: EXPLAIN is read-only (plans but doesn't execute), making this safe
94-
begin
95-
if lower(format) = 'json' then
96-
execute 'explain (verbose, settings, generic_plan, format json) ' || v_clean_query
97-
into result;
98-
else
99-
for v_line in execute 'explain (verbose, settings, generic_plan) ' || v_clean_query loop
100-
v_lines := array_append(v_lines, v_line."QUERY PLAN");
101-
end loop;
102-
result := array_to_string(v_lines, e'\n');
103-
end if;
104-
exception when others then
105-
-- Clean up hypothetical index before re-raising
106-
if hypopg_index is not null then
107-
perform hypopg_reset();
108-
end if;
109-
raise;
110-
end;
111-
112-
-- Clean up hypothetical index
113-
if hypopg_index is not null then
114-
perform hypopg_reset();
115-
end if;
116-
end;
117-
$$;
118-
119-
comment on function postgres_ai.explain_generic(text, text, text) is
120-
'Returns generic EXPLAIN plan with optional HypoPG index testing (requires PG16+)';
121-
122-
-- Grant execute to the monitoring user
123-
grant execute on function postgres_ai.explain_generic(text, text, text) to {{ROLE_IDENT}};
124-
1255
/*
1266
* table_describe
1277
*
@@ -435,5 +315,3 @@ comment on function postgres_ai.table_describe(text) is
435315
'Returns comprehensive table information in compact text format for LLM analysis';
436316

437317
grant execute on function postgres_ai.table_describe(text) to {{ROLE_IDENT}};
438-
439-

‎cli/test/init.integration.test.ts‎

Lines changed: 9 additions & 79 deletions
Original file line numberDiff line numberDiff line change
@@ -407,12 +407,14 @@ describe.skipIf(skipTests)("integration: prepare-db", () => {
407407
}
408408
}, { timeout: TEST_TIMEOUT });
409409

410-
// 60s timeout for PostgreSQL startup + multiple SQL queries in slow CI
411-
test("explain_generic validates input and prevents SQL injection", async () => {
410+
// Security regression for #387: explain_generic was a SECURITY DEFINER helper that
411+
// ran caller-supplied SQL through EXPLAIN. The planner folds IMMUTABLE/STABLE functions
412+
// at plan time in the definer's (superuser) context, making it an RCE/data-exfil vector.
413+
// It has been removed; prepare-db must NOT create it.
414+
test("explain_generic is not created by prepare-db (RCE helper removed, #387)", async () => {
412415
pg = await createTempPostgres();
413416

414417
try {
415-
// Run init first
416418
{
417419
const r = runCliInit([pg.adminUri, "--password", "pw1", "--skip-optional-permissions"]);
418420
expect(r.status).toBe(0);
@@ -422,82 +424,10 @@ describe.skipIf(skipTests)("integration: prepare-db", () => {
422424
await c.connect();
423425

424426
try {
425-
// Check PostgreSQL version - generic_plan requires 16+
426-
const versionRes = await c.query("show server_version_num");
427-
const version = parseInt(versionRes.rows[0].server_version_num, 10);
428-
429-
if (version < 160000) {
430-
// Skip this test on older PostgreSQL versions
431-
console.log("Skipping explain_generic tests: requires PostgreSQL 16+");
432-
return;
433-
}
434-
435-
// Test 1: Empty query should be rejected
436-
await expect(
437-
c.query("select postgres_ai.explain_generic('')")
438-
).rejects.toThrow(/query cannot be empty/);
439-
440-
// Test 2: Null query should be rejected
441-
await expect(
442-
c.query("select postgres_ai.explain_generic(null)")
443-
).rejects.toThrow(/query cannot be empty/);
444-
445-
// Test 3: Multiple statements (semicolon in middle) should be rejected
446-
await expect(
447-
c.query("select postgres_ai.explain_generic('select 1; select 2')")
448-
).rejects.toThrow(/semicolon|multiple statements/i);
449-
450-
// Test 4: Trailing semicolon should be stripped and work
451-
{
452-
const res = await c.query("select postgres_ai.explain_generic('select 1;') as result");
453-
expect(res.rows[0].result).toBeTruthy();
454-
expect(res.rows[0].result).toMatch(/Result/i);
455-
}
456-
457-
// Test 5: Valid query should work
458-
{
459-
const res = await c.query("select postgres_ai.explain_generic('select $1::int', 'text') as result");
460-
expect(res.rows[0].result).toBeTruthy();
461-
}
462-
463-
// Test 6: JSON format should work
464-
{
465-
const res = await c.query("select postgres_ai.explain_generic('select 1', 'json') as result");
466-
const plan = JSON.parse(res.rows[0].result);
467-
expect(Array.isArray(plan)).toBe(true);
468-
expect(plan[0].Plan).toBeTruthy();
469-
}
470-
471-
// Test 7: Whitespace-only query should be rejected
472-
await expect(
473-
c.query("select postgres_ai.explain_generic(' ')")
474-
).rejects.toThrow(/query cannot be empty/);
475-
476-
// Test 8: Semicolon in string literal is rejected (documented limitation)
477-
// Note: This is a known limitation - the simple heuristic cannot parse SQL strings
478-
await expect(
479-
c.query("select postgres_ai.explain_generic('select ''hello;world''')")
480-
).rejects.toThrow(/semicolon/i);
481-
482-
// Test 9: SQL comments should work (no semicolons)
483-
{
484-
const res = await c.query("select postgres_ai.explain_generic('select 1 -- comment') as result");
485-
expect(res.rows[0].result).toBeTruthy();
486-
}
487-
488-
// Test 10: Escaped quotes should work (no semicolons)
489-
{
490-
const res = await c.query("select postgres_ai.explain_generic('select ''test''''s value''') as result");
491-
expect(res.rows[0].result).toBeTruthy();
492-
}
493-
494-
// Test 11: Case-insensitive format parameter
495-
{
496-
const res = await c.query("select postgres_ai.explain_generic('select 1', 'JSON') as result");
497-
const plan = JSON.parse(res.rows[0].result);
498-
expect(Array.isArray(plan)).toBe(true);
499-
}
500-
427+
const res = await c.query(
428+
"select count(*)::int as n from pg_proc where proname = 'explain_generic' and pronamespace = (select oid from pg_namespace where nspname = 'postgres_ai')"
429+
);
430+
expect(res.rows[0].n).toBe(0);
501431
} finally {
502432
await c.end();
503433
}

0 commit comments

Comments
 (0)