diff --git a/bin/installcheck b/bin/installcheck index 659aa2f..3a74aa7 100755 --- a/bin/installcheck +++ b/bin/installcheck @@ -52,7 +52,7 @@ else fi # Execute the test fixtures -psql -v ON_ERROR_STOP= -f test/fixtures.sql -f lints/0001*.sql -f lints/0002*.sql -f lints/0003*.sql -f lints/0004*.sql -f lints/0005*.sql -f lints/0006*.sql -f lints/0007*.sql -f lints/0008*.sql -f lints/0009*.sql -f lints/0010*.sql -f lints/0011*.sql -f lints/0013*.sql -f lints/0014*.sql -f lints/0015*.sql -f lints/0016*.sql -f lints/0017*.sql -f lints/0018*.sql -f lints/0019*.sql -f lints/0020*.sql -f lints/0021*.sql -f lints/0022*.sql -f lints/0023*.sql -f lints/0024*.sql -f lints/0025*.sql -f lints/0026*.sql -f lints/0027*.sql -f lints/0028*.sql -f lints/0029*.sql -f lints/0030*.sql -d contrib_regression +psql -v ON_ERROR_STOP= -f test/fixtures.sql -f lints/0001*.sql -f lints/0002*.sql -f lints/0003*.sql -f lints/0004*.sql -f lints/0005*.sql -f lints/0006*.sql -f lints/0007*.sql -f lints/0008*.sql -f lints/0009*.sql -f lints/0010*.sql -f lints/0011*.sql -f lints/0013*.sql -f lints/0014*.sql -f lints/0015*.sql -f lints/0016*.sql -f lints/0017*.sql -f lints/0018*.sql -f lints/0019*.sql -f lints/0020*.sql -f lints/0021*.sql -f lints/0022*.sql -f lints/0023*.sql -f lints/0024*.sql -f lints/0025*.sql -f lints/0026*.sql -f lints/0027*.sql -f lints/0028*.sql -f lints/0029*.sql -f lints/0030*.sql -f lints/0031*.sql -f lints/0032*.sql -d contrib_regression # Run tests ${REGRESS} --use-existing --dbname=contrib_regression --inputdir=${TESTDIR} ${TESTS} diff --git a/docs/0031_anon_dynamic_sql_function_executable.md b/docs/0031_anon_dynamic_sql_function_executable.md new file mode 100644 index 0000000..19a906f --- /dev/null +++ b/docs/0031_anon_dynamic_sql_function_executable.md @@ -0,0 +1,64 @@ +**Level:** ERROR / WARN + +**Summary:** This function is callable without signing in and appears to `EXECUTE` a SQL string passed in by the caller. + +**Ramification:** The function is reachable through `POST /rest/v1/rpc/`, so anyone with the public anon key can run arbitrary SQL through it. If the function is `SECURITY DEFINER` the SQL runs as the function owner and bypasses RLS. If it is `SECURITY INVOKER` the SQL runs as `anon` and is limited by that role's grants and RLS. The level is `ERROR` for a `SECURITY DEFINER` function whose parameter appears directly in an `EXECUTE`, and `WARN` for everything else, including `SECURITY INVOKER` functions and functions matched only because a parameter has a SQL-like name. Either way the caller can read your schema and any data `anon` can reach. Revoke `EXECUTE`, or replace the function with fixed operations. + +> **See also: lint [0032_authenticated_dynamic_sql_function_executable](0032_authenticated_dynamic_sql_function_executable.md).** It is the same check for signed-in users. Revoking from `anon` alone usually leaves the function callable by every signed-up user, so address both together. Lint [0028_anon_security_definer_function_executable](0028_anon_security_definer_function_executable.md) covers `SECURITY DEFINER` functions exposed to `anon` in general. + +--- + +### Rationale + +Helpers like `exec_sql(query text)` or `run_query(sql text)` let a client send SQL to the database and get the result back. Postgres grants `EXECUTE` on new functions to `PUBLIC`, and Supabase also grants it to `anon` and `authenticated`, so a function like this is callable by clients unless `EXECUTE` is revoked. Checking that the string starts with `select` does not make it safe. + +This lint reads the function body, and only for PL/pgSQL functions in exposed schemas. A function is flagged when: + +1. a `text` or `varchar` parameter (or an array of either, including `VARIADIC`) is used inside an `EXECUTE` statement, or +2. a `text` or `varchar` parameter (or an array of either, including `VARIADIC`) is named like SQL (`sql`, `query`, `stmt`, `statement`, `command`, `cmd`, ...) and the body contains an `EXECUTE` anywhere. + +Parameters bound with `EXECUTE ... USING` and parameters wrapped in `quote_ident()`, `quote_literal()` or `quote_nullable()` are not counted. + +### How to Resolve + +**Option 1: Revoke `EXECUTE`** + +```sql +revoke execute on function public.exec_sql(text) from anon, public; +``` + +**Option 2: Drop the function** + +```sql +drop function public.exec_sql(text); +``` + +**Option 3: Replace it with fixed operations** + +Expose one function per operation with typed arguments and no string-built SQL. If you have to build SQL, pass values with `EXECUTE ... USING`, quote identifiers with `quote_ident()`, and check table and column names against an allow-list. + +### Example + +Given this function: + +```sql +create function public.exec_sql(query text) returns json +language plpgsql security definer as $$ +declare r json; +begin + execute 'select json_agg(t) from (' || query || ') t' into r; + return r; +end $$; +``` + +Fix: + +```sql +revoke execute on function public.exec_sql(text) from anon, authenticated, public; +``` + +### False Positives + +- A `text` parameter used as an identifier through `format('%I', param)` is safe but can still be flagged. Use `quote_ident(param)` instead. +- A function that checks the string against a strict allow-list before running it. Confirm the allow-list is complete, then suppress the finding. +- Only PL/pgSQL bodies are inspected. Functions in other languages, or that build SQL in a helper function, are not detected. diff --git a/docs/0032_authenticated_dynamic_sql_function_executable.md b/docs/0032_authenticated_dynamic_sql_function_executable.md new file mode 100644 index 0000000..c682cb1 --- /dev/null +++ b/docs/0032_authenticated_dynamic_sql_function_executable.md @@ -0,0 +1,64 @@ +**Level:** ERROR / WARN + +**Summary:** This function is callable by signed-in users and appears to `EXECUTE` a SQL string passed in by the caller. + +**Ramification:** The function is reachable through `POST /rest/v1/rpc/`, so any signed-in user (with open signup, anyone with a throwaway account) can run arbitrary SQL through it. If the function is `SECURITY DEFINER` the SQL runs as the function owner and bypasses RLS. If it is `SECURITY INVOKER` the SQL runs as `authenticated` and is limited by that role's grants and RLS. The level is `ERROR` for a `SECURITY DEFINER` function whose parameter appears directly in an `EXECUTE`, and `WARN` for everything else, including `SECURITY INVOKER` functions and functions matched only because a parameter has a SQL-like name. Either way the caller can read your schema and any data `authenticated` can reach. Revoke `EXECUTE`, or replace the function with fixed operations. + +> **See also: lint [0031_anon_dynamic_sql_function_executable](0031_anon_dynamic_sql_function_executable.md).** It is the same check for `anon`. Revoking from `authenticated` alone can leave the function callable without signing in, so address both together. Lint [0029_authenticated_security_definer_function_executable](0029_authenticated_security_definer_function_executable.md) covers `SECURITY DEFINER` functions exposed to `authenticated` in general. + +--- + +### Rationale + +Helpers like `exec_sql(query text)` or `run_query(sql text)` let a client send SQL to the database and get the result back. Postgres grants `EXECUTE` on new functions to `PUBLIC`, and Supabase also grants it to `anon` and `authenticated`, so a function like this is callable by clients unless `EXECUTE` is revoked. Checking that the string starts with `select` does not make it safe. + +This lint reads the function body, and only for PL/pgSQL functions in exposed schemas. A function is flagged when: + +1. a `text` or `varchar` parameter (or an array of either, including `VARIADIC`) is used inside an `EXECUTE` statement, or +2. a `text` or `varchar` parameter (or an array of either, including `VARIADIC`) is named like SQL (`sql`, `query`, `stmt`, `statement`, `command`, `cmd`, ...) and the body contains an `EXECUTE` anywhere. + +Parameters bound with `EXECUTE ... USING` and parameters wrapped in `quote_ident()`, `quote_literal()` or `quote_nullable()` are not counted. + +### How to Resolve + +**Option 1: Revoke `EXECUTE`** + +```sql +revoke execute on function public.exec_sql(text) from authenticated, public; +``` + +**Option 2: Drop the function** + +```sql +drop function public.exec_sql(text); +``` + +**Option 3: Replace it with fixed operations** + +Expose one function per operation with typed arguments and no string-built SQL. If you have to build SQL, pass values with `EXECUTE ... USING`, quote identifiers with `quote_ident()`, and check table and column names against an allow-list. + +### Example + +Given this function: + +```sql +create function public.exec_sql(query text) returns json +language plpgsql security definer as $$ +declare r json; +begin + execute 'select json_agg(t) from (' || query || ') t' into r; + return r; +end $$; +``` + +Fix: + +```sql +revoke execute on function public.exec_sql(text) from anon, authenticated, public; +``` + +### False Positives + +- A `text` parameter used as an identifier through `format('%I', param)` is safe but can still be flagged. Use `quote_ident(param)` instead. +- A function that checks the string against a strict allow-list before running it. Confirm the allow-list is complete, then suppress the finding. +- Only PL/pgSQL bodies are inspected. Functions in other languages, or that build SQL in a helper function, are not detected. diff --git a/lints/0031_anon_dynamic_sql_function_executable.sql b/lints/0031_anon_dynamic_sql_function_executable.sql new file mode 100644 index 0000000..b60a8a4 --- /dev/null +++ b/lints/0031_anon_dynamic_sql_function_executable.sql @@ -0,0 +1,231 @@ +create view lint."0031_anon_dynamic_sql_function_executable" as + +-- Detects PL/pgSQL functions that the `anon` role can EXECUTE and that +-- appear to EXECUTE a SQL string passed in by the caller, e.g. +-- `exec_sql(query text)`. PostgREST exposes them at +-- `/rest/v1/rpc/`, so anyone with the anon key can run arbitrary SQL as the +-- function owner (SECURITY DEFINER) or as `anon` (SECURITY INVOKER). +-- +-- A function is flagged when either: +-- 1. a text/varchar parameter (or array of either) is used inside an EXECUTE statement, +-- or in an expression assigned to a variable (`v := ... || param`) that +-- is then used in an EXECUTE (EXECUTE ... USING bound values and +-- quote_ident/quote_literal/quote_nullable(param) are ignored), or +-- 2. a text/varchar parameter (or array of either) has a SQL-like name (sql, query, stmt, +-- ...) and the body contains an EXECUTE anywhere. +-- +-- Level is ERROR for SECURITY DEFINER functions matching (1), WARN +-- otherwise. Unlike 0028/0029 this also covers SECURITY INVOKER. +-- +-- See lint 0032 for the equivalent check against `authenticated`. +with candidates as ( + select + n.nspname as schema_name, + p.proname as function_name, + pg_catalog.pg_get_function_identity_arguments(p.oid) as function_args, + l.lanname as function_language, + p.prosecdef as is_security_definer, + pg_catalog.pg_get_userbyid(p.proowner) as owner_name, + regexp_replace(lower(p.prosrc), '--[^\n]*', '', 'g') as body, + p.proargnames, + p.proargmodes, + coalesce(p.proallargtypes, p.proargtypes::pg_catalog.oid[]) as all_arg_types + from + pg_catalog.pg_proc p + join pg_catalog.pg_namespace n + on p.pronamespace = n.oid + join pg_catalog.pg_language l + on p.prolang = l.oid + where + l.lanname = 'plpgsql' + and pg_catalog.has_function_privilege('anon', p.oid, 'EXECUTE') + and n.nspname = any(array(select trim(unnest(string_to_array(coalesce(current_setting('pgrst.db_schemas', 't'), 'public'), ','))))) + and n.nspname not in ( + '_timescaledb_cache', '_timescaledb_catalog', '_timescaledb_config', '_timescaledb_internal', 'auth', 'cron', 'extensions', 'graphql', 'graphql_public', 'information_schema', 'net', 'pgmq', 'pgroonga', 'pgsodium', 'pgsodium_masks', 'pgtle', 'pgbouncer', 'pg_catalog', 'realtime', 'repack', 'storage', 'supabase_functions', 'supabase_migrations', 'tiger', 'topology', 'vault' + ) + -- exclude functions that belong to an extension + and not exists ( + select 1 + from pg_catalog.pg_depend d + where d.objid = p.oid + and d.classid = 'pg_catalog.pg_proc'::pg_catalog.regclass + and d.deptype = 'e' + ) + and lower(p.prosrc) like '%execute%' +), +text_params as ( + select + c.*, + ip.pos as arg_pos, + lower(coalesce(nullif(c.proargnames[a.ord], ''), '$' || ip.pos)) as arg_name + from + candidates c + cross join lateral unnest(c.all_arg_types) with ordinality as a(typ, ord) + -- position among input arguments, which is what `$N` refers to + cross join lateral ( + select count(*) as pos + from generate_series(1, a.ord::int) g + where coalesce(c.proargmodes[g]::text, 'i') in ('i', 'b', 'v') + ) ip + where + coalesce(c.proargmodes[a.ord]::text, 'i') in ('i', 'b', 'v') + and a.typ in ( + 'pg_catalog.text'::pg_catalog.regtype, + 'pg_catalog.varchar'::pg_catalog.regtype, + 'pg_catalog.text[]'::pg_catalog.regtype, + 'pg_catalog.varchar[]'::pg_catalog.regtype + ) +), +escaped as ( + select + tp.*, + -- match the parameter by name or by position (`$N` also works for + -- named parameters in PL/pgSQL) + '(?:' || regexp_replace(tp.arg_name, '([^a-z0-9_])', '\\\1', 'g') || '|\$' || tp.arg_pos || ')' as arg_re + from + text_params tp +), +formatted as ( + select + e.*, + -- a parameter passed to format() in a %I or %L slot is quoted, so + -- it is replaced with `null` (like quote_ident/quote_literal). + -- Only format() calls with simple, comma-free arguments and + -- sequential specifiers (no `%1$s`) are recognised; anything else + -- is left in place and counts as a use of the parameter. + regexp_replace( + e.body, + '(?<=(?:' || ( + select string_agg( + 'format\s*\(\s*''(?:[^''%]|%%|'''')*(?:%[-0-9]*[sil](?:[^''%]|%%|'''')*){' || (k - 1) || '}%[-0-9]*[il](?:[^'']|'''')*''\s*(?:,[^,()]+){' || (k - 1) || '},\s*', + '|' + ) + from generate_series(1, 8) k + ) || '))' || e.arg_re || '(?=\s*[,)])', + 'null', + 'g' + ) as fmt_body + from + escaped e +), +cleaned as ( + select + f.*, + -- string literals blanked out (so keywords and semicolons inside + -- them are not mistaken for code), `USING` bound values removed + -- (only a real USING clause is left to match), and quote_*(param) + -- wrappers removed + regexp_replace( + regexp_replace( + regexp_replace( + regexp_replace( + f.fmt_body, + '\$([a-z_]*?)\$.*?\$\1\$', + '''''', + 'g' + ), + '''([^'']|'''')*''', + '''''', + 'g' + ), + '\musing\s[^\x3b]*', + '', + 'g' + ), + 'quote_(ident|literal|nullable)\s*\(\s*' || f.arg_re || '\s*\)', + '', + 'g' + ) as exec_body + from + formatted f +), +matches as ( + select + cl.schema_name, + cl.function_name, + cl.function_args, + cl.function_language, + cl.is_security_definer, + cl.owner_name, + cl.arg_name, + ( + -- the parameter is used inside an EXECUTE statement + cl.exec_body ~ ('\mexecute\M[^\x3b]*[^a-z0-9_$]' || cl.arg_re || '([^a-z0-9_]|$)') + -- or a variable assigned from the parameter (`v := ... || param` + -- or `declare v text := ... || param`) is used in an EXECUTE + or exists ( + select 1 + from regexp_matches( + cl.exec_body, + '(?:([a-z_][a-z0-9_]*)\s+(?:text|varchar(?:\(\d+\))?|character\s+varying(?:\(\d+\))?)|([a-z_][a-z0-9_]*))\s*:=([^\x3b]*[^a-z0-9_$])?' || cl.arg_re || '([^a-z0-9_]|$)', + 'g' + ) as m + where cl.exec_body ~ ('\mexecute\M[^\x3b]*[^a-z0-9_$]' || coalesce(m[1], m[2]) || '([^a-z0-9_]|$)') + ) + ) as direct_use, + cl.arg_name in ('sql', 'query', 'queries', 'stmt', 'statement', 'command', 'cmd', 'sql_text', 'sql_query', 'sql_string', 'query_text', 'query_string', 'raw_sql', 'raw_query') + and cl.exec_body ~ '\mexecute\M' as renamed_use + from + cleaned cl +), +flagged as ( + select + schema_name, + function_name, + function_args, + function_language, + is_security_definer, + owner_name, + bool_or(direct_use) as has_direct_use, + array_agg(arg_name order by arg_name) as sql_parameters + from + matches + where + direct_use or renamed_use + group by + schema_name, + function_name, + function_args, + function_language, + is_security_definer, + owner_name +) +select + 'anon_dynamic_sql_function_executable' as name, + 'Public Can Execute Dynamic SQL Function' as title, + case when is_security_definer and has_direct_use then 'ERROR' else 'WARN' end as level, + 'EXTERNAL' as facing, + array['SECURITY'] as categories, + 'Detects functions callable without signing in that appear to run a SQL string passed in by the caller. Revoke `EXECUTE` or replace the function with fixed operations.' as description, + format( + 'Function `%s.%s(%s)` is `SECURITY %s`, can be executed by the `anon` role via `/rest/v1/rpc/%s`, and appears to run SQL passed in `%s`. Revoke `EXECUTE` if that is not intentional.', + schema_name, + function_name, + function_args, + case when is_security_definer then 'DEFINER' else 'INVOKER' end, + function_name, + array_to_string(sql_parameters, ', ') + ) as detail, + 'https://supabase.com/docs/guides/database/database-linter?lint=0031_anon_dynamic_sql_function_executable' as remediation, + jsonb_build_object( + 'schema', schema_name, + 'name', function_name, + 'type', 'function', + 'arguments', function_args, + 'language', function_language, + 'security_definer', is_security_definer, + 'owner', owner_name, + 'sql_parameters', sql_parameters + ) as metadata, + format( + 'anon_dynamic_sql_function_executable_%s_%s_%s', + schema_name, + function_name, + function_args + ) as cache_key +from + flagged +order by + schema_name, + function_name, + function_args; diff --git a/lints/0032_authenticated_dynamic_sql_function_executable.sql b/lints/0032_authenticated_dynamic_sql_function_executable.sql new file mode 100644 index 0000000..63df83b --- /dev/null +++ b/lints/0032_authenticated_dynamic_sql_function_executable.sql @@ -0,0 +1,231 @@ +create view lint."0032_authenticated_dynamic_sql_function_executable" as + +-- Detects PL/pgSQL functions that the `authenticated` role can EXECUTE and that +-- appear to EXECUTE a SQL string passed in by the caller, e.g. +-- `exec_sql(query text)`. PostgREST exposes them at +-- `/rest/v1/rpc/`, so any signed-in user can run arbitrary SQL as the +-- function owner (SECURITY DEFINER) or as `authenticated` (SECURITY INVOKER). +-- +-- A function is flagged when either: +-- 1. a text/varchar parameter (or array of either) is used inside an EXECUTE statement, +-- or in an expression assigned to a variable (`v := ... || param`) that +-- is then used in an EXECUTE (EXECUTE ... USING bound values and +-- quote_ident/quote_literal/quote_nullable(param) are ignored), or +-- 2. a text/varchar parameter (or array of either) has a SQL-like name (sql, query, stmt, +-- ...) and the body contains an EXECUTE anywhere. +-- +-- Level is ERROR for SECURITY DEFINER functions matching (1), WARN +-- otherwise. Unlike 0028/0029 this also covers SECURITY INVOKER. +-- +-- See lint 0031 for the equivalent check against `anon`. +with candidates as ( + select + n.nspname as schema_name, + p.proname as function_name, + pg_catalog.pg_get_function_identity_arguments(p.oid) as function_args, + l.lanname as function_language, + p.prosecdef as is_security_definer, + pg_catalog.pg_get_userbyid(p.proowner) as owner_name, + regexp_replace(lower(p.prosrc), '--[^\n]*', '', 'g') as body, + p.proargnames, + p.proargmodes, + coalesce(p.proallargtypes, p.proargtypes::pg_catalog.oid[]) as all_arg_types + from + pg_catalog.pg_proc p + join pg_catalog.pg_namespace n + on p.pronamespace = n.oid + join pg_catalog.pg_language l + on p.prolang = l.oid + where + l.lanname = 'plpgsql' + and pg_catalog.has_function_privilege('authenticated', p.oid, 'EXECUTE') + and n.nspname = any(array(select trim(unnest(string_to_array(coalesce(current_setting('pgrst.db_schemas', 't'), 'public'), ','))))) + and n.nspname not in ( + '_timescaledb_cache', '_timescaledb_catalog', '_timescaledb_config', '_timescaledb_internal', 'auth', 'cron', 'extensions', 'graphql', 'graphql_public', 'information_schema', 'net', 'pgmq', 'pgroonga', 'pgsodium', 'pgsodium_masks', 'pgtle', 'pgbouncer', 'pg_catalog', 'realtime', 'repack', 'storage', 'supabase_functions', 'supabase_migrations', 'tiger', 'topology', 'vault' + ) + -- exclude functions that belong to an extension + and not exists ( + select 1 + from pg_catalog.pg_depend d + where d.objid = p.oid + and d.classid = 'pg_catalog.pg_proc'::pg_catalog.regclass + and d.deptype = 'e' + ) + and lower(p.prosrc) like '%execute%' +), +text_params as ( + select + c.*, + ip.pos as arg_pos, + lower(coalesce(nullif(c.proargnames[a.ord], ''), '$' || ip.pos)) as arg_name + from + candidates c + cross join lateral unnest(c.all_arg_types) with ordinality as a(typ, ord) + -- position among input arguments, which is what `$N` refers to + cross join lateral ( + select count(*) as pos + from generate_series(1, a.ord::int) g + where coalesce(c.proargmodes[g]::text, 'i') in ('i', 'b', 'v') + ) ip + where + coalesce(c.proargmodes[a.ord]::text, 'i') in ('i', 'b', 'v') + and a.typ in ( + 'pg_catalog.text'::pg_catalog.regtype, + 'pg_catalog.varchar'::pg_catalog.regtype, + 'pg_catalog.text[]'::pg_catalog.regtype, + 'pg_catalog.varchar[]'::pg_catalog.regtype + ) +), +escaped as ( + select + tp.*, + -- match the parameter by name or by position (`$N` also works for + -- named parameters in PL/pgSQL) + '(?:' || regexp_replace(tp.arg_name, '([^a-z0-9_])', '\\\1', 'g') || '|\$' || tp.arg_pos || ')' as arg_re + from + text_params tp +), +formatted as ( + select + e.*, + -- a parameter passed to format() in a %I or %L slot is quoted, so + -- it is replaced with `null` (like quote_ident/quote_literal). + -- Only format() calls with simple, comma-free arguments and + -- sequential specifiers (no `%1$s`) are recognised; anything else + -- is left in place and counts as a use of the parameter. + regexp_replace( + e.body, + '(?<=(?:' || ( + select string_agg( + 'format\s*\(\s*''(?:[^''%]|%%|'''')*(?:%[-0-9]*[sil](?:[^''%]|%%|'''')*){' || (k - 1) || '}%[-0-9]*[il](?:[^'']|'''')*''\s*(?:,[^,()]+){' || (k - 1) || '},\s*', + '|' + ) + from generate_series(1, 8) k + ) || '))' || e.arg_re || '(?=\s*[,)])', + 'null', + 'g' + ) as fmt_body + from + escaped e +), +cleaned as ( + select + f.*, + -- string literals blanked out (so keywords and semicolons inside + -- them are not mistaken for code), `USING` bound values removed + -- (only a real USING clause is left to match), and quote_*(param) + -- wrappers removed + regexp_replace( + regexp_replace( + regexp_replace( + regexp_replace( + f.fmt_body, + '\$([a-z_]*?)\$.*?\$\1\$', + '''''', + 'g' + ), + '''([^'']|'''')*''', + '''''', + 'g' + ), + '\musing\s[^\x3b]*', + '', + 'g' + ), + 'quote_(ident|literal|nullable)\s*\(\s*' || f.arg_re || '\s*\)', + '', + 'g' + ) as exec_body + from + formatted f +), +matches as ( + select + cl.schema_name, + cl.function_name, + cl.function_args, + cl.function_language, + cl.is_security_definer, + cl.owner_name, + cl.arg_name, + ( + -- the parameter is used inside an EXECUTE statement + cl.exec_body ~ ('\mexecute\M[^\x3b]*[^a-z0-9_$]' || cl.arg_re || '([^a-z0-9_]|$)') + -- or a variable assigned from the parameter (`v := ... || param` + -- or `declare v text := ... || param`) is used in an EXECUTE + or exists ( + select 1 + from regexp_matches( + cl.exec_body, + '(?:([a-z_][a-z0-9_]*)\s+(?:text|varchar(?:\(\d+\))?|character\s+varying(?:\(\d+\))?)|([a-z_][a-z0-9_]*))\s*:=([^\x3b]*[^a-z0-9_$])?' || cl.arg_re || '([^a-z0-9_]|$)', + 'g' + ) as m + where cl.exec_body ~ ('\mexecute\M[^\x3b]*[^a-z0-9_$]' || coalesce(m[1], m[2]) || '([^a-z0-9_]|$)') + ) + ) as direct_use, + cl.arg_name in ('sql', 'query', 'queries', 'stmt', 'statement', 'command', 'cmd', 'sql_text', 'sql_query', 'sql_string', 'query_text', 'query_string', 'raw_sql', 'raw_query') + and cl.exec_body ~ '\mexecute\M' as renamed_use + from + cleaned cl +), +flagged as ( + select + schema_name, + function_name, + function_args, + function_language, + is_security_definer, + owner_name, + bool_or(direct_use) as has_direct_use, + array_agg(arg_name order by arg_name) as sql_parameters + from + matches + where + direct_use or renamed_use + group by + schema_name, + function_name, + function_args, + function_language, + is_security_definer, + owner_name +) +select + 'authenticated_dynamic_sql_function_executable' as name, + 'Signed-In Users Can Execute Dynamic SQL Function' as title, + case when is_security_definer and has_direct_use then 'ERROR' else 'WARN' end as level, + 'EXTERNAL' as facing, + array['SECURITY'] as categories, + 'Detects functions callable by signed-in users that appear to run a SQL string passed in by the caller. Revoke `EXECUTE` or replace the function with fixed operations.' as description, + format( + 'Function `%s.%s(%s)` is `SECURITY %s`, can be executed by the `authenticated` role via `/rest/v1/rpc/%s`, and appears to run SQL passed in `%s`. Revoke `EXECUTE` if that is not intentional.', + schema_name, + function_name, + function_args, + case when is_security_definer then 'DEFINER' else 'INVOKER' end, + function_name, + array_to_string(sql_parameters, ', ') + ) as detail, + 'https://supabase.com/docs/guides/database/database-linter?lint=0032_authenticated_dynamic_sql_function_executable' as remediation, + jsonb_build_object( + 'schema', schema_name, + 'name', function_name, + 'type', 'function', + 'arguments', function_args, + 'language', function_language, + 'security_definer', is_security_definer, + 'owner', owner_name, + 'sql_parameters', sql_parameters + ) as metadata, + format( + 'authenticated_dynamic_sql_function_executable_%s_%s_%s', + schema_name, + function_name, + function_args + ) as cache_key +from + flagged +order by + schema_name, + function_name, + function_args; diff --git a/mkdocs.yaml b/mkdocs.yaml index 02864ac..bf45eb5 100644 --- a/mkdocs.yaml +++ b/mkdocs.yaml @@ -38,6 +38,8 @@ nav: - Public Can Execute SECURITY DEFINER Function: '0028_anon_security_definer_function_executable.md' - Signed-In Users Can Execute SECURITY DEFINER Function: '0029_authenticated_security_definer_function_executable.md' - Autovacuum Disabled: '0030_autovacuum_disabled.md' + - Public Can Execute Dynamic SQL Function: '0031_anon_dynamic_sql_function_executable.md' + - Signed-In Users Can Execute Dynamic SQL Function: '0032_authenticated_dynamic_sql_function_executable.md' theme: name: 'material' diff --git a/splinter.sql b/splinter.sql index 46bb331..dd36e57 100644 --- a/splinter.sql +++ b/splinter.sql @@ -1874,4 +1874,466 @@ where ) order by nsp.nspname, - cls.relname) \ No newline at end of file + cls.relname) +union all +( +-- Detects PL/pgSQL functions that the `anon` role can EXECUTE and that +-- appear to EXECUTE a SQL string passed in by the caller, e.g. +-- `exec_sql(query text)`. PostgREST exposes them at +-- `/rest/v1/rpc/`, so anyone with the anon key can run arbitrary SQL as the +-- function owner (SECURITY DEFINER) or as `anon` (SECURITY INVOKER). +-- +-- A function is flagged when either: +-- 1. a text/varchar parameter (or array of either) is used inside an EXECUTE statement, +-- or in an expression assigned to a variable (`v := ... || param`) that +-- is then used in an EXECUTE (EXECUTE ... USING bound values and +-- quote_ident/quote_literal/quote_nullable(param) are ignored), or +-- 2. a text/varchar parameter (or array of either) has a SQL-like name (sql, query, stmt, +-- ...) and the body contains an EXECUTE anywhere. +-- +-- Level is ERROR for SECURITY DEFINER functions matching (1), WARN +-- otherwise. Unlike 0028/0029 this also covers SECURITY INVOKER. +-- +-- See lint 0032 for the equivalent check against `authenticated`. +with candidates as ( + select + n.nspname as schema_name, + p.proname as function_name, + pg_catalog.pg_get_function_identity_arguments(p.oid) as function_args, + l.lanname as function_language, + p.prosecdef as is_security_definer, + pg_catalog.pg_get_userbyid(p.proowner) as owner_name, + regexp_replace(lower(p.prosrc), '--[^\n]*', '', 'g') as body, + p.proargnames, + p.proargmodes, + coalesce(p.proallargtypes, p.proargtypes::pg_catalog.oid[]) as all_arg_types + from + pg_catalog.pg_proc p + join pg_catalog.pg_namespace n + on p.pronamespace = n.oid + join pg_catalog.pg_language l + on p.prolang = l.oid + where + l.lanname = 'plpgsql' + and pg_catalog.has_function_privilege('anon', p.oid, 'EXECUTE') + and n.nspname = any(array(select trim(unnest(string_to_array(coalesce(current_setting('pgrst.db_schemas', 't'), 'public'), ','))))) + and n.nspname not in ( + '_timescaledb_cache', '_timescaledb_catalog', '_timescaledb_config', '_timescaledb_internal', 'auth', 'cron', 'extensions', 'graphql', 'graphql_public', 'information_schema', 'net', 'pgmq', 'pgroonga', 'pgsodium', 'pgsodium_masks', 'pgtle', 'pgbouncer', 'pg_catalog', 'realtime', 'repack', 'storage', 'supabase_functions', 'supabase_migrations', 'tiger', 'topology', 'vault' + ) + -- exclude functions that belong to an extension + and not exists ( + select 1 + from pg_catalog.pg_depend d + where d.objid = p.oid + and d.classid = 'pg_catalog.pg_proc'::pg_catalog.regclass + and d.deptype = 'e' + ) + and lower(p.prosrc) like '%execute%' +), +text_params as ( + select + c.*, + ip.pos as arg_pos, + lower(coalesce(nullif(c.proargnames[a.ord], ''), '$' || ip.pos)) as arg_name + from + candidates c + cross join lateral unnest(c.all_arg_types) with ordinality as a(typ, ord) + -- position among input arguments, which is what `$N` refers to + cross join lateral ( + select count(*) as pos + from generate_series(1, a.ord::int) g + where coalesce(c.proargmodes[g]::text, 'i') in ('i', 'b', 'v') + ) ip + where + coalesce(c.proargmodes[a.ord]::text, 'i') in ('i', 'b', 'v') + and a.typ in ( + 'pg_catalog.text'::pg_catalog.regtype, + 'pg_catalog.varchar'::pg_catalog.regtype, + 'pg_catalog.text[]'::pg_catalog.regtype, + 'pg_catalog.varchar[]'::pg_catalog.regtype + ) +), +escaped as ( + select + tp.*, + -- match the parameter by name or by position (`$N` also works for + -- named parameters in PL/pgSQL) + '(?:' || regexp_replace(tp.arg_name, '([^a-z0-9_])', '\\\1', 'g') || '|\$' || tp.arg_pos || ')' as arg_re + from + text_params tp +), +formatted as ( + select + e.*, + -- a parameter passed to format() in a %I or %L slot is quoted, so + -- it is replaced with `null` (like quote_ident/quote_literal). + -- Only format() calls with simple, comma-free arguments and + -- sequential specifiers (no `%1$s`) are recognised anything else + -- is left in place and counts as a use of the parameter. + regexp_replace( + e.body, + '(?<=(?:' || ( + select string_agg( + 'format\s*\(\s*''(?:[^''%]|%%|'''')*(?:%[-0-9]*[sil](?:[^''%]|%%|'''')*){' || (k - 1) || '}%[-0-9]*[il](?:[^'']|'''')*''\s*(?:,[^,()]+){' || (k - 1) || '},\s*', + '|' + ) + from generate_series(1, 8) k + ) || '))' || e.arg_re || '(?=\s*[,)])', + 'null', + 'g' + ) as fmt_body + from + escaped e +), +cleaned as ( + select + f.*, + -- string literals blanked out (so keywords and semicolons inside + -- them are not mistaken for code), `USING` bound values removed + -- (only a real USING clause is left to match), and quote_*(param) + -- wrappers removed + regexp_replace( + regexp_replace( + regexp_replace( + regexp_replace( + f.fmt_body, + '\$([a-z_]*?)\$.*?\$\1\$', + '''''', + 'g' + ), + '''([^'']|'''')*''', + '''''', + 'g' + ), + '\musing\s[^\x3b]*', + '', + 'g' + ), + 'quote_(ident|literal|nullable)\s*\(\s*' || f.arg_re || '\s*\)', + '', + 'g' + ) as exec_body + from + formatted f +), +matches as ( + select + cl.schema_name, + cl.function_name, + cl.function_args, + cl.function_language, + cl.is_security_definer, + cl.owner_name, + cl.arg_name, + ( + -- the parameter is used inside an EXECUTE statement + cl.exec_body ~ ('\mexecute\M[^\x3b]*[^a-z0-9_$]' || cl.arg_re || '([^a-z0-9_]|$)') + -- or a variable assigned from the parameter (`v := ... || param` + -- or `declare v text := ... || param`) is used in an EXECUTE + or exists ( + select 1 + from regexp_matches( + cl.exec_body, + '(?:([a-z_][a-z0-9_]*)\s+(?:text|varchar(?:\(\d+\))?|character\s+varying(?:\(\d+\))?)|([a-z_][a-z0-9_]*))\s*:=([^\x3b]*[^a-z0-9_$])?' || cl.arg_re || '([^a-z0-9_]|$)', + 'g' + ) as m + where cl.exec_body ~ ('\mexecute\M[^\x3b]*[^a-z0-9_$]' || coalesce(m[1], m[2]) || '([^a-z0-9_]|$)') + ) + ) as direct_use, + cl.arg_name in ('sql', 'query', 'queries', 'stmt', 'statement', 'command', 'cmd', 'sql_text', 'sql_query', 'sql_string', 'query_text', 'query_string', 'raw_sql', 'raw_query') + and cl.exec_body ~ '\mexecute\M' as renamed_use + from + cleaned cl +), +flagged as ( + select + schema_name, + function_name, + function_args, + function_language, + is_security_definer, + owner_name, + bool_or(direct_use) as has_direct_use, + array_agg(arg_name order by arg_name) as sql_parameters + from + matches + where + direct_use or renamed_use + group by + schema_name, + function_name, + function_args, + function_language, + is_security_definer, + owner_name +) +select + 'anon_dynamic_sql_function_executable' as name, + 'Public Can Execute Dynamic SQL Function' as title, + case when is_security_definer and has_direct_use then 'ERROR' else 'WARN' end as level, + 'EXTERNAL' as facing, + array['SECURITY'] as categories, + 'Detects functions callable without signing in that appear to run a SQL string passed in by the caller. Revoke `EXECUTE` or replace the function with fixed operations.' as description, + format( + 'Function `%s.%s(%s)` is `SECURITY %s`, can be executed by the `anon` role via `/rest/v1/rpc/%s`, and appears to run SQL passed in `%s`. Revoke `EXECUTE` if that is not intentional.', + schema_name, + function_name, + function_args, + case when is_security_definer then 'DEFINER' else 'INVOKER' end, + function_name, + array_to_string(sql_parameters, ', ') + ) as detail, + 'https://supabase.com/docs/guides/database/database-linter?lint=0031_anon_dynamic_sql_function_executable' as remediation, + jsonb_build_object( + 'schema', schema_name, + 'name', function_name, + 'type', 'function', + 'arguments', function_args, + 'language', function_language, + 'security_definer', is_security_definer, + 'owner', owner_name, + 'sql_parameters', sql_parameters + ) as metadata, + format( + 'anon_dynamic_sql_function_executable_%s_%s_%s', + schema_name, + function_name, + function_args + ) as cache_key +from + flagged +order by + schema_name, + function_name, + function_args) +union all +( +-- Detects PL/pgSQL functions that the `authenticated` role can EXECUTE and that +-- appear to EXECUTE a SQL string passed in by the caller, e.g. +-- `exec_sql(query text)`. PostgREST exposes them at +-- `/rest/v1/rpc/`, so any signed-in user can run arbitrary SQL as the +-- function owner (SECURITY DEFINER) or as `authenticated` (SECURITY INVOKER). +-- +-- A function is flagged when either: +-- 1. a text/varchar parameter (or array of either) is used inside an EXECUTE statement, +-- or in an expression assigned to a variable (`v := ... || param`) that +-- is then used in an EXECUTE (EXECUTE ... USING bound values and +-- quote_ident/quote_literal/quote_nullable(param) are ignored), or +-- 2. a text/varchar parameter (or array of either) has a SQL-like name (sql, query, stmt, +-- ...) and the body contains an EXECUTE anywhere. +-- +-- Level is ERROR for SECURITY DEFINER functions matching (1), WARN +-- otherwise. Unlike 0028/0029 this also covers SECURITY INVOKER. +-- +-- See lint 0031 for the equivalent check against `anon`. +with candidates as ( + select + n.nspname as schema_name, + p.proname as function_name, + pg_catalog.pg_get_function_identity_arguments(p.oid) as function_args, + l.lanname as function_language, + p.prosecdef as is_security_definer, + pg_catalog.pg_get_userbyid(p.proowner) as owner_name, + regexp_replace(lower(p.prosrc), '--[^\n]*', '', 'g') as body, + p.proargnames, + p.proargmodes, + coalesce(p.proallargtypes, p.proargtypes::pg_catalog.oid[]) as all_arg_types + from + pg_catalog.pg_proc p + join pg_catalog.pg_namespace n + on p.pronamespace = n.oid + join pg_catalog.pg_language l + on p.prolang = l.oid + where + l.lanname = 'plpgsql' + and pg_catalog.has_function_privilege('authenticated', p.oid, 'EXECUTE') + and n.nspname = any(array(select trim(unnest(string_to_array(coalesce(current_setting('pgrst.db_schemas', 't'), 'public'), ','))))) + and n.nspname not in ( + '_timescaledb_cache', '_timescaledb_catalog', '_timescaledb_config', '_timescaledb_internal', 'auth', 'cron', 'extensions', 'graphql', 'graphql_public', 'information_schema', 'net', 'pgmq', 'pgroonga', 'pgsodium', 'pgsodium_masks', 'pgtle', 'pgbouncer', 'pg_catalog', 'realtime', 'repack', 'storage', 'supabase_functions', 'supabase_migrations', 'tiger', 'topology', 'vault' + ) + -- exclude functions that belong to an extension + and not exists ( + select 1 + from pg_catalog.pg_depend d + where d.objid = p.oid + and d.classid = 'pg_catalog.pg_proc'::pg_catalog.regclass + and d.deptype = 'e' + ) + and lower(p.prosrc) like '%execute%' +), +text_params as ( + select + c.*, + ip.pos as arg_pos, + lower(coalesce(nullif(c.proargnames[a.ord], ''), '$' || ip.pos)) as arg_name + from + candidates c + cross join lateral unnest(c.all_arg_types) with ordinality as a(typ, ord) + -- position among input arguments, which is what `$N` refers to + cross join lateral ( + select count(*) as pos + from generate_series(1, a.ord::int) g + where coalesce(c.proargmodes[g]::text, 'i') in ('i', 'b', 'v') + ) ip + where + coalesce(c.proargmodes[a.ord]::text, 'i') in ('i', 'b', 'v') + and a.typ in ( + 'pg_catalog.text'::pg_catalog.regtype, + 'pg_catalog.varchar'::pg_catalog.regtype, + 'pg_catalog.text[]'::pg_catalog.regtype, + 'pg_catalog.varchar[]'::pg_catalog.regtype + ) +), +escaped as ( + select + tp.*, + -- match the parameter by name or by position (`$N` also works for + -- named parameters in PL/pgSQL) + '(?:' || regexp_replace(tp.arg_name, '([^a-z0-9_])', '\\\1', 'g') || '|\$' || tp.arg_pos || ')' as arg_re + from + text_params tp +), +formatted as ( + select + e.*, + -- a parameter passed to format() in a %I or %L slot is quoted, so + -- it is replaced with `null` (like quote_ident/quote_literal). + -- Only format() calls with simple, comma-free arguments and + -- sequential specifiers (no `%1$s`) are recognised anything else + -- is left in place and counts as a use of the parameter. + regexp_replace( + e.body, + '(?<=(?:' || ( + select string_agg( + 'format\s*\(\s*''(?:[^''%]|%%|'''')*(?:%[-0-9]*[sil](?:[^''%]|%%|'''')*){' || (k - 1) || '}%[-0-9]*[il](?:[^'']|'''')*''\s*(?:,[^,()]+){' || (k - 1) || '},\s*', + '|' + ) + from generate_series(1, 8) k + ) || '))' || e.arg_re || '(?=\s*[,)])', + 'null', + 'g' + ) as fmt_body + from + escaped e +), +cleaned as ( + select + f.*, + -- string literals blanked out (so keywords and semicolons inside + -- them are not mistaken for code), `USING` bound values removed + -- (only a real USING clause is left to match), and quote_*(param) + -- wrappers removed + regexp_replace( + regexp_replace( + regexp_replace( + regexp_replace( + f.fmt_body, + '\$([a-z_]*?)\$.*?\$\1\$', + '''''', + 'g' + ), + '''([^'']|'''')*''', + '''''', + 'g' + ), + '\musing\s[^\x3b]*', + '', + 'g' + ), + 'quote_(ident|literal|nullable)\s*\(\s*' || f.arg_re || '\s*\)', + '', + 'g' + ) as exec_body + from + formatted f +), +matches as ( + select + cl.schema_name, + cl.function_name, + cl.function_args, + cl.function_language, + cl.is_security_definer, + cl.owner_name, + cl.arg_name, + ( + -- the parameter is used inside an EXECUTE statement + cl.exec_body ~ ('\mexecute\M[^\x3b]*[^a-z0-9_$]' || cl.arg_re || '([^a-z0-9_]|$)') + -- or a variable assigned from the parameter (`v := ... || param` + -- or `declare v text := ... || param`) is used in an EXECUTE + or exists ( + select 1 + from regexp_matches( + cl.exec_body, + '(?:([a-z_][a-z0-9_]*)\s+(?:text|varchar(?:\(\d+\))?|character\s+varying(?:\(\d+\))?)|([a-z_][a-z0-9_]*))\s*:=([^\x3b]*[^a-z0-9_$])?' || cl.arg_re || '([^a-z0-9_]|$)', + 'g' + ) as m + where cl.exec_body ~ ('\mexecute\M[^\x3b]*[^a-z0-9_$]' || coalesce(m[1], m[2]) || '([^a-z0-9_]|$)') + ) + ) as direct_use, + cl.arg_name in ('sql', 'query', 'queries', 'stmt', 'statement', 'command', 'cmd', 'sql_text', 'sql_query', 'sql_string', 'query_text', 'query_string', 'raw_sql', 'raw_query') + and cl.exec_body ~ '\mexecute\M' as renamed_use + from + cleaned cl +), +flagged as ( + select + schema_name, + function_name, + function_args, + function_language, + is_security_definer, + owner_name, + bool_or(direct_use) as has_direct_use, + array_agg(arg_name order by arg_name) as sql_parameters + from + matches + where + direct_use or renamed_use + group by + schema_name, + function_name, + function_args, + function_language, + is_security_definer, + owner_name +) +select + 'authenticated_dynamic_sql_function_executable' as name, + 'Signed-In Users Can Execute Dynamic SQL Function' as title, + case when is_security_definer and has_direct_use then 'ERROR' else 'WARN' end as level, + 'EXTERNAL' as facing, + array['SECURITY'] as categories, + 'Detects functions callable by signed-in users that appear to run a SQL string passed in by the caller. Revoke `EXECUTE` or replace the function with fixed operations.' as description, + format( + 'Function `%s.%s(%s)` is `SECURITY %s`, can be executed by the `authenticated` role via `/rest/v1/rpc/%s`, and appears to run SQL passed in `%s`. Revoke `EXECUTE` if that is not intentional.', + schema_name, + function_name, + function_args, + case when is_security_definer then 'DEFINER' else 'INVOKER' end, + function_name, + array_to_string(sql_parameters, ', ') + ) as detail, + 'https://supabase.com/docs/guides/database/database-linter?lint=0032_authenticated_dynamic_sql_function_executable' as remediation, + jsonb_build_object( + 'schema', schema_name, + 'name', function_name, + 'type', 'function', + 'arguments', function_args, + 'language', function_language, + 'security_definer', is_security_definer, + 'owner', owner_name, + 'sql_parameters', sql_parameters + ) as metadata, + format( + 'authenticated_dynamic_sql_function_executable_%s_%s_%s', + schema_name, + function_name, + function_args + ) as cache_key +from + flagged +order by + schema_name, + function_name, + function_args) \ No newline at end of file diff --git a/test/expected/0031_anon_dynamic_sql_function_executable.out b/test/expected/0031_anon_dynamic_sql_function_executable.out new file mode 100644 index 0000000..c3859bb --- /dev/null +++ b/test/expected/0031_anon_dynamic_sql_function_executable.out @@ -0,0 +1,351 @@ +begin; + set local search_path = ''; + set local pgrst.db_schemas = 'public'; + -- BASELINE: no functions, no rows + select * from lint."0031_anon_dynamic_sql_function_executable"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + + savepoint a; + ---------------------------------------- + -- NEGATIVE: dynamic SQL, but the text parameter is only passed via + -- EXECUTE ... USING (bound value) and the SQL text is static. + ---------------------------------------- + create function public.find_user(p_name text) returns int + language plpgsql + security definer + as $$ + declare r int; + begin + execute 'select count(*)::int from auth.users where email = $1' into r using p_name; + return r; + end $$; + select * from lint."0031_anon_dynamic_sql_function_executable"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + + -- NEGATIVE: parameter only used through quote_literal/quote_ident + create function public.quoted(tbl text) returns int + language plpgsql + security definer + as $$ + declare r int; + begin + execute 'select count(*)::int from ' || quote_ident(tbl) into r; + return r; + end $$; + select * from lint."0031_anon_dynamic_sql_function_executable"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + + -- NEGATIVE: no EXECUTE at all, even with a SQL-shaped parameter name + create function public.search(query text) returns int + language plpgsql + security definer + as $$ + begin + return length(query); + end $$; + select * from lint."0031_anon_dynamic_sql_function_executable"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + + -- NEGATIVE: EXECUTE only mentioned in a comment + create function public.commented(query text) returns int + language plpgsql + as $$ + begin + -- we used to execute query here + return length(query); + end $$; + select * from lint."0031_anon_dynamic_sql_function_executable"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + + -- NEGATIVE: the parameter is only bound with USING; the variable holds + -- static SQL + create function public.static_var(p_name text) returns int + language plpgsql + security definer + as $$ + declare q text := 'select count(*)::int from auth.users where email = $1'; r int; + begin + execute q into r using p_name; + return r; + end $$; + select * from lint."0031_anon_dynamic_sql_function_executable"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + + -- NEGATIVE: the parameter is quoted before it is put in a variable + create function public.quoted_var(p_table text) returns int + language plpgsql + security definer + as $$ + declare q text; r int; + begin + q := 'select count(*)::int from ' || quote_ident(p_table); + execute q into r; + return r; + end $$; + select * from lint."0031_anon_dynamic_sql_function_executable"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + + -- NEGATIVE: parameters passed to format() only in %I / %L slots + create function public.fmt_quoted(p_table text, p_val text) returns int + language plpgsql + security definer + as $$ + declare r int; + begin + execute format('select count(*)::int from %I where name = %L', p_table, p_val) into r; + return r; + end $$; + select * from lint."0031_anon_dynamic_sql_function_executable"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + + -- POSITIVE (ERROR): the second argument goes into a %s slot + create function public.fmt_unquoted(p_table text, p_val text) returns int + language plpgsql + security definer + as $$ + declare r int; + begin + execute format('select count(*)::int from %I where name = %s', p_table, p_val) into r; + return r; + end $$; + select name, level, metadata->'sql_parameters' as sql_parameters, cache_key + from lint."0031_anon_dynamic_sql_function_executable"; + name | level | sql_parameters | cache_key +--------------------------------------+-------+----------------+----------------------------------------------------------------------------------- + anon_dynamic_sql_function_executable | ERROR | ["p_val"] | anon_dynamic_sql_function_executable_public_fmt_unquoted_p_table text, p_val text +(1 row) + + rollback to savepoint a; + savepoint case_definer; + ---------------------------------------- + -- POSITIVE (ERROR): the exec_sql shape. SECURITY DEFINER, the + -- caller-supplied string is concatenated into an EXECUTE. + ---------------------------------------- + create function public.exec_sql(query text) returns json + language plpgsql + security definer + as $$ + declare r json; + begin + execute 'select json_agg(t) from (' || query || ') t' into r; + return r; + end $$; + select + name, + level, + detail, + metadata->>'security_definer' as security_definer, + metadata->'sql_parameters' as sql_parameters, + cache_key + from lint."0031_anon_dynamic_sql_function_executable"; + name | level | detail | security_definer | sql_parameters | cache_key +--------------------------------------+-------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+------------------+----------------+----------------------------------------------------------------- + anon_dynamic_sql_function_executable | ERROR | Function `public.exec_sql(query text)` is `SECURITY DEFINER`, can be executed by the `anon` role via `/rest/v1/rpc/exec_sql`, and appears to run SQL passed in `query`. Revoke `EXECUTE` if that is not intentional. | true | ["query"] | anon_dynamic_sql_function_executable_public_exec_sql_query text +(1 row) + + ---------------------------------------- + -- RESOLUTION: revoke EXECUTE from anon and PUBLIC + ---------------------------------------- + revoke execute on function public.exec_sql(text) from anon, public; + select * from lint."0031_anon_dynamic_sql_function_executable"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + + rollback to savepoint case_definer; + savepoint case_invoker; + ---------------------------------------- + -- POSITIVE (WARN): same shape but SECURITY INVOKER, the function runs + -- as anon. Not covered by lint 0028. + ---------------------------------------- + create function public.run_query(sql text) returns void + language plpgsql + security invoker + as $$ + begin + execute sql; + end $$; + select name, level, metadata->>'security_definer' as security_definer, cache_key + from lint."0031_anon_dynamic_sql_function_executable"; + name | level | security_definer | cache_key +--------------------------------------+-------+------------------+---------------------------------------------------------------- + anon_dynamic_sql_function_executable | WARN | false | anon_dynamic_sql_function_executable_public_run_query_sql text +(1 row) + + rollback to savepoint case_invoker; + savepoint case_renamed; + ---------------------------------------- + -- POSITIVE (WARN): renamed wrapper that copies the SQL parameter into a + -- local variable before EXECUTE, so it is not referenced directly. + ---------------------------------------- + create function public.insert_entries(sql text) returns void + language plpgsql + security definer + as $$ + declare stmt text; + begin + stmt := sql; + execute stmt; + end $$; + select name, level, metadata->'sql_parameters' as sql_parameters, cache_key + from lint."0031_anon_dynamic_sql_function_executable"; + name | level | sql_parameters | cache_key +--------------------------------------+-------+----------------+--------------------------------------------------------------------- + anon_dynamic_sql_function_executable | ERROR | ["sql"] | anon_dynamic_sql_function_executable_public_insert_entries_sql text +(1 row) + + rollback to savepoint case_renamed; + savepoint case_unnamed; + ---------------------------------------- + -- POSITIVE (ERROR): unnamed parameter used as `EXECUTE $1` + ---------------------------------------- + create function public.exec_unnamed(text) returns void + language plpgsql + security definer + as $$ + begin + execute $1; + end $$; + select name, level, metadata->'sql_parameters' as sql_parameters, cache_key + from lint."0031_anon_dynamic_sql_function_executable"; + name | level | sql_parameters | cache_key +--------------------------------------+-------+----------------+--------------------------------------------------------------- + anon_dynamic_sql_function_executable | ERROR | ["$1"] | anon_dynamic_sql_function_executable_public_exec_unnamed_text +(1 row) + + rollback to savepoint case_unnamed; + savepoint case_positional; + ---------------------------------------- + -- POSITIVE (ERROR): named parameter referenced as $1 in the EXECUTE + ---------------------------------------- + create function public.run_positional(p text) returns void + language plpgsql + security definer + as $$ + begin + execute $1; + end $$; + select name, level, metadata->'sql_parameters' as sql_parameters, cache_key + from lint."0031_anon_dynamic_sql_function_executable"; + name | level | sql_parameters | cache_key +--------------------------------------+-------+----------------+------------------------------------------------------------------- + anon_dynamic_sql_function_executable | ERROR | ["p"] | anon_dynamic_sql_function_executable_public_run_positional_p text +(1 row) + + rollback to savepoint case_positional; + savepoint case_variable; + ---------------------------------------- + -- POSITIVE (ERROR): the parameter is built into a local variable, either + -- by assignment or in the declaration, and the variable is EXECUTEd. + ---------------------------------------- + create function public.count_rows(p_table text) returns int + language plpgsql + security definer + as $$ + declare q text; r int; + begin + q := 'select count(*)::int from ' || p_table; + execute q into r; + return r; + end $$; + create function public.count_rows_declared(p_table text) returns int + language plpgsql + security definer + as $$ + declare q text := 'select count(*)::int from ' || p_table; r int; + begin + execute q into r; + return r; + end $$; + select name, level, metadata->'name' as func_name, cache_key + from lint."0031_anon_dynamic_sql_function_executable"; + name | level | func_name | cache_key +--------------------------------------+-------+-----------------------+------------------------------------------------------------------------------ + anon_dynamic_sql_function_executable | ERROR | "count_rows" | anon_dynamic_sql_function_executable_public_count_rows_p_table text + anon_dynamic_sql_function_executable | ERROR | "count_rows_declared" | anon_dynamic_sql_function_executable_public_count_rows_declared_p_table text +(2 rows) + + rollback to savepoint case_variable; + savepoint case_using_in_literal; + ---------------------------------------- + -- POSITIVE (ERROR): the SQL text contains the word `using` before the + -- concatenated parameter (DELETE ... USING, CREATE INDEX ... USING). + -- These are not EXECUTE's own USING clause and must not hide the + -- parameter. The `;` inside the literal must not hide it either. + ---------------------------------------- + create function public.exec_delete_using(query text) returns void + language plpgsql + security definer + as $$ + begin + execute 'delete from public.t using public.u where ' || query; + end $$; + create function public.exec_index_using(query text) returns void + language plpgsql + security definer + as $$ + begin + execute 'select 1; create index on public.t using gin (' || query || ')'; + end $$; + select name, level, metadata->'name' as func_name, cache_key + from lint."0031_anon_dynamic_sql_function_executable"; + name | level | func_name | cache_key +--------------------------------------+-------+---------------------+-------------------------------------------------------------------------- + anon_dynamic_sql_function_executable | ERROR | "exec_delete_using" | anon_dynamic_sql_function_executable_public_exec_delete_using_query text + anon_dynamic_sql_function_executable | ERROR | "exec_index_using" | anon_dynamic_sql_function_executable_public_exec_index_using_query text +(2 rows) + + rollback to savepoint case_using_in_literal; + savepoint case_variadic; + ---------------------------------------- + -- POSITIVE (ERROR): VARIADIC text[] parameter used in an EXECUTE + ---------------------------------------- + create function public.exec_many(variadic queries text[]) returns void + language plpgsql + security definer + as $$ + begin + execute queries[1]; + end $$; + select name, level, metadata->'sql_parameters' as sql_parameters, cache_key + from lint."0031_anon_dynamic_sql_function_executable"; + name | level | sql_parameters | cache_key +--------------------------------------+-------+----------------+------------------------------------------------------------------------------- + anon_dynamic_sql_function_executable | ERROR | ["queries"] | anon_dynamic_sql_function_executable_public_exec_many_VARIADIC queries text[] +(1 row) + + rollback to savepoint case_variadic; + savepoint case_system_schema; + ---------------------------------------- + -- NEGATIVE: function in a system schema is excluded + ---------------------------------------- + create function auth.exec_sql(query text) returns void + language plpgsql + security definer + as $$ + begin + execute query; + end $$; + select * from lint."0031_anon_dynamic_sql_function_executable"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + + rollback to savepoint case_system_schema; +rollback; diff --git a/test/expected/0032_authenticated_dynamic_sql_function_executable.out b/test/expected/0032_authenticated_dynamic_sql_function_executable.out new file mode 100644 index 0000000..f3e7346 --- /dev/null +++ b/test/expected/0032_authenticated_dynamic_sql_function_executable.out @@ -0,0 +1,351 @@ +begin; + set local search_path = ''; + set local pgrst.db_schemas = 'public'; + -- BASELINE: no functions, no rows + select * from lint."0032_authenticated_dynamic_sql_function_executable"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + + savepoint a; + ---------------------------------------- + -- NEGATIVE: dynamic SQL, but the text parameter is only passed via + -- EXECUTE ... USING (bound value) and the SQL text is static. + ---------------------------------------- + create function public.find_user(p_name text) returns int + language plpgsql + security definer + as $$ + declare r int; + begin + execute 'select count(*)::int from auth.users where email = $1' into r using p_name; + return r; + end $$; + select * from lint."0032_authenticated_dynamic_sql_function_executable"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + + -- NEGATIVE: parameter only used through quote_literal/quote_ident + create function public.quoted(tbl text) returns int + language plpgsql + security definer + as $$ + declare r int; + begin + execute 'select count(*)::int from ' || quote_ident(tbl) into r; + return r; + end $$; + select * from lint."0032_authenticated_dynamic_sql_function_executable"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + + -- NEGATIVE: no EXECUTE at all, even with a SQL-shaped parameter name + create function public.search(query text) returns int + language plpgsql + security definer + as $$ + begin + return length(query); + end $$; + select * from lint."0032_authenticated_dynamic_sql_function_executable"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + + -- NEGATIVE: EXECUTE only mentioned in a comment + create function public.commented(query text) returns int + language plpgsql + as $$ + begin + -- we used to execute query here + return length(query); + end $$; + select * from lint."0032_authenticated_dynamic_sql_function_executable"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + + -- NEGATIVE: the parameter is only bound with USING; the variable holds + -- static SQL + create function public.static_var(p_name text) returns int + language plpgsql + security definer + as $$ + declare q text := 'select count(*)::int from auth.users where email = $1'; r int; + begin + execute q into r using p_name; + return r; + end $$; + select * from lint."0032_authenticated_dynamic_sql_function_executable"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + + -- NEGATIVE: the parameter is quoted before it is put in a variable + create function public.quoted_var(p_table text) returns int + language plpgsql + security definer + as $$ + declare q text; r int; + begin + q := 'select count(*)::int from ' || quote_ident(p_table); + execute q into r; + return r; + end $$; + select * from lint."0032_authenticated_dynamic_sql_function_executable"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + + -- NEGATIVE: parameters passed to format() only in %I / %L slots + create function public.fmt_quoted(p_table text, p_val text) returns int + language plpgsql + security definer + as $$ + declare r int; + begin + execute format('select count(*)::int from %I where name = %L', p_table, p_val) into r; + return r; + end $$; + select * from lint."0032_authenticated_dynamic_sql_function_executable"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + + -- POSITIVE (ERROR): the second argument goes into a %s slot + create function public.fmt_unquoted(p_table text, p_val text) returns int + language plpgsql + security definer + as $$ + declare r int; + begin + execute format('select count(*)::int from %I where name = %s', p_table, p_val) into r; + return r; + end $$; + select name, level, metadata->'sql_parameters' as sql_parameters, cache_key + from lint."0032_authenticated_dynamic_sql_function_executable"; + name | level | sql_parameters | cache_key +-----------------------------------------------+-------+----------------+-------------------------------------------------------------------------------------------- + authenticated_dynamic_sql_function_executable | ERROR | ["p_val"] | authenticated_dynamic_sql_function_executable_public_fmt_unquoted_p_table text, p_val text +(1 row) + + rollback to savepoint a; + savepoint case_definer; + ---------------------------------------- + -- POSITIVE (ERROR): the exec_sql shape. SECURITY DEFINER, the + -- caller-supplied string is concatenated into an EXECUTE. + ---------------------------------------- + create function public.exec_sql(query text) returns json + language plpgsql + security definer + as $$ + declare r json; + begin + execute 'select json_agg(t) from (' || query || ') t' into r; + return r; + end $$; + select + name, + level, + detail, + metadata->>'security_definer' as security_definer, + metadata->'sql_parameters' as sql_parameters, + cache_key + from lint."0032_authenticated_dynamic_sql_function_executable"; + name | level | detail | security_definer | sql_parameters | cache_key +-----------------------------------------------+-------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+------------------+----------------+-------------------------------------------------------------------------- + authenticated_dynamic_sql_function_executable | ERROR | Function `public.exec_sql(query text)` is `SECURITY DEFINER`, can be executed by the `authenticated` role via `/rest/v1/rpc/exec_sql`, and appears to run SQL passed in `query`. Revoke `EXECUTE` if that is not intentional. | true | ["query"] | authenticated_dynamic_sql_function_executable_public_exec_sql_query text +(1 row) + + ---------------------------------------- + -- RESOLUTION: revoke EXECUTE from authenticated and PUBLIC + ---------------------------------------- + revoke execute on function public.exec_sql(text) from authenticated, public; + select * from lint."0032_authenticated_dynamic_sql_function_executable"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + + rollback to savepoint case_definer; + savepoint case_invoker; + ---------------------------------------- + -- POSITIVE (WARN): same shape but SECURITY INVOKER, the function runs + -- as authenticated. Not covered by lint 0029. + ---------------------------------------- + create function public.run_query(sql text) returns void + language plpgsql + security invoker + as $$ + begin + execute sql; + end $$; + select name, level, metadata->>'security_definer' as security_definer, cache_key + from lint."0032_authenticated_dynamic_sql_function_executable"; + name | level | security_definer | cache_key +-----------------------------------------------+-------+------------------+------------------------------------------------------------------------- + authenticated_dynamic_sql_function_executable | WARN | false | authenticated_dynamic_sql_function_executable_public_run_query_sql text +(1 row) + + rollback to savepoint case_invoker; + savepoint case_renamed; + ---------------------------------------- + -- POSITIVE (WARN): renamed wrapper that copies the SQL parameter into a + -- local variable before EXECUTE, so it is not referenced directly. + ---------------------------------------- + create function public.insert_entries(sql text) returns void + language plpgsql + security definer + as $$ + declare stmt text; + begin + stmt := sql; + execute stmt; + end $$; + select name, level, metadata->'sql_parameters' as sql_parameters, cache_key + from lint."0032_authenticated_dynamic_sql_function_executable"; + name | level | sql_parameters | cache_key +-----------------------------------------------+-------+----------------+------------------------------------------------------------------------------ + authenticated_dynamic_sql_function_executable | ERROR | ["sql"] | authenticated_dynamic_sql_function_executable_public_insert_entries_sql text +(1 row) + + rollback to savepoint case_renamed; + savepoint case_unnamed; + ---------------------------------------- + -- POSITIVE (ERROR): unnamed parameter used as `EXECUTE $1` + ---------------------------------------- + create function public.exec_unnamed(text) returns void + language plpgsql + security definer + as $$ + begin + execute $1; + end $$; + select name, level, metadata->'sql_parameters' as sql_parameters, cache_key + from lint."0032_authenticated_dynamic_sql_function_executable"; + name | level | sql_parameters | cache_key +-----------------------------------------------+-------+----------------+------------------------------------------------------------------------ + authenticated_dynamic_sql_function_executable | ERROR | ["$1"] | authenticated_dynamic_sql_function_executable_public_exec_unnamed_text +(1 row) + + rollback to savepoint case_unnamed; + savepoint case_positional; + ---------------------------------------- + -- POSITIVE (ERROR): named parameter referenced as $1 in the EXECUTE + ---------------------------------------- + create function public.run_positional(p text) returns void + language plpgsql + security definer + as $$ + begin + execute $1; + end $$; + select name, level, metadata->'sql_parameters' as sql_parameters, cache_key + from lint."0032_authenticated_dynamic_sql_function_executable"; + name | level | sql_parameters | cache_key +-----------------------------------------------+-------+----------------+---------------------------------------------------------------------------- + authenticated_dynamic_sql_function_executable | ERROR | ["p"] | authenticated_dynamic_sql_function_executable_public_run_positional_p text +(1 row) + + rollback to savepoint case_positional; + savepoint case_variable; + ---------------------------------------- + -- POSITIVE (ERROR): the parameter is built into a local variable, either + -- by assignment or in the declaration, and the variable is EXECUTEd. + ---------------------------------------- + create function public.count_rows(p_table text) returns int + language plpgsql + security definer + as $$ + declare q text; r int; + begin + q := 'select count(*)::int from ' || p_table; + execute q into r; + return r; + end $$; + create function public.count_rows_declared(p_table text) returns int + language plpgsql + security definer + as $$ + declare q text := 'select count(*)::int from ' || p_table; r int; + begin + execute q into r; + return r; + end $$; + select name, level, metadata->'name' as func_name, cache_key + from lint."0032_authenticated_dynamic_sql_function_executable"; + name | level | func_name | cache_key +-----------------------------------------------+-------+-----------------------+--------------------------------------------------------------------------------------- + authenticated_dynamic_sql_function_executable | ERROR | "count_rows" | authenticated_dynamic_sql_function_executable_public_count_rows_p_table text + authenticated_dynamic_sql_function_executable | ERROR | "count_rows_declared" | authenticated_dynamic_sql_function_executable_public_count_rows_declared_p_table text +(2 rows) + + rollback to savepoint case_variable; + savepoint case_using_in_literal; + ---------------------------------------- + -- POSITIVE (ERROR): the SQL text contains the word `using` before the + -- concatenated parameter (DELETE ... USING, CREATE INDEX ... USING). + -- These are not EXECUTE's own USING clause and must not hide the + -- parameter. The `;` inside the literal must not hide it either. + ---------------------------------------- + create function public.exec_delete_using(query text) returns void + language plpgsql + security definer + as $$ + begin + execute 'delete from public.t using public.u where ' || query; + end $$; + create function public.exec_index_using(query text) returns void + language plpgsql + security definer + as $$ + begin + execute 'select 1; create index on public.t using gin (' || query || ')'; + end $$; + select name, level, metadata->'name' as func_name, cache_key + from lint."0032_authenticated_dynamic_sql_function_executable"; + name | level | func_name | cache_key +-----------------------------------------------+-------+---------------------+----------------------------------------------------------------------------------- + authenticated_dynamic_sql_function_executable | ERROR | "exec_delete_using" | authenticated_dynamic_sql_function_executable_public_exec_delete_using_query text + authenticated_dynamic_sql_function_executable | ERROR | "exec_index_using" | authenticated_dynamic_sql_function_executable_public_exec_index_using_query text +(2 rows) + + rollback to savepoint case_using_in_literal; + savepoint case_variadic; + ---------------------------------------- + -- POSITIVE (ERROR): VARIADIC text[] parameter used in an EXECUTE + ---------------------------------------- + create function public.exec_many(variadic queries text[]) returns void + language plpgsql + security definer + as $$ + begin + execute queries[1]; + end $$; + select name, level, metadata->'sql_parameters' as sql_parameters, cache_key + from lint."0032_authenticated_dynamic_sql_function_executable"; + name | level | sql_parameters | cache_key +-----------------------------------------------+-------+----------------+---------------------------------------------------------------------------------------- + authenticated_dynamic_sql_function_executable | ERROR | ["queries"] | authenticated_dynamic_sql_function_executable_public_exec_many_VARIADIC queries text[] +(1 row) + + rollback to savepoint case_variadic; + savepoint case_system_schema; + ---------------------------------------- + -- NEGATIVE: function in a system schema is excluded + ---------------------------------------- + create function auth.exec_sql(query text) returns void + language plpgsql + security definer + as $$ + begin + execute query; + end $$; + select * from lint."0032_authenticated_dynamic_sql_function_executable"; + name | title | level | facing | categories | description | detail | remediation | metadata | cache_key +------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- +(0 rows) + + rollback to savepoint case_system_schema; +rollback; diff --git a/test/expected/queries_are_unionable.out b/test/expected/queries_are_unionable.out index 058f2dd..e7738bd 100644 --- a/test/expected/queries_are_unionable.out +++ b/test/expected/queries_are_unionable.out @@ -56,7 +56,11 @@ begin; union all select * from lint."0029_authenticated_security_definer_function_executable" union all - select * from lint."0030_autovacuum_disabled"; + select * from lint."0030_autovacuum_disabled" + union all + select * from lint."0031_anon_dynamic_sql_function_executable" + union all + select * from lint."0032_authenticated_dynamic_sql_function_executable"; name | title | level | facing | categories | description | detail | remediation | metadata | cache_key ------+-------+-------+--------+------------+-------------+--------+-------------+----------+----------- (0 rows) diff --git a/test/sql/0031_anon_dynamic_sql_function_executable.sql b/test/sql/0031_anon_dynamic_sql_function_executable.sql new file mode 100644 index 0000000..0c44b26 --- /dev/null +++ b/test/sql/0031_anon_dynamic_sql_function_executable.sql @@ -0,0 +1,312 @@ +begin; + set local search_path = ''; + set local pgrst.db_schemas = 'public'; + + -- BASELINE: no functions, no rows + select * from lint."0031_anon_dynamic_sql_function_executable"; + + savepoint a; + + ---------------------------------------- + -- NEGATIVE: dynamic SQL, but the text parameter is only passed via + -- EXECUTE ... USING (bound value) and the SQL text is static. + ---------------------------------------- + create function public.find_user(p_name text) returns int + language plpgsql + security definer + as $$ + declare r int; + begin + execute 'select count(*)::int from auth.users where email = $1' into r using p_name; + return r; + end $$; + select * from lint."0031_anon_dynamic_sql_function_executable"; + + -- NEGATIVE: parameter only used through quote_literal/quote_ident + create function public.quoted(tbl text) returns int + language plpgsql + security definer + as $$ + declare r int; + begin + execute 'select count(*)::int from ' || quote_ident(tbl) into r; + return r; + end $$; + select * from lint."0031_anon_dynamic_sql_function_executable"; + + -- NEGATIVE: no EXECUTE at all, even with a SQL-shaped parameter name + create function public.search(query text) returns int + language plpgsql + security definer + as $$ + begin + return length(query); + end $$; + select * from lint."0031_anon_dynamic_sql_function_executable"; + + -- NEGATIVE: EXECUTE only mentioned in a comment + create function public.commented(query text) returns int + language plpgsql + as $$ + begin + -- we used to execute query here + return length(query); + end $$; + select * from lint."0031_anon_dynamic_sql_function_executable"; + + -- NEGATIVE: the parameter is only bound with USING; the variable holds + -- static SQL + create function public.static_var(p_name text) returns int + language plpgsql + security definer + as $$ + declare q text := 'select count(*)::int from auth.users where email = $1'; r int; + begin + execute q into r using p_name; + return r; + end $$; + select * from lint."0031_anon_dynamic_sql_function_executable"; + + -- NEGATIVE: the parameter is quoted before it is put in a variable + create function public.quoted_var(p_table text) returns int + language plpgsql + security definer + as $$ + declare q text; r int; + begin + q := 'select count(*)::int from ' || quote_ident(p_table); + execute q into r; + return r; + end $$; + select * from lint."0031_anon_dynamic_sql_function_executable"; + + -- NEGATIVE: parameters passed to format() only in %I / %L slots + create function public.fmt_quoted(p_table text, p_val text) returns int + language plpgsql + security definer + as $$ + declare r int; + begin + execute format('select count(*)::int from %I where name = %L', p_table, p_val) into r; + return r; + end $$; + select * from lint."0031_anon_dynamic_sql_function_executable"; + + -- POSITIVE (ERROR): the second argument goes into a %s slot + create function public.fmt_unquoted(p_table text, p_val text) returns int + language plpgsql + security definer + as $$ + declare r int; + begin + execute format('select count(*)::int from %I where name = %s', p_table, p_val) into r; + return r; + end $$; + select name, level, metadata->'sql_parameters' as sql_parameters, cache_key + from lint."0031_anon_dynamic_sql_function_executable"; + + rollback to savepoint a; + + savepoint case_definer; + + ---------------------------------------- + -- POSITIVE (ERROR): the exec_sql shape. SECURITY DEFINER, the + -- caller-supplied string is concatenated into an EXECUTE. + ---------------------------------------- + create function public.exec_sql(query text) returns json + language plpgsql + security definer + as $$ + declare r json; + begin + execute 'select json_agg(t) from (' || query || ') t' into r; + return r; + end $$; + + select + name, + level, + detail, + metadata->>'security_definer' as security_definer, + metadata->'sql_parameters' as sql_parameters, + cache_key + from lint."0031_anon_dynamic_sql_function_executable"; + + ---------------------------------------- + -- RESOLUTION: revoke EXECUTE from anon and PUBLIC + ---------------------------------------- + revoke execute on function public.exec_sql(text) from anon, public; + select * from lint."0031_anon_dynamic_sql_function_executable"; + + rollback to savepoint case_definer; + + savepoint case_invoker; + + ---------------------------------------- + -- POSITIVE (WARN): same shape but SECURITY INVOKER, the function runs + -- as anon. Not covered by lint 0028. + ---------------------------------------- + create function public.run_query(sql text) returns void + language plpgsql + security invoker + as $$ + begin + execute sql; + end $$; + + select name, level, metadata->>'security_definer' as security_definer, cache_key + from lint."0031_anon_dynamic_sql_function_executable"; + + rollback to savepoint case_invoker; + + savepoint case_renamed; + + ---------------------------------------- + -- POSITIVE (WARN): renamed wrapper that copies the SQL parameter into a + -- local variable before EXECUTE, so it is not referenced directly. + ---------------------------------------- + create function public.insert_entries(sql text) returns void + language plpgsql + security definer + as $$ + declare stmt text; + begin + stmt := sql; + execute stmt; + end $$; + + select name, level, metadata->'sql_parameters' as sql_parameters, cache_key + from lint."0031_anon_dynamic_sql_function_executable"; + + rollback to savepoint case_renamed; + + savepoint case_unnamed; + + ---------------------------------------- + -- POSITIVE (ERROR): unnamed parameter used as `EXECUTE $1` + ---------------------------------------- + create function public.exec_unnamed(text) returns void + language plpgsql + security definer + as $$ + begin + execute $1; + end $$; + + select name, level, metadata->'sql_parameters' as sql_parameters, cache_key + from lint."0031_anon_dynamic_sql_function_executable"; + + rollback to savepoint case_unnamed; + + savepoint case_positional; + + ---------------------------------------- + -- POSITIVE (ERROR): named parameter referenced as $1 in the EXECUTE + ---------------------------------------- + create function public.run_positional(p text) returns void + language plpgsql + security definer + as $$ + begin + execute $1; + end $$; + + select name, level, metadata->'sql_parameters' as sql_parameters, cache_key + from lint."0031_anon_dynamic_sql_function_executable"; + + rollback to savepoint case_positional; + + savepoint case_variable; + + ---------------------------------------- + -- POSITIVE (ERROR): the parameter is built into a local variable, either + -- by assignment or in the declaration, and the variable is EXECUTEd. + ---------------------------------------- + create function public.count_rows(p_table text) returns int + language plpgsql + security definer + as $$ + declare q text; r int; + begin + q := 'select count(*)::int from ' || p_table; + execute q into r; + return r; + end $$; + create function public.count_rows_declared(p_table text) returns int + language plpgsql + security definer + as $$ + declare q text := 'select count(*)::int from ' || p_table; r int; + begin + execute q into r; + return r; + end $$; + + select name, level, metadata->'name' as func_name, cache_key + from lint."0031_anon_dynamic_sql_function_executable"; + + rollback to savepoint case_variable; + + savepoint case_using_in_literal; + + ---------------------------------------- + -- POSITIVE (ERROR): the SQL text contains the word `using` before the + -- concatenated parameter (DELETE ... USING, CREATE INDEX ... USING). + -- These are not EXECUTE's own USING clause and must not hide the + -- parameter. The `;` inside the literal must not hide it either. + ---------------------------------------- + create function public.exec_delete_using(query text) returns void + language plpgsql + security definer + as $$ + begin + execute 'delete from public.t using public.u where ' || query; + end $$; + create function public.exec_index_using(query text) returns void + language plpgsql + security definer + as $$ + begin + execute 'select 1; create index on public.t using gin (' || query || ')'; + end $$; + + select name, level, metadata->'name' as func_name, cache_key + from lint."0031_anon_dynamic_sql_function_executable"; + + rollback to savepoint case_using_in_literal; + + savepoint case_variadic; + + ---------------------------------------- + -- POSITIVE (ERROR): VARIADIC text[] parameter used in an EXECUTE + ---------------------------------------- + create function public.exec_many(variadic queries text[]) returns void + language plpgsql + security definer + as $$ + begin + execute queries[1]; + end $$; + + select name, level, metadata->'sql_parameters' as sql_parameters, cache_key + from lint."0031_anon_dynamic_sql_function_executable"; + + rollback to savepoint case_variadic; + + savepoint case_system_schema; + + ---------------------------------------- + -- NEGATIVE: function in a system schema is excluded + ---------------------------------------- + create function auth.exec_sql(query text) returns void + language plpgsql + security definer + as $$ + begin + execute query; + end $$; + select * from lint."0031_anon_dynamic_sql_function_executable"; + + rollback to savepoint case_system_schema; + +rollback; diff --git a/test/sql/0032_authenticated_dynamic_sql_function_executable.sql b/test/sql/0032_authenticated_dynamic_sql_function_executable.sql new file mode 100644 index 0000000..66c3fc6 --- /dev/null +++ b/test/sql/0032_authenticated_dynamic_sql_function_executable.sql @@ -0,0 +1,312 @@ +begin; + set local search_path = ''; + set local pgrst.db_schemas = 'public'; + + -- BASELINE: no functions, no rows + select * from lint."0032_authenticated_dynamic_sql_function_executable"; + + savepoint a; + + ---------------------------------------- + -- NEGATIVE: dynamic SQL, but the text parameter is only passed via + -- EXECUTE ... USING (bound value) and the SQL text is static. + ---------------------------------------- + create function public.find_user(p_name text) returns int + language plpgsql + security definer + as $$ + declare r int; + begin + execute 'select count(*)::int from auth.users where email = $1' into r using p_name; + return r; + end $$; + select * from lint."0032_authenticated_dynamic_sql_function_executable"; + + -- NEGATIVE: parameter only used through quote_literal/quote_ident + create function public.quoted(tbl text) returns int + language plpgsql + security definer + as $$ + declare r int; + begin + execute 'select count(*)::int from ' || quote_ident(tbl) into r; + return r; + end $$; + select * from lint."0032_authenticated_dynamic_sql_function_executable"; + + -- NEGATIVE: no EXECUTE at all, even with a SQL-shaped parameter name + create function public.search(query text) returns int + language plpgsql + security definer + as $$ + begin + return length(query); + end $$; + select * from lint."0032_authenticated_dynamic_sql_function_executable"; + + -- NEGATIVE: EXECUTE only mentioned in a comment + create function public.commented(query text) returns int + language plpgsql + as $$ + begin + -- we used to execute query here + return length(query); + end $$; + select * from lint."0032_authenticated_dynamic_sql_function_executable"; + + -- NEGATIVE: the parameter is only bound with USING; the variable holds + -- static SQL + create function public.static_var(p_name text) returns int + language plpgsql + security definer + as $$ + declare q text := 'select count(*)::int from auth.users where email = $1'; r int; + begin + execute q into r using p_name; + return r; + end $$; + select * from lint."0032_authenticated_dynamic_sql_function_executable"; + + -- NEGATIVE: the parameter is quoted before it is put in a variable + create function public.quoted_var(p_table text) returns int + language plpgsql + security definer + as $$ + declare q text; r int; + begin + q := 'select count(*)::int from ' || quote_ident(p_table); + execute q into r; + return r; + end $$; + select * from lint."0032_authenticated_dynamic_sql_function_executable"; + + -- NEGATIVE: parameters passed to format() only in %I / %L slots + create function public.fmt_quoted(p_table text, p_val text) returns int + language plpgsql + security definer + as $$ + declare r int; + begin + execute format('select count(*)::int from %I where name = %L', p_table, p_val) into r; + return r; + end $$; + select * from lint."0032_authenticated_dynamic_sql_function_executable"; + + -- POSITIVE (ERROR): the second argument goes into a %s slot + create function public.fmt_unquoted(p_table text, p_val text) returns int + language plpgsql + security definer + as $$ + declare r int; + begin + execute format('select count(*)::int from %I where name = %s', p_table, p_val) into r; + return r; + end $$; + select name, level, metadata->'sql_parameters' as sql_parameters, cache_key + from lint."0032_authenticated_dynamic_sql_function_executable"; + + rollback to savepoint a; + + savepoint case_definer; + + ---------------------------------------- + -- POSITIVE (ERROR): the exec_sql shape. SECURITY DEFINER, the + -- caller-supplied string is concatenated into an EXECUTE. + ---------------------------------------- + create function public.exec_sql(query text) returns json + language plpgsql + security definer + as $$ + declare r json; + begin + execute 'select json_agg(t) from (' || query || ') t' into r; + return r; + end $$; + + select + name, + level, + detail, + metadata->>'security_definer' as security_definer, + metadata->'sql_parameters' as sql_parameters, + cache_key + from lint."0032_authenticated_dynamic_sql_function_executable"; + + ---------------------------------------- + -- RESOLUTION: revoke EXECUTE from authenticated and PUBLIC + ---------------------------------------- + revoke execute on function public.exec_sql(text) from authenticated, public; + select * from lint."0032_authenticated_dynamic_sql_function_executable"; + + rollback to savepoint case_definer; + + savepoint case_invoker; + + ---------------------------------------- + -- POSITIVE (WARN): same shape but SECURITY INVOKER, the function runs + -- as authenticated. Not covered by lint 0029. + ---------------------------------------- + create function public.run_query(sql text) returns void + language plpgsql + security invoker + as $$ + begin + execute sql; + end $$; + + select name, level, metadata->>'security_definer' as security_definer, cache_key + from lint."0032_authenticated_dynamic_sql_function_executable"; + + rollback to savepoint case_invoker; + + savepoint case_renamed; + + ---------------------------------------- + -- POSITIVE (WARN): renamed wrapper that copies the SQL parameter into a + -- local variable before EXECUTE, so it is not referenced directly. + ---------------------------------------- + create function public.insert_entries(sql text) returns void + language plpgsql + security definer + as $$ + declare stmt text; + begin + stmt := sql; + execute stmt; + end $$; + + select name, level, metadata->'sql_parameters' as sql_parameters, cache_key + from lint."0032_authenticated_dynamic_sql_function_executable"; + + rollback to savepoint case_renamed; + + savepoint case_unnamed; + + ---------------------------------------- + -- POSITIVE (ERROR): unnamed parameter used as `EXECUTE $1` + ---------------------------------------- + create function public.exec_unnamed(text) returns void + language plpgsql + security definer + as $$ + begin + execute $1; + end $$; + + select name, level, metadata->'sql_parameters' as sql_parameters, cache_key + from lint."0032_authenticated_dynamic_sql_function_executable"; + + rollback to savepoint case_unnamed; + + savepoint case_positional; + + ---------------------------------------- + -- POSITIVE (ERROR): named parameter referenced as $1 in the EXECUTE + ---------------------------------------- + create function public.run_positional(p text) returns void + language plpgsql + security definer + as $$ + begin + execute $1; + end $$; + + select name, level, metadata->'sql_parameters' as sql_parameters, cache_key + from lint."0032_authenticated_dynamic_sql_function_executable"; + + rollback to savepoint case_positional; + + savepoint case_variable; + + ---------------------------------------- + -- POSITIVE (ERROR): the parameter is built into a local variable, either + -- by assignment or in the declaration, and the variable is EXECUTEd. + ---------------------------------------- + create function public.count_rows(p_table text) returns int + language plpgsql + security definer + as $$ + declare q text; r int; + begin + q := 'select count(*)::int from ' || p_table; + execute q into r; + return r; + end $$; + create function public.count_rows_declared(p_table text) returns int + language plpgsql + security definer + as $$ + declare q text := 'select count(*)::int from ' || p_table; r int; + begin + execute q into r; + return r; + end $$; + + select name, level, metadata->'name' as func_name, cache_key + from lint."0032_authenticated_dynamic_sql_function_executable"; + + rollback to savepoint case_variable; + + savepoint case_using_in_literal; + + ---------------------------------------- + -- POSITIVE (ERROR): the SQL text contains the word `using` before the + -- concatenated parameter (DELETE ... USING, CREATE INDEX ... USING). + -- These are not EXECUTE's own USING clause and must not hide the + -- parameter. The `;` inside the literal must not hide it either. + ---------------------------------------- + create function public.exec_delete_using(query text) returns void + language plpgsql + security definer + as $$ + begin + execute 'delete from public.t using public.u where ' || query; + end $$; + create function public.exec_index_using(query text) returns void + language plpgsql + security definer + as $$ + begin + execute 'select 1; create index on public.t using gin (' || query || ')'; + end $$; + + select name, level, metadata->'name' as func_name, cache_key + from lint."0032_authenticated_dynamic_sql_function_executable"; + + rollback to savepoint case_using_in_literal; + + savepoint case_variadic; + + ---------------------------------------- + -- POSITIVE (ERROR): VARIADIC text[] parameter used in an EXECUTE + ---------------------------------------- + create function public.exec_many(variadic queries text[]) returns void + language plpgsql + security definer + as $$ + begin + execute queries[1]; + end $$; + + select name, level, metadata->'sql_parameters' as sql_parameters, cache_key + from lint."0032_authenticated_dynamic_sql_function_executable"; + + rollback to savepoint case_variadic; + + savepoint case_system_schema; + + ---------------------------------------- + -- NEGATIVE: function in a system schema is excluded + ---------------------------------------- + create function auth.exec_sql(query text) returns void + language plpgsql + security definer + as $$ + begin + execute query; + end $$; + select * from lint."0032_authenticated_dynamic_sql_function_executable"; + + rollback to savepoint case_system_schema; + +rollback; diff --git a/test/sql/queries_are_unionable.sql b/test/sql/queries_are_unionable.sql index 3c5c39d..6169d02 100644 --- a/test/sql/queries_are_unionable.sql +++ b/test/sql/queries_are_unionable.sql @@ -58,6 +58,10 @@ begin; union all select * from lint."0029_authenticated_security_definer_function_executable" union all - select * from lint."0030_autovacuum_disabled"; + select * from lint."0030_autovacuum_disabled" + union all + select * from lint."0031_anon_dynamic_sql_function_executable" + union all + select * from lint."0032_authenticated_dynamic_sql_function_executable"; rollback;