Subject: can we mark upper/lower/textlike functions leakproof?

From: Andrew Dunstan <[email-masked]>
Date: Tue, 30 Jul 2024 17:35:27 -0400

Several years ago, an EDB customer complained that if they used a 
functional index involving upper(), lower(), or textlike(), where RLS 
was involved, the indexes were not used because these functions are not 
marked leakproof. We presented the customer with several options for 
addressing the problem, the simplest of which was simply to mark the 
functions as leakproof, and this was the solution they adopted.

The consensus of discussion at the time among our senior developers was 
that there was probably no reason why at least upper() and lower() 
should not be marked leakproof, and quite possibly initcap() and 
textlike() also. It was suggested that we had not been terribly rigorous 
in assessing whether or not functions can be considered leakproof.

At the time we should have asked the community about it, but we didn't.

Fast forward to now. The customer has found no observable ill effects of 
marking these functions leakproof. The would like to know if there is 
any reason why we can't mark them leakproof, so that they don't have to 
do this in every database of every cluster they use.

Thoughts?

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

---

From: David Rowley <[email-masked]>
Date: Wed, 31 Jul 2024 10:51:25 +1200

> do this in every database of every cluster they use.

According to [1], it's just not been done yet due to concerns about
risk to reward ratios.  Nobody mentioned any reason why it couldn't
be, but there were some fears that future code changes could yield new
failure paths.

David

[1] https://postgr.es/m/02BDFCCF-BDBB-4658-9717-4D95F9A91561%40thebuild.com

---

From: Andrew Dunstan <[email-masked]>
Date: Wed, 31 Jul 2024 05:47:40 -0400

On 2024-07-30 Tu 6:51 PM, David Rowley wrote:
> On Wed, 31 Jul 2024 at 09:35, Andrew Dunstan<[email-masked]>  wrote:
> [1]https://postgr.es/m/02BDFCCF-BDBB-4658-9717-4D95F9A91561%40thebuild.com

Hmm, somehow I missed that thread in searching, and clearly I'd 
forgotten it.

Still, I'm not terribly convinced by arguments along the lines you're 
suggesting. "Sufficient unto the day is the evil thereof." Maybe we need 
a test to make sure we don't make changes along those lines, although I 
have no idea what such a test would look like.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

---

From: Joe Conway <[email-masked]>
Date: Wed, 31 Jul 2024 09:14:37 -0400

> have no idea what such a test would look like.

I think I have expressed this opinion before (which was shot down), and 
I will grant that it is hand-wavy, but I will give it another try.

In my opinion, for this use case and others, it should be possible to 
redact the values substituted into log messages based on some criteria. 
One of those criteria could be "I am in a leakproof call right now". In 
fact in a similar fashion, an extension ought to be able to mutate the 
log message based on the entire string, e.g. when "ALTER 
ROLE...PASSWORD..." is spotted I would like to be able to redact 
everything after "PASSWORD".

Yes it might render the error message unhelpful, but I know of users 
that would accept that tradeoff in order to get better performance and 
security on their production workloads. Or in some cases (e.g. PASSWORD) 
just better security.

-- 
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: Andrew Dunstan <[email-masked]>
Date: Wed, 31 Jul 2024 09:38:16 -0400

On 2024-07-31 We 9:14 AM, Joe Conway wrote:
>> suggesting. "Sufficient unto the day is the evil thereof." Maybe we 
>> need a test to make sure we don't make changes along those lines, 
>> although I have no idea what such a test would look like.
>
>
> I think I have expressed this opinion before (which was shot down), 
> and I will grant that it is hand-wavy, but I will give it another try.
>
> redact the values substituted into log messages based on some 
> criteria. One of those criteria could be "I am in a leakproof call 
> right now". In fact in a similar fashion, an extension ought to be 
> able to mutate the log message based on the entire string, e.g. when 
> "ALTER ROLE...PASSWORD..." is spotted I would like to be able to 
> redact everything after "PASSWORD".
>
> security on their production workloads. Or in some cases (e.g. 
> PASSWORD) just better security.
>

---

From: Robert Haas <[email-masked]>
Date: Wed, 31 Jul 2024 13:39:45 -0400

> everything after "PASSWORD".

This might be helpful, and unfortunately I'm all too familiar with the
ALTER ROLE...PASSWORD example, but I think it's not really related to
the question of whether we can mark upper() and lower() leakproof.

If there are some inputs that cause upper() and lower() to fail and
others that do not, the functions aren't leakproof, because an
attacker can extract information about values that they can't see by
feeding those values into these functions and seeing whether they get
a failure or not. It doesn't matter what error message is produced;
the fact that the function throws an error of any kind for some input
values but enough for others is enough to make it unsafe, and it seems
to me that we've repeatedly found that there's often a way to turn
even what seems like a small leak into a very large one, so we need to
be quite careful here.

Annoyingly, we have a WHOLE BUNCH of different code paths that need to
be assessed individually here. I think we can ignore the fact that
upper() can throw errors when it's unclear which collation to use; I
think that's a property of the query string rather than the input
value. It's a bit less clear whether we can ignore out of memory
conditions, but my judgement is that a possible OOM from a small
allocation is not generally going to be useful as an attack vector. If
a long string produces an error and a short one doesn't, that might
qualify as a real leak. And other errors that depend on the input
value are also leaks. So let's go through the code paths:

- When lc_ctype_is_c(), we call asc_toupper(), which calls
pg_ascii_toupper(), which just replaces a-z with A-Z. No errors.

- When mylocale->provider == COLLPROVIDER_ICU, we call icu_to_uchar()
and icu_convert_case(). icu_to_uchar() calls uchar_length(), which has
this:

        if (U_FAILURE(status) && status != U_BUFFER_OVERFLOW_ERROR)
                ereport(ERROR,
                                (errmsg("%s failed: %s",
"ucnv_toUChars", u_errorName(status))));

I don't know what errors can be reported here, or if this ever fails
in practice, so I can't prove it never fails consistently for some
particular input string. uchar_convert() and icu_convert_case() have
similar error reporting.

- When mylocale->provider == COLLPROVIDER_BUILTIN, we call
unicode_strupper() which calls unicode_to_utf8() which clearly throws
no errors. Likewise unicode_utf8len() does not error. I don't see how
we can get an error out of this path.

- In cases not covered by the above, we take different paths depending
on whether a multi-byte encoding is in use. In single-byte encodings,
we rely on either pg_toupper() or toupper_l(). The latter is an
OS-provided function so can't ereport(), and the former calls either
does the work itself or calls toupper() which again is an OS-provided
function and can't report().

- Finally, when mylocale == NULL or the provider is COLLPROVIDER_LIBC
and the encoding is multi-byte, we use char2wchar(), then towupper_l()
or towupper(), then wchar2char(). The whole thing can fall apart if
the string is too long, which might be enough to declare this
leakproof but it depends on whether the error guarded by /* Overflow
paranoia */ is really just paranoia or whether it's actually
reachable. Otherwise, towupper*() won't ereport because it's not part
of PG, so we need to assess char2wchar() and wchar2char(). Here I note
that char2wchar() claims that it does an ereport() if the input is
invalidly encoded. This is kind of surprising when you think about it,
because our usual policy is not to allow invalidly encoded data into
the database in the first place, but whoever wrote this thought it
would be possible to hit this case "if LC_CTYPE locale is different
from the database encoding." But it seems that the logic here dates to
commit 2ab0796d7a3a7116a79b65531fd33f1548514b52 back in 2011, so it
seems at least possible to me that we've tightened things up enough
since then that you can't actually hit this any more. But then again,
maybe not.

So in summary, I think upper() is ... pretty close to leakproof. But
if ICU sometimes fails on certain strings, then it isn't. And if the
multi-byte libc path can be made to fail reliably either with really
long strings or with certain choices of the LC_CTYPE locale, then it
isn't.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Tom Lane <[email-masked]>
Date: Wed, 31 Jul 2024 14:03:46 -0400

Robert Haas <[email-masked]> writes:
> a failure or not.

> [ rather exhaustive analysis redacted ]

> long strings or with certain choices of the LC_CTYPE locale, then it

The problem here is that marking these functions leakproof is a
promise about a *whole bunch* of code, much of it not under our
control; worse, there's no reason to think all that code is stable.
A large fraction of it didn't even exist a few versions ago.

Even if we could convince ourselves that the possible issues Robert
mentions aren't real at the moment, I think marking these leakproof
is mighty risky.  It's unlikely we'd remember to revisit the marking
the next time someone drops a bunch of new code in here.

			regards, tom lane

---

From: Joe Conway <[email-masked]>
Date: Wed, 31 Jul 2024 14:43:40 -0400

> the next time someone drops a bunch of new code in here.

I still maintain that there is a whole host of users that would accept 
the risk of side channel attacks via existence of an error or not, if 
they could only be sure nothing sensitive leaks directly into the logs 
or to the clients. We should give them that choice.

---

From: Andrew Dunstan <[email-masked]>
Date: Wed, 31 Jul 2024 15:54:29 -0400

>>> [ rather exhaustive analysis redacted ]

First, thanks you very much, Robert for the analysis.

> or to the clients. We should give them that choice.

As I meant to say in my previous empty reply, I think your suggestions 
make lots of sense.

cheers

andrew

---

From: Robert Haas <[email-masked]>
Date: Wed, 31 Jul 2024 16:10:41 -0400

> or to the clients. We should give them that choice.

I'm not sure what design you have in mind. A lot of possible designs
seem to end up like this:

1. You can't directly select the invisible value.

2. But you can write a plpgsql procedure that tries a bunch of things
in a loop and catches errors and uses which things error and which
things don't to figure out and return the invisible value.

And I would argue that's not really that useful. Especially if that
plpgsql procedure can extract the hidden values in like 1ms/row.

---

From: Robert Haas <[email-masked]>
Date: Wed, 31 Jul 2024 16:26:00 -0400

> A large fraction of it didn't even exist a few versions ago.

I think it's a fair critique. It could be reasonable to say, well, a
particular user could reasonably judge that the risk of marking
upper() as leak-proof is acceptable, but it's a little too risky for
us to want to do it as a project. After all, they can know things like
"we don't even use ICU," which we as a project cannot know.

However, the risk is that an end-user is going to be much less able to
evaluate what is and isn't safe than we are. I think some people are
going to be like -- well the core project doesn't mark enough stuff
leakproof, so I'll just go add markings to a bunch of stuff myself.
And they probably won't stop at stuff like UPPER which is almost
leakproof. They might add it to stuff such as LIKE which results in
immediately giving away the farm. By not giving people any guidance,
we invite them to make up their own rules.

Perhaps it's still right to be conservative; after all, no matter what
an end-user does, it's not a CVE for us, and CVEs suck. On the other
hand, shipping a system that is not useful in practice until you
modify a bunch of stuff also sucks, especially when it's not at all
clear which things are safe to modify.

I'm not sure what the right thing to do here is, but I think that it's
wrong to imagine that being unwilling to endorse probably-leakproof
things as leakproof -- or unwilling to put in the work to MAKE them
leakproof if they currently aren't -- has no security costs.

---

From: Joe Conway <[email-masked]>
Date: Wed, 31 Jul 2024 16:42:51 -0400

> plpgsql procedure can extract the hidden values in like 1ms/row.

You are assuming that everyone allows direct logins with the ability to 
create procedures. Plenty don't.

---

From: Tom Lane <[email-masked]>
Date: Wed, 31 Jul 2024 17:28:03 -0400

> leakproof if they currently aren't -- has no security costs.

Well, we *have* been a little bit spongy about that --- notably,
that texteq and friends are marked leakproof.  But IMV, marking
upper/lower as leakproof is substantially riskier and offers
substantially less benefit than those did.

In general, I'm worried about a slippery slope here.  If we
start marking things as leakproof because we cannot prove
they leak, rather than because we can prove they don't,
we are eventually going to find ourselves in a very bad place.

			regards, tom lane

---

From: Laurenz Albe <[email-masked]>
Date: Thu, 01 Aug 2024 13:17:22 +0200

> or to the clients. We should give them that choice.

I think that you are right.

But what do you tell the users who would not accept that risk?

Yours,
Laurenz Albe

---

From: Robert Haas <[email-masked]>
Date: Thu, 1 Aug 2024 07:57:05 -0400

> create procedures. Plenty don't.

Well, if you can send queries, then you can do the same thing, driven
by client-side logic.

If you can't directly send queries in any form, then I guess things
are different. But I don't really understand what kind of system you
have in mind.

---

From: Joe Conway <[email-masked]>
Date: Thu, 1 Aug 2024 09:54:52 -0400

> I think that you are right.

thanks

> But what do you tell the users who would not accept that risk?

Document that the option should not be used if that is the case

¯\_(ツ)_/¯

---

From: Joe Conway <[email-masked]>
Date: Thu, 1 Aug 2024 10:05:44 -0400

> Well, if you can send queries, then you can do the same thing, driven

Sure. Of course you should be monitoring your production servers for 
anomalous workloads, no? "Gee, why is Joe running the same query 
millions of times that keeps throwing errors? Maybe we should go see 
what Joe is up to"

> are different.

Right, and there are plenty of those. I have even worked with at least 
one rather large one on behalf of your employer some years ago.

> But I don't really understand what kind of system you have in mind.

Well I did say I was being hand wavy ;-)

It has been a long time since I thought deeply about this. I will try to 
come back with something more concrete if no one beats me to it.

---

From: Tom Lane <[email-masked]>
Date: Thu, 01 Aug 2024 10:26:05 -0400

> Document that the option should not be used if that is the case

Are you proposing that we invent two levels of leakproofness
with different guarantees?  That seems like a mess, not least
because there are going to be varying opinions about where we
should set the bar for the lower level.

			regards, tom lane

---

From: Robert Haas <[email-masked]>
Date: Thu, 1 Aug 2024 10:43:54 -0400

> millions of times that keeps throwing errors? Maybe we should go see

I think it's possible that something like this could be part of some
useful approach, but I think it's difficult. If it would take Joe a
month of pounding on the server to steal enough data to matter, then I
think monitoring could be one required part of an information
protection strategy. However, if Joe, or someone with Joe's
credentials, can steal all the secret data in under an hour, the
monitoring system probably doesn't help much. A human being probably
won't react quickly enough to stop something bad from happening,
especially if the person with Joe's credentials begins the attack at
2am on Christmas.

More generally, I think it's difficult for us to build infrastructure
into PostgreSQL that relies on complex assumptions about what the
customer environment is. To some extent, we are already relying on
users to prevent certain types of attacks. For example, RLS supposes
that timing attacks or plan-shape based attacks won't be feasible, but
we don't do anything to prevent them; we just hope the user takes care
of it. That's already a shaky assumption, because timing attacks could
well be feasible across a fairly deep application stack e.g. the user
issues an HTTP query for a web page and can detect variations in the
underlying database query latency.

When you start proposing assumptions that the user can't execute DDL
or can't execute SQL queries or that there's monitoring of the error
log in place, I feel the whole thing gets very hard to reason about.
First, you have to decide on exactly what the assumptions are - no
DDL, no direct SQL at all, something else? Different situations could
exist for different users, so whatever assumption we make will not
apply to everyone. Second, for some of this stuff, there's a sliding
scale. If we stipulate that a user is going to need a monitoring
system, how good does that monitoring system have to be? What does it
have to catch, and how quickly are the humans required to respond? If
we stipulate that the attacker can't execute SQL directly, how much
control over the generated SQL are they allowed to have?

I don't want to make it sound like I think it's hopeless to come up
with something clever here. The current situation kind of sucks, and I
do have hopes that there are better ideas out there. At the same time,
we need to be able to articulate clearly what we are and are not
guaranteeing and under what set of assumptions, and it doesn't seem
easy to me to come up with something satisfying.

---

From: Jacob Champion <[email-masked]>
Date: Thu, 1 Aug 2024 13:45:00 -0700

> we invite them to make up their own rules.

+1.

Would it provide enough value for effort to explicitly mark leaky
procedures as such? Maybe that could shrink the grey area enough to be
protective?

--Jacob

---

From: Jacob Champion <[email-masked]>
Date: Thu, 1 Aug 2024 13:51:41 -0700

> should set the bar for the lower level.

It kind of reminds me of the kernel's "paranoia level" tunables, which
seem to proliferate in weird ways [1] and not make for a particularly
great UX.

--Jacob

[1] https://askubuntu.com/questions/1400874/what-does-perf-paranoia-level-four-do

---

From: Robert Haas <[email-masked]>
Date: Thu, 1 Aug 2024 21:02:46 -0400

> procedures as such? Maybe that could shrink the grey area enough to be

You mean like proleakproof = true/false/maybe?

---

From: Jacob Champion <[email-masked]>
Date: Fri, 2 Aug 2024 06:48:00 -0700

> You mean like proleakproof = true/false/maybe?

Yeah, exactly.

--Jacob

---

From: Joe Conway <[email-masked]>
Date: Fri, 2 Aug 2024 09:58:37 -0400

>> You mean like proleakproof = true/false/maybe?

<dons flameproof suit>
Hmmm, and then have "leakproof_mode" = strict/lax/off where 'strict' is 
current behavior, 'lax' allows the 'maybe's to get pushed down, and 
'off' ignores the leakproof attribute entirely and pushes down anything 
that merits being pushed?
</dons flameproof suit>

---

From: "David G. Johnston" <[email-masked]>
Date: Fri, 2 Aug 2024 07:14:52 -0700

> that merits being pushed?
Another approach would be to do something like:

select "upper"!(column)

to demark within the query usage itself that this function with a maybe
leakproof attribute gets interpreted as yes.

or even something like:

select "upper"[leakproof](column)

This is both more readable and provides a way, not that we seem to need it,
to place more than one label (or just alternative labels using the same
syntax construct, like with explain (...)) inside the "array".

Discoverability of when to add such a marker would be left up to the query
author, with the safe default mode being a not leakproof interpretation.

David J.

---

From: Tom Lane <[email-masked]>
Date: Fri, 02 Aug 2024 11:07:55 -0400

> that merits being pushed?

So in other words, we might as well just remove RLS.

			regards, tom lane

---

From: Joe Conway <[email-masked]>
Date: Fri, 2 Aug 2024 12:13:31 -0400

> So in other words, we might as well just remove RLS.

Perhaps deciding where to draw the line for 'maybe' is too hard, but I 
don't understand how you can say that in a general sense.

'strict' mode would provide the same guarantees as today. And even 'off' 
has utility for cases where (1) no logins are allowed except for the app 
(which is quite common in production environments) and no database 
errors are propagated to the end client (again pretty standard best 
practice); or (2) non-production environments, e.g. for testing or 
debugging; or (3) use cases that utilize RLS as a notationally 
convenient filtering mechanism and are not bothered by some leakage in 
the worst case.

---

From: Jacob Champion <[email-masked]>
Date: Fri, 2 Aug 2024 09:22:36 -0700

> don't understand how you can say that in a general sense.

I'm more sympathetic to the "maybe" case, but for the "off" proposal I
find myself agreeing with Tom. If you want "off", turn off RLS.

> And even 'off'
> practice);

I'm extremely skeptical of this statement, but I've been out of the
full-stack world for a bit. How does a modern best-practice
application hide the fact that it ran into an error and couldn't
complete a query?

> or (2) non-production environments, e.g. for testing or
> debugging; or

Changing the execution behavior between dev and prod seems like an
anti-goal. When would turning this off help you debug something?

> (3) use cases that utilize RLS as a notationally

Catering to this use case doesn't seem like it should be a priority.
If a security feature is useful for you in a non-security setting,
awesome, but we shouldn't poke holes in it for that situation, nor
should it be surprising if the security gets stronger and becomes
harder to use for non-security.

--Jacob