[PATCH] Fix leaky VIEWs for RLS
in [PgSql]
|
From: Tom Lane on 7 Jun 2010 20:46 KaiGai Kohei <kaigai(a)ak.jp.nec.com> writes: > In this case, is it unnecessary to expose the given argument in > the error message (from security perspective), isn't it? Yes, if all you care about is security and not usability, that looks like a great solution. We're *not* doing it. 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: KaiGai Kohei on 7 Jun 2010 20:46 (2010/06/08 9:23), KaiGai Kohei wrote: > (2010/06/07 21:56), Stephen Frost wrote: >> * Heikki Linnakangas (heikki.linnakangas(a)enterprisedb.com) wrote: >>> WHERE should do it: >>> >>> SELECT * FROM secrets_view WHERE username = 'neighbor' AND >>> password::integer = 1234; >>> ERROR: invalid input syntax for integer: "neighborssecretpassword" >>> >>> Assuming that username = 'neighbor' is evaluated before the cast. >> >> Fair enough, so we can't allow built-ins either, except perhaps in very >> specific/limited situations. Still, if we track that the above WHERE >> and password::integer calls *should* be run as "role X", while the view >> should run as "role Y", maybe we can at least identify the case where >> we've ended up in a situation where we are going to expose unintended >> data. I don't know enough about the optimizer or the planner to have >> any clue how we might teach them to actually avoid doing such, though I >> certainly believe it could end up being a disaster on performance based >> on comments from others who know better. :) >> > > My opinion is that it is a matter in individual functions, not optimizer. > Basically, built-in functions *should* be trusted, because our security > mechanism is not designed to prevent anything from malicious internal > binary modules. > Sorry, it does not mean *all* the built-in functions could be trusted. Some of built-in ones cannot be "trusted" from definitions, such as lowrite(). Perhaps, it eventually needs a flag in the pg_proc to mark a function being either trusted or untrusted. Then, planner may be able to check the flag to decide whether is can be pushed down, or not. If so, we can mark int4in() as trusted, when we revise the issue of error message. I think the idea makes this problem more simple. Thanks, > Historically, we have not known the risk to leak invisible information > using error messages for a long time, so most of internal functions have > not been designed not to return users unnecessary information. > If so, it needs to revise error messages, but it will not complete with > a single commit. > > Thanks, -- KaiGai Kohei <kaigai(a)ak.jp.nec.com> -- 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: KaiGai Kohei on 7 Jun 2010 21:09 (2010/06/08 9:46), Tom Lane wrote: > KaiGai Kohei<kaigai(a)ak.jp.nec.com> writes: >> In this case, is it unnecessary to expose the given argument in >> the error message (from security perspective), isn't it? > > Yes, if all you care about is security and not usability, that looks > like a great solution. We're *not* doing it. > Sorry, are you saying we should not revise error messages because of usability?? If so, and if we decide the middle-threat also should be fixed, it is necessary to distinguish functions trusted and untrusted, even if a function is built-in. Perhaps, pg_proc takes a new flag to represent it. Thanks, -- KaiGai Kohei <kaigai(a)ak.jp.nec.com> -- 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: Greg Stark on 7 Jun 2010 21:02 On Tue, Jun 8, 2010 at 1:46 AM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote: > KaiGai Kohei <kaigai(a)ak.jp.nec.com> writes: >> In this case, is it unnecessary to expose the given argument in >> the error message (from security perspective), isn't it? > > Yes, if all you care about is security and not usability, that looks > like a great solution. �We're *not* doing it. It's possible to take a more nuanced angle on this approach. You could imagine a flag indicating whether the call is within a SECURE VIEW which if enabled caused error messages to elide data taken from arguments. In order for this not to destroy the code legibility I think you would need to design some new %-escapes for error messages to indicate such arguments or some similar trick. You wouldn't want to have to put extra conditionals everywhere. And I'm not sure where to conveniently put such a flag so it would have the right semantics. It would still be a lot of work and a big patch, but mostly mechanical and it wouldn't impact usability for any case where it wasn't necessary. It would still have the problem described earlier that we would keep finding new omissions for years. I can't see us implementing variable taint tracking in C to do it automatically though. Perhaps we could get it from one of the static analysis tools. -- greg -- 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: Stephen Frost on 7 Jun 2010 21:17
KaiGai, * KaiGai Kohei (kaigai(a)ak.jp.nec.com) wrote: > Perhaps, pg_proc takes a new flag to represent it. Without an actual well-formed approach for dealing with this which requires it, it's far too soon to be asking for changes in the catalog. Please stop suggesting individual maybe-this-would-help changes. We need an actual solution which addresses all, or at least most, of the concerns described, followed by a patch which implements the mechanics of the solution. If the solution calls for an additional pg_proc field, then the patch should include it. Not sure if this would translate well, but asking for new flags to be added to pg_proc right now is putting the cart before the horse. We don't even know which functions we might mark as trusted or not yet, nor is it even clear that adding such a flag would actually help. Adding a flag to pg_proc is certainly nothing like a solution to this problem by itself. Thanks, Stephen |