Thread: Row Level Security − leakproof-ness and performance implications
Hello
In order to increase our security, we have started deploying row-level
security in order to add another safety net if any issue was to happen in our
applications.
After a careful monitoring of our databases, we found out that a lot of
queries started to go south, going extremely slow.
The root of these slowdowns is that a lot of the PostgreSQL functions are not
marked as leakproof, especially the ones used for operators.
In current git master, the following query returns 258 functions that are used
by operators returning booleans and not marked leakproof:
SELECT proname FROM pg_proc
WHERE exists(select 1 from pg_operator where oprcode::name = proname)
AND NOT(proleakproof)
AND prorettype = 16;
Among these, if you have for instance a table like:
create table demo_enum (
username text,
flag my_enum
);
With an index on (username, flag), the index will only be used on username
because flag is an enum and the enum_eq function is not marked leakproof.
For simple functions like enum_eq/enum_ne, marking them leakproof is an
obvious fix (patch attached to this email, including also textin/textout). And
if we had a 'RLS-enabled' context on functions, a way to make a lot of built-
in functions leakproof would simply be to prevent them from logging errors
containing values.
For others, like arraycontains, it's much trickier : arraycontains can be
leakproof only if the comparison function of the inner type is leakproof too.
This raises some interesting issues, like arraycontains being marked as strict
parallel safe while there is nothing making it mandatory for the inner type.
In order to optimize queries on a table like:
create table demo_array (username text, my_array int[]);
one would need to mark arraycontains leakproof, thus implying that any
comparison operator must be leakproof too? Another possibility, not that
complicated, would be to create specific-types entries in pg_proc for each
type that has a leakproof comparison operator. Or the complicated path would
be to have a 'dynamic' leakproof for functions like arraycontains that depend
on the inner type, but since this is determined at planning, it seems very
complicated to implement.
A third issue we noticed is the usage of functional indexes. If you create an
index on, for instance, (username, leaky_function(my_field)), and then search
on leaky_functon(my_field) = 42, the functional index can be used only if
leaky_function is marked leakproof, even if it is never going to be executed
on invalid rows thanks to the index. After a quick look at the source code, it
also seems complicated to implement since the decision to reject potential
dangerous calls to leaky_function is done really early in the process, before
the optimizer starts.
I am willing to spend some time on these issues, but I have no specific
knowledge of the planner and optimizer, and I fear proper fixes would require
much digging there. If you think it would be appropriate for functions like
arraycontains or range_contains to require the 'inner' comparison function to
be leakproof, or just keep looking at the other functions in pg_proc and
leakproof the ones that can be, I would be happy to write the corresponding
patches.
Best regards
Attachment
Pierre Ducroquet wrote: > In order to increase our security, we have started deploying row-level > security in order to add another safety net if any issue was to happen in our > applications. > After a careful monitoring of our databases, we found out that a lot of > queries started to go south, going extremely slow. > The root of these slowdowns is that a lot of the PostgreSQL functions are not > marked as leakproof, especially the ones used for operators. > In current git master, the following query returns 258 functions that are used > by operators returning booleans and not marked leakproof: > > SELECT proname FROM pg_proc > WHERE exists(select 1 from pg_operator where oprcode::name = proname) > AND NOT(proleakproof) > AND prorettype = 16; > > > Among these, if you have for instance a table like: > create table demo_enum ( > username text, > flag my_enum > ); > > With an index on (username, flag), the index will only be used on username > because flag is an enum and the enum_eq function is not marked leakproof. > > For simple functions like enum_eq/enum_ne, marking them leakproof is an > obvious fix (patch attached to this email, including also textin/textout). And The enum_eq part looks totally safe, but the text functions allocate memory, so you could create memory pressure, wait for error messages that tell you the size of the allocation that failed and this way learn about the data. Is this a paranoid idea? > if we had a 'RLS-enabled' context on functions, a way to make a lot of built- > in functions leakproof would simply be to prevent them from logging errors > containing values. > > For others, like arraycontains, it's much trickier :[...] I feel a little out of my depth here, so I won't comment. > A third issue we noticed is the usage of functional indexes. If you create an > index on, for instance, (username, leaky_function(my_field)), and then search > on leaky_functon(my_field) = 42, the functional index can be used only if > leaky_function is marked leakproof, even if it is never going to be executed > on invalid rows thanks to the index. After a quick look at the source code, it > also seems complicated to implement since the decision to reject potential > dangerous calls to leaky_function is done really early in the process, before > the optimizer starts. If there is a bitmap index scan, the condition will be rechecked, so the function will be executed. > I am willing to spend some time on these issues, but I have no specific > knowledge of the planner and optimizer, and I fear proper fixes would require > much digging there. If you think it would be appropriate for functions like > arraycontains or range_contains to require the 'inner' comparison function to > be leakproof, or just keep looking at the other functions in pg_proc and > leakproof the ones that can be, I would be happy to write the corresponding > patches. Thanks, and I think that every function that can safely be marked leakproof is a progress! Yours, Laurenz Albe
On Wednesday, February 20, 2019 12:43:50 AM CET Laurenz Albe wrote: > Pierre Ducroquet wrote: > > In order to increase our security, we have started deploying row-level > > security in order to add another safety net if any issue was to happen in > > our applications. > > After a careful monitoring of our databases, we found out that a lot of > > queries started to go south, going extremely slow. > > The root of these slowdowns is that a lot of the PostgreSQL functions are > > not marked as leakproof, especially the ones used for operators. > > In current git master, the following query returns 258 functions that are > > used by operators returning booleans and not marked leakproof: > > > > SELECT proname FROM pg_proc > > > > WHERE exists(select 1 from pg_operator where oprcode::name = > > proname) > > AND NOT(proleakproof) > > AND prorettype = 16; > > > > Among these, if you have for instance a table like: > > create table demo_enum ( > > > > username text, > > flag my_enum > > > > ); > > > > With an index on (username, flag), the index will only be used on username > > because flag is an enum and the enum_eq function is not marked leakproof. > > > > For simple functions like enum_eq/enum_ne, marking them leakproof is an > > obvious fix (patch attached to this email, including also textin/textout). > > And > The enum_eq part looks totally safe, but the text functions allocate memory, > so you could create memory pressure, wait for error messages that tell you > the size of the allocation that failed and this way learn about the data. > > Is this a paranoid idea? This is not paranoid, it depends on your threat model. In the model we implemented, the biggest threat we consider from an user point of view is IDOR (Insecure Direct Object Reference): a developer forgetting to check that its input is sane and matches the other parts of the URL or the current session user. Exploiting the leak you mentioned in such a situation is almost impossible, and to be honest the attacker has much easier targets if he gets to that level… Maybe leakproof is too simple? Should we be able to specify a 'paranoid-level' to allow some leaks depending on our threat model? > > if we had a 'RLS-enabled' context on functions, a way to make a lot of > > built- in functions leakproof would simply be to prevent them from > > logging errors containing values. > > > > For others, like arraycontains, it's much trickier :[...] > > I feel a little out of my depth here, so I won't comment. > > > A third issue we noticed is the usage of functional indexes. If you create > > an index on, for instance, (username, leaky_function(my_field)), and then > > search on leaky_functon(my_field) = 42, the functional index can be used > > only if leaky_function is marked leakproof, even if it is never going to > > be executed on invalid rows thanks to the index. After a quick look at > > the source code, it also seems complicated to implement since the > > decision to reject potential dangerous calls to leaky_function is done > > really early in the process, before the optimizer starts. > > If there is a bitmap index scan, the condition will be rechecked, so the > function will be executed. That's exactly my point: using a bitmap index scan would be dangerous and thus should not be allowed, but an index scan works fine. Or the recheck on bitmap scan must first check the RLS condition before doing its check. > > I am willing to spend some time on these issues, but I have no specific > > knowledge of the planner and optimizer, and I fear proper fixes would > > require much digging there. If you think it would be appropriate for > > functions like arraycontains or range_contains to require the 'inner' > > comparison function to be leakproof, or just keep looking at the other > > functions in pg_proc and leakproof the ones that can be, I would be happy > > to write the corresponding patches. > > Thanks, and I think that every function that can safely be marked leakproof > is a progress! Thank you for your comments
On 2/19/19 6:43 PM, Laurenz Albe wrote: > Pierre Ducroquet wrote: >> if we had a 'RLS-enabled' context on functions, a way to make a lot of built- >> in functions leakproof would simply be to prevent them from logging errors >> containing values. >> >> For others, like arraycontains, it's much trickier :[...] > > I feel a little out of my depth here, so I won't comment. I had more-or-less the same idea, and even did a PoC patch (not preventing the log entries but rather redacting the variable data), but after discussing offlist with some other hackers I got the impression that it would be a really hard sell. It does seem to me though that such a feature would be pretty useful. Something like use a GUC to turn it on, and while on log messages get redacted. If you really needed to see the details, you could either duplicate your issue on another installation with the redaction turned off, or maybe turn it off on production in a controlled manner just long enough to capture the full error message. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Attachment
Pierre Ducroquet <p.psql@pinaraf.info> writes: > For simple functions like enum_eq/enum_ne, marking them leakproof is an > obvious fix (patch attached to this email, including also textin/textout). This is not nearly as "obvious" as you think. See prior discussions, notably https://www.postgresql.org/message-id/flat/31042.1546194242%40sss.pgh.pa.us Up to now we've taken a very strict definition of what leakproofness means; as Noah stated, if a function can throw errors for some inputs, it's not considered leakproof, even if those inputs should never be encountered in practice. Most of the things we've been willing to mark leakproof are straight-line code that demonstrably can't throw any errors at all. The previous thread seemed to have consensus that the hazards in text_cmp and friends were narrow enough that nobody had a practical problem with marking them leakproof --- but we couldn't define an objective policy statement that would allow making such decisions, so nothing's been changed as yet. I think it is important to have an articulable policy about this, not just a seat-of-the-pants conclusion about the risk level in a particular function. regards, tom lane
On Wednesday, February 20, 2019 5:24:17 PM CET Tom Lane wrote: > Pierre Ducroquet <p.psql@pinaraf.info> writes: > > For simple functions like enum_eq/enum_ne, marking them leakproof is an > > obvious fix (patch attached to this email, including also textin/textout). > > This is not nearly as "obvious" as you think. See prior discussions, > notably > https://www.postgresql.org/message-id/flat/31042.1546194242%40sss.pgh.pa.us > > Up to now we've taken a very strict definition of what leakproofness > means; as Noah stated, if a function can throw errors for some inputs, > it's not considered leakproof, even if those inputs should never be > encountered in practice. Most of the things we've been willing to > mark leakproof are straight-line code that demonstrably can't throw > any errors at all. > > The previous thread seemed to have consensus that the hazards in > text_cmp and friends were narrow enough that nobody had a practical > problem with marking them leakproof --- but we couldn't define an > objective policy statement that would allow making such decisions, > so nothing's been changed as yet. I think it is important to have > an articulable policy about this, not just a seat-of-the-pants > conclusion about the risk level in a particular function. > > regards, tom lane I undestand these decisions, but it makes RLS quite fragile, with numerous un- documented side-effects. In order to save difficulties from future users, I wrote this patch to the documentation, listing the biggest restrictions I hit with RLS so far. Regards Pierre
Attachment
On 2/20/19 11:24 AM, Tom Lane wrote: > Pierre Ducroquet <p.psql@pinaraf.info> writes: >> For simple functions like enum_eq/enum_ne, marking them leakproof is an >> obvious fix (patch attached to this email, including also textin/textout). > > This is not nearly as "obvious" as you think. See prior discussions, > notably > https://www.postgresql.org/message-id/flat/31042.1546194242%40sss.pgh.pa.us > > Up to now we've taken a very strict definition of what leakproofness > means; as Noah stated, if a function can throw errors for some inputs, > it's not considered leakproof, even if those inputs should never be > encountered in practice. Most of the things we've been willing to > mark leakproof are straight-line code that demonstrably can't throw > any errors at all. > > The previous thread seemed to have consensus that the hazards in > text_cmp and friends were narrow enough that nobody had a practical > problem with marking them leakproof --- but we couldn't define an > objective policy statement that would allow making such decisions, > so nothing's been changed as yet. I think it is important to have > an articulable policy about this, not just a seat-of-the-pants > conclusion about the risk level in a particular function. What if we provided an option to redact all client messages (leaving logged messages as-is). Separately we could provide a GUC to force all functions to be resolved as leakproof. Depending on your requirements, having both options turned on could be perfectly acceptable. Patch for discussion attached. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Attachment
On Wed, Feb 27, 2019 at 6:03 PM Joe Conway <mail@joeconway.com> wrote: > Patch for discussion attached. So... you're just going to replace ALL error messages of any kind with "ERROR: missing error text" when this option is enabled? That sounds unusable. I mean if I'm reading it right this would get not only messages from SQL-callable functions but also things like "deadlock detected" and "could not read block %u in file %s" and "database is not accepting commands to avoid wraparound data loss in database with OID %u". You can't even shut it off conveniently, because the way you've designed it it has to be PGC_POSTMASTER to avoid TOCTTOU vulnerabilities. Maybe I'm misreading the patch? I don't think it would be crazy to have a mode where we try to redact the particular error messages that might leak information, but I think we'd need to make it only those. A wild idea might be to let proleakproof take on three values: yes, no, and maybe. When 'maybe' functions are involved, we tell them whether or not the current query involves any security barriers, and if so they self-censor. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2/28/19 9:12 AM, Robert Haas wrote: > On Wed, Feb 27, 2019 at 6:03 PM Joe Conway <mail@joeconway.com> wrote: >> Patch for discussion attached. > > So... you're just going to replace ALL error messages of any kind with > "ERROR: missing error text" when this option is enabled? That sounds > unusable. I mean if I'm reading it right this would get not only > messages from SQL-callable functions but also things like "deadlock > detected" and "could not read block %u in file %s" and "database is > not accepting commands to avoid wraparound data loss in database with > OID %u". You can't even shut it off conveniently, because the way > you've designed it it has to be PGC_POSTMASTER to avoid TOCTTOU > vulnerabilities. Maybe I'm misreading the patch? You have it correct. I completely disagree that is is unusable though. The way I envision this is that you enable force_leakproof on your development machine without suppress_client_messages being turned on. Do your debugging there. On production, both are turned on. You still get full unredacted messages in your pg log. The client on a prod system does not need these details. If you *really* need to, you can restart to turn it on for a short while on prod, but hopefully you have a non prod system where you reproduce issues for debugging anyway. I am not married to making this only changeable via restart though -- that's why I posted the patch for discussion. Perhaps a superuserset would be better so debugging could be done on one session only on the prod machine. > I don't think it would be crazy to have a mode where we try to redact > the particular error messages that might leak information, but I think > we'd need to make it only those. A wild idea might be to let > proleakproof take on three values: yes, no, and maybe. When 'maybe' > functions are involved, we tell them whether or not the current query > involves any security barriers, and if so they self-censor. Again, I disagree. See above -- you have all you need in the server logs. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Attachment
On Thu, 28 Feb 2019 at 14:13, Robert Haas <robertmhaas@gmail.com> wrote: > A wild idea might be to let > proleakproof take on three values: yes, no, and maybe. When 'maybe' > functions are involved, we tell them whether or not the current query > involves any security barriers, and if so they self-censor. > Does self-censoring mean that they might still throw an error for some inputs, but that error won't reveal any information about the input values? That's not entirely consistent with my understanding of the definition of leakproof, but maybe there are situations where that amount of information leakage would be OK. So maybe we could have "strictly leakproof" functions that never throw errors and "weakly leakproof" functions (needs a better name) that can throw errors, as long as those errors don't include data values. Then we could allow strict and weak security barriers on a per-table basis, depending on how sensitive the data is in each table (I'm not a fan of using GUCs to control this). Regards, Dean
On Thu, Feb 28, 2019 at 9:12 AM Robert Haas <robertmhaas@gmail.com> wrote: Hi, Robert, it has been a while :) > > So... you're just going to replace ALL error messages of any kind with > "ERROR: missing error text" when this option is enabled? That sounds > unusable. I mean if I'm reading it right this would get not only > messages from SQL-callable functions but also things like "deadlock > detected" and "could not read block %u in file %s" and "database is > not accepting commands to avoid wraparound data loss in database with > OID %u". You can't even shut it off conveniently, because the way This makes complete sense to me. The client side of a client/server protocol doesn't have any way to fix 'could not read block %u in file %s', the client doesn't need that kind of detailed information about a server, and in fact that information could be security sensitive. Imagine connecting to a webserver with a web browser and getting a similar message. When something critical happens the servers logs can be viewed and the error can be addressed there, on the server.
On 2/28/19 9:52 AM, Dean Rasheed wrote:
> Does self-censoring mean that they might still throw an error for some
> inputs, but that error won't reveal any information about the input
> values? That's not entirely consistent with my understanding of the
> definition of leakproof
That's the question I was also preparing to ask ... I understood the
definition to exclude even the possibility that some inputs could
produce errors.
> amount of information leakage would be OK. So maybe we could have
> "strictly leakproof" functions that never throw errors and "weakly
> leakproof" functions (needs a better name) that can throw errors, as
> long as those errors don't include data values. Then we could allow
> strict and weak security barriers on a per-table basis
Interesting idea. I wonder if the set { strictly, weakly } would be
better viewed as a user-definable set (a site might define "leakproof
wrt HIPAA", "leakproof wrt FERPA", etc.), and then configure which
combination of leakproof properties must apply where.
OTOH, I'd have to wonder about the feasibility of auditing code for
leakproofness at that kind of granularity.
Regards,
-Chap
Joshua Brindle <joshua.brindle@crunchydata.com> writes: > On Thu, Feb 28, 2019 at 9:12 AM Robert Haas <robertmhaas@gmail.com> wrote: >> So... you're just going to replace ALL error messages of any kind with >> "ERROR: missing error text" when this option is enabled? That sounds >> unusable. I mean if I'm reading it right this would get not only >> messages from SQL-callable functions but also things like "deadlock >> detected" and "could not read block %u in file %s" and "database is >> not accepting commands to avoid wraparound data loss in database with >> OID %u". You can't even shut it off conveniently, because the way > This makes complete sense to me. The client side of a client/server > protocol doesn't have any way to fix 'could not read block %u in file > %s', the client doesn't need that kind of detailed information about a > server, and in fact that information could be security sensitive. I agree with Robert that this idea is a nonstarter. Not only is it a complete disaster from a usability standpoint, but *it does not fix the problem*. The mere fact that an error occurred, or didn't, is already an information leak. Sure, you can only extract one bit per query that way, but a slow leak is still a leak. See the Spectre vulnerability for a pretty exact parallel. The direction I think we're going to need to go in is to weaken our standards for what we'll consider a leakproof function, and/or try harder to make common WHERE-clause operators leakproof. The thread over at https://www.postgresql.org/message-id/flat/7DF52167-4379-4A1E-A957-90D774EBDF21%40winand.at raises the question of why we don't expect that *all* indexable operators are leakproof, at least with respect to the index column contents. (Failing to distinguish which of the inputs can be leaked seems like a pretty fundamental mistake in hindsight. For instance, some opclasses can directly index regex operators, and one would not wish to give up the ability for a regex operator to complain about an invalid pattern. But it could be leakproof with respect to the data side.) regards, tom lane
On Thu, Feb 28, 2019 at 10:49 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Joshua Brindle <joshua.brindle@crunchydata.com> writes: > > On Thu, Feb 28, 2019 at 9:12 AM Robert Haas <robertmhaas@gmail.com> wrote: > >> So... you're just going to replace ALL error messages of any kind with > >> "ERROR: missing error text" when this option is enabled? That sounds > >> unusable. I mean if I'm reading it right this would get not only > >> messages from SQL-callable functions but also things like "deadlock > >> detected" and "could not read block %u in file %s" and "database is > >> not accepting commands to avoid wraparound data loss in database with > >> OID %u". You can't even shut it off conveniently, because the way > > > This makes complete sense to me. The client side of a client/server > > protocol doesn't have any way to fix 'could not read block %u in file > > %s', the client doesn't need that kind of detailed information about a > > server, and in fact that information could be security sensitive. > > I agree with Robert that this idea is a nonstarter. Not only is it > a complete disaster from a usability standpoint, but *it does not > fix the problem*. The mere fact that an error occurred, or didn't, > is already an information leak. Sure, you can only extract one bit > per query that way, but a slow leak is still a leak. See the Spectre > vulnerability for a pretty exact parallel. How is leakproof defined WRT Postgres? Generally speaking a 1 bit error path would be considered a covert channel on most systems and are relatively slow even compared to e.g., timing channels. Redacting error information, outside of the global leakproof setting, seems useful to prevent data leakage to a client on another system, such as Robert's example above "could not read block %u in file %s". Although, and Joe may hate me for saying this, I think only the non-constants should be redacted to keep some level of usability for regular SQL errors. Maybe system errors like the above should be removed from client messages in general. > The direction I think we're going to need to go in is to weaken our > standards for what we'll consider a leakproof function, and/or try > harder to make common WHERE-clause operators leakproof. The thread > over at > https://www.postgresql.org/message-id/flat/7DF52167-4379-4A1E-A957-90D774EBDF21%40winand.at > raises the question of why we don't expect that *all* indexable > operators are leakproof, at least with respect to the index column > contents. (Failing to distinguish which of the inputs can be > leaked seems like a pretty fundamental mistake in hindsight. > For instance, some opclasses can directly index regex operators, > and one would not wish to give up the ability for a regex operator > to complain about an invalid pattern. But it could be leakproof > with respect to the data side.) > > regards, tom lane
On 2/28/19 11:03 AM, Joshua Brindle wrote: > How is leakproof defined WRT Postgres? Generally speaking a 1 bit From the CREATE FUNCTION reference page: LEAKPROOF indicates that the function has no side effects. It reveals no information about its arguments other than by its return value. For example, a function which *throws an error message for some argument values but not others*, or which includes the argument values in any error message, is not leakproof. (*emphasis* mine.) Regards, -Chap
On 2/28/19 11:03 AM, Joshua Brindle wrote: > On Thu, Feb 28, 2019 at 10:49 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> >> Joshua Brindle <joshua.brindle@crunchydata.com> writes: >> > On Thu, Feb 28, 2019 at 9:12 AM Robert Haas <robertmhaas@gmail.com> wrote: >> >> So... you're just going to replace ALL error messages of any kind with >> >> "ERROR: missing error text" when this option is enabled? That sounds >> >> unusable. I mean if I'm reading it right this would get not only >> >> messages from SQL-callable functions but also things like "deadlock >> >> detected" and "could not read block %u in file %s" and "database is >> >> not accepting commands to avoid wraparound data loss in database with >> >> OID %u". You can't even shut it off conveniently, because the way >> >> > This makes complete sense to me. The client side of a client/server >> > protocol doesn't have any way to fix 'could not read block %u in file >> > %s', the client doesn't need that kind of detailed information about a >> > server, and in fact that information could be security sensitive. >> >> I agree with Robert that this idea is a nonstarter. Not only is it >> a complete disaster from a usability standpoint, but *it does not >> fix the problem*. The mere fact that an error occurred, or didn't, >> is already an information leak. Sure, you can only extract one bit >> per query that way, but a slow leak is still a leak. See the Spectre >> vulnerability for a pretty exact parallel. > > How is leakproof defined WRT Postgres? Generally speaking a 1 bit > error path would be considered a covert channel on most systems and > are relatively slow even compared to e.g., timing channels. Yes, I am pretty sure there are plenty of very security conscious environments that would be willing to make this tradeoff in order to get reliable RLS performance. > Redacting error information, outside of the global leakproof setting, > seems useful to prevent data leakage to a client on another system, > such as Robert's example above "could not read block %u in file %s". True > Although, and Joe may hate me for saying this, I think only the > non-constants should be redacted to keep some level of usability for > regular SQL errors. Maybe system errors like the above should be > removed from client messages in general. I started down this path and it looked fragile. I guess if there is generally enough support to think this might be viable I could open up that door again, but I don't want to waste time if the approach is really a non-starter as stated upthread :-/. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Attachment
On Thu, Feb 28, 2019 at 11:14 AM Joe Conway <mail@joeconway.com> wrote: > > > Although, and Joe may hate me for saying this, I think only the > > non-constants should be redacted to keep some level of usability for > > regular SQL errors. Maybe system errors like the above should be > > removed from client messages in general. > > I started down this path and it looked fragile. I guess if there is > generally enough support to think this might be viable I could open up > that door again, but I don't want to waste time if the approach is > really a non-starter as stated upthread :-/. > The only non-starter for Tom was weakening leakproof, right? Can we keep the suppression, and work on strengthening leakproof as a separate activity?
On Thu, Feb 28, 2019 at 11:14 AM Joe Conway <mail@joeconway.com> wrote: > > Although, and Joe may hate me for saying this, I think only the > > non-constants should be redacted to keep some level of usability for > > regular SQL errors. Maybe system errors like the above should be > > removed from client messages in general. > > I started down this path and it looked fragile. I guess if there is > generally enough support to think this might be viable I could open up > that door again, but I don't want to waste time if the approach is > really a non-starter as stated upthread :-/. Hmm. It seems to me that if there's a function that sometimes throws an error and other times does not, and if that behavior is dependent on the input, then even redacting the error message down to 'ERROR: error' does not remove the leak. So it seems to me that regardless of what one thinks about the proposal from a usability perspective, it's probably not correct from a security standpoint. Information that couldn't be leaked until present rules would leak with this change, when the new GUCs were turned on. Am I wrong? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2/28/19 11:37 AM, Robert Haas wrote: > On Thu, Feb 28, 2019 at 11:14 AM Joe Conway <mail@joeconway.com> wrote: >> > Although, and Joe may hate me for saying this, I think only the >> > non-constants should be redacted to keep some level of usability for >> > regular SQL errors. Maybe system errors like the above should be >> > removed from client messages in general. >> >> I started down this path and it looked fragile. I guess if there is >> generally enough support to think this might be viable I could open up >> that door again, but I don't want to waste time if the approach is >> really a non-starter as stated upthread :-/. > > Hmm. It seems to me that if there's a function that sometimes throws > an error and other times does not, and if that behavior is dependent > on the input, then even redacting the error message down to 'ERROR: > error' does not remove the leak. So it seems to me that regardless of > what one thinks about the proposal from a usability perspective, it's > probably not correct from a security standpoint. Information that > couldn't be leaked until present rules would leak with this change, > when the new GUCs were turned on. > > Am I wrong? No, and Tom stated as much too, but life is all about tradeoffs. Some people will find this an acceptable compromise. For those that don't they don't have to use it. IMHO we tend toward too much nannyism too often. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Attachment
On Thu, Feb 28, 2019 at 11:44 AM Joe Conway <mail@joeconway.com> wrote: > No, and Tom stated as much too, but life is all about tradeoffs. Some > people will find this an acceptable compromise. For those that don't > they don't have to use it. IMHO we tend toward too much nannyism too often. Well, I agree with that, too. Hmm. I don't think there's anything preventing you from implementing this in "userspace," is there? A logging hook could suppress all error message text, and you could just mark all functions leakproof after that, and you'd have this exact behavior in an existing release with no core code changes, I think. If you do that, or just stick this patch into your own distro, I would be interested to hear some experiences from customers (and those who support them) after some time had gone by. I find it hard to imagine delivering customer support in an environment configured this way, but sometimes my imagination is limited. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2/28/19 11:50 AM, Robert Haas wrote: > On Thu, Feb 28, 2019 at 11:44 AM Joe Conway <mail@joeconway.com> wrote: >> No, and Tom stated as much too, but life is all about tradeoffs. Some >> people will find this an acceptable compromise. For those that don't >> they don't have to use it. IMHO we tend toward too much nannyism too often. > > Well, I agree with that, too. > > Hmm. I don't think there's anything preventing you from implementing > this in "userspace," is there? A logging hook could suppress all > error message text, and you could just mark all functions leakproof > after that, and you'd have this exact behavior in an existing release > with no core code changes, I think. I think that would affect the server logs too, no? Worth thinking about though... Also manually marking all functions leakproof is far less convenient than turning off the check as this patch effectively allows. You would want to keep track of the initial condition and be able to restore it if needed. Doable but much uglier. Perhaps we could tolerate a hook that would allow an extension to do this though? > If you do that, or just stick this patch into your own distro, I would > be interested to hear some experiences from customers (and those who > support them) after some time had gone by. I find it hard to imagine > delivering customer support in an environment configured this way, but > sometimes my imagination is limited. Again, remember that the actual messages are available in the server logs. The presumption is that the server logs are kept secure, and it is ok to leak information into them. How the customer does or does not decide to pass some of that information on to a support group becomes a problem to deal with on a case by case basis. Also, as mentioned up-thread, in many cases there is or should be a non-production instance available to use for reproducing problems to debug them. Presumably the data on such a system is faked or has already been cleaned up for a wider audience. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Attachment
On Thu, Feb 28, 2019 at 12:05 PM Joe Conway <mail@joeconway.com> wrote: > I think that would affect the server logs too, no? Worth thinking about > though... Yeah, I suppose so, although there might be a way to work around that. > Also manually marking all functions leakproof is far less convenient > than turning off the check as this patch effectively allows. You would > want to keep track of the initial condition and be able to restore it if > needed. Doable but much uglier. Perhaps we could tolerate a hook that > would allow an extension to do this though? Yeah, possibly. I guess we'd have to see how ugly that looks, but.... > Again, remember that the actual messages are available in the server > logs. The presumption is that the server logs are kept secure, and it is > ok to leak information into them. How the customer does or does not > decide to pass some of that information on to a support group becomes a > problem to deal with on a case by case basis. > > Also, as mentioned up-thread, in many cases there is or should be a > non-production instance available to use for reproducing problems to > debug them. Presumably the data on such a system is faked or has already > been cleaned up for a wider audience. Mmmph. If your customers always have a non-production instance where problems from production can be easily reproduced, your customers are not much like our customers. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2/28/19 12:28 PM, Robert Haas wrote: > Mmmph. If your customers always have a non-production instance where > problems from production can be easily reproduced, your customers are > not much like our customers. Well I certainly did not mean to imply that this is always the case ;-) But I think it is fair to tell customers that have these tradeoffs in front of them that it would be even more wise in the case they decided to use this capability. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On 2019-02-21 15:56, Pierre Ducroquet wrote: > I undestand these decisions, but it makes RLS quite fragile, with numerous un- > documented side-effects. In order to save difficulties from future users, I > wrote this patch to the documentation, listing the biggest restrictions I hit > with RLS so far. This appears to be the patch of record for this commit fest entry. I agree that it would be useful to document and discuss better which built-in operators are leak-proof and which are not. But I don't think the CREATE POLICY reference page is the place to do it. Note that the leak-proofness mechanism was originally introduced for security-barrier views (an early form of RLS if you will), so someone could also reasonably expect a discussion there. I'm not sure of the best place to put it. Perhaps adding a section to the Functions and Operators chapter would work. Also, once you start such a list, there will be an expectation that it's complete. So that would need to be ensured. You only list a few things you found. Are there others? How do we keep this up to date? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-02-28 00:03, Joe Conway wrote: > What if we provided an option to redact all client messages (leaving > logged messages as-is). Separately we could provide a GUC to force all > functions to be resolved as leakproof. Depending on your requirements, > having both options turned on could be perfectly acceptable. There are two commit fest entries for this thread, one in Pierre's name and one in yours. Is your entry for the error message redacting functionality? I think that approach has been found not to actually satisfy the leakproofness criteria. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/18/19 3:52 PM, Peter Eisentraut wrote: > On 2019-02-28 00:03, Joe Conway wrote: >> What if we provided an option to redact all client messages (leaving >> logged messages as-is). Separately we could provide a GUC to force all >> functions to be resolved as leakproof. Depending on your requirements, >> having both options turned on could be perfectly acceptable. > > There are two commit fest entries for this thread, one in Pierre's name > and one in yours. Is your entry for the error message redacting > functionality? I think that approach has been found not to actually > satisfy the leakproofness criteria. It is a matter of opinion with regard to what the criteria actually is, and when it ought to apply. But in any case the clear consensus was against me, so I guess I'll assume "my patch was rejected by PostgreSQL all I got was this tee shirt" (...I know I have one that says something like that somewhere...) ;-) I have no idea what the other entry is all about as I have not had the time to look. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Attachment
Hi Pierre, On 3/18/19 8:13 PM, Joe Conway wrote: > > I have no idea what the other entry is all about as I have not had the > time to look. There doesn't seem to be consensus on your patch, either -- I'm planning to mark it rejected at the end of the CF unless you have a new patch for consideration. This thread got a bit hijacked and is hard to follow. If you do submit a new patch I recommend creating a new thread. Regards, -- -David david@pgmasters.net
On 2019-03-18 20:08, Peter Eisentraut wrote: > I agree that it would be useful to document and discuss better which > built-in operators are leak-proof and which are not. But I don't think > the CREATE POLICY reference page is the place to do it. Note that the > leak-proofness mechanism was originally introduced for security-barrier > views (an early form of RLS if you will), so someone could also > reasonably expect a discussion there. It sounds like this will need significant additional work, so setting as returned with feedback for now. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services