From c6b90aac8ade705eba522d7a3ca473f68351e5ff Mon Sep 17 00:00:00 2001 From: Raul Marin Date: Thu, 3 Oct 2019 13:59:51 +0200 Subject: [PATCH 01/10] GhostTables: Set secure search_path --- scripts-available/CDB_GhostTables.sql | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/scripts-available/CDB_GhostTables.sql b/scripts-available/CDB_GhostTables.sql index d402618..42bc7b1 100644 --- a/scripts-available/CDB_GhostTables.sql +++ b/scripts-available/CDB_GhostTables.sql @@ -63,7 +63,11 @@ AS $$ PERFORM @extschema@._CDB_LinkGhostTables(username, db_name, event_name); RAISE NOTICE '_CDB_LinkGhostTables() called with username=%, event_name=%', username, event_name; END; -$$ LANGUAGE plpgsql VOLATILE PARALLEL UNSAFE SECURITY DEFINER; +$$ LANGUAGE plpgsql + VOLATILE + PARALLEL UNSAFE + SECURITY DEFINER + SET search_path = @extschema@, pg_temp; -- Trigger function to call CDB_LinkGhostTables() CREATE OR REPLACE FUNCTION @extschema@._CDB_LinkGhostTablesTrigger() @@ -76,7 +80,11 @@ AS $$ PERFORM @extschema@.CDB_LinkGhostTables(ddl_tag); RETURN NULL; END; -$$ LANGUAGE plpgsql VOLATILE PARALLEL UNSAFE SECURITY DEFINER; +$$ LANGUAGE plpgsql + VOLATILE + PARALLEL UNSAFE + SECURITY DEFINER + SET search_path = @extschema@, pg_temp; -- Event trigger to save the current transaction in @extschema@.cdb_ddl_execution CREATE OR REPLACE FUNCTION @extschema@.CDB_SaveDDLTransaction() @@ -85,7 +93,11 @@ AS $$ BEGIN INSERT INTO @extschema@.cdb_ddl_execution VALUES (txid_current(), tg_tag) ON CONFLICT ON CONSTRAINT cdb_ddl_execution_pkey DO NOTHING; END; -$$ LANGUAGE plpgsql VOLATILE PARALLEL UNSAFE SECURITY DEFINER; +$$ LANGUAGE plpgsql + VOLATILE + PARALLEL UNSAFE + SECURITY DEFINER + SET search_path = @extschema@, pg_temp; -- Creates the trigger on DDL events to link ghost tables CREATE OR REPLACE FUNCTION @extschema@.CDB_EnableGhostTablesTrigger() From 4dfd71639a0f9db91a6631061e323ada79b34c71 Mon Sep 17 00:00:00 2001 From: Raul Marin Date: Thu, 3 Oct 2019 15:14:40 +0200 Subject: [PATCH 02/10] CDB_OAuthReassignTableOwnerOnCreation: Use CDB_Conf_GetConf Instead of accessing the cdb_conf table directly --- scripts-available/CDB_OAuth.sql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts-available/CDB_OAuth.sql b/scripts-available/CDB_OAuth.sql index 29f3e39..f2258f1 100644 --- a/scripts-available/CDB_OAuth.sql +++ b/scripts-available/CDB_OAuth.sql @@ -17,11 +17,11 @@ BEGIN obj.schema_name, obj.object_identity; SELECT rolname FROM pg_class JOIN pg_roles ON relowner = pg_roles.oid WHERE pg_class.oid = obj.objid INTO creator_role; - SELECT value->>'ownership_role_name' from cdb_conf where key = 'api_keys_' || creator_role INTO owner_role; + SELECT value->>'ownership_role_name' from @extschema@.CDB_Conf_GetConf('api_keys_' || quote_ident(creator_role)) value INTO owner_role; IF owner_role IS NULL OR owner_role = '' THEN CONTINUE; ELSE - EXECUTE 'ALTER ' || obj.object_type || ' ' || obj.object_identity || ' OWNER TO ' || QUOTE_IDENT(owner_role); + EXECUTE 'ALTER ' || obj.object_type || ' ' || obj.object_identity || ' OWNER TO ' || quote_ident(owner_role); EXECUTE 'GRANT ALL ON ' || obj.object_identity || ' TO ' || QUOTE_IDENT(creator_role); RAISE DEBUG 'Changing ownership from % to %', creator_role, owner_role; END IF; From 0898881470175e1d1e90219072d22aa4d4ed6c30 Mon Sep 17 00:00:00 2001 From: Raul Marin Date: Thu, 3 Oct 2019 16:53:02 +0200 Subject: [PATCH 03/10] Oauth: Set secure search_path --- scripts-available/CDB_OAuth.sql | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/scripts-available/CDB_OAuth.sql b/scripts-available/CDB_OAuth.sql index f2258f1..330da4d 100644 --- a/scripts-available/CDB_OAuth.sql +++ b/scripts-available/CDB_OAuth.sql @@ -1,7 +1,6 @@ -- Function that reassign the owner of a table to their ownership_role CREATE OR REPLACE FUNCTION @extschema@.CDB_OAuthReassignTableOwnerOnCreation() RETURNS event_trigger - SECURITY DEFINER AS $$ DECLARE obj record; @@ -27,7 +26,11 @@ BEGIN END IF; END LOOP; END; -$$ LANGUAGE plpgsql VOLATILE PARALLEL UNSAFE; +$$ LANGUAGE plpgsql + VOLATILE + PARALLEL UNSAFE + SECURITY DEFINER + SET search_path = @extschema@, pg_temp; -- Creates the trigger on DDL events in order to reassign the owner CREATE OR REPLACE FUNCTION @extschema@.CDB_EnableOAuthReassignTablesTrigger() From 186ee37a570b2baaaafcb200ee4c6f3462f57f66 Mon Sep 17 00:00:00 2001 From: Raul Marin Date: Thu, 3 Oct 2019 16:54:55 +0200 Subject: [PATCH 04/10] CDB_Username: Set secure search_path --- scripts-available/CDB_Username.sql | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/scripts-available/CDB_Username.sql b/scripts-available/CDB_Username.sql index adc1427..7aa3c77 100644 --- a/scripts-available/CDB_Username.sql +++ b/scripts-available/CDB_Username.sql @@ -2,5 +2,9 @@ CREATE OR REPLACE FUNCTION @extschema@.CDB_Username() RETURNS text AS $$ - SELECT @extschema@.CDB_Conf_GetConf(CONCAT('api_keys_', session_user))->>'username'; -$$ LANGUAGE SQL STABLE PARALLEL SAFE SECURITY DEFINER; + SELECT @extschema@.CDB_Conf_GetConf(concat('api_keys_', session_user))->>'username'; +$$ LANGUAGE SQL + STABLE + PARALLEL SAFE + SECURITY DEFINER + SET search_path = @extschema@, pg_temp; From 52b3290d26ef6e18c0b16d60fd9b595f171588de Mon Sep 17 00:00:00 2001 From: Raul Marin Date: Thu, 3 Oct 2019 17:27:40 +0200 Subject: [PATCH 05/10] CDB_TableMetadata: Use secure search_path --- scripts-available/CDB_TableMetadata.sql | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/scripts-available/CDB_TableMetadata.sql b/scripts-available/CDB_TableMetadata.sql index 4d68831..8d401d3 100644 --- a/scripts-available/CDB_TableMetadata.sql +++ b/scripts-available/CDB_TableMetadata.sql @@ -43,7 +43,7 @@ BEGIN ); WITH nv as ( - SELECT TG_RELID as tabname, NOW() as t + SELECT TG_RELID as tabname, now() as t ), updated as ( UPDATE @extschema@.CDB_TableMetadata x SET updated_at = nv.t FROM nv WHERE x.tabname = nv.tabname @@ -55,8 +55,11 @@ BEGIN RETURN NULL; END; -$$ -LANGUAGE plpgsql VOLATILE PARALLEL UNSAFE SECURITY DEFINER; +$$ LANGUAGE plpgsql + VOLATILE + PARALLEL UNSAFE + SECURITY DEFINER + SET search_path = @extschema@, pg_temp; -- -- Trigger invalidating varnish whenever CDB_TableMetadata @@ -116,8 +119,11 @@ BEGIN RETURN NULL; END; -$$ -LANGUAGE plpgsql VOLATILE PARALLEL UNSAFE SECURITY DEFINER; +$$ LANGUAGE plpgsql + VOLATILE + PARALLEL UNSAFE + SECURITY DEFINER + SET search_path = @extschema@, pg_temp; DROP TRIGGER IF EXISTS table_modified ON @extschema@.CDB_TableMetadata; -- NOTE: on DELETE we would be unable to convert the table From 0e891eff7f7c0b8242426e10e96f8396f4c89fff Mon Sep 17 00:00:00 2001 From: Raul Marin Date: Thu, 3 Oct 2019 17:29:35 +0200 Subject: [PATCH 06/10] Remove cartodb from search path All calls to the extension functions should, and are, be properly qualified, so there is no need to keep the cartodb schema in the search_path --- scripts-available/CDB_GhostTables.sql | 6 +++--- scripts-available/CDB_OAuth.sql | 2 +- scripts-available/CDB_TableMetadata.sql | 4 ++-- scripts-available/CDB_Username.sql | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/scripts-available/CDB_GhostTables.sql b/scripts-available/CDB_GhostTables.sql index 42bc7b1..0b72087 100644 --- a/scripts-available/CDB_GhostTables.sql +++ b/scripts-available/CDB_GhostTables.sql @@ -67,7 +67,7 @@ $$ LANGUAGE plpgsql VOLATILE PARALLEL UNSAFE SECURITY DEFINER - SET search_path = @extschema@, pg_temp; + SET search_path = pg_temp; -- Trigger function to call CDB_LinkGhostTables() CREATE OR REPLACE FUNCTION @extschema@._CDB_LinkGhostTablesTrigger() @@ -84,7 +84,7 @@ $$ LANGUAGE plpgsql VOLATILE PARALLEL UNSAFE SECURITY DEFINER - SET search_path = @extschema@, pg_temp; + SET search_path = pg_temp; -- Event trigger to save the current transaction in @extschema@.cdb_ddl_execution CREATE OR REPLACE FUNCTION @extschema@.CDB_SaveDDLTransaction() @@ -97,7 +97,7 @@ $$ LANGUAGE plpgsql VOLATILE PARALLEL UNSAFE SECURITY DEFINER - SET search_path = @extschema@, pg_temp; + SET search_path = pg_temp; -- Creates the trigger on DDL events to link ghost tables CREATE OR REPLACE FUNCTION @extschema@.CDB_EnableGhostTablesTrigger() diff --git a/scripts-available/CDB_OAuth.sql b/scripts-available/CDB_OAuth.sql index 330da4d..ee0a617 100644 --- a/scripts-available/CDB_OAuth.sql +++ b/scripts-available/CDB_OAuth.sql @@ -30,7 +30,7 @@ $$ LANGUAGE plpgsql VOLATILE PARALLEL UNSAFE SECURITY DEFINER - SET search_path = @extschema@, pg_temp; + SET search_path = pg_temp; -- Creates the trigger on DDL events in order to reassign the owner CREATE OR REPLACE FUNCTION @extschema@.CDB_EnableOAuthReassignTablesTrigger() diff --git a/scripts-available/CDB_TableMetadata.sql b/scripts-available/CDB_TableMetadata.sql index 8d401d3..c9b3cd5 100644 --- a/scripts-available/CDB_TableMetadata.sql +++ b/scripts-available/CDB_TableMetadata.sql @@ -59,7 +59,7 @@ $$ LANGUAGE plpgsql VOLATILE PARALLEL UNSAFE SECURITY DEFINER - SET search_path = @extschema@, pg_temp; + SET search_path = pg_temp; -- -- Trigger invalidating varnish whenever CDB_TableMetadata @@ -123,7 +123,7 @@ $$ LANGUAGE plpgsql VOLATILE PARALLEL UNSAFE SECURITY DEFINER - SET search_path = @extschema@, pg_temp; + SET search_path = pg_temp; DROP TRIGGER IF EXISTS table_modified ON @extschema@.CDB_TableMetadata; -- NOTE: on DELETE we would be unable to convert the table diff --git a/scripts-available/CDB_Username.sql b/scripts-available/CDB_Username.sql index 7aa3c77..59a2557 100644 --- a/scripts-available/CDB_Username.sql +++ b/scripts-available/CDB_Username.sql @@ -7,4 +7,4 @@ $$ LANGUAGE SQL STABLE PARALLEL SAFE SECURITY DEFINER - SET search_path = @extschema@, pg_temp; + SET search_path = pg_temp; From a580bedefc7b216b20af61948db3cc0e55e45cf4 Mon Sep 17 00:00:00 2001 From: Raul Marin Date: Thu, 3 Oct 2019 17:37:11 +0200 Subject: [PATCH 07/10] Set safe schema on some functions --- scripts-available/CDB_AnalysisCheck.sql | 7 +++++-- scripts-available/CDB_EstimateRowCount.sql | 7 ++++++- scripts-available/CDB_Overviews.sql | 8 ++++++-- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/scripts-available/CDB_AnalysisCheck.sql b/scripts-available/CDB_AnalysisCheck.sql index 8645967..9800f7d 100644 --- a/scripts-available/CDB_AnalysisCheck.sql +++ b/scripts-available/CDB_AnalysisCheck.sql @@ -6,8 +6,11 @@ $$ BEGIN RETURN @extschema@.CDB_Conf_GetConf('analysis_quota_factor')::text::float8; END; -$$ -LANGUAGE 'plpgsql' STABLE PARALLEL SAFE SECURITY DEFINER; +$$ LANGUAGE 'plpgsql' + STABLE + PARALLEL SAFE + SECURITY DEFINER + SET search_path = pg_temp; -- Get the factor (fraction of the quota) for Camshaft cached analysis tables diff --git a/scripts-available/CDB_EstimateRowCount.sql b/scripts-available/CDB_EstimateRowCount.sql index 06e65de..a7e3f51 100644 --- a/scripts-available/CDB_EstimateRowCount.sql +++ b/scripts-available/CDB_EstimateRowCount.sql @@ -12,7 +12,12 @@ BEGIN EXECUTE Format('ANALYZE %s;', reloid); END IF; END -$$ LANGUAGE 'plpgsql' VOLATILE STRICT PARALLEL UNSAFE SECURITY DEFINER; +$$ LANGUAGE 'plpgsql' + VOLATILE + STRICT + PARALLEL UNSAFE + SECURITY DEFINER + SET search_path = pg_temp; -- Return a row count estimate of the result of a query using statistics CREATE OR REPLACE FUNCTION @extschema@.CDB_EstimateRowCount(query text) diff --git a/scripts-available/CDB_Overviews.sql b/scripts-available/CDB_Overviews.sql index 216c294..ec4d307 100644 --- a/scripts-available/CDB_Overviews.sql +++ b/scripts-available/CDB_Overviews.sql @@ -318,7 +318,7 @@ $$ LANGUAGE PLPGSQL VOLATILE PARALLEL UNSAFE; -- This function is declared SECURITY DEFINER so it executes with the privileges -- of the function creator to have a chance to alter the privileges of the -- overview table to match those of the dataset. It will only perform any change --- if the overview table belgons to the same scheme as the dataset and it +-- if the overview table belongs to the same scheme as the dataset and it -- matches the scheme naming for overview tables. CREATE OR REPLACE FUNCTION @extschema@._CDB_Register_Overview(dataset REGCLASS, overview_table REGCLASS, overview_z INTEGER) RETURNS VOID @@ -362,7 +362,11 @@ AS $$ -- it should be done here (CDB_Overviews would consume such metadata) END IF; END -$$ LANGUAGE PLPGSQL VOLATILE PARALLEL UNSAFE SECURITY DEFINER; +$$ LANGUAGE PLPGSQL + VOLATILE + PARALLEL UNSAFE + SECURITY DEFINER + SET search_path = pg_temp; -- Dataset attributes (column names other than the -- CartoDB primary key and geometry columns) which should be aggregated From 5a1203360977bd823363641b85cdc8ebbf1caf0a Mon Sep 17 00:00:00 2001 From: Raul Marin Date: Thu, 3 Oct 2019 17:57:09 +0200 Subject: [PATCH 08/10] Adapt tests to changes --- test/extension/test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extension/test.sh b/test/extension/test.sh index 73491ec..d23b4b0 100755 --- a/test/extension/test.sh +++ b/test/extension/test.sh @@ -362,7 +362,7 @@ function test_cdb_tablemetadatatouch_fails_from_user_without_permission() { function test_cdb_tablemetadatatouch_fully_qualifies_names() { sql postgres "CREATE TABLE touch_invalidations (table_name text);" - sql postgres "create or replace function cartodb.cdb_invalidate_varnish(table_name text) returns void as \$\$ begin insert into touch_invalidations select table_name; end; \$\$ language 'plpgsql';" + sql postgres "create or replace function cartodb.cdb_invalidate_varnish(table_name text) returns void as \$\$ begin insert into public.touch_invalidations select table_name; end; \$\$ language 'plpgsql';" #default schema sql "CREATE TABLE touch_example (a int);" From 048234cd807cc6ea6cec920ec9b90ced52e17c05 Mon Sep 17 00:00:00 2001 From: Raul Marin Date: Thu, 3 Oct 2019 18:51:28 +0200 Subject: [PATCH 09/10] CDB_Groups_API: Secure search_path --- scripts-available/CDB_Groups_API.sql | 42 +++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/scripts-available/CDB_Groups_API.sql b/scripts-available/CDB_Groups_API.sql index 370e4b8..eda868a 100644 --- a/scripts-available/CDB_Groups_API.sql +++ b/scripts-available/CDB_Groups_API.sql @@ -22,7 +22,11 @@ $$ body = '{ "name": "%s", "database_role": "%s" }' % (group_name, group_role) query = "select @extschema@._CDB_Group_API_Request('POST', '%s', '%s', '{200, 409}') as response_status" % (url, body) plpy.execute(query) -$$ LANGUAGE 'plpythonu' VOLATILE PARALLEL UNSAFE SECURITY DEFINER; +$$ LANGUAGE 'plpythonu' + VOLATILE + PARALLEL UNSAFE + SECURITY DEFINER + SET search_path = pg_temp; CREATE OR REPLACE FUNCTION @extschema@._CDB_Group_DropGroup_API(group_name text) @@ -35,7 +39,11 @@ $$ query = "select @extschema@._CDB_Group_API_Request('DELETE', '%s', '', '{204, 404}') as response_status" % url plpy.execute(query) -$$ LANGUAGE 'plpythonu' VOLATILE PARALLEL UNSAFE SECURITY DEFINER; +$$ LANGUAGE 'plpythonu' + VOLATILE + PARALLEL UNSAFE + SECURITY DEFINER + SET search_path = pg_temp; CREATE OR REPLACE FUNCTION @extschema@._CDB_Group_RenameGroup_API(old_group_name text, new_group_name text, new_group_role text) @@ -48,7 +56,11 @@ $$ body = '{ "name": "%s", "database_role": "%s" }' % (new_group_name, new_group_role) query = "select @extschema@._CDB_Group_API_Request('PUT', '%s', '%s', '{200, 409}') as response_status" % (url, body) plpy.execute(query) -$$ LANGUAGE 'plpythonu' VOLATILE PARALLEL UNSAFE SECURITY DEFINER; +$$ LANGUAGE 'plpythonu' + VOLATILE + PARALLEL UNSAFE + SECURITY DEFINER + SET search_path = pg_temp; CREATE OR REPLACE FUNCTION @extschema@._CDB_Group_AddUsers_API(group_name text, usernames text[]) @@ -61,7 +73,11 @@ $$ body = "{ \"users\": [\"%s\"] }" % "\",\"".join(usernames) query = "select @extschema@._CDB_Group_API_Request('POST', '%s', '%s', '{200, 409}') as response_status" % (url, body) plpy.execute(query) -$$ LANGUAGE 'plpythonu' VOLATILE SECURITY DEFINER; +$$ LANGUAGE 'plpythonu' + VOLATILE + PARALLEL UNSAFE + SECURITY DEFINER + SET search_path = pg_temp; CREATE OR REPLACE FUNCTION @extschema@._CDB_Group_RemoveUsers_API(group_name text, usernames text[]) @@ -74,7 +90,11 @@ $$ body = "{ \"users\": [\"%s\"] }" % "\",\"".join(usernames) query = "select @extschema@._CDB_Group_API_Request('DELETE', '%s', '%s', '{200, 404}') as response_status" % (url, body) plpy.execute(query) -$$ LANGUAGE 'plpythonu' VOLATILE PARALLEL UNSAFE SECURITY DEFINER; +$$ LANGUAGE 'plpythonu' + VOLATILE + PARALLEL UNSAFE + SECURITY DEFINER + SET search_path = pg_temp; DO LANGUAGE 'plpgsql' $$ BEGIN @@ -95,7 +115,11 @@ $$ body = '{ "access": "%s" }' % access query = "select @extschema@._CDB_Group_API_Request('PUT', '%s', '%s', '{200, 409}') as response_status" % (url, body) plpy.execute(query) -$$ LANGUAGE 'plpythonu' VOLATILE PARALLEL UNSAFE SECURITY DEFINER; +$$ LANGUAGE 'plpythonu' + VOLATILE + PARALLEL UNSAFE + SECURITY DEFINER + SET search_path = pg_temp; DO LANGUAGE 'plpgsql' $$ BEGIN @@ -115,7 +139,11 @@ $$ url = '/api/v1/databases/{0}/groups/%s/permission/%s/tables/%s' % (urllib.pathname2url(group_name), username, table_name) query = "select @extschema@._CDB_Group_API_Request('DELETE', '%s', '', '{200, 404}') as response_status" % url plpy.execute(query) -$$ LANGUAGE 'plpythonu' VOLATILE PARALLEL UNSAFE SECURITY DEFINER; +$$ LANGUAGE 'plpythonu' + VOLATILE + PARALLEL UNSAFE + SECURITY DEFINER + SET search_path = pg_temp; DO LANGUAGE 'plpgsql' $$ BEGIN From 2533d0996cd004d7b882ee334ae197a3b951e890 Mon Sep 17 00:00:00 2001 From: Raul Marin Date: Thu, 3 Oct 2019 18:59:01 +0200 Subject: [PATCH 10/10] Add NEWs --- NEWS.md | 1 + 1 file changed, 1 insertion(+) diff --git a/NEWS.md b/NEWS.md index fc39c37..8c86279 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,6 @@ 0.31.0 (XXXX-XX-XX) * Ghost tables: Add missing tags (#370) +* Set search_path in security definer functions. 0.30.0 (2019-07-17) * Added new admin functions to connect CARTO with user FDW's (#369)