Thread: Fixes for missing schema qualifications

Hi all,

In light of CVE-2018-1058, user's applications need to be careful about
the use of schema-unqualified queries.  A lookup at the upstream code is
showing four areas which are missing such handling:
- psql has one problem in get_create_object_cmd which misses twice to
qualify array_remove().
- isolationtester is missing one for a call to pg_backend_pid()
- information_schema.sql has one problem as well: the function
_pg_interval_type does not qualify upper().  Please note that there is
no need to care about view's bodies because those use OID references, so
only the function body need to be taken care of.
- worker_spi scans pg_namespace and uses count() without schema
qualification.

Attached is a patch which fixes all four of them, and which should be
back-patched.  For information_schema.sql, users can always replace the
body of the function by redefining them (using SET search_path in CREATE
FUNCTION would work as well however this is more costly than a simple
qualification).

Thoughts?
--
Michael

On 3/9/18 2:55 AM, Michael Paquier wrote:
> 
> In light of CVE-2018-1058, user's applications need to be careful about
> the use of schema-unqualified queries.  A lookup at the upstream code is
> showing four areas which are missing such handling:
> - psql has one problem in get_create_object_cmd which misses twice to
> qualify array_remove().
> - isolationtester is missing one for a call to pg_backend_pid()
> - information_schema.sql has one problem as well: the function
> _pg_interval_type does not qualify upper().  Please note that there is
> no need to care about view's bodies because those use OID references, so
> only the function body need to be taken care of.
> - worker_spi scans pg_namespace and uses count() without schema
> qualification.
> 
> Attached is a patch which fixes all four of them, and which should be
> back-patched.  For information_schema.sql, users can always replace the
> body of the function by redefining them (using SET search_path in CREATE
> FUNCTION would work as well however this is more costly than a simple
> qualification).

These look sane to me.  Did you check the back branches for anything
that might not exist in HEAD?

Regards,
-- 
-David
david@pgmasters.net

On Fri, Mar 09, 2018 at 09:35:22AM -0500, David Steele wrote:
> These look sane to me.  Did you check the back branches for anything
> that might not exist in HEAD?

I did, but I have not spotted anything extra.  Impossible to say that I
did not miss one though, such scanning is tiring.
--
Michael

On Fri, Mar 09, 2018 at 04:55:38PM +0900, Michael Paquier wrote:
> --- a/src/backend/catalog/information_schema.sql
> +++ b/src/backend/catalog/information_schema.sql
> @@ -186,7 +186,7 @@ CREATE FUNCTION _pg_interval_type(typid oid, mod int4) RETURNS text
>      AS
>  $$SELECT
>    CASE WHEN $1 IN (1186) /* interval */
> -           THEN upper(substring(format_type($1, $2) from 'interval[()0-9]* #"%#"' for '#'))
> +           THEN pg_catalog.upper(substring(pg_catalog.format_type($1, $2) from 'interval[()0-9]* #"%#"' for '#'))
>         ELSE null
>    END$$;
>  
> diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
> index 3560318749..f345572c8c 100644
> --- a/src/bin/psql/command.c
> +++ b/src/bin/psql/command.c
> @@ -4483,7 +4483,7 @@ get_create_object_cmd(EditableObjectType obj_type, Oid oid,
>                  printfPQExpBuffer(query,
>                                    "SELECT nspname, relname, relkind, "
>                                    "pg_catalog.pg_get_viewdef(c.oid, true), "
> -
"array_remove(array_remove(c.reloptions,'check_option=local'),'check_option=cascaded')AS reloptions, "
 
> +
"pg_catalog.array_remove(pg_catalog.array_remove(c.reloptions,'check_option=local'),'check_option=cascaded')AS
reloptions,"
 
>                                    "CASE WHEN 'check_option=local' = ANY (c.reloptions) THEN 'LOCAL'::text "
>                                    "WHEN 'check_option=cascaded' = ANY (c.reloptions) THEN 'CASCADED'::text ELSE NULL
ENDAS checkoption "
 
>                                    "FROM pg_catalog.pg_class c "
> diff --git a/src/test/isolation/isolationtester.c b/src/test/isolation/isolationtester.c
> index 4ecad038bd..64d666f5cd 100644
> --- a/src/test/isolation/isolationtester.c
> +++ b/src/test/isolation/isolationtester.c
> @@ -184,7 +184,7 @@ main(int argc, char **argv)
>          PQclear(res);
>  
>          /* Get the backend pid for lock wait checking. */
> -        res = PQexec(conns[i], "SELECT pg_backend_pid()");
> +        res = PQexec(conns[i], "SELECT pg_catalog.pg_backend_pid()");
>          if (PQresultStatus(res) == PGRES_TUPLES_OK)
>          {
>              if (PQntuples(res) == 1 && PQnfields(res) == 1)
> diff --git a/src/test/modules/worker_spi/worker_spi.c b/src/test/modules/worker_spi/worker_spi.c
> index 3b98b1682b..547bdb26c4 100644
> --- a/src/test/modules/worker_spi/worker_spi.c
> +++ b/src/test/modules/worker_spi/worker_spi.c
> @@ -115,7 +115,9 @@ initialize_worker_spi(worktable *table)
>  
>      /* XXX could we use CREATE SCHEMA IF NOT EXISTS? */
>      initStringInfo(&buf);
> -    appendStringInfo(&buf, "select count(*) from pg_namespace where nspname = '%s'",
> +    appendStringInfo(&buf,
> +                     "select pg_catalog.count(*) "
> +                     "from pg_catalog.pg_namespace where nspname = '%s'",

This qualifies some functions, but it leaves plenty of unqualified operators.

Noah Misch wrote:
> On Fri, Mar 09, 2018 at 04:55:38PM +0900, Michael Paquier wrote:
> > --- a/src/backend/catalog/information_schema.sql
> > +++ b/src/backend/catalog/information_schema.sql
> > @@ -186,7 +186,7 @@ CREATE FUNCTION _pg_interval_type(typid oid, mod int4) RETURNS text
> >      AS
> >  $$SELECT
> >    CASE WHEN $1 IN (1186) /* interval */
> > -           THEN upper(substring(format_type($1, $2) from 'interval[()0-9]* #"%#"' for '#'))
> > +           THEN pg_catalog.upper(substring(pg_catalog.format_type($1, $2) from 'interval[()0-9]* #"%#"' for '#'))
> >         ELSE null
> >    END$$;

> This qualifies some functions, but it leaves plenty of unqualified operators.

... and substring() ...

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Sat, Mar 10, 2018 at 03:13:09PM -0300, Alvaro Herrera wrote:
> ... and substring() ...

substring(A from B for C) gets parsed.
--
Michael

>> +                     "select pg_catalog.count(*) "
>> +                     "from pg_catalog.pg_namespace where nspname = '%s'",
> 
> This qualifies some functions, but it leaves plenty of unqualified operators.

So this should go something like this?

select pg_catalog.count(*) from pg_catalog.pg_namespace where nameeq(nspname, '%s');

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

>>> +                     "select pg_catalog.count(*) "
>>> +                     "from pg_catalog.pg_namespace where nspname = '%s'",
>> 
>> This qualifies some functions, but it leaves plenty of unqualified operators.
> 
> So this should go something like this?
> 
> select pg_catalog.count(*) from pg_catalog.pg_namespace where nameeq(nspname, '%s');

Oops. I meant:

select pg_catalog.count(*) from pg_catalog.pg_namespace where pg_catalog.nameeq(nspname, '%s');

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

>> select pg_catalog.count(*) from pg_catalog.pg_namespace where
>> pg_catalog.nameeq(nspname, '%s');
>>
>>
> ​I'd rather write that:
> 
> select [...] where nspname operator(pg_catalog.=) '%s'​
> 
> Introducing undocumented implementation functions to these queries is
> undesirable; and besides, indexing and equivalence relies on operators and
> not the underlying functions so there would be some risk of performance
> issues if the functions were used directly.

Thanks. Yours looks much better.

Next question is, should we update the manual? There are bunch of
places where example queries are shown without schema qualifications.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

On Sat, Mar 10, 2018 at 08:36:34AM +0000, Noah Misch wrote:
> This qualifies some functions, but it leaves plenty of unqualified operators.

Yeah, I know that, and i don't have a perfect reply to offer to you.
There are a couple of methods that we could use to tackle that:
1) For functions, enforce search_path with a SET search_path =
'pg_catalog' command.  However this has a performance impact.
2) Enforce operators qualification with operator(pg_catalog.foo).  This
has no impact on performance, but repeating that all over the place is
rather ugly, particularly for psql's describe.c and tab-completion.c.
3) Tweak dynamically search_path before running a query:
- Save the existing search_path value by issuing SHOW search_path.
- Use ALWAYS_SECURE_SEARCH_PATH_SQL to enforce the path.
- Set back search_path based on the previous value.
This logic can happen in a dedicated wrapper, but this impacts
performance as it requires extra round trips to the server.

For information_schema.sql, we are talking about tweaking 12 functions.
So I think that we could live with 2).  To simplify user's life, we
could also recommend just to users to issue a ALTER FUNCTION SET
search_path to fix the problem for all functions, that's easier to
digest.

For the rest, which basically concerns psql, I have been thinking that
actually using 2) would be the most painful approach, still something
which does not impact the user experience, while 3) is easier to
back-patch by minimizing the code footprint and avoids also any kind of
future problems.

Thoughts?
--
Michael

On Tue, Mar 13, 2018 at 05:19:50PM -0700, David G. Johnston wrote:
> On Tue, Mar 13, 2018 at 5:11 PM, Tatsuo Ishii <ishii@sraoss.co.jp> wrote:
>> select pg_catalog.count(*) from pg_catalog.pg_namespace where
>> pg_catalog.nameeq(nspname, '%s');
>>
>>
> ​I'd rather write that:
>
> select [...] where nspname operator(pg_catalog.=) '%s'​
>
> Introducing undocumented implementation functions to these queries is
> undesirable; and besides, indexing and equivalence relies on operators and
> not the underlying functions so there would be some risk of performance
> issues if the functions were used directly.

Using directly the function calls calls under the wood of what an
operator does is a potential solution, though I would discard it as it
becomes harder for the reader to undertand that this is an operator.
Things become even more confusing when dealing with input parameters of
different types for simple maths like addition, multiplication or
division using several kinds of integers, leading to more complications
in maintaining this code in the future.  And the operator is careful
about doing proper casting itself.
--
Michael

On Wed, Mar 14, 2018 at 10:50:38AM +0900, Michael Paquier wrote:
> For the rest, which basically concerns psql, I have been thinking that
> actually using 2) would be the most painful approach, still something
> which does not impact the user experience, while 3) is easier to
> back-patch by minimizing the code footprint and avoids also any kind of
> future problems.

Actually, there is also the case of pgbench, where there are two items
to be careful about:
1) Tweak correctly parameters in built-in benchmark queries.
2) Make pgbench gain an extra option allowing one to run the benchmark
on a wanted schema.

1) is essential to do, 2) perhaps less as custom benchmarks can always
be designed so the query strings are secured.  Most users don't run
pgbench on multi-user, untrusted systems as well (right?), giving less
value to 2).
--
Michael

On Tue, Mar 13, 2018 at 07:30:23PM -0700, David G. Johnston wrote:
> On Tue, Mar 13, 2018 at 6:50 PM, Michael Paquier <michael@paquier.xyz>
> wrote:
>> To simplify user's life, we
>> could also recommend just to users to issue a ALTER FUNCTION SET
>> search_path to fix the problem for all functions, that's easier to
>> digest.
>
> ​I'm unclear as to what scope you are suggesting the above advice (and
> option #1) apply to.  All pg_catalog/information_schema functions or all
> functions including those created by users?
> ​

I am suggesting that to keep simple the release notes of the next minor
versions, but still patch information_schema.sql with the changes based
on operator(pg_catalog.foo) for all functions internally as as new
deployments get the right call.
--
Michael

On Wed, Mar 14, 2018 at 09:26:15AM +0900, Tatsuo Ishii wrote:
> > select [...] where nspname operator(pg_catalog.=) '%s'​

> Next question is, should we update the manual? There are bunch of
> places where example queries are shown without schema qualifications.

I gave https://www.postgresql.org/docs/devel/static/extend-extensions.html
such an update, because extensions should assume little about the database
they occupy.  Perhaps a few other pages deserve that, but not the whole
manual.  Ordinary SQL targets a particular database and can know the set of
available operators.

On Wed, Mar 14, 2018 at 10:50:38AM +0900, Michael Paquier wrote:
> On Sat, Mar 10, 2018 at 08:36:34AM +0000, Noah Misch wrote:
> > This qualifies some functions, but it leaves plenty of unqualified operators.
> 
> Yeah, I know that, and i don't have a perfect reply to offer to you.
> There are a couple of methods that we could use to tackle that:
> 1) For functions, enforce search_path with a SET search_path =
> 'pg_catalog' command.  However this has a performance impact.
> 2) Enforce operators qualification with operator(pg_catalog.foo).  This
> has no impact on performance, but repeating that all over the place is
> rather ugly, particularly for psql's describe.c and tab-completion.c.
> 3) Tweak dynamically search_path before running a query:
> - Save the existing search_path value by issuing SHOW search_path.
> - Use ALWAYS_SECURE_SEARCH_PATH_SQL to enforce the path.
> - Set back search_path based on the previous value.
> This logic can happen in a dedicated wrapper, but this impacts
> performance as it requires extra round trips to the server.
> 
> For information_schema.sql, we are talking about tweaking 12 functions.
> So I think that we could live with 2).  To simplify user's life, we
> could also recommend just to users to issue a ALTER FUNCTION SET
> search_path to fix the problem for all functions, that's easier to
> digest.

For information_schema, I'd pick (1).  Performance is not very important
there, and reading or editing code like this is painful:

  (($2 OPERATOR(pg_catalog.-) 4) OPERATOR(pg_catalog.>>) 16) OPERATOR(pg_catalog.&) 65535

(If performance becomes important, one could implement a way to automatically
translate sql-language function source to fully-qualified SQL at CREATE
FUNCTION time or at plan time.)

> For the rest, which basically concerns psql, I have been thinking that
> actually using 2) would be the most painful approach, still something
> which does not impact the user experience, while 3) is easier to
> back-patch by minimizing the code footprint and avoids also any kind of
> future problems.

Dozens of psql queries call pg_*_is_visible functions, which need the
search_path pertinent for user-entered queries.  By itself, (3) doesn't work
for such queries.  Even if you implemented (2), using psql with a hostile
search_path would remain approximately hopeless.  It's too hard for psql users
to write safe input.  Thus, I'd be -1 on accepting (2) or a similarly-ugly
change in psql.  Any proposal for schema qualification in psql faces stiff
competition from the alternative of doing nothing.

For src/test, I would change nothing.  If tests malfunction in a hostile
database, that is not important.  Keeping tests easy to add, modify and review
is more important.

nm

On Thu, Mar 15, 2018 at 01:42:08AM -0700, Noah Misch wrote:
> Dozens of psql queries call pg_*_is_visible functions, which need the
> search_path pertinent for user-entered queries.  By itself, (3) doesn't work
> for such queries.  Even if you implemented (2), using psql with a hostile
> search_path would remain approximately hopeless.  It's too hard for psql users
> to write safe input.  Thus, I'd be -1 on accepting (2) or a similarly-ugly
> change in psql.  Any proposal for schema qualification in psql faces stiff
> competition from the alternative of doing nothing.

Good point.  One thing that could happen here is to extend
pg_*_is_visible with an extra parameter which allows the caller to
enforce the value of search_path. This actually brings more value to
approach 3), because by fetching first the value of search_path, you
could enforce the visibility functions to scan this given namespace for
the time of their execution, but still make the whole query run using a
safe search_path.

> For src/test, I would change nothing.  If tests malfunction in a hostile
> database, that is not important.  Keeping tests easy to add, modify and review
> is more important.

OK.  I would still suggest to fix the schema qualification for
pg_backend_pid though.  This is a one-liner, and simple to address.
This applies as well to psql for array_remove().

So based on the feedback here is what we could at least do now as a
minimal fix set, in the shape of:
- Patch functions in information_schema.sql, using either operator() or
SET search_path.
- Patch function qualifications I found here and there.  Those are
mainly one-liners, and gives readers better references for their own
queries.
--
Michael

On Fri, Mar 16, 2018 at 10:18:59AM +0900, Michael Paquier wrote:
> So based on the feedback here is what we could at least do now as a
> minimal fix set, in the shape of:
> - Patch functions in information_schema.sql, using either operator() or
> SET search_path.
> - Patch function qualifications I found here and there.  Those are
> mainly one-liners, and gives readers better references for their own
> queries.

From what I can see in my backlog, this never actually got into the
tree, and it seems to me that fixing those issues is always better than
nothing:
https://www.postgresql.org/message-id/20180309075538.GD9376@paquier.xyz

Any thoughts?  This does not reinvent the wheel..
--
Michael

On Thu, Nov 29, 2018 at 04:20:24PM +0900, Michael Paquier wrote:
> From what I can see in my backlog, this never actually got into the
> tree, and it seems to me that fixing those issues is always better than
> nothing:
> https://www.postgresql.org/message-id/20180309075538.GD9376@paquier.xyz
> 
> Any thoughts?  This does not reinvent the wheel..

On Fri, Mar 09, 2018 at 04:55:38PM +0900, Michael Paquier wrote:
> Subject: [PATCH] Fix missing schema qualifications in code
> 
> Per CVE-2018-1058, not using proper schema qualifications can allow an
> attacker who has an account on the server to execute arbitrary code as a
> superuser even if he has no such rights.  After monitoring the whole
> code of Postgres, I have bumped into four places that need to be
> addressed:

This patch provides no meaningful increment in security or reliability, but it
does improve stylistic consistency.  Fine to proceed on those grounds, but
this description doesn't fit.

> --- a/src/test/modules/worker_spi/worker_spi.c
> +++ b/src/test/modules/worker_spi/worker_spi.c
> @@ -115,7 +115,9 @@ initialize_worker_spi(worktable *table)
>  
>      /* XXX could we use CREATE SCHEMA IF NOT EXISTS? */
>      initStringInfo(&buf);
> -    appendStringInfo(&buf, "select count(*) from pg_namespace where nspname = '%s'",
> +    appendStringInfo(&buf,
> +                     "select pg_catalog.count(*) "
> +                     "from pg_catalog.pg_namespace where nspname = '%s'",
>                       table->schema);

Remove this change.  The rest of the file doesn't schema-qualify, which is
appropriate for code implementing a test case.

On Thu, Nov 29, 2018 at 10:29:04PM -0800, Noah Misch wrote:
> This patch provides no meaningful increment in security or reliability, but it
> does improve stylistic consistency.  Fine to proceed on those grounds, but
> this description doesn't fit.

Indeed, you are right.  I agree.

> > --- a/src/test/modules/worker_spi/worker_spi.c
> > +++ b/src/test/modules/worker_spi/worker_spi.c
> > @@ -115,7 +115,9 @@ initialize_worker_spi(worktable *table)
> >
> >      /* XXX could we use CREATE SCHEMA IF NOT EXISTS? */
> >      initStringInfo(&buf);
> > -    appendStringInfo(&buf, "select count(*) from pg_namespace where nspname = '%s'",
> > +    appendStringInfo(&buf,
> > +                     "select pg_catalog.count(*) "
> > +                     "from pg_catalog.pg_namespace where nspname = '%s'",
> >                       table->schema);
>
> Remove this change.  The rest of the file doesn't schema-qualify, which is
> appropriate for code implementing a test case.

No problem with that either.  Thanks Noah for the lookup.
--
Michael

On Fri, Nov 30, 2018 at 05:18:09PM +0900, Michael Paquier wrote:
> No problem with that either.  Thanks Noah for the lookup.

And done, without the change for worker_spi.
--
Michael