pgsql: Revoke PUBLIC CREATE from public schema, now owned by pg_databas

Noah Misch <noah@leadboat.com> writes:

Revoke PUBLIC CREATE from public schema, now owned by pg_database_owner.

I've just stumbled across a testing problem created by this commit:
if you try to skip the tablespace test, the rest of the run falls
over, because this bit doesn't get executed:

-- Rest of this suite can use the public schema freely.
GRANT ALL ON SCHEMA public TO public;

Skipping the tablespace test is something I've been accustomed to do
when testing replication with the standby on the same machine as the
primary, because otherwise you've got to fool with keeping the
standby from overwriting the primary's tablespaces. This hack made
that a lot more painful.

I'm inclined to think the cleanest fix is to move this step into a
new script, say "test_setup.sql", that is scheduled by itself just
after tablespace.sql. It's sort of annoying to fire up a psql+backend
for just one command, but perhaps there's other stuff that could be
put there too.

Another possibility is to add that GRANT to the list of stuff that
pg_regress.c does by default. If there's actually reason for
tablespace.sql to run without that, it could revoke and re-grant
the public permissions. This way would have the advantage of
being less likely to break other test suites.

regards, tom lane

On Fri, Dec 17, 2021 at 12:52:39PM -0500, Tom Lane wrote:

Noah Misch <noah@leadboat.com> writes:

Revoke PUBLIC CREATE from public schema, now owned by pg_database_owner.

I've just stumbled across a testing problem created by this commit:
if you try to skip the tablespace test, the rest of the run falls
over, because this bit doesn't get executed:

-- Rest of this suite can use the public schema freely.
GRANT ALL ON SCHEMA public TO public;

Skipping the tablespace test is something I've been accustomed to do
when testing replication with the standby on the same machine as the
primary, because otherwise you've got to fool with keeping the
standby from overwriting the primary's tablespaces. This hack made
that a lot more painful.

I'm inclined to think the cleanest fix is to move this step into a
new script, say "test_setup.sql", that is scheduled by itself just
after tablespace.sql.

I like that solution for your use case.

It's sort of annoying to fire up a psql+backend
for just one command, but perhaps there's other stuff that could be
put there too.

Yes. The src/test/regress suite would be in a better place if one could run
most test files via a schedule containing only two files, the setup file and
the file of interest. Adding things like the "CREATE TABLE tenk1" to the
setup file would help that.

Noah Misch <noah@leadboat.com> writes:

On Fri, Dec 17, 2021 at 12:52:39PM -0500, Tom Lane wrote:

It's sort of annoying to fire up a psql+backend
for just one command, but perhaps there's other stuff that could be
put there too.

Yes. The src/test/regress suite would be in a better place if one could run
most test files via a schedule containing only two files, the setup file and
the file of interest. Adding things like the "CREATE TABLE tenk1" to the
setup file would help that.

If we're thinking of a generalized setup file, putting it after the
tablespace test feels pretty weird. What was your motivation for
doing this at the end of tablespace.source rather than the start?
It doesn't look like that test in itself had any interesting
dependencies on public not being writable.

regards, tom lane

On Fri, Dec 17, 2021 at 01:41:00PM -0500, Tom Lane wrote:

Noah Misch <noah@leadboat.com> writes:

On Fri, Dec 17, 2021 at 12:52:39PM -0500, Tom Lane wrote:

It's sort of annoying to fire up a psql+backend
for just one command, but perhaps there's other stuff that could be
put there too.

Yes. The src/test/regress suite would be in a better place if one could run
most test files via a schedule containing only two files, the setup file and
the file of interest. Adding things like the "CREATE TABLE tenk1" to the
setup file would help that.

If we're thinking of a generalized setup file, putting it after the
tablespace test feels pretty weird. What was your motivation for
doing this at the end of tablespace.source rather than the start?

I did it that way so a bit of the "make check" suite would exercise the
standard user experience. That's a minor concern, so putting the setup file
before the tablespace file is fine. Various contrib and TAP suites will still
test the standard user experience.

It doesn't look like that test in itself had any interesting
dependencies on public not being writable.

Right.

Noah Misch <noah@leadboat.com> writes:

On Fri, Dec 17, 2021 at 01:41:00PM -0500, Tom Lane wrote:

If we're thinking of a generalized setup file, putting it after the
tablespace test feels pretty weird. What was your motivation for
doing this at the end of tablespace.source rather than the start?

I did it that way so a bit of the "make check" suite would exercise the
standard user experience. That's a minor concern, so putting the setup file
before the tablespace file is fine. Various contrib and TAP suites will still
test the standard user experience.

Check. I'll make it so in a little bit.

regards, tom lane

On Fri, Sep 10, 2021 at 2:39 AM Noah Misch <noah@leadboat.com> wrote:

Revoke PUBLIC CREATE from public schema, now owned by pg_database_owner.

This switches the default ACL to what the documentation has recommended
since CVE-2018-1058. Upgrades will carry forward any old ownership and
ACL. Sites that declined the 2018 recommendation should take a fresh
look. Recipes for commissioning a new database cluster from scratch may
need to create a schema, grant more privileges, etc. Out-of-tree test
suites may require such updates.

I was looking at the changes that this commit made to ddl.sgml today
and I feel that it's not quite ideal. Under "Constrain ordinary users
to user-private schemas" it first says "To implement this, first issue
<literal>REVOKE CREATE ON SCHEMA public FROM PUBLIC</literal>" and
then later says, oh but wait, you actually don't need to do that
unless you're upgrading. That seems a bit backwards to me: I think we
should talk about the current state of play first, and then add the
notes about upgrading afterwards.

Here's a proposed patch to do that.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, Nov 29, 2022 at 02:22:59PM -0500, Robert Haas wrote:

Here's a proposed patch to do that.

If I'm not wrong, you message includes a diffstat but without the patch
itself.

On Tue, Nov 29, 2022 at 2:32 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

On Tue, Nov 29, 2022 at 02:22:59PM -0500, Robert Haas wrote:

Here's a proposed patch to do that.

If I'm not wrong, you message includes a diffstat but without the patch
itself.

D'oh.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, Nov 29, 2022 at 02:22:59PM -0500, Robert Haas wrote:

On Fri, Sep 10, 2021 at 2:39 AM Noah Misch <noah@leadboat.com> wrote:

Revoke PUBLIC CREATE from public schema, now owned by pg_database_owner.

This switches the default ACL to what the documentation has recommended
since CVE-2018-1058. Upgrades will carry forward any old ownership and
ACL. Sites that declined the 2018 recommendation should take a fresh
look. Recipes for commissioning a new database cluster from scratch may
need to create a schema, grant more privileges, etc. Out-of-tree test
suites may require such updates.

I was looking at the changes that this commit made to ddl.sgml today
and I feel that it's not quite ideal. Under "Constrain ordinary users
to user-private schemas" it first says "To implement this, first issue
<literal>REVOKE CREATE ON SCHEMA public FROM PUBLIC</literal>" and
then later says, oh but wait, you actually don't need to do that
unless you're upgrading. That seems a bit backwards to me: I think we
should talk about the current state of play first, and then add the
notes about upgrading afterwards.

In general, the documentation should prefer simpler decision trees.
Especially so where the wrong choice causes no error, yet leaves a security
vulnerability. The unconditional REVOKE has no drawbacks; it's harmless where
it's a no-op. That was the rationale behind the current text. Upgrades
aren't the only issue; another DBA may have changed the ACL since initdb.

On Wed, Nov 30, 2022 at 2:07 AM Noah Misch <noah@leadboat.com> wrote:

In general, the documentation should prefer simpler decision trees.

True, but I found the current text confusing, which is also something
to consider.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Nov 30, 2022 at 08:39:23AM -0500, Robert Haas wrote:

On Wed, Nov 30, 2022 at 2:07 AM Noah Misch <noah@leadboat.com> wrote:

In general, the documentation should prefer simpler decision trees.

True, but I found the current text confusing, which is also something
to consider.

Could remove the paragraph about v14. Could have that paragraph say
explicitly that the REVOKE is a no-op. Would either of those be an
improvement?

On Wed, Nov 30, 2022 at 10:01 AM Noah Misch <noah@leadboat.com> wrote:

On Wed, Nov 30, 2022 at 08:39:23AM -0500, Robert Haas wrote:

On Wed, Nov 30, 2022 at 2:07 AM Noah Misch <noah@leadboat.com> wrote:

In general, the documentation should prefer simpler decision trees.

True, but I found the current text confusing, which is also something
to consider.

Could remove the paragraph about v14. Could have that paragraph say
explicitly that the REVOKE is a no-op. Would either of those be an
improvement?

Well, I thought what I proposed was a nice improvement, but I guess if
you don't like it I'm not inclined to spend a lot of time discussing
other possibilities. If we get some opinions from more people that may
make it clearer which direction to go; if I'm the only one that
doesn't like the way it is now, it's probably not that important.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Nov 30, 2022 at 10:01 AM Noah Misch <noah@leadboat.com> wrote:

Could remove the paragraph about v14. Could have that paragraph say
explicitly that the REVOKE is a no-op. Would either of those be an
improvement?

Well, I thought what I proposed was a nice improvement, but I guess if
you don't like it I'm not inclined to spend a lot of time discussing
other possibilities. If we get some opinions from more people that may
make it clearer which direction to go; if I'm the only one that
doesn't like the way it is now, it's probably not that important.

Hey, I'll step up to the plate ;-)

I agree that it's confusing to tell people to do a REVOKE that might do
nothing. A parenthetical note explaining that might help, but the text
is pretty dense already, so really I'd rather have that info in a
separate para.

Also, I'd like to structure things so that the first para covers what
you need to know in a clean v15+ installation, and details that only
apply in upgrade scenarios are in the second para. The upgrade scenario
is going to be interesting to fewer and fewer people over time, so let's
not clutter the lede with it.

So maybe about like this?

Constrain ordinary users to user-private schemas. To implement
this pattern, for every user needing to create non-temporary
objects, create a schema with the same name as that user. (Recall
that the default search path starts with $user, which resolves to
the user name. Therefore, if each user has a separate schema, they
access their own schemas by default.) Also ensure that no other
schemas have public CREATE privileges. This pattern is a secure
schema usage pattern unless an untrusted user is the database
owner or holds the CREATEROLE privilege, in which case no secure
schema usage pattern exists.

In PostgreSQL 15 and later, the default configuration supports
this usage pattern. In prior versions, or when using a database
that has been upgraded from a prior version, you will need to
remove the public CREATE privilege from the public schema (issue
REVOKE CREATE ON SCHEMA public FROM PUBLIC). Then consider
auditing the public schema for objects named like objects in
schema pg_catalog.

This is close to what Robert wrote, but not exactly the same,
so probably it will make neither of you happy ;-)

BTW, is "create a schema with the same name" sufficient detail?
You have to either make it owned by that user, or explicitly
grant CREATE permission on it. I'm not sure if that detail
belongs here, but it feels like maybe it does.

regards, tom lane

On Wed, 30 Nov 2022 at 17:35, Tom Lane <tgl@sss.pgh.pa.us> wrote:

BTW, is "create a schema with the same name" sufficient detail?

You have to either make it owned by that user, or explicitly
grant CREATE permission on it. I'm not sure if that detail
belongs here, but it feels like maybe it does.

It might be worth mentioning AUTHORIZATION. The easiest way to create an
appropriately named schema for a user is "CREATE SCHEMA AUTHORIZATION
username".

On Wed, Nov 30, 2022 at 3:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

BTW, is "create a schema with the same name" sufficient detail?
You have to either make it owned by that user, or explicitly
grant CREATE permission on it. I'm not sure if that detail
belongs here, but it feels like maybe it does.

I'd mention the ownership variant and suggest using the AUTHORIZATION
clause, with an explicit example.

CREATE SCHEMA role_name AUTHORIZATION role_name;

David J.

On Wed, Nov 30, 2022 at 5:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Also, I'd like to structure things so that the first para covers what
you need to know in a clean v15+ installation, and details that only
apply in upgrade scenarios are in the second para. The upgrade scenario
is going to be interesting to fewer and fewer people over time, so let's
not clutter the lede with it.

Right, that was my main feeling about this.

So maybe about like this?

Constrain ordinary users to user-private schemas. To implement
this pattern, for every user needing to create non-temporary
objects, create a schema with the same name as that user. (Recall
that the default search path starts with $user, which resolves to
the user name. Therefore, if each user has a separate schema, they
access their own schemas by default.) Also ensure that no other
schemas have public CREATE privileges. This pattern is a secure
schema usage pattern unless an untrusted user is the database
owner or holds the CREATEROLE privilege, in which case no secure
schema usage pattern exists.

In PostgreSQL 15 and later, the default configuration supports
this usage pattern. In prior versions, or when using a database
that has been upgraded from a prior version, you will need to
remove the public CREATE privilege from the public schema (issue
REVOKE CREATE ON SCHEMA public FROM PUBLIC). Then consider
auditing the public schema for objects named like objects in
schema pg_catalog.

This is close to what Robert wrote, but not exactly the same,
so probably it will make neither of you happy ;-)

I haven't looked at how it's different from what I wrote exactly, but
it seems fine to me.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Nov 30, 2022 at 05:35:01PM -0500, Tom Lane wrote:

Also, I'd like to structure things so that the first para covers what
you need to know in a clean v15+ installation, and details that only
apply in upgrade scenarios are in the second para. The upgrade scenario
is going to be interesting to fewer and fewer people over time, so let's
not clutter the lede with it.

So maybe about like this?

Constrain ordinary users to user-private schemas. To implement
this pattern, for every user needing to create non-temporary
objects, create a schema with the same name as that user. (Recall
that the default search path starts with $user, which resolves to
the user name. Therefore, if each user has a separate schema, they
access their own schemas by default.) Also ensure that no other
schemas have public CREATE privileges. This pattern is a secure
schema usage pattern unless an untrusted user is the database
owner or holds the CREATEROLE privilege, in which case no secure
schema usage pattern exists.

This is free from the problem found in ddl-create-public-reorg-really.patch.
However, the word "other" doesn't belong there. (The per-user schemas should
not have public CREATE privilege.) I would also move that same sentence up
front, like this:

Constrain ordinary users to user-private schemas. To implement this
pattern, first ensure that no schemas have public CREATE privileges.
Then, for every user needing to create non-temporary objects, create a
schema with the same name as that user. (Recall that the default search
path starts with $user, which resolves to the user name. Therefore, if
each user has a separate schema, they access their own schemas by
default.) This pattern is a secure schema usage pattern unless an
untrusted user is the database owner or holds the CREATEROLE privilege, in
which case no secure schema usage pattern exists.

With that, I think you have improved on the status quo. Thanks.

In PostgreSQL 15 and later, the default configuration supports
this usage pattern. In prior versions, or when using a database
that has been upgraded from a prior version, you will need to
remove the public CREATE privilege from the public schema (issue
REVOKE CREATE ON SCHEMA public FROM PUBLIC). Then consider
auditing the public schema for objects named like objects in
schema pg_catalog.

BTW, is "create a schema with the same name" sufficient detail?
You have to either make it owned by that user, or explicitly
grant CREATE permission on it. I'm not sure if that detail
belongs here, but it feels like maybe it does.

Maybe. Failing to GRANT that will yield a clear error when the user starts
work, so it's not critical to explain here.

On 2022-Dec-01, Noah Misch wrote:

This is free from the problem found in ddl-create-public-reorg-really.patch.
However, the word "other" doesn't belong there. (The per-user schemas should
not have public CREATE privilege.) I would also move that same sentence up
front, like this:

Constrain ordinary users to user-private schemas. To implement this
pattern, first ensure that no schemas have public CREATE privileges.
Then, for every user needing to create non-temporary objects, create a
schema with the same name as that user. (Recall that the default search
path starts with $user, which resolves to the user name. Therefore, if
each user has a separate schema, they access their own schemas by
default.) This pattern is a secure schema usage pattern unless an
untrusted user is the database owner or holds the CREATEROLE privilege, in
which case no secure schema usage pattern exists.

+1 LGTM

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

On 2022-Dec-01, Noah Misch wrote:

This is free from the problem found in ddl-create-public-reorg-really.patch.
However, the word "other" doesn't belong there. (The per-user schemas should
not have public CREATE privilege.) I would also move that same sentence up
front, like this:

Constrain ordinary users to user-private schemas. To implement this
pattern, first ensure that no schemas have public CREATE privileges.
Then, for every user needing to create non-temporary objects, create a
schema with the same name as that user. (Recall that the default search
path starts with $user, which resolves to the user name. Therefore, if
each user has a separate schema, they access their own schemas by
default.) This pattern is a secure schema usage pattern unless an
untrusted user is the database owner or holds the CREATEROLE privilege, in
which case no secure schema usage pattern exists.

+1 LGTM

Sounds good. I'll make it so in a bit.

regards, tom lane