Обсуждение: Addressing SECURITY DEFINER Function Vulnerabilities in PostgreSQL Extensions

On Wed, 2024-06-05 at 14:36 +0530, Ashutosh Sharma wrote:
> Thank you, Ashutosh, for the quick response. I've drafted a patch
> aimed at addressing this issue. The patch attempts to solve this
> issue by configuring the search_path for all security definer
> functions created by the extension.

I like the general direction you propose, but I think it needs more
discussion about the details.

* What exactly is the right search_path for a function defined in an
extension?

* Do we need a new magic search_path value of "$extension_schema" that
resolves to the extension's schema, so that it can handle ALTER
EXTENSION ... SET SCHEMA?

* What do we do for functions that want the current behavior and how do
we handle migration issues?

* What about SECURITY INVOKER functions? Those can still be vulnerable
to manipulation by the caller by setting search_path, which can cause
an incorrect value to be returned. That can matter in some contexts
like a CHECK constraint.

Regards,
    Jeff Davis

On Thu, 2024-06-06 at 21:17 +0530, Ashutosh Sharma wrote:
> That can be controlled via some GUC if needed, I guess.

That's a possibility, but it's easy to create a mess that way. I don't
necessarily oppose it, but we'd need some pretty strong agreement that
we are somehow moving users in a better direction and not just creating
two behaviors that last forever.

I also think there should be a way to explicitly request the old
behavior -- obtaining search_path from the session -- regardless of how
the GUC is set.

> I didn't get you completely here. w.r.t extensions how will this have
> an impact if we set the search_path for definer functions. 

If we only set the search path for SECURITY DEFINER functions, I don't
think that solves the whole problem.

Regards,
    Jeff Davis

On Thu, 6 Jun 2024 at 20:10, Isaac Morland <isaac.morland@gmail.com> wrote:
>
> On Thu, 6 Jun 2024 at 12:53, Jeff Davis <pgsql@j-davis.com> wrote:
>
>>
>> > I didn't get you completely here. w.r.t extensions how will this have
>> > an impact if we set the search_path for definer functions.
>>
>> If we only set the search path for SECURITY DEFINER functions, I don't
>> think that solves the whole problem.
>
>
> Indeed. While the ability for a caller to set the search_path for a security definer functions introduces security
problemsthat are different than for security invoker functions, it's still weird for the behaviour of a function to
dependon the caller's search_path. It’s even weirder for the default search path behaviour to be different depending on
whetheror not the function is security definer. 

+1

And +1 to the general idea and direction this thread is going in. I
definitely think we should be making extensions more secure by
default, and this is an important piece of it.

Even by default making the search_path "pg_catalog, pg_temp" for
functions created by extensions would be very useful.

On Fri, 2024-06-07 at 00:19 +0200, Jelte Fennema-Nio wrote:
> Even by default making the search_path "pg_catalog, pg_temp" for
> functions created by extensions would be very useful.

Right now there's no syntax to override that. We'd need something to
say "get the search_path from the session".

Regards,
    Jeff Davis

On Thu, Jun 6, 2024 at 2:36 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Wed, 2024-06-05 at 14:36 +0530, Ashutosh Sharma wrote:
> > Thank you, Ashutosh, for the quick response. I've drafted a patch
> > aimed at addressing this issue. The patch attempts to solve this
> > issue by configuring the search_path for all security definer
> > functions created by the extension.
>
> I like the general direction you propose, but I think it needs more
> discussion about the details.
>
> * What exactly is the right search_path for a function defined in an
> extension?
>
> * Do we need a new magic search_path value of "$extension_schema" that
> resolves to the extension's schema, so that it can handle ALTER
> EXTENSION ... SET SCHEMA?
>
> * What do we do for functions that want the current behavior and how do
> we handle migration issues?
>
> * What about SECURITY INVOKER functions? Those can still be vulnerable
> to manipulation by the caller by setting search_path, which can cause
> an incorrect value to be returned. That can matter in some contexts
> like a CHECK constraint.
>

Attached is the new version of patch addressing aforementioned
comments. It implements the following changes:

1) Extends the CREATE EXTENSION command to support a new option, SET
SEARCH_PATH.
2) If the SET SEARCH_PATH option is specified with the CREATE
EXTENSION command, the implicit search_path for functions created by
an extension is set, if not already configured. This is true for both
SECURITY DEFINER and SECURITY INVOKER functions.
3) When the ALTER EXTENSION SET SCHEMA command is executed and if the
function's search_path contains the old schema of the extension, it is
updated with the new schema.

Please have a look and let me know your comments.

--
With Regards,
Ashutosh Sharma.

On Tue, 11 Jun 2024 at 11:54, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> 1) Extends the CREATE EXTENSION command to support a new option, SET
> SEARCH_PATH.


I don't think it makes sense to add such an option to CREATE EXTENSION.
I feel like such a thing should be part of the extension control file
instead. That way the extension author controls the search path, not
the person that installs the extension.

Hi,

On Tue, Jun 11, 2024 at 5:02 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> On Tue, 11 Jun 2024 at 11:54, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> > 1) Extends the CREATE EXTENSION command to support a new option, SET
> > SEARCH_PATH.
>
>
> I don't think it makes sense to add such an option to CREATE EXTENSION.
> I feel like such a thing should be part of the extension control file
> instead. That way the extension author controls the search path, not
> the person that installs the extension.

If the author has configured the search_path for any desired function,
using this option with the CREATE EXTENSION command will not affect
those functions.

--
With Regards,
Ashutosh Sharma.

Hi Alexander,

On Tue, Jun 11, 2024 at 6:26 PM Alexander Kukushkin <cyberdemn@gmail.com> wrote:
>
> Hi,
>
> On Tue, 11 Jun 2024 at 14:50, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>>
>> If the author has configured the search_path for any desired function,
>> using this option with the CREATE EXTENSION command will not affect
>> those functions.
>
>
> Then effectively this feature is useless.
> Now attackers can just set search_path for the current session.
> With this feature they will also be able to influence search_path of not protected functions when they create an
extension.
>

Apologies for any confusion, but I'm not entirely following your
explanation. Could you kindly provide further clarification?
Additionally, would you mind reviewing the problem description
outlined in the initial email?

--
With Regards,
Ashutosh Sharma.

On Tue, 2024-06-11 at 14:56 +0200, Alexander Kukushkin wrote:
> Now attackers can just set search_path for the current session.

IIUC, the proposal is that only the function's "SET" clause can
override the behavior, not a top-level SET command.

Regards,
    Jeff Davis

On Tue, 2024-06-11 at 15:24 +0530, Ashutosh Sharma wrote:
> 3) When the ALTER EXTENSION SET SCHEMA command is executed and if the
> function's search_path contains the old schema of the extension, it
> is
> updated with the new schema.

I don't think it's reasonable to search-and-replace within a function's
SET clause at ALTER time.

I believe we need a new special search_path item, like
"$extension_schema", to mean the schema of the extension owning the
function. It would, like "$user", automatically adjust to the current
value when changed.

That sounds like a useful and non-controversial change.

Regards,
    Jeff Davis

Hi Jeff,

On Wed, Jun 12, 2024 at 3:07 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Tue, 2024-06-11 at 15:24 +0530, Ashutosh Sharma wrote:
> > 3) When the ALTER EXTENSION SET SCHEMA command is executed and if the
> > function's search_path contains the old schema of the extension, it
> > is
> > updated with the new schema.
>
> I don't think it's reasonable to search-and-replace within a function's
> SET clause at ALTER time.
>
> I believe we need a new special search_path item, like
> "$extension_schema", to mean the schema of the extension owning the
> function. It would, like "$user", automatically adjust to the current
> value when changed.
>
> That sounds like a useful and non-controversial change.

I've definitely thought about it, and I agree that this approach could
help simplify things. However, we do have some challenges with the
resolution of $extension_schema variable compared to $user. Firstly,
we need something like CurrentExtensionId to resolve
$extension_schema, and we need to decide where to manage it
(CurrentExtensionId). From what I understand, we should set
CurrentExtensionId when we're getting ready to execute a function, as
that's when we recompute the namespace path. But to set
CurrentExtensionId at this point, we need information in pg_proc to
distinguish between extension-specific and non-extension functions. If
it's an extension specific function, it should have the extension OID
in pg_proc, which we can use to find the extension's namespace and
eventually resolve $extension_schema. So, to summarize this at a high
level, here's is what I think can be done:

1) Include extension-specific details, possibly the extension OID, for
functions in pg_proc during function creation.

2) Check if a function is extension-specific while preparing for
function execution and set CurrentExtensionId accordingly.

3) Utilize CurrentExtensionId to resolve the namespace path during
recomputation.

4) Above steps will automatically repeat if the function is nested.

5) Upon completion of function execution, reset CurrentExtensionId to
InvalidOid.

I think this should effectively tackle the outlined challenges with
resolution of $extension_schema during namespace path recomputation.
What are your thoughts on it?

Thanks,

--
With Regards,
Ashutosh Sharma.

On Wed, Jun 12, 2024 at 9:35 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> Hi Jeff,
>
> On Wed, Jun 12, 2024 at 3:07 AM Jeff Davis <pgsql@j-davis.com> wrote:
> >
> > On Tue, 2024-06-11 at 15:24 +0530, Ashutosh Sharma wrote:
> > > 3) When the ALTER EXTENSION SET SCHEMA command is executed and if the
> > > function's search_path contains the old schema of the extension, it
> > > is
> > > updated with the new schema.
> >
> > I don't think it's reasonable to search-and-replace within a function's
> > SET clause at ALTER time.
> >
> > I believe we need a new special search_path item, like
> > "$extension_schema", to mean the schema of the extension owning the
> > function. It would, like "$user", automatically adjust to the current
> > value when changed.
> >
> > That sounds like a useful and non-controversial change.
>
> I've definitely thought about it, and I agree that this approach could
> help simplify things. However, we do have some challenges with the
> resolution of $extension_schema variable compared to $user. Firstly,
> we need something like CurrentExtensionId to resolve
> $extension_schema, and we need to decide where to manage it
> (CurrentExtensionId). From what I understand, we should set
> CurrentExtensionId when we're getting ready to execute a function, as
> that's when we recompute the namespace path. But to set
> CurrentExtensionId at this point, we need information in pg_proc to
> distinguish between extension-specific and non-extension functions. If
> it's an extension specific function, it should have the extension OID
> in pg_proc, which we can use to find the extension's namespace and
> eventually resolve $extension_schema. So, to summarize this at a high
> level, here's is what I think can be done:
>
> 1) Include extension-specific details, possibly the extension OID, for
> functions in pg_proc during function creation.
>

Alternatively, we could leverage the extension dependency information
to determine whether the function is created by an extension or not.

> 2) Check if a function is extension-specific while preparing for
> function execution and set CurrentExtensionId accordingly.
>
> 3) Utilize CurrentExtensionId to resolve the namespace path during
> recomputation.
>
> 4) Above steps will automatically repeat if the function is nested.
>
> 5) Upon completion of function execution, reset CurrentExtensionId to
> InvalidOid.
>
> I think this should effectively tackle the outlined challenges with
> resolution of $extension_schema during namespace path recomputation.
> What are your thoughts on it?
>
> Thanks,
>
> --
> With Regards,
> Ashutosh Sharma.

Hi,

> I know about the problem and have seen the original email.

I'm sympathetic to the problem of potential privilege escalation when
executing functions. At the same time I'm not sure if there's that
much of a difference between 'CREATE EXTENSION' vs  superusers copying
a series of 'CREATE FUNCTION' where they don't understand these same
nuances.

CREATE FUNCTION already provides an ability to set the search_path. So
I'm wondering what the problem we want to solve here is. Is it that
there's too much friction for extension authors to have to set
search_path as part of the function definition and we want to make it
easier for them to "set once and forget"?


> But, I also agree with Jelte, it should be a property of a control file, rather than a user controlled parameter, so
thatan attacker can't opt out.
 

+1. Also curious what happens if an extension author has search_path
already set in proconfig for a function that doesn't match what's in
the control file. I'm guessing the function one should take
precedence.


--
John Hsu - Amazon Web Services

On Wed, 2024-06-12 at 12:13 +0530, Ashutosh Bapat wrote:
> > Alternatively, we could leverage the extension dependency
> > information
> > to determine whether the function is created by an extension or
> > not.
>
> That will be simpler. We do that sort of thing for identity
> sequences. So there's a precedent to do that kind of stuff. 

I did not look at the details, but +1 for using information we already
have. There's a little bit of extra work to resolve it, but thanks to
the search_path cache it should only need to be done once per unique
search_path setting per session.

Regards,
    Jeff Davis

On Wed, Jun 12, 2024 at 2:05 PM John H <johnhyvr@gmail.com> wrote:>
> I'm sympathetic to the problem of potential privilege escalation when
> executing functions. At the same time I'm not sure if there's that
> much of a difference between 'CREATE EXTENSION' vs  superusers copying
> a series of 'CREATE FUNCTION' where they don't understand these same
> nuances.

+1.

> CREATE FUNCTION already provides an ability to set the search_path. So
> I'm wondering what the problem we want to solve here is. Is it that
> there's too much friction for extension authors to have to set
> search_path as part of the function definition and we want to make it
> easier for them to "set once and forget"?

If that's the problem we want to solve, I'm unconvinced that it's
worth doing anything. But I think there's another problem, which is
that if the extension is relocatable, how do you set a secure
search_path? You could say SET search_path = foo, pg_catalog if you
know the extension will be installed in schema foo, but if you don't
know in what schema the extension will be installed, then what are you
supposed to do? The proposal of litting $extension_schema could help
with that ...

...except I'm not sure that's really a full solution either, because
what if the extension is installed into a schema that's writable by
others, like public? If $extension_schema resolves to public and
public allows CREATE access to normal users, you're in a lot of
trouble again already, because now an attacker can try to capture
operator references within your function, or function calls that are
approximate matches to some existing function by introducing perfect
matches that take priority. Perhaps we want to consider the case where
the attacker can write to the schema containing the extension as an
unsupported scenario, but if so, we'd better be explicit about that.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, 2024-06-12 at 15:36 -0400, Robert Haas wrote:
> But I think there's another problem, which is
> that if the extension is relocatable, how do you set a secure
> search_path? You could say SET search_path = foo, pg_catalog if you
> know the extension will be installed in schema foo, but if you don't
> know in what schema the extension will be installed, then what are
> you
> supposed to do? The proposal of litting $extension_schema could help
> with that ...
>
> ...except I'm not sure that's really a full solution either, because
> what if the extension is installed into a schema that's writable by
> others, like public?

Jelte proposed something to fix that here:

https://www.postgresql.org/message-id/CAGECzQQzDqDzakBkR71ZkQ1N1ffTjAaruRSqppQAKu3WF%2B6rNQ%40mail.gmail.com


Regards,
    Jeff Davis

Hi,

On Wed, Jun 12, 2024 at 11:35 PM John H <johnhyvr@gmail.com> wrote:
>
> > But, I also agree with Jelte, it should be a property of a control file, rather than a user controlled parameter,
sothat an attacker can't opt out. 
>

This will be addressed in the next patch version.

> +1. Also curious what happens if an extension author has search_path
> already set in proconfig for a function that doesn't match what's in
> the control file. I'm guessing the function one should take
> precedence.
>

Yes, if the author has explicitly set the proconfig, it will take precedence.

--
With Regards,
Ashutosh Sharma.

Hi,

Please find the attached patch addressing all the comments. I kindly
request your review and feedback. Your thoughts are greatly
appreciated.

--
With Regards,
Ashutosh Sharma.

Hi Ashutosh,

Thinking about this more, could you clarify the problem/issue at hand?
I think it's still not clear to me.
Yes, CREATE EXTENSION can create functions that lead to unexpected
privilege escalation, regardless
 if they are SECURITY DEFINER or SECURITY INVOKER (if the function is
inadvertently executed by superuser).
But that's also true for a general CREATE FUNCTION call outside of extensions.

If I read the commit message I see:

> This flag controls PostgreSQL's behavior in setting the implicit
> search_path within the proconfig for functions created by an extension
> that does not have the search_path explicitly set in proconfig

If that's the "general" issue you're trying to solve, I would wonder
why we we wouldn't for instance be extending
the functionality to normal CREATE FUNCTION calls (ie, schema qualify
based off search_path)

Thanks,
John H

On Thu, Jun 13, 2024 at 1:09 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> Hi,
>
> Please find the attached patch addressing all the comments. I kindly
> request your review and feedback. Your thoughts are greatly
> appreciated.
>
> --
> With Regards,
> Ashutosh Sharma.



--
John Hsu - Amazon Web Services

Hi John,

On Tue, Jun 18, 2024 at 2:35 AM John H <johnhyvr@gmail.com> wrote:
>
> Hi Ashutosh,
>
> Thinking about this more, could you clarify the problem/issue at hand?
> I think it's still not clear to me.
> Yes, CREATE EXTENSION can create functions that lead to unexpected
> privilege escalation, regardless
>  if they are SECURITY DEFINER or SECURITY INVOKER (if the function is
> inadvertently executed by superuser).
> But that's also true for a general CREATE FUNCTION call outside of extensions.
>

This specifically applies to extension functions, not standalone
functions created independently. The difference is that installing
extensions typically requires superuser privileges, which is not the
case with standalone functions.

--
With Regards,
Ashutosh Sharma.

Hi Ashutosh,

Thanks for clarifying.

> not standalone functions created independently

I'm wondering why we wouldn't want to extend that functionality so
that if users (including superusers) do want to automatically
configure search_path
when they're creating functions it's available.

>  The difference is that installing extensions typically requires superuser privileges,
> which is not the case with standalone functions.

Yes, but inadvertent access to different roles can still occur even if
it's not a superuser.

It's not clear to me who the audience of this commit is aimed at,
cause I see two sort of
different use cases?

1. Make it easier for extension authors to configure search_path when
creating functions

The proposed mechanism via control file makes sense, although I would
like to understand
why specifying it today in CREATE FUNCTION doesn't work. Is it an
awareness issue?
I suppose it's easier if you only need to set it once in the control file?
Is that ease-of-use what we want to solve?

2. Make it easier to avoid inadvertent escalations when functions are created
(e.g CREATE EXTENSION/CREATE FUNCTION)

Then I think it's better to provide users a way to force the
search_path on functions when
extensions are created so superusers aren't reliant on extension authors.

Thanks,

--
John Hsu - Amazon Web Services

Hi,

On Wed, Jun 19, 2024 at 6:06 AM John H <johnhyvr@gmail.com> wrote:
>
> Hi Ashutosh,
>
> Thanks for clarifying.
>
> > not standalone functions created independently
>
> I'm wondering why we wouldn't want to extend that functionality so
> that if users (including superusers) do want to automatically
> configure search_path
> when they're creating functions it's available.
>
> >  The difference is that installing extensions typically requires superuser privileges,
> > which is not the case with standalone functions.
>
> Yes, but inadvertent access to different roles can still occur even if
> it's not a superuser.
>
> It's not clear to me who the audience of this commit is aimed at,
> cause I see two sort of
> different use cases?
>
> 1. Make it easier for extension authors to configure search_path when
> creating functions
>
> The proposed mechanism via control file makes sense, although I would
> like to understand
> why specifying it today in CREATE FUNCTION doesn't work. Is it an
> awareness issue?
> I suppose it's easier if you only need to set it once in the control file?
> Is that ease-of-use what we want to solve?
>
> 2. Make it easier to avoid inadvertent escalations when functions are created
> (e.g CREATE EXTENSION/CREATE FUNCTION)
>
> Then I think it's better to provide users a way to force the
> search_path on functions when
> extensions are created so superusers aren't reliant on extension authors.
>

For standalone functions, users can easily adjust the search_path
settings as needed. However, managing this becomes challenging for
functions created via extensions. Extensions are relocatable, making
it difficult to determine and apply search_path settings in advance
within the extension_name--*.sql file when defining functions or
procedures. Introducing a new control flag option allows users to set
implicit search_path settings for functions created by the extension,
if needed. The main aim here is to enhance security for functions
created by extensions by setting search_path at the function level.
This ensures precise control over how objects are accessed within each
function, making behavior more predictable and minimizing security
risks, especially for SECURITY DEFINER functions associated with
extensions created by superusers.

--
With Regards,
Ashutosh Sharma.

On Wed, 2024-06-19 at 08:53 +0530, Ashutosh Sharma wrote:
> For standalone functions, users can easily adjust the search_path
> settings as needed. However, managing this becomes challenging for
> functions created via extensions. Extensions are relocatable, making
> it difficult to determine and apply search_path settings in advance
> within the extension_name--*.sql file when defining functions or
> procedures.

A special search_path variable "$extension_schema" would be a better
solution to this problem. We need something like that anyway, in case
the extension is relocated, so that we don't have to dig through the
catalog and update all the settings.

>  Introducing a new control flag option allows users to set
> implicit search_path settings for functions created by the extension,
> if needed. The main aim here is to enhance security for functions
> created by extensions by setting search_path at the function level.
> This ensures precise control over how objects are accessed within
> each
> function, making behavior more predictable and minimizing security
> risks,

That leaves me wondering whether it's being addressed at the right
level.

For instance, did you consider just having a GUC to control the default
search_path for a function? That may be too magical, but if so, why
would an extension-only setting be better?

> especially for SECURITY DEFINER functions associated with
> extensions created by superusers.

I'm not sure that it's specific to SECURITY DEFINER functions.

Regards,
    Jeff Davis

On Tue, Jun 25, 2024 at 12:40 AM Jeff Davis <pgsql@j-davis.com> wrote:
>
> On Wed, 2024-06-19 at 08:53 +0530, Ashutosh Sharma wrote:
> > For standalone functions, users can easily adjust the search_path
> > settings as needed. However, managing this becomes challenging for
> > functions created via extensions. Extensions are relocatable, making
> > it difficult to determine and apply search_path settings in advance
> > within the extension_name--*.sql file when defining functions or
> > procedures.
>
> A special search_path variable "$extension_schema" would be a better
> solution to this problem. We need something like that anyway, in case
> the extension is relocated, so that we don't have to dig through the
> catalog and update all the settings.
>
> >  Introducing a new control flag option allows users to set
> > implicit search_path settings for functions created by the extension,
> > if needed. The main aim here is to enhance security for functions
> > created by extensions by setting search_path at the function level.
> > This ensures precise control over how objects are accessed within
> > each
> > function, making behavior more predictable and minimizing security
> > risks,
>
> That leaves me wondering whether it's being addressed at the right
> level.
>
> For instance, did you consider just having a GUC to control the default
> search_path for a function? That may be too magical, but if so, why
> would an extension-only setting be better?
>

I haven't given any such thought, my current focus is on extensions,
specifically increasing their security with respect to superuser
elevation.

Regarding your first question which you had raised earlier : "What
exactly is the right search_path for a function defined in an
extension?"

As I understand it, we cannot precisely determine the search_path for
functions at the time of function creation in the code. The most
accurate path (or something close to it) we can identify for functions
created by extensions is the search_path set by the extension during
its creation, which is what we aim to achieve with $extension_schema.
This setting is up to the discretion of the author as to whether they
are comfortable with this implicit search_path configuration at the
function level. If not, they have the option to disable it. However,
AFAIU, in most cases, especially when the extension can be relocated,
this situation is unlikely to occur.

--
With Regards,
Ashutosh Sharma,