Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

Hi!

On Thu, Aug 22, 2024 at 7:33 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> In response to some concerns raised about this fix on the
> pgsql-release list today, I spent some time investigating this patch.
> Unfortunately, I think there are too many problems here to be
> reasonably fixed before release, and I think all of SPLIT/MERGE
> PARTITION needs to be reverted.
>
> I focused my investigation on createPartitionTable(), which is a
> helper for both SPLIT PARTITION and MERGE PARTITION, and it works by
> consing up a CREATE TABLE AS statement and then feeding that back
> through
> ProcessUtility. I think it's bad design to use such a high-level
> facility here; it is unlike what we do elsewhere in tablecmds.c and
> opens us up to a variety of problems. The first thing that I
> discovered is that this patch does not fix all of the repeated name
> lookup problems. There is still this:
>
> tlc->relation =
> makeRangeVar(get_namespace_name(RelationGetNamespace(modelRel)),
> RelationGetRelationName(modelRel), -1);
>
> And also this:
>
> createStmt->tablespacename =
> get_tablespace_name(modelRel->rd_rel->reltablespace);
>
> In both cases, we do a reverse lookup on an OID to get a name which
> the CREATE TABLE code will later turn back into an OID. If we don't
> get the same value, that's at least a bug and probably a security
> vulnerability, and there is no way to be certain that we will get the
> same value. The only remedy is to not repeat the lookup in the first
> place.
>
> Then I got to looking at this:
>
> tlc->options = CREATE_TABLE_LIKE_ALL &
> ~(CREATE_TABLE_LIKE_INDEXES | CREATE_TABLE_LIKE_IDENTITY |
> CREATE_TABLE_LIKE_STATISTICS);
>
> It's not obvious at first glance that there is a critical problem
> here, but there are reasons to be nervous. We're deploying a lot of
> machinery here to copy a lot of stuff and, while that's efficient from
> a coding perspective, it means that stuff you might not expect can
> just kind of happen. For instance:
>
> robert.haas=# \d+
> List of relations
> Schema | Name | Type | Owner | Persistence |
> Access method | Size | Description
> --------+------+-------------------+-------------+-------------+---------------+------------+-------------
> public | foo | partitioned table | robert.haas | permanent |
> | 0 bytes |
> public | foo1 | table | robert.haas | permanent | heap
> | 8192 bytes |
> public | foo2 | table | bob | permanent | heap
> | 8192 bytes |
> (3 rows)
> robert.haas=# alter table foo split partition foo2 into (partition
> foo3 for values from (10) to (15), partition foo4 for values from (15)
> to (20));
> ALTER TABLE
> robert.haas=# \d+
> List of relations
> Schema | Name | Type | Owner | Persistence |
> Access method | Size | Description
> --------+------+-------------------+-------------+-------------+---------------+------------+-------------
> public | foo | partitioned table | robert.haas | permanent |
> | 0 bytes |
> public | foo1 | table | robert.haas | permanent | heap
> | 8192 bytes |
> public | foo3 | table | robert.haas | permanent | heap
> | 8192 bytes |
> public | foo4 | table | robert.haas | permanent | heap
> | 8192 bytes |
> (4 rows)
>
> I've split a partition owned by bob into two partitions owned by
> robert.haas. That's rather surprising. It doesn't work to split a
> partition that I don't own (and thus gain access to it) but if the
> superuser splits a non-superuser's partition, the superuser ends
> upowning the new partitions. I don't know if that's a vulnerability or
> just unexpected. However, then I found this, which I'm pretty well
> certain is a vulnerability:
>
> robert.haas=# set role bob;
> SET
> robert.haas=> create table foo (a int, b text) partition by range (a);
> CREATE TABLE
> robert.haas=> create table foo1 partition of foo for values from (0) to (10);
> CREATE TABLE
> robert.haas=> create table foo2 partition of foo for values from (10) to (20);
> CREATE TABLE
> robert.haas=> insert into foo values (11, 'carrots'), (16, 'pineapple');
> INSERT 0 2
> robert.haas=> create or replace function run_me(integer) returns
> integer as $$begin raise notice 'you are running me as %',
> current_user; return $1; end$$ language plpgsql immutable;
> CREATE FUNCTION
> robert.haas=> create index on foo (run_me(a));
> NOTICE: you are running me as bob
> NOTICE: you are running me as bob
> CREATE INDEX
> robert.haas=> reset role;
> RESET
> robert.haas=# alter table foo split partition foo2 into (partition
> foo3 for values from (10) to (15), partition foo4 for values from (15)
> to (20));
> NOTICE: you are running me as robert.haas
> NOTICE: you are running me as robert.haas
> ALTER TABLE
>
> I think it is very unlikely that the problems mentioned above are the
> only ones. They're just what I found in an hour or two of testing.
> Even if they were, we're probably too close to release to be rushing
> out last minute fixes to multiple unanticipated security problems. But
> because of the design that was chosen here, I think there is probably
> more stuff here that is not right, some of which is security relevant
> and some of which is just a question of whether we're really getting
> the behavior that we want. And I don't think we can fix all that
> without either a very large number of grotty hacks similar to the one
> installed by 04158e7fa37c2dda9c3421ca922d02807b86df19, or a complete
> redesign of the feature. I believe the latter is probably a wiser
> course of action.

Thank you for your feedback. Yes, it seems that there is not enough
time to even carefully analyze all the issues in these features. The
rule of thumb I can get from this experience is "think multiple times
before accessing something already opened by its name". I'm going to
revert these features during next couple days.

------
Regards,
Alexander Korotkov
Supabase