Обсуждение: User with BYPASSRLS privilege can't change password

Hi,

observed on PG12.4:

CREATE USER alice;
SET ROLE alice;
ALTER USER alice PASSOWRD 'x';
-- works

RESET ROLE;

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

I would expect bob to be able to change his password here.

Best

Wolfgang

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

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

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

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

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>

diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 9ce9a66921..293e7e4c0c 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,
@@ -735,11 +737,11 @@ AlterRole(AlterRoleStmt *stmt)
     }
     else if (!have_createrole_privilege())
     {
+        /* We already checked issuper, isreplication, and bypassrls */
         if (!(inherit < 0 &&
               createrole < 0 &&
               createdb < 0 &&
               canlogin < 0 &&
-              isreplication < 0 &&
               !dconnlimit &&
               !rolemembers &&
               !validUntil &&

"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

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