Обсуждение: pg_parameter_aclcheck() and trusted extensions
Hi hackers,
I found that as of a0ffa88, it's possible to set a PGC_SUSET GUC defined by
a trusted extension as a non-superuser. I've confirmed that this only
affects v15 and later versions.
postgres=# CREATE ROLE testuser;
CREATE ROLE
postgres=# GRANT CREATE ON DATABASE postgres TO testuser;
GRANT
postgres=# SET ROLE testuser;
SET
postgres=> SET plperl.on_plperl_init = 'test';
SET
postgres=> CREATE EXTENSION plperl;
CREATE EXTENSION
postgres=> SELECT setting FROM pg_settings WHERE name = 'plperl.on_plperl_init';
setting
---------
test
(1 row)
On previous versions, the CREATE EXTENSION command emits the following
WARNING, and the setting does not take effect:
WARNING: permission denied to set parameter "plperl.on_plperl_init"
I think the call to superuser_arg() in pg_parameter_aclmask() is causing
set_config_option() to bypass the normal privilege checks, as
execute_extension_script() will have set the user ID to the bootstrap
superuser for trusted extensions like plperl. I don't have a patch or a
proposal at the moment, but I thought it was worth starting the discussion.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Wed, Jul 06, 2022 at 03:47:27PM -0700, Nathan Bossart wrote: > I think the call to superuser_arg() in pg_parameter_aclmask() is causing > set_config_option() to bypass the normal privilege checks, as > execute_extension_script() will have set the user ID to the bootstrap > superuser for trusted extensions like plperl. I don't have a patch or a > proposal at the moment, but I thought it was worth starting the discussion. Looks like a bug to me, so I have added an open item assigned to Tom. -- Michael
Вложения
Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Jul 06, 2022 at 03:47:27PM -0700, Nathan Bossart wrote:
>> I think the call to superuser_arg() in pg_parameter_aclmask() is causing
>> set_config_option() to bypass the normal privilege checks, as
>> execute_extension_script() will have set the user ID to the bootstrap
>> superuser for trusted extensions like plperl. I don't have a patch or a
>> proposal at the moment, but I thought it was worth starting the discussion.
> Looks like a bug to me, so I have added an open item assigned to Tom.
Yeah. So the fix here seems pretty obvious: rather than applying the
permissions check using bare GetUserId(), we need to remember the role
OID that originally applied the setting, and use that.
The problem with this sketch is that
(1) we need an OID field in struct config_generic, as well as GucStack,
which means an ABI break for any extensions that look directly at GUC
records. There probably aren't many, but ...
(2) we need an additional parameter to set_config_option, which
again is a compatibility break for anything calling that directly.
There surely are such callers --- our own extensions do it.
Can we get away with doing these things in beta3? We could avoid
breaking (2) in the v15 branch by making set_config_option into
a wrapper around set_config_option_ext, or something like that;
but the problem with struct config_generic seems inescapable.
(Putting the new field at the end would solve nothing, since
config_generic is embedded into larger structs.)
The alternative to API/ABI breaks seems to be to revert the
feature, which would be sad.
regards, tom lane
On Thu, Jul 07, 2022 at 10:04:18AM +0900, Michael Paquier wrote: > On Wed, Jul 06, 2022 at 03:47:27PM -0700, Nathan Bossart wrote: >> I think the call to superuser_arg() in pg_parameter_aclmask() is causing >> set_config_option() to bypass the normal privilege checks, as >> execute_extension_script() will have set the user ID to the bootstrap >> superuser for trusted extensions like plperl. I don't have a patch or a >> proposal at the moment, but I thought it was worth starting the discussion. > > Looks like a bug to me, so I have added an open item assigned to Tom. Thanks. I've been thinking about this one a bit. For simple cases like plperl, it would be easy enough to temporarily revert the superuser switch when calling _PG_init() or one of the DefineCustomXXXVariable functions. Unfortunately, I think there are more complicated scenarios. For example, what role should pg_parameter_aclmask() use when a trusted extension script loads a library after SET ROLE? The original user might not ordinarily be able to assume this role, so the trusted extension script could still be a way to set parameters you don't have privileges for. Should we just always use the role that's calling CREATE EXTENSION? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Thu, Jul 07, 2022 at 12:41:00PM -0400, Tom Lane wrote: > Yeah. So the fix here seems pretty obvious: rather than applying the > permissions check using bare GetUserId(), we need to remember the role > OID that originally applied the setting, and use that. Please ignore my previous message. This makes sense. > The problem with this sketch is that > > (1) we need an OID field in struct config_generic, as well as GucStack, > which means an ABI break for any extensions that look directly at GUC > records. There probably aren't many, but ... > > (2) we need an additional parameter to set_config_option, which > again is a compatibility break for anything calling that directly. > There surely are such callers --- our own extensions do it. > > Can we get away with doing these things in beta3? We could avoid > breaking (2) in the v15 branch by making set_config_option into > a wrapper around set_config_option_ext, or something like that; > but the problem with struct config_generic seems inescapable. > (Putting the new field at the end would solve nothing, since > config_generic is embedded into larger structs.) > > The alternative to API/ABI breaks seems to be to revert the > feature, which would be sad. I personally lean more towards the compatibility break than reverting the feature. There are still a couple of months before 15.0, and I suspect it won't be too difficult to fix any extensions that break because of this. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Nathan Bossart <nathandbossart@gmail.com> writes: > On Thu, Jul 07, 2022 at 12:41:00PM -0400, Tom Lane wrote: >> Can we get away with doing these things in beta3? We could avoid >> breaking (2) in the v15 branch by making set_config_option into >> a wrapper around set_config_option_ext, or something like that; >> but the problem with struct config_generic seems inescapable. > I personally lean more towards the compatibility break than reverting the > feature. There are still a couple of months before 15.0, and I suspect it > won't be too difficult to fix any extensions that break because of this. I checked http://codesearch.debian.net and found only a couple of extensions that #include guc_tables.h at all, so I'm satisfied that the struct config_generic ABI issue is tolerable. Recompiling after beta3 would be enough to fix any problem there, and it's hard to believe that anyone is trying to ship production-ready v15 extensions already. The aspect that is a bit more debatable is whether to trouble with a set_config_option() wrapper to avoid the API break in v15. I think we'd still be making people deal with an API break in v16, so making them do it this year rather than next doesn't seem like a big deal ... but maybe someone wants to argue it's too late for API breaks in v15? regards, tom lane
On 7/7/22 15:00, Tom Lane wrote: > Nathan Bossart <nathandbossart@gmail.com> writes: >> On Thu, Jul 07, 2022 at 12:41:00PM -0400, Tom Lane wrote: >>> Can we get away with doing these things in beta3? We could avoid >>> breaking (2) in the v15 branch by making set_config_option into >>> a wrapper around set_config_option_ext, or something like that; >>> but the problem with struct config_generic seems inescapable. > >> I personally lean more towards the compatibility break than reverting the >> feature. There are still a couple of months before 15.0, and I suspect it >> won't be too difficult to fix any extensions that break because of this. > > I checked http://codesearch.debian.net and found only a couple of > extensions that #include guc_tables.h at all, so I'm satisfied > that the struct config_generic ABI issue is tolerable. Recompiling > after beta3 would be enough to fix any problem there, and it's > hard to believe that anyone is trying to ship production-ready > v15 extensions already. There are a handful here as well: https://github.com/search?q=guc_tables.h+and+PG_MODULE_MAGIC&type=Code&ref=advsearch&l=&l= But as one of the affected authors I would say recompiling after beta3 is fine. > The aspect that is a bit more debatable is whether to trouble with > a set_config_option() wrapper to avoid the API break in v15. > I think we'd still be making people deal with an API break in v16, > so making them do it this year rather than next doesn't seem like > a big deal ... but maybe someone wants to argue it's too late > for API breaks in v15? Well there are other API breaks that affect me in v15, and to be honest I have done little except keep an eye out for the ones likely to affect extensions I maintain so far, so may as well inflict the pain now as later ¯\_(ツ)_/¯ Joe -- Joe Conway RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Thu, Jul 07, 2022 at 03:43:03PM -0400, Joe Conway wrote: > On 7/7/22 15:00, Tom Lane wrote: >> The aspect that is a bit more debatable is whether to trouble with >> a set_config_option() wrapper to avoid the API break in v15. >> I think we'd still be making people deal with an API break in v16, >> so making them do it this year rather than next doesn't seem like >> a big deal ... but maybe someone wants to argue it's too late >> for API breaks in v15? > > Well there are other API breaks that affect me in v15, and to be honest I > have done little except keep an eye out for the ones likely to affect > extensions I maintain so far, so may as well inflict the pain now as later > ¯\_(ツ)_/¯ With my RMT and hacker hat on, I see no reason to not break ABI or APIs while we are still in beta, as long as the GA result is as best as we can make it. I have not looked at the reasoning behind the issue, but if you think that this feature will work better in the long term by having an extra field to track the role OID in one of the GUC structs or in one of its API arguments, that's fine by me. If this requires more work, a revert can of course be discussed, but I am not getting that this is really necessary here. This would be the last option to consider. -- Michael
Вложения
On Fri, Jul 8, 2022 at 1:09 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Jul 07, 2022 at 03:43:03PM -0400, Joe Conway wrote:
> > On 7/7/22 15:00, Tom Lane wrote:
> >> The aspect that is a bit more debatable is whether to trouble with
> >> a set_config_option() wrapper to avoid the API break in v15.
> >> I think we'd still be making people deal with an API break in v16,
> >> so making them do it this year rather than next doesn't seem like
> >> a big deal ... but maybe someone wants to argue it's too late
> >> for API breaks in v15?
> >
> > Well there are other API breaks that affect me in v15, and to be honest I
> > have done little except keep an eye out for the ones likely to affect
> > extensions I maintain so far, so may as well inflict the pain now as later
> > ¯\_(ツ)_/¯
>
> With my RMT and hacker hat on, I see no reason to not break ABI or
> APIs while we are still in beta, as long as the GA result is as best
> as we can make it. I have not looked at the reasoning behind the
> issue, but if you think that this feature will work better in the long
> term by having an extra field to track the role OID in one of the GUC
> structs or in one of its API arguments, that's fine by me.
>
> If this requires more work, a revert can of course be discussed, but I
> am not getting that this is really necessary here. This would be the
> last option to consider.
The RMT has discussed this item further, and we agree an ABI break is acceptable for resolving this issue.
--
John Naylor
EDB: http://www.enterprisedb.com
John Naylor <john.naylor@enterprisedb.com> writes:
> The RMT has discussed this item further, and we agree an ABI break is
> acceptable for resolving this issue.
Cool, I'll produce a patch soon.
regards, tom lane
I wrote:
> Michael Paquier <michael@paquier.xyz> writes:
>> Looks like a bug to me, so I have added an open item assigned to Tom.
> Yeah. So the fix here seems pretty obvious: rather than applying the
> permissions check using bare GetUserId(), we need to remember the role
> OID that originally applied the setting, and use that.
Here's a draft patch for that. I initially ran around and changed all
the set_config_option callers as I threatened before, but as I did it
I could not help observing that they were all changing in exactly the
same way: basically, they were passing GetUserId() if the GucContext
is PGC_S_SESSION and BOOTSTRAP_SUPERUSERID otherwise. Not counting
guc.c internal call sites, there is a grand total of one caller that
fails to fit the pattern. So that brought me around to liking the idea
of keeping set_config_option's API stable by making it a thin wrapper
around another function with an explicit role argument. The result,
attached, poses far less of an API/ABI hazard than I was anticipating.
If you're not poking into the GUC tables you have little to fear.
Most of the bulk of this is mechanical changes to pass the source
role around properly in guc.c's data structures. That's all basically
copy-and-paste from the code to track the source context (scontext).
I noted something that ought to be looked at separately:
validate_option_array_item() seems like it needs to be taught about
grantable permissions on GUCs. I think that right now it may report
permissions failures in some cases where it should succeed.
regards, tom lane
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 3db859c3ea..6b6720c690 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -912,6 +912,9 @@ execute_extension_script(Oid extensionOid, ExtensionControlFile *control,
* We use the equivalent of a function SET option to allow the setting to
* persist for exactly the duration of the script execution. guc.c also
* takes care of undoing the setting on error.
+ *
+ * log_min_messages can't be set by ordinary users, so for that one we
+ * pretend to be superuser.
*/
save_nestlevel = NewGUCNestLevel();
@@ -920,9 +923,10 @@ execute_extension_script(Oid extensionOid, ExtensionControlFile *control,
PGC_USERSET, PGC_S_SESSION,
GUC_ACTION_SAVE, true, 0, false);
if (log_min_messages < WARNING)
- (void) set_config_option("log_min_messages", "warning",
- PGC_SUSET, PGC_S_SESSION,
- GUC_ACTION_SAVE, true, 0, false);
+ (void) set_config_option_ext("log_min_messages", "warning",
+ PGC_SUSET, PGC_S_SESSION,
+ BOOTSTRAP_SUPERUSERID,
+ GUC_ACTION_SAVE, true, 0, false);
/*
* Similarly disable check_function_bodies, to ensure that SQL functions
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 0328029d43..9e6cb590c7 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -5172,7 +5172,8 @@ static void reapply_stacked_values(struct config_generic *variable,
struct config_string *pHolder,
GucStack *stack,
const char *curvalue,
- GucContext curscontext, GucSource cursource);
+ GucContext curscontext, GucSource cursource,
+ Oid cursrole);
static void ShowGUCConfigOption(const char *name, DestReceiver *dest);
static void ShowAllGUCConfig(DestReceiver *dest);
static char *_ShowOption(struct config_generic *record, bool use_units);
@@ -5897,10 +5898,10 @@ InitializeWalConsistencyChecking(void)
check_wal_consistency_checking_deferred = false;
- set_config_option("wal_consistency_checking",
- wal_consistency_checking_string,
- PGC_POSTMASTER, guc->source,
- GUC_ACTION_SET, true, ERROR, false);
+ set_config_option_ext("wal_consistency_checking",
+ wal_consistency_checking_string,
+ guc->scontext, guc->source, guc->srole,
+ GUC_ACTION_SET, true, ERROR, false);
/* checking should not be deferred again */
Assert(!check_wal_consistency_checking_deferred);
@@ -5979,6 +5980,8 @@ InitializeOneGUCOption(struct config_generic *gconf)
gconf->reset_source = PGC_S_DEFAULT;
gconf->scontext = PGC_INTERNAL;
gconf->reset_scontext = PGC_INTERNAL;
+ gconf->srole = BOOTSTRAP_SUPERUSERID;
+ gconf->reset_srole = BOOTSTRAP_SUPERUSERID;
gconf->stack = NULL;
gconf->extra = NULL;
gconf->last_reported = NULL;
@@ -6356,6 +6359,7 @@ ResetAllOptions(void)
gconf->source = gconf->reset_source;
gconf->scontext = gconf->reset_scontext;
+ gconf->srole = gconf->reset_srole;
if (gconf->flags & GUC_REPORT)
{
@@ -6401,6 +6405,7 @@ push_old_value(struct config_generic *gconf, GucAction action)
{
/* SET followed by SET LOCAL, remember SET's value */
stack->masked_scontext = gconf->scontext;
+ stack->masked_srole = gconf->srole;
set_stack_value(gconf, &stack->masked);
stack->state = GUC_SET_LOCAL;
}
@@ -6439,6 +6444,7 @@ push_old_value(struct config_generic *gconf, GucAction action)
}
stack->source = gconf->source;
stack->scontext = gconf->scontext;
+ stack->srole = gconf->srole;
set_stack_value(gconf, &stack->prior);
gconf->stack = stack;
@@ -6583,6 +6589,7 @@ AtEOXact_GUC(bool isCommit, int nestLevel)
{
/* LOCAL migrates down */
prev->masked_scontext = stack->scontext;
+ prev->masked_srole = stack->srole;
prev->masked = stack->prior;
prev->state = GUC_SET_LOCAL;
}
@@ -6598,6 +6605,7 @@ AtEOXact_GUC(bool isCommit, int nestLevel)
discard_stack_value(gconf, &stack->prior);
/* copy down the masked state */
prev->masked_scontext = stack->masked_scontext;
+ prev->masked_srole = stack->masked_srole;
if (prev->state == GUC_SET_LOCAL)
discard_stack_value(gconf, &prev->masked);
prev->masked = stack->masked;
@@ -6614,18 +6622,21 @@ AtEOXact_GUC(bool isCommit, int nestLevel)
config_var_value newvalue;
GucSource newsource;
GucContext newscontext;
+ Oid newsrole;
if (restoreMasked)
{
newvalue = stack->masked;
newsource = PGC_S_SESSION;
newscontext = stack->masked_scontext;
+ newsrole = stack->masked_srole;
}
else
{
newvalue = stack->prior;
newsource = stack->source;
newscontext = stack->scontext;
+ newsrole = stack->srole;
}
switch (gconf->vartype)
@@ -6740,6 +6751,7 @@ AtEOXact_GUC(bool isCommit, int nestLevel)
/* And restore source information */
gconf->source = newsource;
gconf->scontext = newscontext;
+ gconf->srole = newsrole;
}
/* Finish popping the state stack */
@@ -7520,7 +7532,7 @@ parse_and_validate_value(struct config_generic *record,
/*
- * Sets option `name' to given value.
+ * set_config_option: sets option `name' to given value.
*
* The value should be a string, which will be parsed and converted to
* the appropriate data type. The context and source parameters indicate
@@ -7563,6 +7575,46 @@ set_config_option(const char *name, const char *value,
GucContext context, GucSource source,
GucAction action, bool changeVal, int elevel,
bool is_reload)
+{
+ Oid srole;
+
+ /*
+ * Non-interactive sources should be treated as having all privileges,
+ * except for PGC_S_CLIENT. Note in particular that this is true for
+ * pg_db_role_setting sources (PGC_S_GLOBAL etc): we assume a suitable
+ * privilege check was done when the pg_db_role_setting entry was made.
+ */
+ if (source >= PGC_S_INTERACTIVE || source == PGC_S_CLIENT)
+ srole = GetUserId();
+ else
+ srole = BOOTSTRAP_SUPERUSERID;
+
+ return set_config_option_ext(name, value,
+ context, source, srole,
+ action, changeVal, elevel,
+ is_reload);
+}
+
+/*
+ * set_config_option_ext: sets option `name' to given value.
+ *
+ * This API adds the ability to explicitly specify which role OID
+ * is considered to be setting the value. Most external callers can use
+ * set_config_option() and let it determine that based on the GucSource,
+ * but there are a few that are supplying a value that was determined
+ * in some special way and need to override the decision. Also, when
+ * restoring a previously-assigned value, it's important to supply the
+ * same role OID that set the value originally; so all guc.c callers
+ * that are doing that type of thing need to call this directly.
+ *
+ * Generally, srole should be GetUserId() when the source is a SQL operation,
+ * or BOOTSTRAP_SUPERUSERID if the source is a config file or similar.
+ */
+int
+set_config_option_ext(const char *name, const char *value,
+ GucContext context, GucSource source, Oid srole,
+ GucAction action, bool changeVal, int elevel,
+ bool is_reload)
{
struct config_generic *record;
union config_var_val newval_union;
@@ -7667,12 +7719,12 @@ set_config_option(const char *name, const char *value,
if (context == PGC_BACKEND)
{
/*
- * Check whether the current user has been granted privilege
- * to set this GUC.
+ * Check whether the requesting user has been granted
+ * privilege to set this GUC.
*/
AclResult aclresult;
- aclresult = pg_parameter_aclcheck(name, GetUserId(), ACL_SET);
+ aclresult = pg_parameter_aclcheck(name, srole, ACL_SET);
if (aclresult != ACLCHECK_OK)
{
/* No granted privilege */
@@ -7725,12 +7777,12 @@ set_config_option(const char *name, const char *value,
if (context == PGC_USERSET || context == PGC_BACKEND)
{
/*
- * Check whether the current user has been granted privilege
- * to set this GUC.
+ * Check whether the requesting user has been granted
+ * privilege to set this GUC.
*/
AclResult aclresult;
- aclresult = pg_parameter_aclcheck(name, GetUserId(), ACL_SET);
+ aclresult = pg_parameter_aclcheck(name, srole, ACL_SET);
if (aclresult != ACLCHECK_OK)
{
/* No granted privilege */
@@ -7847,6 +7899,7 @@ set_config_option(const char *name, const char *value,
newextra = conf->reset_extra;
source = conf->gen.reset_source;
context = conf->gen.reset_scontext;
+ srole = conf->gen.reset_srole;
}
if (prohibitValueChange)
@@ -7881,6 +7934,7 @@ set_config_option(const char *name, const char *value,
newextra);
conf->gen.source = source;
conf->gen.scontext = context;
+ conf->gen.srole = srole;
}
if (makeDefault)
{
@@ -7893,6 +7947,7 @@ set_config_option(const char *name, const char *value,
newextra);
conf->gen.reset_source = source;
conf->gen.reset_scontext = context;
+ conf->gen.reset_srole = srole;
}
for (stack = conf->gen.stack; stack; stack = stack->prev)
{
@@ -7903,6 +7958,7 @@ set_config_option(const char *name, const char *value,
newextra);
stack->source = source;
stack->scontext = context;
+ stack->srole = srole;
}
}
}
@@ -7941,6 +7997,7 @@ set_config_option(const char *name, const char *value,
newextra = conf->reset_extra;
source = conf->gen.reset_source;
context = conf->gen.reset_scontext;
+ srole = conf->gen.reset_srole;
}
if (prohibitValueChange)
@@ -7975,6 +8032,7 @@ set_config_option(const char *name, const char *value,
newextra);
conf->gen.source = source;
conf->gen.scontext = context;
+ conf->gen.srole = srole;
}
if (makeDefault)
{
@@ -7987,6 +8045,7 @@ set_config_option(const char *name, const char *value,
newextra);
conf->gen.reset_source = source;
conf->gen.reset_scontext = context;
+ conf->gen.reset_srole = srole;
}
for (stack = conf->gen.stack; stack; stack = stack->prev)
{
@@ -7997,6 +8056,7 @@ set_config_option(const char *name, const char *value,
newextra);
stack->source = source;
stack->scontext = context;
+ stack->srole = srole;
}
}
}
@@ -8035,6 +8095,7 @@ set_config_option(const char *name, const char *value,
newextra = conf->reset_extra;
source = conf->gen.reset_source;
context = conf->gen.reset_scontext;
+ srole = conf->gen.reset_srole;
}
if (prohibitValueChange)
@@ -8069,6 +8130,7 @@ set_config_option(const char *name, const char *value,
newextra);
conf->gen.source = source;
conf->gen.scontext = context;
+ conf->gen.srole = srole;
}
if (makeDefault)
{
@@ -8081,6 +8143,7 @@ set_config_option(const char *name, const char *value,
newextra);
conf->gen.reset_source = source;
conf->gen.reset_scontext = context;
+ conf->gen.reset_srole = srole;
}
for (stack = conf->gen.stack; stack; stack = stack->prev)
{
@@ -8091,6 +8154,7 @@ set_config_option(const char *name, const char *value,
newextra);
stack->source = source;
stack->scontext = context;
+ stack->srole = srole;
}
}
}
@@ -8145,6 +8209,7 @@ set_config_option(const char *name, const char *value,
newextra = conf->reset_extra;
source = conf->gen.reset_source;
context = conf->gen.reset_scontext;
+ srole = conf->gen.reset_srole;
}
if (prohibitValueChange)
@@ -8189,6 +8254,7 @@ set_config_option(const char *name, const char *value,
newextra);
conf->gen.source = source;
conf->gen.scontext = context;
+ conf->gen.srole = srole;
}
if (makeDefault)
@@ -8202,6 +8268,7 @@ set_config_option(const char *name, const char *value,
newextra);
conf->gen.reset_source = source;
conf->gen.reset_scontext = context;
+ conf->gen.reset_srole = srole;
}
for (stack = conf->gen.stack; stack; stack = stack->prev)
{
@@ -8213,6 +8280,7 @@ set_config_option(const char *name, const char *value,
newextra);
stack->source = source;
stack->scontext = context;
+ stack->srole = srole;
}
}
}
@@ -8254,6 +8322,7 @@ set_config_option(const char *name, const char *value,
newextra = conf->reset_extra;
source = conf->gen.reset_source;
context = conf->gen.reset_scontext;
+ srole = conf->gen.reset_srole;
}
if (prohibitValueChange)
@@ -8288,6 +8357,7 @@ set_config_option(const char *name, const char *value,
newextra);
conf->gen.source = source;
conf->gen.scontext = context;
+ conf->gen.srole = srole;
}
if (makeDefault)
{
@@ -8300,6 +8370,7 @@ set_config_option(const char *name, const char *value,
newextra);
conf->gen.reset_source = source;
conf->gen.reset_scontext = context;
+ conf->gen.reset_srole = srole;
}
for (stack = conf->gen.stack; stack; stack = stack->prev)
{
@@ -8310,6 +8381,7 @@ set_config_option(const char *name, const char *value,
newextra);
stack->source = source;
stack->scontext = context;
+ stack->srole = srole;
}
}
}
@@ -9354,17 +9426,19 @@ define_custom_variable(struct config_generic *variable)
/* First, apply the reset value if any */
if (pHolder->reset_val)
- (void) set_config_option(name, pHolder->reset_val,
- pHolder->gen.reset_scontext,
- pHolder->gen.reset_source,
- GUC_ACTION_SET, true, WARNING, false);
+ (void) set_config_option_ext(name, pHolder->reset_val,
+ pHolder->gen.reset_scontext,
+ pHolder->gen.reset_source,
+ pHolder->gen.reset_srole,
+ GUC_ACTION_SET, true, WARNING, false);
/* That should not have resulted in stacking anything */
Assert(variable->stack == NULL);
/* Now, apply current and stacked values, in the order they were stacked */
reapply_stacked_values(variable, pHolder, pHolder->gen.stack,
*(pHolder->variable),
- pHolder->gen.scontext, pHolder->gen.source);
+ pHolder->gen.scontext, pHolder->gen.source,
+ pHolder->gen.srole);
/* Also copy over any saved source-location information */
if (pHolder->gen.sourcefile)
@@ -9395,7 +9469,8 @@ reapply_stacked_values(struct config_generic *variable,
struct config_string *pHolder,
GucStack *stack,
const char *curvalue,
- GucContext curscontext, GucSource cursource)
+ GucContext curscontext, GucSource cursource,
+ Oid cursrole)
{
const char *name = variable->name;
GucStack *oldvarstack = variable->stack;
@@ -9405,43 +9480,45 @@ reapply_stacked_values(struct config_generic *variable,
/* First, recurse, so that stack items are processed bottom to top */
reapply_stacked_values(variable, pHolder, stack->prev,
stack->prior.val.stringval,
- stack->scontext, stack->source);
+ stack->scontext, stack->source, stack->srole);
/* See how to apply the passed-in value */
switch (stack->state)
{
case GUC_SAVE:
- (void) set_config_option(name, curvalue,
- curscontext, cursource,
- GUC_ACTION_SAVE, true,
- WARNING, false);
+ (void) set_config_option_ext(name, curvalue,
+ curscontext, cursource, cursrole,
+ GUC_ACTION_SAVE, true,
+ WARNING, false);
break;
case GUC_SET:
- (void) set_config_option(name, curvalue,
- curscontext, cursource,
- GUC_ACTION_SET, true,
- WARNING, false);
+ (void) set_config_option_ext(name, curvalue,
+ curscontext, cursource, cursrole,
+ GUC_ACTION_SET, true,
+ WARNING, false);
break;
case GUC_LOCAL:
- (void) set_config_option(name, curvalue,
- curscontext, cursource,
- GUC_ACTION_LOCAL, true,
- WARNING, false);
+ (void) set_config_option_ext(name, curvalue,
+ curscontext, cursource, cursrole,
+ GUC_ACTION_LOCAL, true,
+ WARNING, false);
break;
case GUC_SET_LOCAL:
/* first, apply the masked value as SET */
- (void) set_config_option(name, stack->masked.val.stringval,
- stack->masked_scontext, PGC_S_SESSION,
- GUC_ACTION_SET, true,
- WARNING, false);
+ (void) set_config_option_ext(name, stack->masked.val.stringval,
+ stack->masked_scontext,
+ PGC_S_SESSION,
+ stack->masked_srole,
+ GUC_ACTION_SET, true,
+ WARNING, false);
/* then apply the current value as LOCAL */
- (void) set_config_option(name, curvalue,
- curscontext, cursource,
- GUC_ACTION_LOCAL, true,
- WARNING, false);
+ (void) set_config_option_ext(name, curvalue,
+ curscontext, cursource, cursrole,
+ GUC_ACTION_LOCAL, true,
+ WARNING, false);
break;
}
@@ -9461,11 +9538,12 @@ reapply_stacked_values(struct config_generic *variable,
*/
if (curvalue != pHolder->reset_val ||
curscontext != pHolder->gen.reset_scontext ||
- cursource != pHolder->gen.reset_source)
+ cursource != pHolder->gen.reset_source ||
+ cursrole != pHolder->gen.reset_srole)
{
- (void) set_config_option(name, curvalue,
- curscontext, cursource,
- GUC_ACTION_SET, true, WARNING, false);
+ (void) set_config_option_ext(name, curvalue,
+ curscontext, cursource, cursrole,
+ GUC_ACTION_SET, true, WARNING, false);
variable->stack = NULL;
}
}
@@ -10577,6 +10655,7 @@ _ShowOption(struct config_generic *record, bool use_units)
* variable sourceline, integer
* variable source, integer
* variable scontext, integer
+* variable srole, OID
*/
static void
write_one_nondefault_variable(FILE *fp, struct config_generic *gconf)
@@ -10643,6 +10722,7 @@ write_one_nondefault_variable(FILE *fp, struct config_generic *gconf)
fwrite(&gconf->sourceline, 1, sizeof(gconf->sourceline), fp);
fwrite(&gconf->source, 1, sizeof(gconf->source), fp);
fwrite(&gconf->scontext, 1, sizeof(gconf->scontext), fp);
+ fwrite(&gconf->srole, 1, sizeof(gconf->srole), fp);
}
void
@@ -10738,6 +10818,7 @@ read_nondefault_variables(void)
int varsourceline;
GucSource varsource;
GucContext varscontext;
+ Oid varsrole;
/*
* Open file
@@ -10774,10 +10855,12 @@ read_nondefault_variables(void)
elog(FATAL, "invalid format of exec config params file");
if (fread(&varscontext, 1, sizeof(varscontext), fp) != sizeof(varscontext))
elog(FATAL, "invalid format of exec config params file");
+ if (fread(&varsrole, 1, sizeof(varsrole), fp) != sizeof(varsrole))
+ elog(FATAL, "invalid format of exec config params file");
- (void) set_config_option(varname, varvalue,
- varscontext, varsource,
- GUC_ACTION_SET, true, 0, true);
+ (void) set_config_option_ext(varname, varvalue,
+ varscontext, varsource, varsrole,
+ GUC_ACTION_SET, true, 0, true);
if (varsourcefile[0])
set_config_sourcefile(varname, varsourcefile, varsourceline);
@@ -10931,6 +11014,7 @@ estimate_variable_size(struct config_generic *gconf)
size = add_size(size, sizeof(gconf->source));
size = add_size(size, sizeof(gconf->scontext));
+ size = add_size(size, sizeof(gconf->srole));
return size;
}
@@ -11076,6 +11160,8 @@ serialize_variable(char **destptr, Size *maxbytes,
sizeof(gconf->source));
do_serialize_binary(destptr, maxbytes, &gconf->scontext,
sizeof(gconf->scontext));
+ do_serialize_binary(destptr, maxbytes, &gconf->srole,
+ sizeof(gconf->srole));
}
/*
@@ -11177,6 +11263,7 @@ RestoreGUCState(void *gucstate)
int varsourceline;
GucSource varsource;
GucContext varscontext;
+ Oid varsrole;
char *srcptr = (char *) gucstate;
char *srcend;
Size len;
@@ -11304,12 +11391,15 @@ RestoreGUCState(void *gucstate)
&varsource, sizeof(varsource));
read_gucstate_binary(&srcptr, srcend,
&varscontext, sizeof(varscontext));
+ read_gucstate_binary(&srcptr, srcend,
+ &varsrole, sizeof(varsrole));
error_context_name_and_value[0] = varname;
error_context_name_and_value[1] = varvalue;
error_context_callback.arg = &error_context_name_and_value[0];
- result = set_config_option(varname, varvalue, varscontext, varsource,
- GUC_ACTION_SET, true, ERROR, true);
+ result = set_config_option_ext(varname, varvalue,
+ varscontext, varsource, varsrole,
+ GUC_ACTION_SET, true, ERROR, true);
if (result <= 0)
ereport(ERROR,
(errcode(ERRCODE_INTERNAL_ERROR),
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index 4d0920c42e..e734493a48 100644
--- a/src/include/utils/guc.h
+++ b/src/include/utils/guc.h
@@ -388,6 +388,11 @@ extern int set_config_option(const char *name, const char *value,
GucContext context, GucSource source,
GucAction action, bool changeVal, int elevel,
bool is_reload);
+extern int set_config_option_ext(const char *name, const char *value,
+ GucContext context, GucSource source,
+ Oid srole,
+ GucAction action, bool changeVal, int elevel,
+ bool is_reload);
extern void AlterSystemSetConfigFile(AlterSystemStmt *altersysstmt);
extern char *GetConfigOptionByName(const char *name, const char **varname,
bool missing_ok);
diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h
index ba44f7437b..067d82ada9 100644
--- a/src/include/utils/guc_tables.h
+++ b/src/include/utils/guc_tables.h
@@ -120,6 +120,8 @@ typedef struct guc_stack
/* masked value's source must be PGC_S_SESSION, so no need to store it */
GucContext scontext; /* context that set the prior value */
GucContext masked_scontext; /* context that set the masked value */
+ Oid srole; /* role that set the prior value */
+ Oid masked_srole; /* role that set the masked value */
config_var_value prior; /* previous value of variable */
config_var_value masked; /* SET value in a GUC_SET_LOCAL entry */
} GucStack;
@@ -131,6 +133,10 @@ typedef struct guc_stack
* applications may use the long description as well, and will append
* it to the short description. (separated by a newline or '. ')
*
+ * srole is the role that set the current value, or BOOTSTRAP_SUPERUSERID
+ * if the value came from an internal source or the config file. Similarly
+ * for reset_srole (which is usually BOOTSTRAP_SUPERUSERID, but not always).
+ *
* Note that sourcefile/sourceline are kept here, and not pushed into stacked
* values, although in principle they belong with some stacked value if the
* active value is session- or transaction-local. This is to avoid bloating
@@ -152,6 +158,8 @@ struct config_generic
GucSource reset_source; /* source of the reset_value */
GucContext scontext; /* context that set the current value */
GucContext reset_scontext; /* context that set the reset value */
+ Oid srole; /* role that set the current value */
+ Oid reset_srole; /* role that set the reset value */
GucStack *stack; /* stacked prior values */
void *extra; /* "extra" pointer for current actual value */
char *last_reported; /* if variable is GUC_REPORT, value last sent
diff --git a/src/pl/plperl/expected/plperl_init.out b/src/pl/plperl/expected/plperl_init.out
index 133828e9f3..4b7e925419 100644
--- a/src/pl/plperl/expected/plperl_init.out
+++ b/src/pl/plperl/expected/plperl_init.out
@@ -1,4 +1,4 @@
--- test plperl.on_plperl_init errors are fatal
+-- test plperl.on_plperl_init
-- This test tests setting on_plperl_init after loading plperl
LOAD 'plperl';
SET SESSION plperl.on_plperl_init = ' system("/nonesuch"); ';
@@ -12,3 +12,29 @@ DO $$ warn 42 $$ language plperl;
ERROR: 'system' trapped by operation mask at line 1.
CONTEXT: while executing plperl.on_plperl_init
PL/Perl anonymous code block
+--
+-- Reconnect (to unload plperl), then test setting on_plperl_init
+-- as an unprivileged user
+--
+\c -
+CREATE ROLE regress_plperl_user;
+SET ROLE regress_plperl_user;
+-- this succeeds, since the GUC isn't known yet
+SET SESSION plperl.on_plperl_init = 'test';
+RESET ROLE;
+LOAD 'plperl';
+WARNING: permission denied to set parameter "plperl.on_plperl_init"
+SHOW plperl.on_plperl_init;
+ plperl.on_plperl_init
+-----------------------
+
+(1 row)
+
+DO $$ warn 42 $$ language plperl;
+WARNING: 42 at line 1.
+-- now we won't be allowed to set it in the first place
+SET ROLE regress_plperl_user;
+SET SESSION plperl.on_plperl_init = 'test';
+ERROR: permission denied to set parameter "plperl.on_plperl_init"
+RESET ROLE;
+DROP ROLE regress_plperl_user;
diff --git a/src/pl/plperl/sql/plperl_init.sql b/src/pl/plperl/sql/plperl_init.sql
index 4ebf3f86eb..2aa3811033 100644
--- a/src/pl/plperl/sql/plperl_init.sql
+++ b/src/pl/plperl/sql/plperl_init.sql
@@ -1,4 +1,4 @@
--- test plperl.on_plperl_init errors are fatal
+-- test plperl.on_plperl_init
-- This test tests setting on_plperl_init after loading plperl
LOAD 'plperl';
@@ -8,3 +8,34 @@ SET SESSION plperl.on_plperl_init = ' system("/nonesuch"); ';
SHOW plperl.on_plperl_init;
DO $$ warn 42 $$ language plperl;
+
+--
+-- Reconnect (to unload plperl), then test setting on_plperl_init
+-- as an unprivileged user
+--
+
+\c -
+
+CREATE ROLE regress_plperl_user;
+
+SET ROLE regress_plperl_user;
+
+-- this succeeds, since the GUC isn't known yet
+SET SESSION plperl.on_plperl_init = 'test';
+
+RESET ROLE;
+
+LOAD 'plperl';
+
+SHOW plperl.on_plperl_init;
+
+DO $$ warn 42 $$ language plperl;
+
+-- now we won't be allowed to set it in the first place
+SET ROLE regress_plperl_user;
+
+SET SESSION plperl.on_plperl_init = 'test';
+
+RESET ROLE;
+
+DROP ROLE regress_plperl_user;
On Thu, Jul 14, 2022 at 04:02:30PM -0400, Tom Lane wrote: > Here's a draft patch for that. I initially ran around and changed all > the set_config_option callers as I threatened before, but as I did it > I could not help observing that they were all changing in exactly the > same way: basically, they were passing GetUserId() if the GucContext > is PGC_S_SESSION and BOOTSTRAP_SUPERUSERID otherwise. Not counting > guc.c internal call sites, there is a grand total of one caller that > fails to fit the pattern. So that brought me around to liking the idea > of keeping set_config_option's API stable by making it a thin wrapper > around another function with an explicit role argument. The result, > attached, poses far less of an API/ABI hazard than I was anticipating. > If you're not poking into the GUC tables you have little to fear. > > Most of the bulk of this is mechanical changes to pass the source > role around properly in guc.c's data structures. That's all basically > copy-and-paste from the code to track the source context (scontext). At first glance, this looks pretty reasonable to me. > I noted something that ought to be looked at separately: > validate_option_array_item() seems like it needs to be taught about > grantable permissions on GUCs. I think that right now it may report > permissions failures in some cases where it should succeed. Which cases do you think might be inappropriately reporting permissions failures? It looked to me like this stuff was mostly used for pg_db_role_setting, which wouldn't be impacted by the current set of grantable GUC permissions. Is the idea that you should be able to do ALTER ROLE SET for GUCs that you have SET permissions for? -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
Nathan Bossart <nathandbossart@gmail.com> writes:
> On Thu, Jul 14, 2022 at 04:02:30PM -0400, Tom Lane wrote:
>> I noted something that ought to be looked at separately:
>> validate_option_array_item() seems like it needs to be taught about
>> grantable permissions on GUCs. I think that right now it may report
>> permissions failures in some cases where it should succeed.
> Which cases do you think might be inappropriately reporting permissions
> failures? It looked to me like this stuff was mostly used for
> pg_db_role_setting, which wouldn't be impacted by the current set of
> grantable GUC permissions. Is the idea that you should be able to do ALTER
> ROLE SET for GUCs that you have SET permissions for?
Well, that's what I'm wondering. Obviously that wouldn't *alone* be
enough permissions, but it seems like it could be a component of it.
Specifically, this bit:
/* manual permissions check so we can avoid an error being thrown */
if (gconf->context == PGC_USERSET)
/* ok */ ;
else if (gconf->context == PGC_SUSET && superuser())
/* ok */ ;
else if (skipIfNoPermissions)
return false;
seems like it's trying to duplicate what set_config_option would do,
and it's now missing a component of that. If it shouldn't check
per-GUC permissions along with superuser(), we should add a comment
explaining why not.
regards, tom lane
On Thu, Jul 14, 2022 at 06:03:45PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> On Thu, Jul 14, 2022 at 04:02:30PM -0400, Tom Lane wrote:
>>> I noted something that ought to be looked at separately:
>>> validate_option_array_item() seems like it needs to be taught about
>>> grantable permissions on GUCs. I think that right now it may report
>>> permissions failures in some cases where it should succeed.
>
>> Which cases do you think might be inappropriately reporting permissions
>> failures? It looked to me like this stuff was mostly used for
>> pg_db_role_setting, which wouldn't be impacted by the current set of
>> grantable GUC permissions. Is the idea that you should be able to do ALTER
>> ROLE SET for GUCs that you have SET permissions for?
>
> Well, that's what I'm wondering. Obviously that wouldn't *alone* be
> enough permissions, but it seems like it could be a component of it.
> Specifically, this bit:
>
> /* manual permissions check so we can avoid an error being thrown */
> if (gconf->context == PGC_USERSET)
> /* ok */ ;
> else if (gconf->context == PGC_SUSET && superuser())
> /* ok */ ;
> else if (skipIfNoPermissions)
> return false;
>
> seems like it's trying to duplicate what set_config_option would do,
> and it's now missing a component of that. If it shouldn't check
> per-GUC permissions along with superuser(), we should add a comment
> explaining why not.
I looked into this a bit closer. I found that having SET permissions on a
GUC seems to allow you to ALTER ROLE SET it to others.
postgres=# CREATE ROLE test CREATEROLE;
CREATE ROLE
postgres=# CREATE ROLE other;
CREATE ROLE
postgres=# GRANT SET ON PARAMETER zero_damaged_pages TO test;
GRANT
postgres=# SET ROLE test;
SET
postgres=> ALTER ROLE other SET zero_damaged_pages = 'on';
ALTER ROLE
postgres=> SELECT * FROM pg_db_role_setting;
setdatabase | setrole | setconfig
-------------+---------+-------------------------
0 | 16385 | {zero_damaged_pages=on}
(1 row)
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.
Regarding whether SET privileges should be enough to allow ALTER ROLE SET,
I'm not sure I have an opinion yet. You would need WITH GRANT OPTION to be
able to grant SET to that role, but that's a bit different than altering
the setting for the role. You'll already have privileges to alter the role
(e.g., CREATEROLE), so requiring extra permissions to set GUCs on roles
feels like it might be excessive. But there might be a good argument for
it.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
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
Вложения
Nathan Bossart <nathandbossart@gmail.com> writes:
>> 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.
Yeah, this is more or less what I was thinking of.
> 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.
I thought about that for a bit. You could almost do it today if you
passed elevel == DEBUG5; the ensuing log chatter for failures would be
down in the noise compared to everything else you would see with
min_messages cranked down that far. However,
(1) As things stand, set_config_option()'s result does not distinguish
no-permissions failures from other problems, so we'd need some rejiggering
of its API anyway.
(2) As you mused upthread, it's possible that ACL_SET isn't what we should
be checking here, but some more-specific privilege. So I'd just as soon
keep this privilege check separate from set_config_option's.
I'll push ahead with fixing it like this.
regards, tom lane
On Tue, Jul 19, 2022 at 04:27:08PM -0400, Tom Lane wrote: > Nathan Bossart <nathandbossart@gmail.com> writes: >> 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. > > I thought about that for a bit. You could almost do it today if you > passed elevel == DEBUG5; the ensuing log chatter for failures would be > down in the noise compared to everything else you would see with > min_messages cranked down that far. However, > > (1) As things stand, set_config_option()'s result does not distinguish > no-permissions failures from other problems, so we'd need some rejiggering > of its API anyway. > > (2) As you mused upthread, it's possible that ACL_SET isn't what we should > be checking here, but some more-specific privilege. So I'd just as soon > keep this privilege check separate from set_config_option's. I think we'd also need to keep the manual permissions checks for placeholders, so it wouldn't save much, anyway. > I'll push ahead with fixing it like this. Sounds good. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com