Received: from malur.postgresql.org ([217.196.149.56])
	by arkaria.postgresql.org with esmtps
 (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256)
	(Exim 4.92)
	(envelope-from <pgsql-bugs-owner+archive@lists.postgresql.org>)
	id 1ka1PV-0002Mj-Oh
	for pgsql-bugs@arkaria.postgresql.org; Tue, 03 Nov 2020 18:52:01 +0000
Received: from localhost ([127.0.0.1] helo=malur.postgresql.org)
	by malur.postgresql.org with esmtp (Exim 4.92)
	(envelope-from <pgsql-bugs-owner+archive@lists.postgresql.org>)
	id 1ka1PU-0001u9-N2
	for pgsql-bugs@arkaria.postgresql.org; Tue, 03 Nov 2020 18:52:00 +0000
Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29])
	by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256)
	(Exim 4.92)
	(envelope-from <tgl@sss.pgh.pa.us>)
	id 1ka1PU-0001u2-Gt
	for pgsql-bugs@lists.postgresql.org; Tue, 03 Nov 2020 18:52:00 +0000
Received: from sss.pgh.pa.us ([66.207.139.130])
	by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256)
	(Exim 4.92)
	(envelope-from <tgl@sss.pgh.pa.us>)
	id 1ka1PO-0001Px-BE
	for pgsql-bugs@lists.postgresql.org; Tue, 03 Nov 2020 18:52:00 +0000
Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1])
	by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 0A3Ipq0L961303;
	Tue, 3 Nov 2020 13:51:52 -0500
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Wolfgang Walther <walther@technowledgy.de>
cc: Stephen Frost <sfrost@snowman.net>, pgsql-bugs@lists.postgresql.org
Subject: Re: User with BYPASSRLS privilege can't change password
In-reply-to: <be39bd26-4024-0bf5-eb28-31984a887265@technowledgy.de>
References: <a5548a9f-89ee-3167-129d-162b5985fcf8@technowledgy.de>
 <923005.1604420579@sss.pgh.pa.us> <956994.1604425451@sss.pgh.pa.us>
 <3e771893-1ed1-97e7-6c4a-7b0d00992103@technowledgy.de>
 <958492.1604427541@sss.pgh.pa.us>
 <be39bd26-4024-0bf5-eb28-31984a887265@technowledgy.de>
Comments: In-reply-to Wolfgang Walther <walther@technowledgy.de>
	message dated "Tue, 03 Nov 2020 19:26:13 +0100"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <961281.1604429490.0@sss.pgh.pa.us>
Date: Tue, 03 Nov 2020 13:51:52 -0500
Message-ID: <961302.1604429512@sss.pgh.pa.us>
List-Id: <pgsql-bugs.lists.postgresql.org>
List-Help: <https://lists.postgresql.org/manage/>
List-Subscribe: <https://lists.postgresql.org/manage/>
List-Post: <mailto:pgsql-bugs@lists.postgresql.org>
List-Owner: <mailto:pgsql-bugs-owner@lists.postgresql.org>
List-Archive: <https://www.postgresql.org/list/pgsql-bugs>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <961281.1604429490.1@sss.pgh.pa.us>

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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-bypassrls-privilege-check-2.patch";
	charset="us-ascii"
Content-ID: <961281.1604429490.2@sss.pgh.pa.us>
Content-Description: fix-bypassrls-privilege-check-2.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/doc/src/sgml/ref/alter_role.sgml b/doc/src/sgml/ref/alter_rol=
e.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=3D"parameter">role_speci=
fication</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</li=
teral>,
+   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 =3D 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 allow=
ed
+	 * to change your own password.
 	 */
 	if (authform->rolsuper || issuper >=3D 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 >=3D 0)
+	else if (bypassrls >=3D 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 &&

------- =_aaaaaaaaaa0--