CATUPDATE confusion?

On Mon, Nov 24, 2014 at 04:28:46PM -0500, Adam Brightwell wrote:

So, where I find this confusing is that the documentation supports this
functionality and the check keeps superuser separate from CATUPDATE...
however... comments and implementation in user.c state/do the opposite and
couple them together.

Documentation:
http://www.postgresql.org/docs/9.4/static/catalog-pg-authid.html - "Role
can update system catalogs directly. (Even a superuser cannot do this
unless this column is true)"

src/backend/commands/user.c

/* superuser gets catupdate right by default */
new_record[Anum_pg_authid_rolcatupdate - 1] = BoolGetDatum(issuper);

and...

/*
* issuper/createrole/catupdate/etc
*
* XXX It's rather unclear how to handle catupdate. It's probably best to
* keep it equal to the superuser status, otherwise you could end up with
* a situation where no existing superuser can alter the catalogs,
* including pg_authid!
*/
if (issuper >= 0)
{
new_record[Anum_pg_authid_rolsuper - 1] = BoolGetDatum(issuper > 0);
new_record_repl[Anum_pg_authid_rolsuper - 1] = true;

new_record[Anum_pg_authid_rolcatupdate - 1] = BoolGetDatum(issuper > 0);
new_record_repl[Anum_pg_authid_rolcatupdate - 1] = true;
}

That documentation is correct as far it goes. It does neglect to mention
that, as you have discovered, any CREATE ROLE or ALTER ROLE [NO]SUPERUSER will
change rolcatupdate to match.

Perhaps this is not an issue but it seemed odd to me, especially after
giving Peter's comment more thought. So, I suppose the question is whether
or not a superuser check should be added to 'has_rolcatupdate' or not? I

I would not. PostgreSQL has had this feature since day one (original name
"usecatupd"). It has two use cases, (1) giving effect to non-superuser
catalog grants and (2) preventing a narrow class of superuser mistakes. We
wouldn't add such a thing today, but one can safely ignore its existence.
Making has_rolcatupdate() approve superusers unconditionally would exclude use
case (2). Neither use case is valuable, but if I had to pick, (2) is more
valuable than (1).

believe I understand the reasoning for coupling the two at role
creation/alter time, however, is there really a case where a superuser
wouldn't be allowed to bypass this check? Based on the comments, there
seems like there is potentially enough concern to allow it. And if it is
allowed, couldn't CATUPDATE then be treated like every other attribute and
the coupling with superuser removed? Thoughts?

A superuser always has _some_ way to bypass the check, if nothing else by
loading a C-language function to update pg_authid. The user.c code you quoted
ensures that, if the DBA manages to remove rolcatupdate from every user, a
simple CREATE ROLE or ALTER ROLE can fix things.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Noah Misch (noah@leadboat.com) wrote:

On Mon, Nov 24, 2014 at 04:28:46PM -0500, Adam Brightwell wrote:

Perhaps this is not an issue but it seemed odd to me, especially after
giving Peter's comment more thought. So, I suppose the question is whether
or not a superuser check should be added to 'has_rolcatupdate' or not? I

I would not. PostgreSQL has had this feature since day one (original name
"usecatupd"). It has two use cases, (1) giving effect to non-superuser
catalog grants and (2) preventing a narrow class of superuser mistakes. We
wouldn't add such a thing today, but one can safely ignore its existence.
Making has_rolcatupdate() approve superusers unconditionally would exclude use
case (2). Neither use case is valuable, but if I had to pick, (2) is more
valuable than (1).

What's confusing about this is that use-case (2) appears to have been
the intent of the option, but because it's tied directly to superuser
and isn't an independently controllable option, the only way change it
is to do an 'update pg_authid ...'. Even when set that way, it isn't
visible that it's been set through \du, you have to query the catalog
directly.

I wonder if the option had originally been intended to be independent
and *not* given to superusers by default, but because of the concern
about dealing with corrupt systems, etc, it was never actually done.
I'm fine with the line of thinking that says we can't deny superusers
this power because of corruption/debugging concerns, but if that's the
case, then there is no use-case (2) and we should just remove the option
entirely.

I don't buy into use-case (1) above being at all reasonable. With it,
one can trivially make themselves a superuser, and in many ways the
catupdate capability is *more* dangerous than superuser- if you have
catupdate but not superuser, you'd be tempted to hack at the catalog to
make the changes you want instead of using the regular ALTER TABLE, etc,
commands.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

* Noah Misch (noah@leadboat.com) wrote:

On Mon, Nov 24, 2014 at 04:28:46PM -0500, Adam Brightwell wrote:

Perhaps this is not an issue but it seemed odd to me, especially after
giving Peter's comment more thought. So, I suppose the question is whether
or not a superuser check should be added to 'has_rolcatupdate' or not? I

I would not. PostgreSQL has had this feature since day one (original name
"usecatupd"). It has two use cases, (1) giving effect to non-superuser
catalog grants and (2) preventing a narrow class of superuser mistakes. We
wouldn't add such a thing today, but one can safely ignore its existence.
Making has_rolcatupdate() approve superusers unconditionally would exclude use
case (2). Neither use case is valuable, but if I had to pick, (2) is more
valuable than (1).

What's confusing about this is that use-case (2) appears to have been
the intent of the option,

Yes. Noah's claim that anybody intended (1) seems to be mere historical
revisionism, especially since as you note CATUPDATE could be trivially
parlayed into superuser if someone were to grant it to a non-superuser.

but because it's tied directly to superuser
and isn't an independently controllable option, the only way change it
is to do an 'update pg_authid ...'. Even when set that way, it isn't
visible that it's been set through \du, you have to query the catalog
directly.

This is not hard to understand either: the option dates from long before
there was any concerted effort to invent bespoke SQL commands for every
interesting catalog manipulation. If CATUPDATE had any significant use,
we'd have extended ALTER USER (and \du, and pg_dump ...) at some point
to make it more easily controllable. But since it's of such marginal
usefulness, nobody ever bothered; and I doubt we should bother now.

In short, I agree with Noah's conclusion (do nothing) though not his
reasoning.

Plan C (remove CATUPDATE altogether) also has some merit. But adding a
superuser override there would be entirely pointless.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Plan C (remove CATUPDATE altogether) also has some merit. But adding a
superuser override there would be entirely pointless.

This is be my recommendation. I do not see the point of carrying the
option around just to confuse new users of pg_authid and reviewers of
the code.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Plan C (remove CATUPDATE altogether) also has some merit. But adding a
superuser override there would be entirely pointless.

This is be my recommendation. I do not see the point of carrying the
option around just to confuse new users of pg_authid and reviewers of
the code.

Yeah ... if no one's found it interesting in the 20 years since the
code left Berkeley, it's unlikely that interest will emerge in the
future.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, Dec 27, 2014 at 06:26:02PM -0500, Tom Lane wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Plan C (remove CATUPDATE altogether) also has some merit. But adding a
superuser override there would be entirely pointless.

This is be my recommendation. I do not see the point of carrying the
option around just to confuse new users of pg_authid and reviewers of
the code.

Yeah ... if no one's found it interesting in the 20 years since the
code left Berkeley, it's unlikely that interest will emerge in the
future.

No objection here.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

All,

On Sat, Dec 27, 2014 at 6:31 PM, Noah Misch <noah@leadboat.com> wrote:

On Sat, Dec 27, 2014 at 06:26:02PM -0500, Tom Lane wrote:

Stephen Frost <sfrost@snowman.net> writes:

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

Plan C (remove CATUPDATE altogether) also has some merit. But adding

a

superuser override there would be entirely pointless.

This is be my recommendation. I do not see the point of carrying the
option around just to confuse new users of pg_authid and reviewers of
the code.

Yeah ... if no one's found it interesting in the 20 years since the
code left Berkeley, it's unlikely that interest will emerge in the
future.

No objection here.

Given this discussion, I have attached a patch that removes CATUPDATE for
review/discussion.

One of the interesting behaviors (or perhaps not) is how 'pg_class_aclmask'
handles an invalid role id when checking permissions against 'rolsuper'
instead of 'rolcatupdate'. This is demonstrated by the
'has_table_privilege' regression test expected results. In summary,
'has_rolcatupdate' raises an error when an invalid OID is provided,
however, as documented in the source 'superuser_arg' does not, simply
returning false. Therefore, when checking for superuser-ness of an
non-existent role, the result will be false and not an error. Perhaps this
is OK, but my concern would be on the expected behavior around items like
'has_table_privilege'. My natural thought would be that we would want to
preserve that specific functionality, though short of adding a
'has_rolsuper' function that will raise an appropriate error, I'm uncertain
of an approach. Thoughts?

Thanks,
Adam

--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

Adam,

* Adam Brightwell (adam.brightwell@crunchydatasolutions.com) wrote:

Given this discussion, I have attached a patch that removes CATUPDATE for
review/discussion.

Thanks!

One of the interesting behaviors (or perhaps not) is how 'pg_class_aclmask'
handles an invalid role id when checking permissions against 'rolsuper'
instead of 'rolcatupdate'. This is demonstrated by the
'has_table_privilege' regression test expected results. In summary,
'has_rolcatupdate' raises an error when an invalid OID is provided,
however, as documented in the source 'superuser_arg' does not, simply
returning false. Therefore, when checking for superuser-ness of an
non-existent role, the result will be false and not an error. Perhaps this
is OK, but my concern would be on the expected behavior around items like
'has_table_privilege'. My natural thought would be that we would want to
preserve that specific functionality, though short of adding a
'has_rolsuper' function that will raise an appropriate error, I'm uncertain
of an approach. Thoughts?

This role oid check is only getting called in this case because it's
pg_authid which is getting checked (because it's a system catalog table)
and it's then falling into this one case. I don't think we need to
preserve that.

If you do the same query with that bogus OID but a normal table, you get
the same 'f' result that the regression test gives with this change.
We're not back-patching this and I seriously doubt anyone is relying on
this special response for system catalog tables.

So, unless anyone wants to object, I'll continue to look over this patch
for eventual application against master. If there are objections, it'd
be great if they could be voiced sooner rather than later.

Thanks!

Stephen

* Adam Brightwell (adam.brightwell@crunchydatasolutions.com) wrote:

Given this discussion, I have attached a patch that removes CATUPDATE for
review/discussion.

Thanks!

I've added this patch (up-thread) to the next CommitFest (2015-02).

-Adam

--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

On 12/29/14 7:16 PM, Adam Brightwell wrote:

Given this discussion, I have attached a patch that removes CATUPDATE
for review/discussion.

One of the interesting behaviors (or perhaps not) is how
'pg_class_aclmask' handles an invalid role id when checking permissions
against 'rolsuper' instead of 'rolcatupdate'.

I'd get rid of that whole check, not just replace rolcatupdate by rolsuper.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter,

Thanks for the review and feedback.

One of the interesting behaviors (or perhaps not) is how

'pg_class_aclmask' handles an invalid role id when checking permissions
against 'rolsuper' instead of 'rolcatupdate'.

I'd get rid of that whole check, not just replace rolcatupdate by rolsuper.

Ah yes, that's a good point. I have updated and attached a new version of
the patch.

Thanks,
Adam

--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

On Fri, Feb 20, 2015 at 8:08 AM, Adam Brightwell <
adam.brightwell@crunchydatasolutions.com> wrote:

Thanks for the review and feedback.

One of the interesting behaviors (or perhaps not) is how

'pg_class_aclmask' handles an invalid role id when checking permissions
against 'rolsuper' instead of 'rolcatupdate'.

I'd get rid of that whole check, not just replace rolcatupdate by
rolsuper.

Ah yes, that's a good point. I have updated and attached a new version of
the patch.

I just had a look at this patch and it works as intended, simply removing
catupdate and all its code paths. As everybody on this thread seems to
agree about the removal of rolcatupdate, I think that we could let a
committer have a look at it.
Thoughts?
--
Michael

* Peter Eisentraut (peter_e@gmx.net) wrote:

On 12/29/14 7:16 PM, Adam Brightwell wrote:

Given this discussion, I have attached a patch that removes CATUPDATE
for review/discussion.

One of the interesting behaviors (or perhaps not) is how
'pg_class_aclmask' handles an invalid role id when checking permissions
against 'rolsuper' instead of 'rolcatupdate'.

I'd get rid of that whole check, not just replace rolcatupdate by rolsuper.

Err, wouldn't this make it possible to grant normal users the ability to
modify system catalogs? I realize that they wouldn't have that
initially, but I'm not sure we want the superuser to be able to grant
that to non-superusers..

I'm fine with making it "if system table and not superuser, error".

Thanks!

Stephen

On 2/25/15 3:39 PM, Stephen Frost wrote:

I'd get rid of that whole check, not just replace rolcatupdate by rolsuper.

Err, wouldn't this make it possible to grant normal users the ability to
modify system catalogs? I realize that they wouldn't have that
initially, but I'm not sure we want the superuser to be able to grant
that to non-superusers..

Why not? I thought we are trying to get rid of special superuser behavior.

After all, superusers can also make the other user a superuser to bypass
this issue.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Peter Eisentraut (peter_e@gmx.net) wrote:

On 2/25/15 3:39 PM, Stephen Frost wrote:

I'd get rid of that whole check, not just replace rolcatupdate by rolsuper.

Err, wouldn't this make it possible to grant normal users the ability to
modify system catalogs? I realize that they wouldn't have that
initially, but I'm not sure we want the superuser to be able to grant
that to non-superusers..

Why not? I thought we are trying to get rid of special superuser behavior.

Agreed, but I'd also like to get rid of any reason, beyond emergency
cases, for people to modify the catalog directly. There's a few places
which we aren't yet doing that, but I'd rather fix those cases than
encourage people to give out rights to modify them and end up making
things like:

"UPDATE pg_database SET datallowconn = false where datname = 'xyz';"

an accepted interface.

After all, superusers can also make the other user a superuser to bypass
this issue.

Sure, but that gives us the option to write off whatever happens next as
not our fault.

Thanks,

Stephen

On 2/25/15 10:05 PM, Stephen Frost wrote:

* Peter Eisentraut (peter_e@gmx.net) wrote:

On 2/25/15 3:39 PM, Stephen Frost wrote:

I'd get rid of that whole check, not just replace rolcatupdate by rolsuper.

Err, wouldn't this make it possible to grant normal users the ability to
modify system catalogs? I realize that they wouldn't have that
initially, but I'm not sure we want the superuser to be able to grant
that to non-superusers..

Why not? I thought we are trying to get rid of special superuser behavior.

Agreed, but I'd also like to get rid of any reason, beyond emergency
cases, for people to modify the catalog directly. There's a few places
which we aren't yet doing that, but I'd rather fix those cases than
encourage people to give out rights to modify them and end up making
things like:

"UPDATE pg_database SET datallowconn = false where datname = 'xyz';"

an accepted interface.

I'm not sure those things are related.

Getting rid of the *reasons* for updating catalogs directly might be
worthwhile, but I don't see why we need to install (or maintain) a
special invisible permission trap for it. We have a permission system,
and it works pretty well.

The Unix kernels don't have special traps for someone to modify
/etc/shadow or similar directly. That file has appropriate permissions,
and that's it. I think that works pretty well.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter,

* Peter Eisentraut (peter_e@gmx.net) wrote:

On 2/25/15 10:05 PM, Stephen Frost wrote:

Agreed, but I'd also like to get rid of any reason, beyond emergency
cases, for people to modify the catalog directly. There's a few places
which we aren't yet doing that, but I'd rather fix those cases than
encourage people to give out rights to modify them and end up making
things like:

"UPDATE pg_database SET datallowconn = false where datname = 'xyz';"

an accepted interface.

I'm not sure those things are related.

Eh, I feel they are, but either way.

Getting rid of the *reasons* for updating catalogs directly might be
worthwhile, but I don't see why we need to install (or maintain) a
special invisible permission trap for it. We have a permission system,
and it works pretty well.

We have this "invisible permission trap" known as checking if you're a
superuser all over the place. I'm not against revisiting those cases
and considering using the GRANT permission system to manage access, but
that's certainly a larger project to work on. What I'm referring to
here are all the functions that check if you're a superuser, instead of
just revoking EXECUTE from public and letting the user manage the
permission.

The Unix kernels don't have special traps for someone to modify
/etc/shadow or similar directly. That file has appropriate permissions,
and that's it. I think that works pretty well.

This isn't really /etc/shadow though, this is more like direct access to
the filesystem through the device node- and you'll note that Linux
certainly has got an independent set of permissions for that called the
capabilities system. That's because messing with those pieces can crash
the kernel. You're not going to crash the kernel if you goof up
/etc/shadow.

Thanks!

Stephen

On 2/28/15 6:32 PM, Stephen Frost wrote:

This isn't really /etc/shadow though, this is more like direct access to
the filesystem through the device node- and you'll note that Linux
certainly has got an independent set of permissions for that called the
capabilities system. That's because messing with those pieces can crash
the kernel. You're not going to crash the kernel if you goof up
/etc/shadow.

I think this characterization is incorrect. The Linux capability system
does not exist because the actions are scary or can crash the kernel.
Capabilities exist because they are not attached to file system objects
and can therefore not be represented using the usual permission system.

Note that one can write directly to raw devices or the kernel memory
through various /dev and /proc files. No "capability" protects against
that. It's only the file permissions, possibly in combination with
mount options.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Any other opinions?

The options are:

0) Leave as is.

1) Remove catupdate and replace with superuser checks.

2) Remove catupdate and rely on regular table permissions on catalogs.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers