Horton

Thread

Commits

  1. Fix crashes with CREATE SCHEMA AUTHORIZATION and schema elements

  1. BUG #17909: CREATE SCHEMA AUTHORIZATION sch CREATE TABLE foo ( id INT ) will coredump

    The Post Office <noreply@postgresql.org> — 2023-04-27T03:44:04Z

    The following bug has been logged on the website:

Bug reference:      17909
Logged by:          Song Hongyu
Email address:      hysong0101@163.com
PostgreSQL version: 15.0
Operating system:   centos7
Description:        

When we CREATE SCHEMA AUTHORIZATION rolname CREATE TABLE/SEQUENCE/VIEW
sch.obj and sch is not NULL,
the database will coredump.

The reason is in parse_utilcmd.c we will check whether schemaName and
rolname are same or not. The pointer is not checked in strcmp there, so the
database coredump is caused.

  2. Re: BUG #17909: CREATE SCHEMA AUTHORIZATION sch CREATE TABLE foo ( id INT ) will coredump

    Michael Paquier <michael@paquier.xyz> — 2023-04-27T07:34:23Z

    On Thu, Apr 27, 2023 at 03:44:04AM +0000, PG Bug reporting form wrote:
> When we CREATE SCHEMA AUTHORIZATION rolname CREATE TABLE/SEQUENCE/VIEW
> sch.obj and sch is not NULL,
> the database will coredump.

It took me a couple of minutes to get what you meant here.  The point
is that schema-qualifying any of the object specified after the CREATE
SCHEMA with a schema name different than the rolname would cause a
crash, when no schema is directly given.  We should fail with the same
error than when a schema is specified, as of, except that the rolename
needs to be specified:
=# create schema popo authorization postgres create table lala.aa (a int);
ERROR:  42P15: CREATE specifies a schema (lala) different from the one being created (popo)

That seems quite old, at quick glance (v11 fails), so this needs to be
fixed all the way down.  Will fix, nice catch!
--
Michael

  3. Re: BUG #17909: CREATE SCHEMA AUTHORIZATION sch CREATE TABLE foo ( id INT ) will coredump

    Richard Guo <guofenglinux@gmail.com> — 2023-04-27T08:59:13Z

    On Thu, Apr 27, 2023 at 3:34 PM Michael Paquier <michael@paquier.xyz> wrote:

> It took me a couple of minutes to get what you meant here.  The point
> is that schema-qualifying any of the object specified after the CREATE
> SCHEMA with a schema name different than the rolname would cause a
> crash, when no schema is directly given.  We should fail with the same
> error than when a schema is specified, as of, except that the rolename
> needs to be specified:
> =# create schema popo authorization postgres create table lala.aa (a int);
> ERROR:  42P15: CREATE specifies a schema (lala) different from the one
> being created (popo)


Aha, now I get the scenario that would crash.

# create schema authorization postgres create table lala.aa (a int);
server closed the connection unexpectedly

In this case the CreateSchemaStmtContext.schemaname is NULL since it is
not explicitly specified, while the schemaname in the schema element is
not NULL as it is specified, and setSchemaName cannot copy with such
situation.  Maybe we should check against RoleSpec.rolename in this case
since that is also the schema's name?


> That seems quite old, at quick glance (v11 fails), so this needs to be
> fixed all the way down.


Yes.  I can see this crash from master all back to v9.5.

Thanks
Richard

  4. Re: BUG #17909: CREATE SCHEMA AUTHORIZATION sch CREATE TABLE foo ( id INT ) will coredump

    Michael Paquier <michael@paquier.xyz> — 2023-04-27T09:31:01Z

    On Thu, Apr 27, 2023 at 04:59:13PM +0800, Richard Guo wrote:
> In this case the CreateSchemaStmtContext.schemaname is NULL since it is
> not explicitly specified, while the schemaname in the schema element is
> not NULL as it is specified, and setSchemaName cannot copy with such
> situation.  Maybe we should check against RoleSpec.rolename in this case
> since that is also the schema's name?

In this case, it is cleaner to just set the schema name in
CreateSchemaStmtContext.schemaname to the role in the RoleSpec if there
is no schema set in the query, because the schema name will have the
same name as the role.  That also makes the handling of each element
in schemaElts simpler.

The regression tests cruelly lacks of checks here.  This is not a
pattern of CREATE SCHEMA known a lot, but we should do better.
--
Michael

  5. Re: BUG #17909: CREATE SCHEMA AUTHORIZATION sch CREATE TABLE foo ( id INT ) will coredump

    Richard Guo <guofenglinux@gmail.com> — 2023-04-27T10:28:28Z

    On Thu, Apr 27, 2023 at 5:31 PM Michael Paquier <michael@paquier.xyz> wrote:

> On Thu, Apr 27, 2023 at 04:59:13PM +0800, Richard Guo wrote:
> > In this case the CreateSchemaStmtContext.schemaname is NULL since it is
> > not explicitly specified, while the schemaname in the schema element is
> > not NULL as it is specified, and setSchemaName cannot copy with such
> > situation.  Maybe we should check against RoleSpec.rolename in this case
> > since that is also the schema's name?
>
> In this case, it is cleaner to just set the schema name in
> CreateSchemaStmtContext.schemaname to the role in the RoleSpec if there
> is no schema set in the query, because the schema name will have the
> same name as the role.  That also makes the handling of each element
> in schemaElts simpler.


I noticed that in CreateSchemaCommand there is logic that fills schema
name with the role name if it is not specified.  Do you think we can
save the new-filled schema name into CreateSchemaStmt.schemaname there?


> The regression tests cruelly lacks of checks here.  This is not a
> pattern of CREATE SCHEMA known a lot, but we should do better.


Agreed.  It's better to have a case covering this pattern of CREATE
SCHEMA.

Thanks
Richard

  6. Re: BUG #17909: CREATE SCHEMA AUTHORIZATION sch CREATE TABLE foo ( id INT ) will coredump

    Michael Paquier <michael@paquier.xyz> — 2023-04-27T23:05:22Z

    On Thu, Apr 27, 2023 at 06:28:28PM +0800, Richard Guo wrote:
> I noticed that in CreateSchemaCommand there is logic that fills schema
> name with the role name if it is not specified.  Do you think we can
> save the new-filled schema name into CreateSchemaStmt.schemaname there?

It's actually much trickier than that, on second look, as a RoleSpec
is embedded in this portion of a query because we need to support
CURRENT_ROLE, CURRENT_SESSION and SESSION_USER.  For example, this
query assigns neither role name nor schema name we could rely on at
transformation for the objects:
create schema authorization current_role create table aa (a int);

Anyway, semantically, something could go very wrong if we decide to
enforce a schema name for the objects based on the RoleSpec at the
time of transformation, because we may finish by executing the CREATE
SCHEMA command under an entirely different context than what we could
assign.  So, I think that we should do the following in the
transformation path if an object is schema-qualified:
- Fail immediately if there is no schema and no role name at hand,
just give up.  This needs a new error message, say:
"CREATE specifies a schema (%object_schema) without providing a schema
name."
- If there is a role name, aka RoleSpec points to a ROLESPEC_CSTRING,
use it as a comparison with the objects schema-qualified.  Note that
there is no case for public, because we would fail on
get_rolespec_oid() when the schema is created.

There is also a very fancy case, if "foo" matches to the role that
would be assigned by GetUserIdAndSecContext() when executing the
schema command:
create schema authorization session_role create table foo.aa (a int);

One could say that this should work, and my proposal would cause an
error to make the query more predictible at an earlier step.  IMO, I
think that this is just saner.  And this case crashes today like the
others.

Any thoughts or objections about doing that?
--
Michael

  7. Re: BUG #17909: CREATE SCHEMA AUTHORIZATION sch CREATE TABLE foo ( id INT ) will coredump

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-04-27T23:48:49Z

    Michael Paquier <michael@paquier.xyz> writes:
> It's actually much trickier than that, on second look, as a RoleSpec
> is embedded in this portion of a query because we need to support
> CURRENT_ROLE, CURRENT_SESSION and SESSION_USER.  For example, this
> query assigns neither role name nor schema name we could rely on at
> transformation for the objects:
> create schema authorization current_role create table aa (a int);

Seems like the answer needs to involve postponing examination of
the contained DDL until we've figured out what the new schema's
name is going to be.

			regards, tom lane

  8. Re: BUG #17909: CREATE SCHEMA AUTHORIZATION sch CREATE TABLE foo ( id INT ) will coredump

    Michael Paquier <michael@paquier.xyz> — 2023-04-27T23:56:27Z

    On Thu, Apr 27, 2023 at 07:48:49PM -0400, Tom Lane wrote:
> Seems like the answer needs to involve postponing examination of
> the contained DDL until we've figured out what the new schema's
> name is going to be.

Actually, wait a min..  The transformation of the objects is applied
during the execution of the CREATE SCHEMA command, but nowhere else,
so if you give to transformCreateSchemaStmt() the name of the expected
schema rather than rely on the schema name from the query this should
work OK.
--
Michael

  9. Re: BUG #17909: CREATE SCHEMA AUTHORIZATION sch CREATE TABLE foo ( id INT ) will coredump

    Michael Paquier <michael@paquier.xyz> — 2023-04-28T01:42:54Z

    On Fri, Apr 28, 2023 at 08:56:27AM +0900, Michael Paquier wrote:
> Actually, wait a min..  The transformation of the objects is applied
> during the execution of the CREATE SCHEMA command, but nowhere else,
> so if you give to transformCreateSchemaStmt() the name of the expected
> schema rather than rely on the schema name from the query this should
> work OK.

So, the source of my confusion is the design currently used for
transformCreateSchemaStmt():
- The schema name is extracted from the query itself, but we have a
schema compiled from a role specification, depending on how the
beginning of CreateSchemaCommand() feels it.
- This routine includes a reference to the role specification in the
context, but makes no use of it.  Perhaps somebody would be
interested in this information in the future if the query support is
improved, but one could also be tempted to feed the schema name based
on the RoleSpec, which I'd rather avoid for the moment.

Attached is what I am finishing with, where I have reworked
transformCreateSchemaStmt() so as it uses in input the list of
elements from CREATE SCHEMA and the schema name computed depending on
the security context, documenting requirements on the way (note the
extra unconstify for the RangeVars' schemas).  I have added a couple
of regression tests for all the object types that have schema
qualication checks, mixed with role specs and schema names.

Thoughts, comments or objections?
--
Michael

  10. Re: BUG #17909: CREATE SCHEMA AUTHORIZATION sch CREATE TABLE foo ( id INT ) will coredump

    Richard Guo <guofenglinux@gmail.com> — 2023-04-28T03:29:11Z

    On Fri, Apr 28, 2023 at 9:43 AM Michael Paquier <michael@paquier.xyz> wrote:

> Attached is what I am finishing with, where I have reworked
> transformCreateSchemaStmt() so as it uses in input the list of
> elements from CREATE SCHEMA and the schema name computed depending on
> the security context, documenting requirements on the way (note the
> extra unconstify for the RangeVars' schemas).  I have added a couple
> of regression tests for all the object types that have schema
> qualication checks, mixed with role specs and schema names.
>
> Thoughts, comments or objections?


+1.  I like the refactor of transformCreateSchemaStmt.

BTW, the comment states that CreateSchemaStmtContext.stmtType is "CREATE
SCHEMA" or "ALTER SCHEMA".   But it seems that there is no chance to set
it to "ALTER SCHEMA".  So should we update that comment, or go even
further to remove CreateSchemaStmtContext.stmtType since it is not used?

Thanks
Richard

  11. Re: BUG #17909: CREATE SCHEMA AUTHORIZATION sch CREATE TABLE foo ( id INT ) will coredump

    Michael Paquier <michael@paquier.xyz> — 2023-04-28T03:56:38Z

    On Fri, Apr 28, 2023 at 11:29:11AM +0800, Richard Guo wrote:
> BTW, the comment states that CreateSchemaStmtContext.stmtType is "CREATE
> SCHEMA" or "ALTER SCHEMA".   But it seems that there is no chance to set
> it to "ALTER SCHEMA".  So should we update that comment, or go even
> further to remove CreateSchemaStmtContext.stmtType since it is not used?

Indeed.  I'd be OK with adjusting the comment, without removing
stmtType to keep some consistency with CreateStmt, and it could be
useful for debugging, perhaps..  (See 46379d6 as one origin point).
--
Michael

  12. Re: BUG #17909: CREATE SCHEMA AUTHORIZATION sch CREATE TABLE foo ( id INT ) will coredump

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-04-28T04:53:39Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Apr 28, 2023 at 11:29:11AM +0800, Richard Guo wrote:
>> BTW, the comment states that CreateSchemaStmtContext.stmtType is "CREATE
>> SCHEMA" or "ALTER SCHEMA".   But it seems that there is no chance to set
>> it to "ALTER SCHEMA".  So should we update that comment, or go even
>> further to remove CreateSchemaStmtContext.stmtType since it is not used?

> Indeed.  I'd be OK with adjusting the comment, without removing
> stmtType to keep some consistency with CreateStmt, and it could be
> useful for debugging, perhaps..  (See 46379d6 as one origin point).

I'd be okay with just dropping that field.  It seems to be much
older than 46379d6, and if it ever had any real use it doesn't now.
(There's no ALTER SCHEMA in the SQL spec at all, let alone one that
has some overlap with CREATE SCHEMA options, so I don't foresee a
future use either.)

			regards, tom lane

  13. Re: BUG #17909: CREATE SCHEMA AUTHORIZATION sch CREATE TABLE foo ( id INT ) will coredump

    Michael Paquier <michael@paquier.xyz> — 2023-04-28T05:07:36Z

    On Fri, Apr 28, 2023 at 12:53:39AM -0400, Tom Lane wrote:
> I'd be okay with just dropping that field.  It seems to be much
> older than 46379d6, and if it ever had any real use it doesn't now.
> (There's no ALTER SCHEMA in the SQL spec at all, let alone one that
> has some overlap with CREATE SCHEMA options, so I don't foresee a
> future use either.)

WFM as well.
--
Michael

  14. Re: BUG #17909: CREATE SCHEMA AUTHORIZATION sch CREATE TABLE foo ( id INT ) will coredump

    Michael Paquier <michael@paquier.xyz> — 2023-04-30T22:09:50Z

    On Fri, Apr 28, 2023 at 02:07:36PM +0900, Michael Paquier wrote:
> WFM as well.

Note that this has been applied as of 4dadd66 down to v11.  Thanks for
the report, the reviews and the discussion!
--
Michael