arkhipov / temporal_tables

Temporal Tables PostgreSQL Extension
BSD 2-Clause "Simplified" License
927 stars 46 forks source link

Added SECURITY DFINER to versioning() and set_system_time(). #12

Closed AndrewRademacher closed 8 years ago

AndrewRademacher commented 8 years ago

I've added SECURITY DEFINER to the exported functions in temporal tables to support row-level security features in Postgres 9.5. I have a motivating example here. The goal is to use row-level security to enable a role to have full control over their data on a table of current values, and be able to fully query their history. However, they should be completely unable to manually make edits to their own data if it resides in the history table.

With this setup and the current version of temporal tables, the test script here produces the following error.

Verifying postgrest_demo
  * public/extensions/uuid-ossp ................. ok
  * postgrest/schema ............................ ok
  * postgrest/roles/anonymous ................... ok
  * postgrest/roles/account ..................... ok
  * postgrest/tables/account .................... ok
  * postgrest/functions/register ................ ok
  * postgrest/functions/get-current-account-id .. ok
  * postgrest/tables/private-logs @v1.0.0 ....... ok
  * public/extensions/temporal-tables ........... ok
  * postgrest/tables/event-space ................ psql:verify/postgrest/tables/event-space.sql:52: WARNING:  system period value of relation "event_space" was adjusted
CONTEXT:  SQL statement "DELETE
   FROM event_space
   WHERE event_space_id = esid"
PL/pgSQL function inline_code_block line 37 at SQL statement
psql:verify/postgrest/tables/event-space.sql:52: ERROR:  permission denied for relation event_space_history
CONTEXT:  SQL statement "INSERT INTO postgrest.event_space_history (event_space_id, name, address, description, valid_between, owned_by) VALUES ($1, $2, $3, $4, $5, $6)"
SQL statement "DELETE
   FROM event_space
   WHERE event_space_id = esid"
PL/pgSQL function inline_code_block line 37 at SQL statement
# Verify script "verify/postgrest/tables/event-space.sql" failed.
not ok

Verify Summary Report
---------------------
Changes: 10
Errors:  1
Verify failed

This occurs because the Postgres default is to run a function as SECURITY INVOKER, and the current role does not have the rights to edit its own history table. Switching to SECURITY DEFINER would make this example possible.


Please let me know if I need to do anything to comply with your versioning rules or code conventions.

Additional Interested Parties: @joeandaverde, @smerchek.

arkhipov commented 8 years ago

I do not think adding SECURITY DEFINER is a good idea. If you add it, one can use it to get access to tables they do not have access to. Since the extension is created by a superuser, SECURITY DEFINER will mean that the trigger function will be invoked with privileges of the superuser. So anyone could create a table, add a trigger on it and write to any table they pass as the history_relation argument of the trigger.

I cannot see any simple solution for your case. You probably could restrict access to your data table and wrap a code modifying the table within a function with SECURITY DEFINER.

AndrewRademacher commented 8 years ago

I see what you are saying, that problem didn't occur to me because in my system the roles that are not already superusers have not been granted the privilege to define tables or triggers. My hunch then is that I will have to wrap the version function in another function that has security definer, but doesn't take the history table as an input. Then have one of those functions per table.

arkhipov commented 8 years ago

Even though your users have no rights to define tables, you cannot restrict them from creating temporary tables and adding triggers on them. So they would still be able to exploit this security hole.

It would be easier if Postgres had something like SECURITY TABLE OWNER for trigger functions.

Another option is to create an updatable view for every table and use RULEs to propagate the changes to the associated tables.

AndrewRademacher commented 8 years ago

Yeah, my thought was something like...

CREATE FUNCTION version_event_table()
    RETURNING TRIGGER AS $$
    RETURN versioning('valid_between', 'event_history', true);
$$ LANGUAGE plpgsql SECURITY DEFINER;

Which naturally didn't work because you can't compose trigger functions. So my next thought would be to alter the versioning function to security definer in my project only, and never grant execute to that function. My unproven assumption is that if a super user defines a trigger, then a non-superuser inserts to a table the trigger will still be able to execute the versioning function. And if a non-superuser tries to define a trigger that calls the versioning function they won't actually be able to execute the trigger.

AndrewRademacher commented 8 years ago

What I mentioned I would try in the previous post does seem to work. In the following test script, roles under postgrest-account which is produced by the register_account function are not even able to define a trigger if it contains a function they do not have execute rights to.

-- Verify postgrest-demo:public/functions/versioning-permissions on pg

BEGIN;

-- Prove that temporary tables of postgrest_accounts cannot execute versioning
-- function.

SET search_path TO postgrest, public;

DO $$
DECLARE
   rstr text;
BEGIN
   SET ROLE postgrest_anonymous;
   rstr := register_account('andrew.rademacher@smrxt.com', 'Andrew Rademacher', '12345');

   EXECUTE format('SET ROLE %s;', rstr);

   CREATE TEMPORARY TABLE temp_event_space(
      event_space_id       uuid           NOT NULL DEFAULT uuid_generate_v4(),
      name                 text           NOT NULL,
      address              text           NOT NULL,
      description          text,

      valid_between        tstzrange      NOT NULL DEFAULT tstzrange(current_timestamp, null),
      owned_by             uuid           NOT NULL DEFAULT get_current_account_id()
   );

   CREATE TRIGGER tr_versioning_test
   BEFORE INSERT OR UPDATE OR DELETE ON temp_event_space
      FOR EACH ROW EXECUTE PROCEDURE versioning(
         'valid_between', 'event_space_history', true
      );
END$$;

ROLLBACK;

Result...

 * public/functions/versioning-permissions ..... psql:verify/public/functions/versioning-permissions.sql:34: ERROR:  permission denied for function versioning
CONTEXT:  SQL statement "CREATE TRIGGER tr_versioning_test
   BEFORE INSERT OR UPDATE OR DELETE ON temp_event_space
      FOR EACH ROW EXECUTE PROCEDURE versioning(
         'valid_between', 'event_space_history', true
      )"