Thread: Per-function search_path => per-function GUC settings
I believe we had consensus that 8.3 needs to include an easier way for a function to set a local value of search_path, as proposed here: http://archives.postgresql.org/pgsql-hackers/2007-03/msg01717.php I've been holding off actually implementing that because adding a column to pg_proc would create merge problems for pending patches. But tsearch was the last one with any major changes to pg_proc.h, so it's probably time to get on with it. A few days ago, Simon suggested that we should generalize this notion to allow per-function settings of any GUC variable: http://archives.postgresql.org/pgsql-hackers/2007-08/msg01155.php My reaction to that was more or less "D'oh, of course!" Stuff like regex_flavor can easily break a function. So rather than thinking only about search_path, it seems to me we should implement a facility that allows function-local settings of any USERSET GUC variable, and probably also SUSET ones if the function is SECURITY DEFINER and owned by a superuser. The most straightforward way to support this syntactically seems to be to follow the per-user and per-database GUC setting features: ALTER FUNCTION func(args) SET var = valueALTER FUNCTION func(args) RESET var The RESET alternative is a lot cleaner than my previous suggestion of "PATH NONE" to remove a non-default path setting, anyway. I thought about ways to include GUC settings directly into CREATE FUNCTION, but it seemed pretty ugly and inconsistent with the existing syntax. So I'm thinking of supporting only the above syntaxes, meaning it'll take at least two commands to create a secure SECURITY DEFINER function. Comments? regards, tom lane
On 9/2/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I thought about ways to include GUC settings directly into CREATE > FUNCTION, but it seemed pretty ugly and inconsistent with the > existing syntax. So I'm thinking of supporting only the above > syntaxes, meaning it'll take at least two commands to create a secure > SECURITY DEFINER function. There's a niceness to being able to tell Postgres everything it needs to know about a function in the one CREATE FUNCTION command. So if we integrated the GUC settings into CREATE FUNCTION, we'd end up writing something like CREATE FUNCTION foo(int) RETURNS int AS $$ ... $$LANGUAGE plpgsqlSTABLESTRICTSECURITY DEFINERRESET search_pathSET regex_flavor = 'cinnamon'; That doesn't seem especially horrible. In what way do you feel it is inconsistent with existing syntax? And ... although I'll admit this is a paranoid thing to mention, if you have to fix the search_path setting *after* creating a function as SECURITY DEFINER, then there is necessarily a short period of time where the function exists and is insecure. Cheers, BJ
"Tom Lane" <tgl@sss.pgh.pa.us> writes: > I thought about ways to include GUC settings directly into CREATE > FUNCTION, but it seemed pretty ugly and inconsistent with the > existing syntax. So I'm thinking of supporting only the above > syntaxes, meaning it'll take at least two commands to create a secure > SECURITY DEFINER function. I think security definer functions should automatically inherit their search_path. The whole "secure by default" thing. It might be best to have a guc variable which controls the variables which are automatically saved. regexp_flavour and maybe a handful of others could be in it by default. But that might depend on how expensive it is at run-time. I wouldn't want trivial SQL functions to no longer be inline-able because one might one day use a regexp for example. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
"Brendan Jurd" <direvus@gmail.com> writes: > CREATE FUNCTION foo(int) RETURNS int AS $$ > ... > $$ > LANGUAGE plpgsql > STABLE > STRICT > SECURITY DEFINER > RESET search_path > SET regex_flavor = 'cinnamon'; > That doesn't seem especially horrible. In what way do you feel it is > inconsistent with existing syntax? Hmm ... I hadn't thought of including SET in the syntax, so I was running into problems with distingushing GUC variable names from the keywords that are already in the syntax. That way would work from a grammar point of view. It still seems a bit inconsistent to me, but we could live with it. Comments anyone? > And ... although I'll admit this is a paranoid thing to mention, if > you have to fix the search_path setting *after* creating a function as > SECURITY DEFINER, then there is necessarily a short period of time > where the function exists and is insecure. You already have that issue with respect to the default public execute permissions on the function. The standard solution is to do it in a transaction block --- then no one can even see the function until you commit. regards, tom lane
Gregory Stark <stark@enterprisedb.com> writes: > I think security definer functions should automatically inherit their > search_path. The whole "secure by default" thing. This assumes that the search path at creation time has something to do with the path you'd like to use at execution, which is unlikely to be the case in existing pg_dump output, to name one example. I don't really want to get into doing the above. > It might be best to have a guc variable which controls the variables which are > automatically saved. regexp_flavour and maybe a handful of others could be in > it by default. But that might depend on how expensive it is at run-time. I > wouldn't want trivial SQL functions to no longer be inline-able because one > might one day use a regexp for example. Well, security definer functions can't be inlined anyway, so as long as you only try to do this for sec-def functions it wouldn't be an issue. I just think it's too big a departure from existing behavior, and will probably break more things than it fixes. regards, tom lane
"Tom Lane" <tgl@sss.pgh.pa.us> writes: > Gregory Stark <stark@enterprisedb.com> writes: >> I think security definer functions should automatically inherit their >> search_path. The whole "secure by default" thing. > > This assumes that the search path at creation time has something to do > with the path you'd like to use at execution, which is unlikely to be > the case in existing pg_dump output, to name one example. I don't > really want to get into doing the above. pg_dump will have to do a ALTER FUNCTION SET command anyways, no? So the default search_path that gets saved doesn't really matter. In general if it's not the search path you want at run-time you just have to change it, but you should always have *something* set or else it's a wide open security hole. I'm not clear why the search path at creation time is such a bad choice anyways, it is security "definer", what's the difference between taking the userid from the defining environment and taking the search path from the defining environment? -- Gregory Stark EnterpriseDB http://www.enterprisedb.com
On Sat, 2007-09-01 at 12:41 -0400, Tom Lane wrote: > A few days ago, Simon suggested that we should generalize this notion > to allow per-function settings of any GUC variable: > http://archives.postgresql.org/pgsql-hackers/2007-08/msg01155.php > My reaction to that was more or less "D'oh, of course!" Stuff like > regex_flavor can easily break a function. So rather than thinking > only about search_path, it seems to me we should implement a facility > that allows function-local settings of any USERSET GUC variable, and > probably also SUSET ones if the function is SECURITY DEFINER and owned by > a superuser. > > The most straightforward way to support this syntactically seems to > be to follow the per-user and per-database GUC setting features: > > ALTER FUNCTION func(args) SET var = value > I was hoping this feature would make it easier for modules to install into any schema. Right now a module can either preprocess a SQL install script to install it into the schema you want, or hard-code the schema into the file and you're stuck with whatever schema the module authors chose (usually either the module name, or "public"). Can we also provide syntax which would be equivalent to setting "var" for the function to be whatever the current value happens to be when the ALTER FUNCTION is run? Possible syntax might be something like: ALTER FUNCTION func(args) SET var TO CURRENT; > I thought about ways to include GUC settings directly into CREATE > FUNCTION, but it seemed pretty ugly and inconsistent with the > existing syntax. So I'm thinking of supporting only the above > syntaxes, meaning it'll take at least two commands to create a secure > SECURITY DEFINER function. That seems a little awkward, because to avoid a security race condition, you'd have to wrap the CREATE/ALTER in a transaction block. However, we already have a similar situation with creating a security definer function and then revoking access, so maybe it's already expected. I don't have a strong opinion, I just wanted to bring that up. Regards,Jeff Davis
Jeff Davis <pgsql@j-davis.com> writes: > Can we also provide syntax which would be equivalent to setting "var" > for the function to be whatever the current value happens to be when the > ALTER FUNCTION is run? Possible syntax might be something like: > ALTER FUNCTION func(args) SET var TO CURRENT; Hmmm ... that's certainly do-able, though I'm not sure how much it helps the use-case you suggest. The search path still has to be set at the top of the module script, no? However, I like an explicit option of this sort a lot better than the automatic version Greg was suggesting ... I'm willing to do it if people want it. One problem is that we'd have to make CURRENT a reserved word to make it work exactly like that. Can anyone think of a variant syntax that doesn't need a new reserved word? regards, tom lane
Gregory Stark <stark@enterprisedb.com> writes: > "Tom Lane" <tgl@sss.pgh.pa.us> writes: >> This assumes that the search path at creation time has something to do >> with the path you'd like to use at execution, which is unlikely to be >> the case in existing pg_dump output, to name one example. I don't >> really want to get into doing the above. > pg_dump will have to do a ALTER FUNCTION SET command anyways, no? You're missing the point: this change will break existing pg_dump output, because pg_dump feels free to set the search_path for its own purposes. The same is true of other GUC variables that might be automatically absorbed into CREATE FUNCTION: there is not any very good reason to suppose that their values when the dump is restored are really what should be used. The argument is slightly more credible with respect to interactively-issued commands, but for pg_dump it's simply wrong. What we might change pg_dump to do in future is a separate topic, but we can't make that kind of change in the semantics of existing dumps. regards, tom lane
On 9/1/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Jeff Davis <pgsql@j-davis.com> writes: > > Can we also provide syntax which would be equivalent to setting "var" > > for the function to be whatever the current value happens to be when the > > ALTER FUNCTION is run? Possible syntax might be something like: > > > ALTER FUNCTION func(args) SET var TO CURRENT; > > Hmmm ... that's certainly do-able, though I'm not sure how much it helps > the use-case you suggest. The search path still has to be set at the > top of the module script, no? > > However, I like an explicit option of this sort a lot better than the > automatic version Greg was suggesting ... I'm willing to do it if people > want it. > > One problem is that we'd have to make CURRENT a reserved word to make it > work exactly like that. Can anyone think of a variant syntax that > doesn't need a new reserved word? > > regards, tom lane > > ---------------------------(end of broadcast)--------------------------- > TIP 2: Don't 'kill -9' the postmaster > +1 for being able to define the entire function in one command, +1 for not inheriting a bunch of GUC settings without the definer's explicit say-so. - Josh/Eggyknap
On Sep 1, 2007, at 1:36 PM, Brendan Jurd wrote: > So if we integrated the GUC settings into CREATE FUNCTION, we'd end up > writing something like > > CREATE FUNCTION foo(int) RETURNS int AS $$ > ... > $$ > LANGUAGE plpgsql > STABLE > STRICT > SECURITY DEFINER > RESET search_path > SET regex_flavor = 'cinnamon'; > > That doesn't seem especially horrible. In what way do you feel it is > inconsistent with existing syntax? I like this and would really like to see way to set everything using CREATE FUNCTION. Using ALTER only requires maintaining a separate copy of of function arguments which can be a hassle for large argument lists. John DeSoi, Ph.D. http://pgedit.com/ Power Tools for PostgreSQL
On Sat, Sep 01, 2007 at 03:03:14PM -0400, Tom Lane wrote: > Jeff Davis <pgsql@j-davis.com> writes: > > Can we also provide syntax which would be equivalent to setting "var" > > for the function to be whatever the current value happens to be when the > > ALTER FUNCTION is run? Possible syntax might be something like: > > > ALTER FUNCTION func(args) SET var TO CURRENT; > > Hmmm ... that's certainly do-able, though I'm not sure how much it helps > the use-case you suggest. The search path still has to be set at the > top of the module script, no? Better one place than scores of places... > However, I like an explicit option of this sort a lot better than the > automatic version Greg was suggesting ... I'm willing to do it if people > want it. > > One problem is that we'd have to make CURRENT a reserved word to make it > work exactly like that. Can anyone think of a variant syntax that > doesn't need a new reserved word? I'm not good at the synonym game, but I did have an idea related to the SET syntax, so I'll just post it here... If you needed to set a bunch of GUCs on a function, having a load of SET statements might be a bit tedious... I'm wondering if there's some way to specify them like an array? SET {'search_path=CURRENT', 'enable_seqscan=false', ...} Or now that we have arrays of complex types, perhaps an array of type GUC... Either case would at least take care of CURRENT... -- Decibel!, aka Jim Nasby decibel@decibel.org EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)
On 9/1/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Jeff Davis <pgsql@j-davis.com> writes: > > Can we also provide syntax which would be equivalent to setting "var" > > for the function to be whatever the current value happens to be when the > > ALTER FUNCTION is run? Possible syntax might be something like: > > > ALTER FUNCTION func(args) SET var TO CURRENT; > > Hmmm ... that's certainly do-able, though I'm not sure how much it helps > the use-case you suggest. The search path still has to be set at the > top of the module script, no? > > However, I like an explicit option of this sort a lot better than the > automatic version Greg was suggesting ... I'm willing to do it if people > want it. > > One problem is that we'd have to make CURRENT a reserved word to make it > work exactly like that. Can anyone think of a variant syntax that > doesn't need a new reserved word? SET var FROM CURRENT SESSION -- marko
On Sat, Sep 01, 2007 at 12:41:28PM -0400, Tom Lane wrote: > I believe we had consensus that 8.3 needs to include an easier way for a > function to set a local value of search_path, as proposed here: > http://archives.postgresql.org/pgsql-hackers/2007-03/msg01717.php > I've been holding off actually implementing that because adding a column > to pg_proc would create merge problems for pending patches. But tsearch > was the last one with any major changes to pg_proc.h, so it's probably > time to get on with it. > > A few days ago, Simon suggested that we should generalize this notion > to allow per-function settings of any GUC variable: > http://archives.postgresql.org/pgsql-hackers/2007-08/msg01155.php > My reaction to that was more or less "D'oh, of course!" Stuff like > regex_flavor can easily break a function. So rather than thinking > only about search_path, it seems to me we should implement a facility > that allows function-local settings of any USERSET GUC variable, and > probably also SUSET ones if the function is SECURITY DEFINER and owned by > a superuser. > > The most straightforward way to support this syntactically seems to > be to follow the per-user and per-database GUC setting features: > > ALTER FUNCTION func(args) SET var = value Would it be hard to extend this into this? ALTER FUNCTION func(args) SET var = value [, var = value ...] > ALTER FUNCTION func(args) RESET var > > The RESET alternative is a lot cleaner than my previous suggestion > of "PATH NONE" to remove a non-default path setting, anyway. > > I thought about ways to include GUC settings directly into CREATE > FUNCTION, but it seemed pretty ugly and inconsistent with the > existing syntax. So I'm thinking of supporting only the above > syntaxes, meaning it'll take at least two commands to create a > secure SECURITY DEFINER function. With the extended syntax I proposed it could take just one command to create such a function :) Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ phone: +1 415 235 3778 AIM: dfetter666 Skype: davidfetter Remember to vote! Consider donating to PostgreSQL: http://www.postgresql.org/about/donate
David Fetter <david@fetter.org> writes: > On Sat, Sep 01, 2007 at 12:41:28PM -0400, Tom Lane wrote: >> The most straightforward way to support this syntactically seems to >> be to follow the per-user and per-database GUC setting features: >> >> ALTER FUNCTION func(args) SET var = value > Would it be hard to extend this into this? > ALTER FUNCTION func(args) SET var = value [, var = value ...] Actually, it would be hard *not* to, because of the way the CREATE/ALTER FUNCTION syntax is already set up --- but without the commas. I've got it working now with SET and RESET as alternatives for common_func_opt_item. regards, tom lane
"Marko Kreen" <markokr@gmail.com> writes: > On 9/1/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> One problem is that we'd have to make CURRENT a reserved word to make it >> work exactly like that. Can anyone think of a variant syntax that >> doesn't need a new reserved word? > SET var FROM CURRENT SESSION Seems a little verbose, but maybe we could do "SET var FROM CURRENT" or "SET var FROM SESSION"? One point worth noting here is that this'd more or less automatically apply to ALTER USER SET and ALTER DATABASE SET as well ... not sure how much use-case there is for those, but it'd fall out ... regards, tom lane
On 9/2/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Marko Kreen" <markokr@gmail.com> writes: > > On 9/1/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> One problem is that we'd have to make CURRENT a reserved word to make it > >> work exactly like that. Can anyone think of a variant syntax that > >> doesn't need a new reserved word? > > > SET var FROM CURRENT SESSION > > Seems a little verbose, but maybe we could do "SET var FROM CURRENT" > or "SET var FROM SESSION"? I'd prefer FROM SESSION then. FROM CURRENT seems unclear. > One point worth noting here is that this'd more or less automatically > apply to ALTER USER SET and ALTER DATABASE SET as well ... not sure > how much use-case there is for those, but it'd fall out ... Does not seem to be a problem. -- marko
"Marko Kreen" <markokr@gmail.com> writes: > On 9/2/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Seems a little verbose, but maybe we could do "SET var FROM CURRENT" >> or "SET var FROM SESSION"? > I'd prefer FROM SESSION then. FROM CURRENT seems unclear. Actually, I think FROM SESSION is unclear, as it opens the question whether the value to be applied is the session-wide setting or the currently active one. Inside a transaction that has done SET LOCAL, these are different things. I think we pretty clearly want to have it take the currently active setting, and I'd vote for FROM CURRENT as the best way of expressing that. regards, tom lane
On Sun, 2007-09-02 at 12:11 -0400, Tom Lane wrote: > I think we pretty clearly want to have it take the currently active > setting, and I'd vote for FROM CURRENT as the best way of expressing > that. FROM CURRENT sounds good to me. Another idea (just brainstorming): SET var AS CURRENT. Regards,Jeff Davis
On Sat, 2007-09-01 at 15:03 -0400, Tom Lane wrote: > > ALTER FUNCTION func(args) SET var TO CURRENT; > > Hmmm ... that's certainly do-able, though I'm not sure how much it helps > the use-case you suggest. The search path still has to be set at the > top of the module script, no? It gives some better options for module authors and people installing those modules: 1. Set it at the top of the file, in one place 2. Connect as the user whose schema you want to install into, i.e.: $ psql my_db jdavis < module_install.sql 3. prepend the "SET search_path=foo" to the input of psql, i.e.: $ echo "SET search_path=foo;" | cat module_install.sql | psql my_db ..or $ psql my_db => SET search_path=foo; => \i module_install.sql Regards,Jeff Davis
On Sat, 2007-09-01 at 13:55 -0400, Tom Lane wrote: > You already have that issue with respect to the default public execute > permissions on the function. The standard solution is to do it in a > transaction block --- then no one can even see the function until you > commit. It might be a good idea to have the ability to revoke privileges at CREATE FUNCTION time also. That could clutter up the CREATE FUNCTION syntax, but would offer an opportunity to document the danger of default public execute in a SECURITY DEFINER function. Something like: CREATE FUNCTION ... LANGUAGE plpgsql SECURITY DEFINER REVOKE EXECUTE FROM PUBLIC; Even if we don't do that, we should at least document your standard solution here: http://www.postgresql.org/docs/8.2/static/sql-createfunction.html It is already documented here: http://www.postgresql.org/docs/8.2/static/sql-grant.html But the CREATE FUNCTION page has a section titled "Writing SECURITY DEFINER Functions Safely", so I think it's useful to have it there, too. Regards,Jeff Davis
On 9/2/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > "Marko Kreen" <markokr@gmail.com> writes: > > On 9/2/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> Seems a little verbose, but maybe we could do "SET var FROM CURRENT" > >> or "SET var FROM SESSION"? > > > I'd prefer FROM SESSION then. FROM CURRENT seems unclear. > > Actually, I think FROM SESSION is unclear, as it opens the question > whether the value to be applied is the session-wide setting or the > currently active one. Inside a transaction that has done SET LOCAL, > these are different things. I did not consider that. > I think we pretty clearly want to have it take the currently active > setting, and I'd vote for FROM CURRENT as the best way of expressing > that. Ok. -- marko
Tom Lane wrote: > > I thought about ways to include GUC settings directly into CREATE > FUNCTION, but it seemed pretty ugly and inconsistent with the > existing syntax. So I'm thinking of supporting only the above > syntaxes, meaning it'll take at least two commands to create a secure > SECURITY DEFINER function. > > Comments? I have a question about what does happen if search path is not defined for SECURITY DEFINER function. My expectation is that SECURITY DEFINER function should defined empty search patch in this case. This behavior is similar to how dynamic linker processes setuid binaries - (ignoring LD_LIBRARY_PATH and so on). Zdenek
Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes: > I have a question about what does happen if search path is not defined > for SECURITY DEFINER function. My expectation is that SECURITY DEFINER > function should defined empty search patch in this case. Your expectation is incorrect. We are not in the business of breaking every application in sight, which is what that would do. Nor do I think it's a good plan to try to be smarter than the programmer. regards, tom lane
Tom Lane wrote: > Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes: >> I have a question about what does happen if search path is not defined >> for SECURITY DEFINER function. My expectation is that SECURITY DEFINER >> function should defined empty search patch in this case. > > Your expectation is incorrect. We are not in the business of breaking > every application in sight, which is what that would do. Oh. I see. In this point of view I suggest to add some warning about potential security issue if SECURITY DEFINER function will create without preset search_path. I'm aware that a lot of developer forget to modify their application. Zdenek
Am Dienstag, 11. September 2007 15:53 schrieb Tom Lane: > Zdenek Kotala <Zdenek.Kotala@Sun.COM> writes: > > I have a question about what does happen if search path is not defined > > for SECURITY DEFINER function. My expectation is that SECURITY DEFINER > > function should defined empty search patch in this case. > > Your expectation is incorrect. We are not in the business of breaking > every application in sight, which is what that would do. Well, a SECURITY DEFINER function either sets its own search path, in which case a default search path would have no effect, or it doesn't set its own search path, in which case it's already broken (albeit in a different way). So setting a default search path can only be a net gain. -- Peter Eisentraut http://developer.postgresql.org/~petere/
Peter Eisentraut <peter_e@gmx.net> writes: > Well, a SECURITY DEFINER function either sets its own search path, in which > case a default search path would have no effect, or it doesn't set its own > search path, in which case it's already broken (albeit in a different way). > So setting a default search path can only be a net gain. It would break functions that actually want to use a caller-specified search path, and protect themselves by explicitly schema-qualifying every other reference than one to some caller-specified object. Which admittedly is notationally a pain in the neck, but it's possible to do. I do not think that we should foreclose potentially useful behavior *and* make a major break in backward compatibility in order to make a very small improvement in security. regards, tom lane
On 9/12/07, Tom Lane <tgl@sss.pgh.pa.us> wrote: > It would break functions that actually want to use a caller-specified > search path, and protect themselves by explicitly schema-qualifying > every other reference than one to some caller-specified object. Which > admittedly is notationally a pain in the neck, but it's possible to do. > I do not think that we should foreclose potentially useful behavior > *and* make a major break in backward compatibility in order to make > a very small improvement in security. In that case, is there anything wrong with Zdenek's suggestion to add a warning on SECURITY DEFINER functions that do not set a search_path? Something to the tune of WARNING: "Your function is defined with SECURITY DEFINER but does not specify a local search path. This is potentially a serious security vulnerability." HINT: "Use the SET clause in CREATE FUNCTION to set a safe search path which is specific to your function."