Addressing SECURITY DEFINER Function Vulnerabilities in PostgreSQL Extensions

On Fri, May 24, 2024 at 12:52 PM Ashutosh Sharma <ashu.coek88@gmail.com>
wrote:

Hi All,

We all know that installing an extension typically requires superuser
privileges, which means the database objects it creates are owned by the
superuser.

If the extension creates any SECURITY DEFINER functions, it can introduce
security vulnerabilities. For example, consider an extension that creates
the following functions, outer_func and inner_func, in the schema s1 when
installed:

CREATE OR REPLACE FUNCTION s1.inner_func(data text)
RETURNS void AS $$
BEGIN
INSERT INTO tab1(data_column) VALUES (data);
END;
$$ LANGUAGE plpgsql;

CREATE OR REPLACE FUNCTION s1.outer_func(data text)
RETURNS void AS $$
BEGIN
PERFORM inner_func(data);
END;
$$ LANGUAGE plpgsql SECURITY DEFINER;

If a regular user creates another function with name "inner_func" with the
same signature in the public schema and sets the search path to public, s1,
the function created by the regular user in the public schema takes
precedence when outer_func is called. Since outer_func is marked as
SECURITY DEFINER, the inner_func created by the user in the public schema
is executed with superuser privileges. This allows the execution of any
statements within the function block, leading to potential security issues.

To address this problem, one potential solution is to adjust the function
resolution logic. For instance, if the caller function belongs to a
specific schema, functions in the same schema should be given preference.
Although I haven’t reviewed the feasibility in the code, this is one
possible approach.

Another solution could be to categorize extension-created functions to
avoid duplication. This might not be an ideal solution, but it's another
consideration worth sharing.

Function call should schema qualify it. That's painful, but it can be
avoided by setting a search path from inside the function. There was some
discussion about setting a search path for a function at [1]/messages/by-id/2710f56add351a1ed553efb677408e51b060e67c.camel@j-davis.com. But the last
message there is non-conclusive. We may want to extend it to extensions
such that all the object references in a given extension are resolved using
extension specific search_path.

[1]: /messages/by-id/2710f56add351a1ed553efb677408e51b060e67c.camel@j-davis.com
/messages/by-id/2710f56add351a1ed553efb677408e51b060e67c.camel@j-davis.com

--
Best Wishes,
Ashutosh Bapat

On Fri, May 24, 2024 at 2:25 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
wrote:

On Fri, May 24, 2024 at 12:52 PM Ashutosh Sharma <ashu.coek88@gmail.com>
wrote:

Hi All,

We all know that installing an extension typically requires superuser
privileges, which means the database objects it creates are owned by the
superuser.

If the extension creates any SECURITY DEFINER functions, it can introduce
security vulnerabilities. For example, consider an extension that creates
the following functions, outer_func and inner_func, in the schema s1 when
installed:

CREATE OR REPLACE FUNCTION s1.inner_func(data text)
RETURNS void AS $$
BEGIN
INSERT INTO tab1(data_column) VALUES (data);
END;
$$ LANGUAGE plpgsql;

CREATE OR REPLACE FUNCTION s1.outer_func(data text)
RETURNS void AS $$
BEGIN
PERFORM inner_func(data);
END;
$$ LANGUAGE plpgsql SECURITY DEFINER;

If a regular user creates another function with name "inner_func" with
the same signature in the public schema and sets the search path to public,
s1, the function created by the regular user in the public schema takes
precedence when outer_func is called. Since outer_func is marked as
SECURITY DEFINER, the inner_func created by the user in the public schema
is executed with superuser privileges. This allows the execution of any
statements within the function block, leading to potential security issues.

To address this problem, one potential solution is to adjust the function
resolution logic. For instance, if the caller function belongs to a
specific schema, functions in the same schema should be given preference.
Although I haven’t reviewed the feasibility in the code, this is one
possible approach.

Another solution could be to categorize extension-created functions to
avoid duplication. This might not be an ideal solution, but it's another
consideration worth sharing.

Function call should schema qualify it. That's painful, but it can be
avoided by setting a search path from inside the function. There was some
discussion about setting a search path for a function at [1]. But the last
message there is non-conclusive. We may want to extend it to extensions
such that all the object references in a given extension are resolved using
extension specific search_path.

[1]
/messages/by-id/2710f56add351a1ed553efb677408e51b060e67c.camel@j-davis.com

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. It ensures they are set to trusted schemas, which includes
the schema where the extension and the function are created. PFA patch.

--
With Regards,
Ashutosh Sharma.

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

Hi,

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.

I agree.

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

Determining the precise value can be challenging. However, since it's a
function installed by an extension, typically, the search_path should
include the extension's search_path and the schema where the function
resides. If the function relies on a schema other than the one we set in
its search_path, which would usually be the one created by the extension,
this approach will enforce the extension developers to set the extension's
specific search_path in the create function statement, if it's not set. The
primary goal here is to ensure that the security definer functions created
by an extension do not refer to any untrusted schema(s).

* 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?

Possibly yes, we can think about it, I think it would be something like the
$user we have now.

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

That can be controlled via some GUC if needed, I guess.

* 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.

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.

--
With Regards,
Ashutosh Sharma.

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 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 problems that are different
than for security invoker functions, it's still weird for the behaviour of
a function to depend on the caller's search_path. It’s even weirder for the
default search path behaviour to be different depending on whether or not
the function is security definer.

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 problems that are different than for security invoker functions, it's still weird for the behaviour of a function to depend on the caller's search_path. It’s even weirder for the default search path behaviour to be different depending on whether or 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,

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.

Regards,
--
Alexander Kukushkin

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.

On Wed, Jun 12, 2024 at 9:51 AM Ashutosh Sharma <ashu.coek88@gmail.com>
wrote:

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.

That will be simpler. We do that sort of thing for identity sequences. So
there's a precedent to do that kind of stuff.
--
Best Wishes,
Ashutosh Bapat

Hi Ashutosh,

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?

I know about the problem and have seen the original email.
What confused me, is that your email didn't specify that SET SEARCH_PATH in
the CREATE EXTENSION is a boolean flag, hence I made an assumption that it
is a TEXT (similar to GUC with the same name). Now after looking at your
code it makes more sense. Sorry about the confusion.

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

Regards,
--
Alexander Kukushkin