Prev: Git: Unable to get pack file
Next: [HACKERS] failover vs. read only queries
From: Heikki Linnakangas on 9 Jun 2010 04:44
(moving to pgsql-hackers)

On 03/06/10 10:37, Heikki Linnakangas wrote:
> However, I'm afraid we're lacking in input validation of read-funcs in
> general. ...
>
> Does anyone have an idea on how
> to validate the input in a more wholesale fashion, so that we don't need
> to plug these holes one by one?

Apparently not :-(.

We have two options:

1. Make pg_get_expr() handle arbitrary (possibly even malicious) input
gracefully.

2. Restrict pg_get_expr() to superusers only.

Does anyone want to argue for option 2? We could create views using
pg_get_expr() over the internal catalogs that store trees, so that
regular users could access those without being able to pass arbitrary
input to pg_get_expr(). However, it would break existing applications,
at least pgAdmin uses pg_get_expr().

Assuming we want to make pg_get_expr() check its input, we need to:

* plug the hole Rushabh reported, and not crash on premature end of string
* check all Node types, so that you e.g. can't pass an Integer in a
field that's supposed to hold a CaseWhenExpr
* similarly, check that all Lists contain elements of the right type.

This can all be done in a straightforward way in readfuncs.c, we just
need a bit more decoration to all the READ_* macros. However, that's
still not enough; the functions in ruleutils.c make a number of other
assumptions, like that an OpExpr always has exactly one or two
arguments. That kind of assumptions will need to be explicitly checked.
Many of them already have Asserts, they need to be turned into elogs.

Thoughts? Attached is a patch for the readfuncs.c changes. Unless
someone has a better idea, I'll start going through ruleutils.c and add
explicit checks for any unsafe assumptions. It's going to be a lot of
work, as there's a lot of code in ruleutils.c and the changes need to be
backpatched as well.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
From: Tom Lane on 9 Jun 2010 10:34
Heikki Linnakangas <heikki.linnakangas(a)enterprisedb.com> writes:
> We have two options:

> 1. Make pg_get_expr() handle arbitrary (possibly even malicious) input
> gracefully.

> 2. Restrict pg_get_expr() to superusers only.

I think #1 is a fool's errand. There is far too much structure to a
node tree that is outside the scope of what readfuncs.c is capable of
understanding.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Tom Lane on 9 Jun 2010 11:07
Heikki Linnakangas <heikki.linnakangas(a)enterprisedb.com> writes:
> On 09/06/10 17:34, Tom Lane wrote:
>> I think #1 is a fool's errand. There is far too much structure to a
>> node tree that is outside the scope of what readfuncs.c is capable of
>> understanding.

> That's why I said that ruleutils.c will need to understand and complain
> about the rest.

And that's what I'm telling you is a hopeless task.

> Are you thinking we should retrict pg_get_expr() to superusers then?

I think that's the only solution that will actually fix the problem,
rather than lead to a never-ending series of security bugs. In
hindsight we should never have exposed that function in that form.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Robert Haas on 9 Jun 2010 13:40
On Wed, Jun 9, 2010 at 1:34 PM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote:
> Kris Jurka <books(a)ejurka.com> writes:
>> On Wed, 9 Jun 2010, Heikki Linnakangas wrote:
>>> Are you thinking we should retrict pg_get_expr() to superusers then?
>
>> That seems like it will cause problems for both pg_dump and drivers which
>> want to return metadata as pg_get_expr has been the recommended way of
>> fetching this information.
>
> Yes, it's not a trivial fix either. �We'll have to provide functions or
> views that replace the current usages without letting the user insert
> untrusted strings.

Maybe I'm all wet here, but don't we need to come up with something we
can back-patch?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Tom Lane on 9 Jun 2010 13:34
Kris Jurka <books(a)ejurka.com> writes:
> On Wed, 9 Jun 2010, Heikki Linnakangas wrote:
>> Are you thinking we should retrict pg_get_expr() to superusers then?

> That seems like it will cause problems for both pg_dump and drivers which
> want to return metadata as pg_get_expr has been the recommended way of
> fetching this information.

Yes, it's not a trivial fix either. We'll have to provide functions or
views that replace the current usages without letting the user insert
untrusted strings.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

 |  Next  |  Last
Pages: 1 2 3
Prev: Git: Unable to get pack file
Next: [HACKERS] failover vs. read only queries