Date: Mon, 18 Jul 2022 14:56:49 -0700
From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Michael Paquier <michael@paquier.xyz>, pgsql-hackers@postgresql.org
Subject: Re: pg_parameter_aclcheck() and trusted extensions
Message-ID: <20220718215649.GA3699123@nathanxps13>
References: <20220706224727.GA2158260@nathanxps13>
 <YsYxEtX8zOYKf3Ru@paquier.xyz>
 <2995921.1657212060@sss.pgh.pa.us>
 <3126292.1657828950@sss.pgh.pa.us>
 <20220714215225.GA3173833@nathanxps13>
 <3159281.1657836225@sss.pgh.pa.us>
 <20220714225735.GB3173833@nathanxps13>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="gKMricLos+KVdGMg"
Content-Disposition: inline
In-Reply-To: <20220714225735.GB3173833@nathanxps13>
Archived-At: 
 <https://www.postgresql.org/message-id/20220718215649.GA3699123%40nathanxps13>
Precedence: bulk


--gKMricLos+KVdGMg
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Thu, Jul 14, 2022 at 03:57:35PM -0700, Nathan Bossart wrote:
> However, ALTER ROLE RESET ALL will be blocked, while resetting only the
> individual GUC will go through.
> 
> 	postgres=> ALTER ROLE other RESET ALL;
> 	ALTER ROLE
> 	postgres=> SELECT * FROM pg_db_role_setting;
> 	 setdatabase | setrole |        setconfig        
> 	-------------+---------+-------------------------
> 	           0 |   16385 | {zero_damaged_pages=on}
> 	(1 row)
> 
> 	postgres=> ALTER ROLE other RESET zero_damaged_pages;
> 	ALTER ROLE
> 	postgres=> SELECT * FROM pg_db_role_setting;
> 	 setdatabase | setrole | setconfig 
> 	-------------+---------+-----------
> 	(0 rows)
> 
> I think this is because GUCArrayReset() is the only caller of
> validate_option_array_item() that sets skipIfNoPermissions to true.  The
> others fall through to set_config_option(), which does a
> pg_parameter_aclcheck().  So, you are right.

Here's a small patch that seems to fix this case.  However, I wonder if a
better way to fix this is to provide a way to stop set_config_option() from
throwing errors (e.g., setting elevel to -1).  That way, we could remove
the manual permissions checks in favor of always using the real ones, which
might help prevent similar bugs in the future.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

--gKMricLos+KVdGMg
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="fix_reset_all_for_gucs.patch"

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 0328029d43..fbc1014824 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -11689,7 +11689,8 @@ validate_option_array_item(const char *name, const char *value,
 		 * We cannot do any meaningful check on the value, so only permissions
 		 * are useful to check.
 		 */
-		if (superuser())
+		if (superuser() ||
+			pg_parameter_aclcheck(gconf->name, GetUserId(), ACL_SET) == ACLCHECK_OK)
 			return true;
 		if (skipIfNoPermissions)
 			return false;
@@ -11703,6 +11704,8 @@ validate_option_array_item(const char *name, const char *value,
 		 /* ok */ ;
 	else if (gconf->context == PGC_SUSET && superuser())
 		 /* ok */ ;
+	else if (pg_parameter_aclcheck(gconf->name, GetUserId(), ACL_SET) == ACLCHECK_OK)
+		 /* ok */ ;
 	else if (skipIfNoPermissions)
 		return false;
 	/* if a permissions error should be thrown, let set_config_option do it */

--gKMricLos+KVdGMg--