Обсуждение: Fix search_path for all maintenance commands
Maintenance commands (ANALYZE, CLUSTER, REFRESH MATERIALIZED VIEW, REINDEX, and VACUUM) currently run as the table owner, and as a SECURITY_RESTRICTED_OPERATION. I propose that we also fix the search_path to "pg_catalog, pg_temp" when running maintenance commands, for two reasons: 1. Make the behavior of maintenance commands more consistent because they'd always have the same search_path. 2. Now that we have the MAINTAIN privilege in 16, privileged non- superusers can execute maintenance commands on other users' tables. That raises the possibility that a user with MAINTAIN privilege may be able to use search_path tricks to escalate privileges to the table owner. The MAINTAIN privilege is only given to highly-privileged users, but there's still some risk. For this reason I also propose that it goes in v16. There's one interesting part: in the code path for creating a materialized view, ExecCreateTableAs() has this comment: /* * For materialized views, lock down security-restricted operations and * arrange to make GUC variable changes local to this command. This is * not necessary for security, but this keeps the behavior similar to * REFRESH MATERIALIZED VIEW. Otherwise, one could create a materialized * view not possible to refresh. */ My patch doesn't address this ExecCreateTableAs() check. To do so, we would need to set the search path after DefineRelation(), otherwise it will try to create the object in pg_catalog. But DefineRelation() is happening at execution time, well after we entered the SECURITY_RESTRICTED_OPERATION, and it doesn't seem good to separate the SECURITY_RESTRICTED_OPERATION from setting search_path. This ExecCreateTableAs() check doesn't seem terribly important, so I don't think it's necessary to improve it as a part of this patch (it won't be perfect anyway: functions can behave inconsistently for all kinds of reasons). But I'm open to suggestion if someone knows a good way to do it. -- Jeff Davis PostgreSQL Contributor Team - AWS
Вложения
On Fri, 26 May 2023 at 19:22, Jeff Davis <pgsql@j-davis.com> wrote: > > Maintenance commands (ANALYZE, CLUSTER, REFRESH MATERIALIZED VIEW, > REINDEX, and VACUUM) currently run as the table owner, and as a > SECURITY_RESTRICTED_OPERATION. > > I propose that we also fix the search_path to "pg_catalog, pg_temp" > when running maintenance commands, for two reasons: > > 1. Make the behavior of maintenance commands more consistent because > they'd always have the same search_path. What exactly would this impact? Offhand... expression indexes where the functions in the expression (which would already be schema qualified) themselves reference other objects without schema qualification? So this would negatively affect someone who was using such a dangerous function definition but was careful to always use the same search_path on it. Perhaps someone who had created an expression index on their own table in their own schema calling their own functions in their own schema. As long as nobody else ever calls it that would work but this would cause superuser to no longer be able to reindex it even if superuser set the same search_path? I guess that's pretty narrow and a reasonable thing to desupport. Users could just mark those functions with search_path or schema qualify the object references in them. Perhaps we should also be picking up cases like that sooner so users realize they've created a footgun for themselves? -- greg
On Thu, Jun 08, 2023 at 06:08:08PM -0400, Greg Stark wrote: > I guess that's pretty narrow and a reasonable thing to desupport. > Users could just mark those functions with search_path or schema > qualify the object references in them. Perhaps we should also be > picking up cases like that sooner so users realize they've created a > footgun for themselves? I'm inclined to agree that this is reasonable to desupport. Relying on the search_path for the cases Greg describes already seems rather fragile, so I'm skeptical that forcing a safe one for maintenance commands would make things significantly worse. At least, it sounds like the right trade-off based on Jeff's note about privilege escalation risks. I bet we could skip forcing the search_path for maintenance commands run as the table owner, but such a discrepancy seems likely to cause far more confusion than anything else. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Thu, 2023-06-08 at 21:55 -0700, Nathan Bossart wrote:
> On Thu, Jun 08, 2023 at 06:08:08PM -0400, Greg Stark wrote:
> > I guess that's pretty narrow and a reasonable thing to desupport.
> > Users could just mark those functions with search_path or schema
> > qualify the object references in them. Perhaps we should also be
> > picking up cases like that sooner so users realize they've created
> > a
> > footgun for themselves?
Many cases will be picked up, for instance CREATE INDEX will error if
the safe search path is not good enough.
> I'm inclined to agree that this is reasonable to desupport.
Committed.
> I bet we could skip forcing the search_path for maintenance commands
> run as
> the table owner, but such a discrepancy seems likely to cause far
> more
> confusion than anything else.
Agreed.
Regards,
Jeff Davis
On Fri, Jun 9, 2023 at 2:00 PM Jeff Davis <pgsql@j-davis.com> wrote: > > On Thu, 2023-06-08 at 21:55 -0700, Nathan Bossart wrote: > > On Thu, Jun 08, 2023 at 06:08:08PM -0400, Greg Stark wrote: > > > I guess that's pretty narrow and a reasonable thing to desupport. > > > Users could just mark those functions with search_path or schema > > > qualify the object references in them. Perhaps we should also be > > > picking up cases like that sooner so users realize they've created > > > a > > > footgun for themselves? > > Many cases will be picked up, for instance CREATE INDEX will error if > the safe search path is not good enough. > > > I'm inclined to agree that this is reasonable to desupport. > > Committed. I'm not sure if mine is a valid concern, and it has been a long time since I looked at the search_path's and switching Role's implications (CVE-2009-4136) so pardon my ignorance. It feels a bit late in release cycle to introduce this breaking change. If they depended on search_path, people and utilities that use these maintenance commands may see failures. Although I can't think of a scenario where such a failure may cause an outage, sometimes these maintenance operations are performed while the users are staring down the barrel of a gun (imminent danger of running out of space, bad statistics causing absurd query plans, etc.). So, if not directly, a failure of these operations may indirectly cause an outage. I feel more thought needs to be given to the impact of this change, and we should to give others more time for feedback. Short of that, it'd be prudent to allow the user to somehow fall back to old behaviour; a command-line option, or GUC, etc. That way we can mark the old behaviour "deprecated", with a workaround for those who may desperately need it, and in another release or so, finally pull the plug on old behaviour. My 2bits. Best regards, Gurjeet http://Gurje.et
On Fri, 2023-06-09 at 16:29 -0700, Gurjeet Singh wrote:
> I'm not sure if mine is a valid concern, and it has been a long time
> since I looked at the search_path's and switching Role's implications
> (CVE-2009-4136) so pardon my ignorance.
>
> It feels a bit late in release cycle to introduce this breaking
> change.
That's a valid concern. It just needs to be weighed against the
potential problems of running maintenance code with different search
paths, and the interaction with the new MAINTAIN privilege.
> I feel more thought needs to be given to the impact of this change,
> and we should to give others more time for feedback.
For context, I initially posted to the -security list in case it needed
to be addressed there, and got some feedback on the patch before
posting to -hackers two weeks ago. So it has been seen by at least 4
people.
But I'm happy to hear more input and I'll backtrack if necessary.
Here are my thoughts:
Lazy VACUUM is by far the most important for the overall system. It's
unaffected by this change; see comment in vacuum_rel():
/*
* Switch to the table owner's userid, so that any index functions
are run
* as that user. Also lock down security-restricted operations and
* arrange to make GUC variable changes local to this command. (This
is
* unnecessary, but harmless, for lazy VACUUM.)
*/
REINDEX, CLUSTER, and VACUUM FULL are potentially affected because of
index functions, but only if the index functions are quite broken (or
at least very fragile) already.
REFRESH MATERIALIZED VIEW is the most likely to be affected because it
is more likely to call "interesting" functions and the author may not
anticipate a different search path.
A normal dump/reload cycle for upgrade testing will catch these
problems because it will create indexes after loading the data
(DefineIndex sets the search path), and it will also call REFRESH
MATERIALIZED VIEW. If using pg_upgrade instead, a post-upgrade ANALYZE
will catch index function problems, but I suppose not MV problems.
So there is some risk to this change. It feels fairly narrow to me, but
non-zero. Perhaps we can do more?
> Short of that, it'd be prudent to allow the user to somehow fall back
> to old behaviour; a command-line option, or GUC, etc. That way we can
> mark the old behaviour "deprecated", with a workaround for those who
> may desperately need it, and in another release or so, finally pull
> the plug on old behaviour.
That sounds wise, though others may not like the idea of a GUC just for
this change.
Regards,
Jeff Davis
On Fri, 2023-05-26 at 16:21 -0700, Jeff Davis wrote: > Maintenance commands (ANALYZE, CLUSTER, REFRESH MATERIALIZED VIEW, > REINDEX, and VACUUM) currently run as the table owner, and as a > SECURITY_RESTRICTED_OPERATION. > > I propose that we also fix the search_path to "pg_catalog, pg_temp" > when running maintenance commands New patch attached. We need this patch for several reasons: * If you have a functional index, and the function depends on the search_path, then it's easy to corrupt your index if you (or a superuser) run a REINDE/CLUSTER with the wrong search_path. * The MAINTAIN privilege needs a safe search_path, and was reverted from 16 because the search_path in 16 is not restricted. * In general, it's a good idea for things like functional indexes and materialized views to be independent of the search_path. * The search_path is already restricted in some other contexts, like logical replication and autoanalyze. Others have raised some concerns though: * It might break for users who have a functional index where the function implicitly depends on a search_path containing a namespace other than pg_catalog. My opinion is that such functional indexes are conceptually broken and we need to desupport them, and there will be some breakage, but I'm open to suggestion about how we minimize that (a compatibility GUC or something?). * The fix might not go far enough or might be in the wrong place. I'm open to suggestion here, too. Maybe we can make it part of the general function call mechanism, and can be overridden by explicitly setting the function search path? Or maybe we need new syntax where the function can acquire the search path from the session explicitly, but uses a safe search path by default? -- Jeff Davis PostgreSQL Contributor Team - AWS
Вложения
On Thu, 6 Jul 2023 at 21:39, Jeff Davis <pgsql@j-davis.com> wrote:
I apologize in advance if anything I’ve written below is either too obvious or too crazy or misinformed to belong here. I hope I have something to say that is on point, but feel unsure what makes sense to say.
* It might break for users who have a functional index where the
function implicitly depends on a search_path containing a namespace
other than pg_catalog. My opinion is that such functional indexes are
conceptually broken and we need to desupport them, and there will be
some breakage, but I'm open to suggestion about how we minimize that (a
compatibility GUC or something?).
I agree this is OK. If somebody has an index whole meaning depends on the search_path, then the best that can be said is that their database hasn't been corrupted yet. At the same time, I can see that somebody would get upset if they couldn't upgrade their database because of this. Maybe pg_upgrade could apply "SET search_path TO pg_catalog, pg_temp" to any function used in a functional index that doesn't have a search_path setting of its own? (BEGIN ATOMIC functions count, if I understand correctly, as having a search_path setting, because the lookups happen at definition time)
Now I'm doing more reading and I'm worried about SET TIME ZONE (or more precisely, its absence) and maybe some other ones.
* The fix might not go far enough or might be in the wrong place. I'm
open to suggestion here, too. Maybe we can make it part of the general
function call mechanism, and can be overridden by explicitly setting
the function search path? Or maybe we need new syntax where the
function can acquire the search path from the session explicitly, but
uses a safe search path by default?
Change it so by default each function gets handled as if "SET search_path FROM CURRENT" was applied to it? That's what I do for all my functions (maybe hurting performance?). Expand on my pg_upgrade idea above by applying it to all functions?
I feel that this may tie into other behaviour issues where to me it is obvious that the expected behaviour should be different from the actual behaviour. If a view calls a function, shouldn't it be called in the context of the view's definer/owner? It's weird that I can write a view that filters a table for users of the view, but as soon as the view calls functions they run in the security context of the user of the view. Are views security definers or not? Similar comment for triggers. Also as far as I can tell there is no way for a security definer function to determine who (which user) invoked it. So I can grant/deny access to run a particular function using permissions, but I can't have the supposed security definer define security for different callers.
Is the fundamental problem that we now find ourselves wanting to do things that require different defaults to work smoothly? On some level I suspect we want lexical scoping, which is what most of us have in our programming languages, in the database; but the database has many elements of dynamic scoping, and changing that is both a compatibility break and requires significant changes in the way the database is designed.
Hi,
On Thu, 2023-07-06 at 23:22 -0400, Isaac Morland wrote:
> Maybe pg_upgrade could apply "SET search_path TO pg_catalog, pg_temp"
> to any function used in a functional index that doesn't have a
> search_path setting of its own?
I don't think we want to go down the road of trying to solve this at
upgrade time.
> Now I'm doing more reading and I'm worried about SET TIME ZONE (or
> more precisely, its absence) and maybe some other ones.
That's a good point that it's not limited to search_path, but
search_path is by far the biggest problem.
For one thing, functions affected by TimeZone or other GUCs are
typically marked STABLE, and can't be used in index expressions. Also,
search_path affects a lot more functions.
> Change it so by default each function gets handled as if "SET
> search_path FROM CURRENT" was applied to it?
Yes, that's one idea, along with some syntax to get the old behavior
(inherit search_path at runtime) if you want.
It feels weird to make search_path too special in the syntax though. If
we want a general solution, we could do something like:
CREATE FUNCTION ...
[DEPENDS ON CONFIGURATION {NONE|{some_guc}[, ...]}]
[CONFIGURATION IS {STATIC|DYNAMIC}]
where STATIC means "all of the GUC dependencies are SET FROM CURRENT
unless specified otherwise" and DYNAMIC means "all of the GUC
dependencies come from the session at runtime unless specified
otherwise".
The default would be "DEPENDS CONFIGURATION search_path CONFIGURATION
IS STATIC".
That would make search_path special only because, by default, every
function would depend on it. Which I think summarizes the reason
search_path really is special.
That also opens up opportunities to do other things we might want to
do:
* have a compatibility GUC to set the default back to DYNAMIC
* track other dependencies of functions better ("DEPENDS ON TABLE
...")
* provide better error messages, like "can't use function xyz in
index expression because it depends on configuration parameter foo"
* be more consistent about using STABLE to mean that the function
depends on a snapshot, rather than overloading it for GUC dependencies
The question is, given that the acute problem is search_path, do we
want to invent all of the syntax above? Are there other use cases for
it, or should we just focus on search_path?
> That's what I do for all my functions (maybe hurting performance?).
It doesn't look cheap, although I think we could optimize it.
> If a view calls a function, shouldn't it be called in the context of
> the view's definer/owner?
Yeah, there are a bunch of problems along those lines. I don't know if
we can solve them all in one release.
> Is the fundamental problem that we now find ourselves wanting to do
> things that require different defaults to work smoothly? On some
> level I suspect we want lexical scoping, which is what most of us
> have in our programming languages, in the database; but the database
> has many elements of dynamic scoping, and changing that is both a
> compatibility break and requires significant changes in the way the
> database is designed.
Does that suggest another approach?
Regards,
Jeff Davis
On Thu, 2023-07-06 at 18:39 -0700, Jeff Davis wrote:
> * The fix might not go far enough or might be in the wrong place. I'm
> open to suggestion here, too. Maybe we can make it part of the
> general
> function call mechanism, and can be overridden by explicitly setting
> the function search path? Or maybe we need new syntax where the
> function can acquire the search path from the session explicitly, but
> uses a safe search path by default?
I'm inclined to proceed with the current approach here, which is to
just fix search_path for maintenance commands. Other approaches may be
possible, but:
(a) We already special-case the way functions are executed for
maintenance commands in other ways -- we run the functions as the table
owner (even SECURITY INVOKER functions) and run them as a
SECURITY_RESTRICTED_OPERATION. Setting the search_path to a safe value
seems like a natural extension of that; and
(b) The lack of a safe search path is blocking other useful features,
such as the MAINTAIN privilege; and
(c) I don't see other proposals, aside from a few ideas I put forward
here[1], which didn't get any responses.
The current approach seemed to get support from Noah, Nathan, and Greg
Stark.
Concerns were raised by Gurjeet, Tom, and Robert in the 16 cycle; but
I'm not sure whether those objections were specific to the 16 cycle or
whether they are objections to the approach entirely. Comments?
Regards,
Jeff Davis
[1]
https://www.postgresql.org/message-id/6781cc79580c464a63fc0a1343637ed2b2b0cf09.camel%40j-davis.com
On Thu, Jul 13, 2023 at 11:56 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> The current approach seemed to get support from Noah, Nathan, and Greg
> Stark.
>
> Concerns were raised by Gurjeet, Tom, and Robert in the 16 cycle; but
I didn't see Tom's or Robert's concerns raised in this thread. I see
now that for some reason there are two threads with slightly different
subjects. I'll catch-up on that, as well.
The other thread's subject: "pgsql: Fix search_path to a safe value
during maintenance operations"
> I'm not sure whether those objections were specific to the 16 cycle or
> whether they are objections to the approach entirely. Comments?
The approach seems good to me. My concern is with this change's
potential to cause an extended database outage. Hence sending it out
as part of v16, without any escape hatch for the DBA, is my objection.
It it were some commercial database, we would have simply introduced a
hidden event, or a GUC, with default off. But a GUC for this feels too
heavy handed.
Perhaps we can provide an escape hatch as follows (warning, pseudocode ahead).
> if (first_element(search_path) != "dont_override_search_patch_for_maintenance")
> SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, ...);
So, if someone desperately wants to just plow through the maintenance
command, and are not ready or able to fix their dependence on their
search_path, the community can show them this escape-hatch.
Best regards,
Gurjeet
http://Gurje.et
On Thu, Jul 13, 2023 at 12:54 PM Gurjeet Singh <gurjeet@singh.im> wrote:
The approach seems good to me. My concern is with this change's
potential to cause an extended database outage. Hence sending it out
as part of v16, without any escape hatch for the DBA, is my objection.
If this is limited to MAINT, which I'm in support of, there is no need for an "escape hatch". A prerequisite for leveraging the new feature is that you fix the code so it conforms to the new way of doing things.
Tom's opinion was a general dislike for differing behavior in different situations. I dislike it too, on purist grounds, but would rather do this than not make any forward progress because we made a poor decision in the past. And I'm against simply breaking the past without any recourse as what we did for pg_dump/pg_restore still bothers me.
David J.
On Thu, Jul 13, 2023 at 1:37 PM David G. Johnston <david.g.johnston@gmail.com> wrote: > > I'm against simply breaking the past without any recourse as what we did for pg_dump/pg_restore still bothers me. I'm sure this is tangential, but can you please provide some context/links to the change you're referring to here. Best regards, Gurjeet http://Gurje.et
On Thu, Jul 13, 2023 at 2:00 PM Gurjeet Singh <gurjeet@singh.im> wrote:
On Thu, Jul 13, 2023 at 1:37 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
>
> I'm against simply breaking the past without any recourse as what we did for pg_dump/pg_restore still bothers me.
I'm sure this is tangential, but can you please provide some
context/links to the change you're referring to here.
Here is the instigating issue and a discussion thread on the aftermath:
David J.
On Thu, 2023-07-13 at 13:37 -0700, David G. Johnston wrote: > If this is limited to MAINT, which I'm in support of, there is no > need for an "escape hatch". A prerequisite for leveraging the new > feature is that you fix the code so it conforms to the new way of > doing things. The current patch is not limited to exercise of the MAINTAIN privilege. > Tom's opinion was a general dislike for differing behavior in > different situations. I dislike it too, on purist grounds, but would > rather do this than not make any forward progress because we made a > poor decision in the past. I believe the opinion you're referring to is here: https://www.postgresql.org/message-id/1659699.1688086436@sss.pgh.pa.us Which was a reaction to another version of my patch which implemented your idea to limit the changes to the MAINTAIN privilege. I agree with you that we should be practical here. The end goal is to move users away from using functions that both (a) implicitly depend on the search_path; and (b) are called implicitly as a side-effect of accessing a table. Whatever is the fastest and smoothest way to get there is fine with me. > And I'm against simply breaking the past without any recourse as what > we did for pg_dump/pg_restore still bothers me. Is a GUC the solution here? Gurjeet called it heavy-handed, and I see the point that carrying such a GUC forever would be unpleasant. But if it reduces the risk of breakage (or at least offers an escape hatch) then it may be wise, and hopefully we can remove it later. Regards, Jeff Davis
On Thu, Jul 13, 2023 at 02:07:27PM -0700, David G. Johnston wrote: > On Thu, Jul 13, 2023 at 2:00 PM Gurjeet Singh <gurjeet@singh.im> wrote: > > On Thu, Jul 13, 2023 at 1:37 PM David G. Johnston <david.g.johnston@gmail.com> wrote: > > > I'm against simply breaking the past without any recourse as what we > > did for pg_dump/pg_restore still bothers me. > > > > I'm sure this is tangential, but can you please provide some > > context/links to the change you're referring to here. > > Here is the instigating issue and a discussion thread on the aftermath: > https://wiki.postgresql.org/wiki/A_Guide_to_CVE-2018-1058%3A_Protect_Your_Search_Path > https://www.postgresql.org/message-id/flat/13033.1531517020%40sss.pgh.pa.us#2aa2e25816d899d62f168926e3ff17b1 I don't blame you for feeling bothered about it. A benefit of having done it is that we gained insight into the level of pain it caused. If it had been sufficiently painful, someone would have quickly added an escape hatch. Five years later, nobody has added one. The 2018 security fixes instigated many function repairs that $SUBJECT would otherwise instigate. That wasn't too painful. The net new pain of $SUBJECT will be less, since the 2018 security fixes prepared the path. Hence, I remain +1 for the latest Davis proposal.
On Sat, Jul 15, 2023 at 02:13:33PM -0700, Noah Misch wrote: > The 2018 security fixes instigated many function repairs that $SUBJECT would > otherwise instigate. That wasn't too painful. The net new pain of $SUBJECT > will be less, since the 2018 security fixes prepared the path. Hence, I > remain +1 for the latest Davis proposal. I concur. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Mon, 2023-07-17 at 10:58 -0700, Nathan Bossart wrote:
> On Sat, Jul 15, 2023 at 02:13:33PM -0700, Noah Misch wrote:
> > The 2018 security fixes instigated many function repairs that
> > $SUBJECT would
> > otherwise instigate. That wasn't too painful. The net new pain of
> > $SUBJECT
> > will be less, since the 2018 security fixes prepared the path.
> > Hence, I
> > remain +1 for the latest Davis proposal.
>
> I concur.
Based on feedback, I plan to commit soon.
Tom's objection seemed specific to v16, and Robert's concern seemed to
be about having the MAINTAIN privilege without this patch. If I missed
any objections to this patch, please let me know.
If we hear about breakage that suggests we need an escape hatch GUC, we
have time to add one later.
I remain open to considering more complete fixes for the search_path
problems.
Regards,
Jeff Davis
On Mon, 2023-07-17 at 12:16 -0700, Jeff Davis wrote:
> Based on feedback, I plan to commit soon.
Attached is a new version.
Changes:
* Also switch the search_path during CREATE MATERIALIZED VIEW, so that
it's consistent with REFRESH. As a part of this change, I slightly
reordered things in ExecCreateTableAs() so that the skipData path
returns early without entering the SECURITY_RESTRICTED_OPERATION. I
don't think that's a problem because (a) that is one place where
SECURITY_RESTRICTED_OPERATION is not used for security, but rather for
consistency; and (b) that path doesn't go through rewriter, planner, or
executor anyway so I don't see why it would matter.
* Use GUC_ACTION_SAVE rather than GUC_ACTION_SET. That was a problem
with the previous patch for index functions executed in parallel
workers, which can happen calling SQL functions from pg_amcheck.
* I used a wrapper function RestrictSearchPath() rather than calling
set_config_option() directly. That provides a nice place in case we
need to add a compatibility GUC to disable it.
Question:
Why do we switch to the table owner and use
SECURITY_RESTRICTED_OPERATION in DefineIndex(), when we will switch in
index_build (etc.) anyway? Similarly, why do we switch in vacuum_rel(),
when it doesn't matter for lazy vacuum and we will switch in
cluster_rel() and do_analyze_rel() anyway?
For now, I left the extra calls to RestrictSearchPath() in for
consistency with the switches to the table owner.
Regards,
Jeff Davis
Вложения
On Fri, Jul 21, 2023 at 03:32:43PM -0700, Jeff Davis wrote: > Why do we switch to the table owner and use > SECURITY_RESTRICTED_OPERATION in DefineIndex(), when we will switch in > index_build (etc.) anyway? Commit a117ceb added that, and it added some test cases that behaved differently without that. > Similarly, why do we switch in vacuum_rel(), > when it doesn't matter for lazy vacuum and we will switch in > cluster_rel() and do_analyze_rel() anyway? It conforms to the "as soon as possible after locking the relation" coding rule that commit a117ceb wrote into miscinit.c. That provides future proofing.
On Sat, 2023-07-22 at 07:04 -0700, Noah Misch wrote:
> Commit a117ceb added that, and it added some test cases that behaved
> differently without that.
Thank you. The reasoning there seems to apply to search_path
restriction as well, so I will leave it as-is.
I'll wait a few more days for comment since I made some changes (also
it's the weekend), but I plan to commit something like the latest
version soon.
I might adjust the CREATE MATERIALIZED VIEW changes to be more minimal,
but that path is not important for security (see pre-existing comment)
so it's probably fine either way.
Regards,
Jeff Davis
On Fri, 2023-07-21 at 15:32 -0700, Jeff Davis wrote:
> Attached is a new version.
Do we still want to do this?
Right now, the MAINTAIN privilege is blocking on some way to prevent
malicious users from abusing the MAINTAIN privilege and search_path to
acquire the table owner's privileges.
The approach of locking down search_path during maintenance commands
would solve the problem, but it means that we are enforcing search_path
in some contexts and not others. That's not great, but it's similar to
what we are doing when we ignore SECURITY INVOKER and run the function
as the table owner during a maintenance command, or (by default) for
subscriptions.
My attempts to more generally try to lock down search_path for
functions attached to tables didn't seem to get much consensus, so if
we do make an exception to lock down search_path for maintenance
commands only, it would stay an exception for the foreseeable future.
Thoughts?
Regards,
Jeff Davis
On Fri, Oct 27, 2023 at 04:04:26PM -0700, Jeff Davis wrote: > Do we still want to do this? > > Right now, the MAINTAIN privilege is blocking on some way to prevent > malicious users from abusing the MAINTAIN privilege and search_path to > acquire the table owner's privileges. I vote +1 for proceeding with this. You've been threatening to commit this since July, and from a quick skim, I don't sense any sustained objections. Given one of the main objections for v16 was the timing, I would rather commit this relatively early in the v17 cycle so we have ample time to deal with any breakage it reveals or to further discuss any nuances. Of course, I am a bit biased because I would love to un-revert MAINTAIN, but I believe others would like to see that un-reverted, too. > The approach of locking down search_path during maintenance commands > would solve the problem, but it means that we are enforcing search_path > in some contexts and not others. That's not great, but it's similar to > what we are doing when we ignore SECURITY INVOKER and run the function > as the table owner during a maintenance command, or (by default) for > subscriptions. Given the experience gained from the 2018 security fixes [0], I think this is okay. [0] https://postgr.es/m/20230715211333.GB3675150%40rfd.leadboat.com -- Nathan Bossart Amazon Web Services: https://aws.amazon.com
On Fri, 27 Oct 2023 at 19:04, Jeff Davis <pgsql@j-davis.com> wrote:
The approach of locking down search_path during maintenance commands
would solve the problem, but it means that we are enforcing search_path
in some contexts and not others. That's not great, but it's similar to
what we are doing when we ignore SECURITY INVOKER and run the function
as the table owner during a maintenance command, or (by default) for
subscriptions.
I don't agree that this is ignoring SECURITY INVOKER. Instead, I see it as running the maintenance command as the owner of the table, which is therefore the invoker of the function. As far as I can tell we need to do this for security anyway - otherwise as soon as superuser runs a maintenance command (which it can already do), the owner of any function called in the course of the maintenance operation has an opportunity to get superuser.
For that matter, what would it even mean to ignore SECURITY INVOKER? Run the function as its owner if it were SECURITY DEFINER?
I understand what ignoring SECURITY DEFINER would mean: obviously, don't adjust the current user on entry and exit.
The privilege boundary should be at the point where the maintenance command starts: the role with MAINTAIN privilege gets to kick off maintenance, but doesn't get to specify anything after that, including the search_path (of course, function execution search paths should not normally depend on the caller's search path anyway, but that's a bigger discussion with an unfortunate backward compatibility problem).
Perhaps the search_path for running a maintenance command should be the search_path set for the table owner (ALTER ROLE … SET search_path …)? If none set, the default "$user", public. After that, change search_path on function invocation as usual rather than having special rules for what happens when a function is invoked during a maintenance command.
My attempts to more generally try to lock down search_path for
functions attached to tables didn't seem to get much consensus, so if
we do make an exception to lock down search_path for maintenance
commands only, it would stay an exception for the foreseeable future.
On Tue, 2023-10-31 at 13:16 -0400, Isaac Morland wrote:
> Perhaps the search_path for running a maintenance command should be
> the search_path set for the table owner (ALTER ROLE … SET search_path
> …)?
That's an interesting idea; I hadn't considered that, or at least not
very deeply. I feel like it came up before but I can't remember what
(if anything) was wrong with it.
If we expanded this idea a bit, I could imagine it applying to SECURITY
DEFINER functions as well, and that would make writing SECURITY DEFINER
functions a lot safer.
I was earlier pushing for search_path to be tied to the function (in my
"CREATE FUNCTION ... SEARCH" proposal) on the grounds that the author
(usually) doesn't want the behavior to depend on the caller's
search_path. That proposal didn't go very well because it required
extra DDL.
But if we tie the search_path to the user-switching behavior rather
than the function, that still protects the function author from many
sorts of search_path attacks, because it's either running as the
function author with the function author's search_path; or running as
the invoking user with their search_path. And there aren't a lot of
cases where a function author would want it to run with their
privileges and the caller's search_path.
[ It would still leave open the problem of a SECURITY INVOKER function
in an index expression returning inconsistent results due to a changing
search_path, which would compromise the index structure and any
constraints using that index. But that problem is more bounded, at
least. ]
> After that, change search_path on function invocation as usual
> rather than having special rules for what happens when a function is
> invoked during a maintenance command.
I don't follow what you mean here.
Regards,
Jeff Davis
On Thu, 2 Nov 2023 at 14:22, Jeff Davis <pgsql@j-davis.com> wrote:
On Tue, 2023-10-31 at 13:16 -0400, Isaac Morland wrote:
> Perhaps the search_path for running a maintenance command should be
> the search_path set for the table owner (ALTER ROLE … SET search_path
> …)?
That's an interesting idea; I hadn't considered that, or at least not
very deeply. I feel like it came up before but I can't remember what
(if anything) was wrong with it.
If we expanded this idea a bit, I could imagine it applying to SECURITY
DEFINER functions as well, and that would make writing SECURITY DEFINER
functions a lot safer.
I still think the right default is that CREATE FUNCTION stores the search_path in effect when it runs with the function, and that is the search_path used to run the function (and don't "BEGIN ATOMIC" functions partially work this way already?). But I suggest the owner search_path as an option which is clearly better than using the caller's search_path in most cases.
I think the problems are essentially the same for security invoker vs. security definer. The difference is that the problems are security problems only for security definers.
> After that, change search_path on function invocation as usual
> rather than having special rules for what happens when a function is
> invoked during a maintenance command.
I don't follow what you mean here.
I’m referring to the idea that the search_path during function execution should be determined at function creation time (or, at least, not at function execution time). While this is a security requirement for security definer functions, I think it’s what is wanted about 99.9% of the time for security invoker functions as well. So when the maintenance command ends up running a function, the search_path in effect during the function execution will be the one established at function definition time; or if we go with this "search_path associated with function owner" idea, then again the search_path is determined by the usual rule (function owner), rather than by any special rules associated with maintenance commands.
Isaac Morland <isaac.morland@gmail.com> writes:
> I still think the right default is that CREATE FUNCTION stores the
> search_path in effect when it runs with the function, and that is the
> search_path used to run the function (and don't "BEGIN ATOMIC" functions
> partially work this way already?).
I don't see how that would possibly fly. Yeah, that behavior is
often what you want, but not always; we would break some peoples'
applications with that rule.
Also, one place where it's clearly NOT what you want is while
restoring a pg_dump script. And we don't have any way that we could
bootstrap ourselves out of breaking everything for everybody during
their next upgrade --- even if you insist that people use a newer
pg_dump, where is it going to find the info in an existing database?
regards, tom lane
On Mon, 6 Nov 2023 at 15:54, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Isaac Morland <isaac.morland@gmail.com> writes:
> I still think the right default is that CREATE FUNCTION stores the
> search_path in effect when it runs with the function, and that is the
> search_path used to run the function (and don't "BEGIN ATOMIC" functions
> partially work this way already?).
I don't see how that would possibly fly. Yeah, that behavior is
often what you want, but not always; we would break some peoples'
applications with that rule.
The behaviour I want is just “SET search_path FROM CURRENT".
I agree there is a backward compatibility issue; if somebody has a schema creation/update script with function definitions with no "SET search_path" they would suddenly start getting the search_path from definition time rather than the caller's search_path.
I don't like adding GUCs but a single one specifying whether no search_path specification means "FROM CURRENT" or the current behaviour (new explicit syntax "FROM CALLER"?) would I think address the backward compatibility issue. This would allow a script to specify at the top which convention it is using; a typical old script could be adapted to a new database by adding a single line at the top to get the old behaviour.
Also, one place where it's clearly NOT what you want is while
restoring a pg_dump script. And we don't have any way that we could
bootstrap ourselves out of breaking everything for everybody during
their next upgrade --- even if you insist that people use a newer
pg_dump, where is it going to find the info in an existing database?
A function with a stored search_path will have a "SET search_path" clause in the pg_dump output, so for these functions pg_dump would be unaffected by my preferred way of doing things. Already I don't believe pg_dump ever puts "SET search_path FROM CURRENT" in its output; it puts the actual search_path. A bigger problem is with existing functions that use the caller's search_path; these would need to specify "FROM CALLER" explicitly; but the new GUC could come into this. In effect a pg_dump created by an old version is an old script which would need the appropriate setting at the top.
But all this is premature if there is still disagreement on the proper default behaviour. To me it is absolutely clear that the right default, in the absence of an installed base with backward compatibility concerns, is "SET search_path FROM CURRENT". This is how substantially all programming languages work: it is quite unusual in modern programming languages to have the meaning of a procedure definition depend on which modules the caller has imported. The tricky bit is dealing smoothly with the installed base. But some of the discussion here makes me think that people have a different attitude about stored procedures.
On Tue, 2023-10-31 at 13:16 -0400, Isaac Morland wrote:
> Perhaps the search_path for running a maintenance command should be
> the search_path set for the table owner (ALTER ROLE … SET search_path
> …)?
After some thought, I don't think that's the right approach. It adds
another way search path can be changed, which adds to the complexity.
Also, by default it's "$user", public; and given that "public" was
world-writable until recently, that doesn't seem like a good idea for a
change intended to prevent search_path manipulation.
Regards,
Jeff Davis
On Thu, Jan 18, 2024 at 9:19 AM Jeff Davis <pgsql@j-davis.com> wrote: > > On Mon, 2023-07-17 at 12:16 -0700, Jeff Davis wrote: > > Based on feedback, I plan to commit soon. > > Attached is a new version. > > Changes: > > * Also switch the search_path during CREATE MATERIALIZED VIEW, so that > it's consistent with REFRESH. As a part of this change, I slightly > reordered things in ExecCreateTableAs() so that the skipData path > returns early without entering the SECURITY_RESTRICTED_OPERATION. I > don't think that's a problem because (a) that is one place where > SECURITY_RESTRICTED_OPERATION is not used for security, but rather for > consistency; and (b) that path doesn't go through rewriter, planner, or > executor anyway so I don't see why it would matter. > > * Use GUC_ACTION_SAVE rather than GUC_ACTION_SET. That was a problem > with the previous patch for index functions executed in parallel > workers, which can happen calling SQL functions from pg_amcheck. > > * I used a wrapper function RestrictSearchPath() rather than calling > set_config_option() directly. That provides a nice place in case we > need to add a compatibility GUC to disable it. > > Question: > > Why do we switch to the table owner and use > SECURITY_RESTRICTED_OPERATION in DefineIndex(), when we will switch in > index_build (etc.) anyway? Similarly, why do we switch in vacuum_rel(), > when it doesn't matter for lazy vacuum and we will switch in > cluster_rel() and do_analyze_rel() anyway? I tried to apply the patch but it is failing at the Head. It is giving the following error: Hunk #7 succeeded at 3772 (offset -12 lines). patching file src/backend/commands/matview.c patching file src/backend/commands/vacuum.c Hunk #2 succeeded at 2169 (offset -19 lines). patching file src/backend/utils/init/usercontext.c patching file src/bin/scripts/t/100_vacuumdb.pl Hunk #1 FAILED at 109. 1 out of 1 hunk FAILED -- saving rejects to file src/bin/scripts/t/100_vacuumdb.pl.rej patching file src/include/utils/usercontext.h patching file src/test/modules/test_oat_hooks/expected/test_oat_hooks.out patching file src/test/regress/expected/matview.out patching file src/test/regress/expected/privileges.out patching file src/test/regress/expected/vacuum.out patching file src/test/regress/sql/matview.sql patching file src/test/regress/sql/privileges.sql patching file src/test/regress/sql/vacuum.sql Please send the Re-base version of the patch. Thanks and Regards, Shubham Khanna.
On Thu, 2024-01-18 at 09:24 +0530, Shubham Khanna wrote:
> I tried to apply the patch but it is failing at the Head. It is
> giving
> the following error:
I am withdrawing this patch from the CF until it's more clear that this
is what we really want to do.
Thank you for looking into it.
Regards,
Jeff Davis