User with BYPASSRLS privilege can't change password

Wolfgang Walther <walther@technowledgy.de> writes:

CREATE USER bob BYPASSRLS;
SET ROLE bob;
ALTER USER bob PASSWORD 'x';
-- ERROR: must be superuser to change bypassrls attribute

Yeah, duplicated here on HEAD. The error message seems to think
the command is trying to remove the BYPASSRLS privilege, which
suggests somebody forgot to copy that flag somewhere where it needs
to be copied. Haven't dug further than that.

regards, tom lane

I wrote:

Wolfgang Walther <walther@technowledgy.de> writes:

CREATE USER bob BYPASSRLS;
SET ROLE bob;
ALTER USER bob PASSWORD 'x';
-- ERROR: must be superuser to change bypassrls attribute

Yeah, duplicated here on HEAD. The error message seems to think
the command is trying to remove the BYPASSRLS privilege, which
suggests somebody forgot to copy that flag somewhere where it needs
to be copied. Haven't dug further than that.

It's a little more subtle than that, but not much. Commit 491c029db
copied-and-pasted the logic used to deny non-superusers the privilege
to change anything about a superuser role. That was certainly not the
intention, because the error message was phrased differently from the
superuser case, but that was the effect. I propose the attached.

(Hm, looks like this behavior is undocumented, too.)

regards, tom lane

Tom Lane:

It's a little more subtle than that, but not much. Commit 491c029db
copied-and-pasted the logic used to deny non-superusers the privilege
to change anything about a superuser role. That was certainly not the
intention, because the error message was phrased differently from the
superuser case, but that was the effect. I propose the attached.

Wouldn't the following change allow a non-superuser with createrole
privilege to grant the replication privilege to a role that does not
have that privilege, yet? This should still be forbidden, I think.

@@ -739,7 +741,6 @@ AlterRole(AlterRoleStmt *stmt)
createrole < 0 &&
createdb < 0 &&
canlogin < 0 &&
- isreplication < 0 &&
!dconnlimit &&
!rolemembers &&
!validUntil &&

This is because the "must be superuser to alter replication users"
condition only triggers when the altered role already has isrepliaction,
so isreplication could very well be >= 0 here.

The other change looks good.

Best

Wolfgang

Greetings,

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

I wrote:

Wolfgang Walther <walther@technowledgy.de> writes:

CREATE USER bob BYPASSRLS;
SET ROLE bob;
ALTER USER bob PASSWORD 'x';
-- ERROR: must be superuser to change bypassrls attribute

Yeah, duplicated here on HEAD. The error message seems to think
the command is trying to remove the BYPASSRLS privilege, which
suggests somebody forgot to copy that flag somewhere where it needs
to be copied. Haven't dug further than that.

It's a little more subtle than that, but not much. Commit 491c029db
copied-and-pasted the logic used to deny non-superusers the privilege
to change anything about a superuser role. That was certainly not the
intention, because the error message was phrased differently from the
superuser case, but that was the effect. I propose the attached.

(Hm, looks like this behavior is undocumented, too.)

The intent was certainly that non-superusers shouldn't be able to tweak
the BYPASSRLS bit, but it wasn't intended to prevent users who have
BYPASSRLS from being able to change their own password.

I certainly recall the discussion about that requirement and we should
document it.

diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 9ce9a66921..5cd479a649 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -709,8 +709,10 @@ AlterRole(AlterRoleStmt *stmt)
roleid = authform->oid;

/*
-	 * To mess with a superuser you gotta be superuser; else you need
-	 * createrole, or just want to change your own password
+	 * To mess with a superuser or replication role in any way you gotta be
+	 * superuser.  We also insist on superuser to change the BYPASSRLS
+	 * property.  Otherwise, if you don't have createrole, you're only allowed
+	 * to change your own password.
*/
if (authform->rolsuper || issuper >= 0)
{
@@ -726,7 +728,7 @@ AlterRole(AlterRoleStmt *stmt)
(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
errmsg("must be superuser to alter replication users")));
}
-	else if (authform->rolbypassrls || bypassrls >= 0)
+	else if (bypassrls >= 0)
{
if (!superuser())
ereport(ERROR,

This change looks correct, we shouldn't be worrying about what's already
been set on the role.

@@ -739,7 +741,6 @@ AlterRole(AlterRoleStmt *stmt)
createrole < 0 &&
createdb < 0 &&
canlogin < 0 &&
- isreplication < 0 &&
!dconnlimit &&
!rolemembers &&
!validUntil &&

This seems like an independent change..? Not saying it's wrong though.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

@@ -739,7 +741,6 @@ AlterRole(AlterRoleStmt *stmt)
createrole < 0 &&
createdb < 0 &&
canlogin < 0 &&
- isreplication < 0 &&
!dconnlimit &&
!rolemembers &&
!validUntil &&

This seems like an independent change..? Not saying it's wrong though.

That test is redundant, since we wouldn't be in this path at all if
isreplication >= 0. You could alternatively argue that this should
redundantly test all three of issuper, isreplication, and bypassrls;
but testing just one of them is confusing and hence bug-prone.

regards, tom lane

Wolfgang Walther <walther@technowledgy.de> writes:

This is because the "must be superuser to alter replication users"
condition only triggers when the altered role already has isrepliaction,
so isreplication could very well be >= 0 here.

How do you figure that? This is in an "else" path after

else if (authform->rolreplication || isreplication >= 0)

so AFAICS it's impossible to get there. If it isn't impossible,
we have a much bigger hole with respect to issuper.

regards, tom lane

Tom Lane:

How do you figure that? This is in an "else" path after

else if (authform->rolreplication || isreplication >= 0)

so AFAICS it's impossible to get there. If it isn't impossible,
we have a much bigger hole with respect to issuper.

Yes, you're right. I read the || as &&. And also missed the ! in else if
(!have_createrole_privilege()) btw. :)

I guess the error message "must be superuser to alter replication users"
led me on the wrong path. I would be more precise as "must be superuser
to alter replication users or change replication attribute" to cover the
change-non-replication-to-replication user case, I think. The same thing
for superusers.

Best

Wolfgang

Wolfgang Walther <walther@technowledgy.de> writes:

Tom Lane:

so AFAICS it's impossible to get there. If it isn't impossible,
we have a much bigger hole with respect to issuper.

Yes, you're right. I read the || as &&. And also missed the ! in else if
(!have_createrole_privilege()) btw. :)

Actually the right way to deal with this potential confusion is to
add a comment, as below. I fixed the docs too.

I guess the error message "must be superuser to alter replication users"
led me on the wrong path. I would be more precise as "must be superuser
to alter replication users or change replication attribute" to cover the
change-non-replication-to-replication user case, I think. The same thing
for superusers.

I'd be okay with changing the error text in HEAD, but less so in the back
branches, since that'd cause thrashing of translatable strings.

regards, tom lane

On Tue, Nov 3, 2020 at 11:06 AM Stephen Frost <sfrost@snowman.net> wrote:

diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 9ce9a66921..5cd479a649 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -709,8 +709,10 @@ AlterRole(AlterRoleStmt *stmt)
roleid = authform->oid;

/*
-      * To mess with a superuser you gotta be superuser; else you need
-      * createrole, or just want to change your own password
+      * To mess with a superuser or replication role in any way you

gotta be

+      * superuser.  We also insist on superuser to change the BYPASSRLS
+      * property.  Otherwise, if you don't have createrole, you're only

allowed

+ * to change your own password.
*/
if (authform->rolsuper || issuper >= 0)
{
@@ -726,7 +728,7 @@ AlterRole(AlterRoleStmt *stmt)

(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),

errmsg("must be superuser to

alter replication users")));

}
-     else if (authform->rolbypassrls || bypassrls >= 0)
+     else if (bypassrls >= 0)
{
if (!superuser())
ereport(ERROR,

This change looks correct, we shouldn't be worrying about what's already
been set on the role.

Is the nuance that in reality a non-superuser cannot specify BypassRLS even
if the effective value is unchanged unimportant here?

David J.

"David G. Johnston" <david.g.johnston@gmail.com> writes:

Is the nuance that in reality a non-superuser cannot specify BypassRLS even
if the effective value is unchanged unimportant here?

I was wondering about that. You could definitely make a case for a
weaker set of rules here. However, the superuser restriction has been
like that for 15-ish years without much complaint, so it doesn't seem
that it bothers people in practice.

regards, tom lane

On Tue, Nov 3, 2020 at 12:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

"David G. Johnston" <david.g.johnston@gmail.com> writes:

Is the nuance that in reality a non-superuser cannot specify BypassRLS

even

if the effective value is unchanged unimportant here?

I was wondering about that. You could definitely make a case for a
weaker set of rules here. However, the superuser restriction has been
like that for 15-ish years without much complaint, so it doesn't seem
that it bothers people in practice.

Agreed that the behavior seems fine to keep. Was more about the
documentation saying "change" instead of "specify".

David J.

Greetings,

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

Wolfgang Walther <walther@technowledgy.de> writes:

Tom Lane:

so AFAICS it's impossible to get there. If it isn't impossible,
we have a much bigger hole with respect to issuper.

Yes, you're right. I read the || as &&. And also missed the ! in else if
(!have_createrole_privilege()) btw. :)

Actually the right way to deal with this potential confusion is to
add a comment, as below. I fixed the docs too.

I guess the error message "must be superuser to alter replication users"
led me on the wrong path. I would be more precise as "must be superuser
to alter replication users or change replication attribute" to cover the
change-non-replication-to-replication user case, I think. The same thing
for superusers.

I'd be okay with changing the error text in HEAD, but less so in the back
branches, since that'd cause thrashing of translatable strings.

I tend to agree with that.

diff --git a/doc/src/sgml/ref/alter_role.sgml b/doc/src/sgml/ref/alter_role.sgml
index aef30521bc..5aa5648ae7 100644
--- a/doc/src/sgml/ref/alter_role.sgml
+++ b/doc/src/sgml/ref/alter_role.sgml
@@ -71,7 +71,9 @@ ALTER ROLE { <replaceable class="parameter">role_specification</replaceable> | A
Attributes not mentioned in the command retain their previous settings.
Database superusers can change any of these settings for any role.
Roles having <literal>CREATEROLE</literal> privilege can change any of these
-   settings, but only for non-superuser and non-replication roles.
+   settings except <literal>SUPERUSER</literal>, <literal>REPLICATION</literal>,
+   and <literal>BYPASSRLS</literal>; but only for non-superuser and
+   non-replication roles.
Ordinary roles can only change their own password.
</para>

This looks good, though should we also mention that you have to be a
superuser to set BYPASSRLS on a role in the CREATE ROLE documentation
too..? The error message there could also be improved, it looks like.
Also, we don't seem to document in CREATE ROLE that you have to be a
superuser to create a REPLICATION role either, even though the code
clearly says that.

Attached is a patch for all of those. As mentioned, the error message
probably only makes sense to change in HEAD, though improving the
documentation could be back-patched.

Thanks,

Stephen

Stephen Frost <sfrost@snowman.net> writes:

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

I'd be okay with changing the error text in HEAD, but less so in the back
branches, since that'd cause thrashing of translatable strings.

I tend to agree with that.

OK, will do it that way.

Attached is a patch for all of those. As mentioned, the error message
probably only makes sense to change in HEAD, though improving the
documentation could be back-patched.

Check. I'll merge this with what I was doing and push it all.

regards, tom lane