Re: Fix search_path for all maintenance commands
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
Re: Fix search_path for all maintenance commands
On Thu, Jan 18, 2024 at 9:19 AM Jeff Davis 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.
Re: Fix search_path for all maintenance commands
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
Re: Fix search_path for all maintenance commands
On Mon, 6 Nov 2023 at 15:54, Tom Lane wrote: > Isaac Morland 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.
Re: Fix search_path for all maintenance commands
Isaac Morland 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
Re: Fix search_path for all maintenance commands
On Thu, 2 Nov 2023 at 14:22, Jeff Davis 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.
Re: Fix search_path for all maintenance commands
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
Re: Fix search_path for all maintenance commands
On Fri, 27 Oct 2023 at 19:04, Jeff Davis 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. >
Re: Fix search_path for all maintenance commands
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
Re: Fix search_path for all maintenance commands
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
Re: Fix search_path for all maintenance commands
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
Re: Fix search_path for all maintenance commands
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.
Re: Fix search_path for all maintenance commands
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 From a2a2468dc9577498d75aaff5ea83726af0fd4d5d Mon Sep 17 00:00:00 2001 From: Jeff Davis Date: Thu, 6 Jul 2023 13:06:22 -0700 Subject: [PATCH v1] Restrict search_path when performing maintenance. When executing maintenance operations (ANALYZE, CLUSTER, CREATE/REFRESH MATERIALIZED VIEW, REINDEX, or VACUUM), set search_path to 'pg_catalog, pg_temp'. This change avoids problems when maintenance commands are executed with a different search_path, which could lead to errors or inconsistent results. It's also a step towards offering the MAINTAIN privilege safely. Functions that are used for functional indexes, partial indexes, or in materialized views that depend on a different search path should be declared with CREATE FUNCTION ... SET search_path='...'. A previous version of this change was committed as 05e1737351, but was not acceptable for 16 and reverted. Discussion: https://postgr.es/m/CA%2BTgmoZVCHERUkXhAMT2Er-sKBc5C6_iX%2BTpxxivBevDHzq2TQ%40mail.gmail.com Discussion: https://postgr.es/m/e44327179e5c9015c8dda67351c04da552066017.camel%40j-davis.com --- contrib/amcheck/verify_nbtree.c | 2 + .../sgml/ref/create_materialized_view.sgml| 6 +- .../sgml/ref/refresh_materialized_view.sgml | 19 ++- src/backend/access/brin/brin.c| 2 + src/backend/catalog/index.c | 5 + src/backend/commands/analyze.c| 2 + src/backend/commands/cluster.c| 2 + src/backend/commands/createas.c | 136 ++ src/backend/commands/indexcmds.c | 7 + src/backend/commands/matview.c| 2 + src/backend/commands/vacuum.c | 2 + src/backend/utils/init/usercontext.c | 14 ++ src/bin/scripts/t/100_vacuumdb.pl | 4 - src/include/utils/usercontext.h | 1 + .../expected/test_oat_hooks.out | 4 + src/test/regress/expected/matview.out | 40 +- src/test/regress/expected/privileges.out | 12 +- src/test/regress/expected/vacuum.out | 2 +- src/test/regress/sql/matview.sql | 23 +++ src/test/regress/sql/privileges.sql | 8 +- src/test/regress/sql/vacuum.sql | 2 +- 21 files changed, 212 insertions(+), 83 deletions(-) diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c index 94a9759322..bc43808d35 100644 --- a/contrib/amcheck/verify_nbtree.c +++ b/contrib/amcheck/verify_nbtree.c @@ -40,6 +40,7 @@ #include "utils/guc.h" #include "utils/memutils.h" #include "utils/snapmgr.h" +#include "utils/usercontext.h" PG_MODULE_MAGIC; @@ -281,6 +282,7 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed, SetUserIdAndSecContext(heaprel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + RestrictSearchPath(); } else { diff --git a/doc/src/sgml/ref/create_materialized_view.sgml b/doc/src/sgml/ref/create_materialized_view.sgml index 0d2fea2b97..06e3d80f70 100644 --- a/doc/src/sgml/ref/create_materialized_view.sgml +++ b/doc/src/sgml/ref/create_materialized_view.sgml @@ -37,8 +37,10 @@ CREATE MATERIALIZED VIEW [ IF NOT EXISTS ] table_name CREATE MATERIALIZED VIEW defines a materialized view of a query. The query is executed and used to populate the view at the time - the command is issued (unless WITH NO DATA is used) and may be - refreshed
Re: Fix search_path for all maintenance commands
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
Re: Fix search_path for all maintenance commands
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
Re: Fix search_path for all maintenance commands
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 wrote: > > On Thu, Jul 13, 2023 at 1:37 PM David G. Johnston > > 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.
Re: Fix search_path for all maintenance commands
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.1688086...@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
Re: Fix search_path for all maintenance commands
On Thu, Jul 13, 2023 at 2:00 PM Gurjeet Singh wrote: > On Thu, Jul 13, 2023 at 1:37 PM David G. Johnston > 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 David J.
Re: Fix search_path for all maintenance commands
On Thu, Jul 13, 2023 at 1:37 PM David G. Johnston 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
Re: Fix search_path for all maintenance commands
On Thu, Jul 13, 2023 at 12:54 PM Gurjeet Singh 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.
Re: Fix search_path for all maintenance commands
On Thu, Jul 13, 2023 at 11:56 AM Jeff Davis 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
Re: Fix search_path for all maintenance commands
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
Re: Fix search_path for all maintenance commands
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
Re: Fix search_path for all maintenance commands
On Thu, 6 Jul 2023 at 21:39, Jeff Davis 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.
Re: Fix search_path for all maintenance commands
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 From 46dbbf86e927e1872004c37f651e6a5b5c62bdd0 Mon Sep 17 00:00:00 2001 From: Jeff Davis Date: Thu, 6 Jul 2023 13:06:22 -0700 Subject: [PATCH v5] Restrict search_path when performing maintenance. When executing maintenance operations (ANALYZE, CLUSTER, REFRESH MATERIALIZED VIEW, REINDEX, or VACUUM), set search_path to 'pg_catalog, pg_temp'. Functions that are used for functional indexes, in index expressions, or in materialized views and depend on a different search path should be declared with CREATE FUNCTION ... SET search_path='...'. This change was previously committed as 05e1737351, but was not acceptable for 16 and reverted. Discussion: https://postgr.es/m/CA%2BTgmoZVCHERUkXhAMT2Er-sKBc5C6_iX%2BTpxxivBevDHzq2TQ%40mail.gmail.com Discussion: https://postgr.es/m/e44327179e5c9015c8dda67351c04da552066017.camel%40j-davis.com --- contrib/amcheck/verify_nbtree.c | 2 ++ src/backend/access/brin/brin.c | 2 ++ src/backend/catalog/index.c | 8 src/backend/commands/analyze.c | 2 ++ src/backend/commands/cluster.c | 2 ++ src/backend/commands/indexcmds.c | 6 ++ src/backend/commands/matview.c | 2 ++ src/backend/commands/vacuum.c| 2 ++ src/bin/scripts/t/100_vacuumdb.pl| 4 src/include/utils/guc.h | 6 ++ .../test_oat_hooks/expected/test_oat_hooks.out | 4 src/test/regress/expected/privileges.out | 12 ++-- src/test/regress/expected/vacuum.out | 2 +- src/test/regress/sql/privileges.sql | 8 src/test/regress/sql/vacuum.sql | 2 +- 15 files changed, 48 insertions(+), 16 deletions(-) diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c index 94a9759322..35035967f9 100644 --- a/contrib/amcheck/verify_nbtree.c +++ b/contrib/amcheck/verify_nbtree.c @@ -281,6 +281,8 @@ bt_index_check_internal(Oid indrelid, bool parentcheck, bool heapallindexed, SetUserIdAndSecContext(heaprel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); } else { diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 3c6a956eaa..11e78cb62c 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -1066,6 +1066,8 @@ brin_summarize_range(PG_FUNCTION_ARGS) SetUserIdAndSecContext(heapRel->rd_rel->relowner, save_sec_context | SECURITY_RESTRICTED_OPERATION); save_nestlevel = NewGUCNestLevel(); + SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH, PGC_USERSET, + PGC_S_SESSION); } else { diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 67b743e251..64127a894f 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -1476,6 +1476,8 @@ index_concurrently_build(Oid heapRelationId,
Re: Fix search_path for all maintenance commands
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
Re: Fix search_path for all maintenance commands
On Fri, Jun 9, 2023 at 2:00 PM Jeff Davis 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
Re: Fix search_path for all maintenance commands
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
Re: Fix search_path for all maintenance commands
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
Re: Fix search_path for all maintenance commands
On Fri, 26 May 2023 at 19:22, 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, 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