Thread: Redact user password on pg_stat_statements

Hi hackers!

Attached a patch to redact the password value from pg_stat_statements_view when
executing:
{ CREATE|ALTER} {USER|ROLE|GROUP } identifier { [WITH] [ENCRYPTED]
PASSWORD 'value' }

To redact the password from the pg_stat_statements view a new field location
was added on String type which represents the password value. The location is
stored on JumbleState when JumbleQuery is called. The JumbleState is then used
on generate_normalized_query from pg_stat_statements.c to replace any location
stored with $%d.

The grammar was also changed to set the location field of the String type only
on these specific commands.

Thoughts?

-- 
Matheus Alcantara

Matheus Alcantara <matheusssilv97@gmail.com> writes:
> Attached a patch to redact the password value from pg_stat_statements_view when
> executing:
> { CREATE|ALTER} {USER|ROLE|GROUP } identifier { [WITH] [ENCRYPTED]
> PASSWORD 'value' }

Please see previous threads about hiding this sort of information,
most recently [1].  It's a slippery slope for which there are no
real fixes, and even partial fixes like this one are horrid kluges.
One obvious objection to the direction you propose here is that it
does nothing for pg_stat_activity, nor for the server log if
log_statement is enabled.

The right answer is to never send cleartext passwords to the server
in the first place.

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/18817-771682052a364bfe%40postgresql.org

> It's a slippery slope for which there are no
> real fixes, and even partial fixes like this one are horrid kluges.

+1,

For example I don't think the current patch can deal with passwords
set in ALTER/CREATE inside DO blocks, and there is probably not
a sensible way to deal with that either.

Regards,

Sami Imseih
Amazon Web Services (AWS)

Greg Sabino Mullane <htamfids@gmail.com> writes:
> I'm wondering what else we can do to discourage this pattern, however.
> There are more secure ways to set/change a password, but we keep seeing
> plain text pop up in various contexts. Maybe a strong warning+hint when
> someone uses these commands? A future GUC to disable it by default?

Hmm, we could imagine a GUC that disables accepting a plain-text
password, all right.  (We already assume the server can tell the
difference between encrypted and plain passwords.)

We already have this behavior:

regression=# set password_encryption = md5;
SET
regression=# create user joe password 'joe';
WARNING:  setting an MD5-encrypted password
DETAIL:  MD5 password support is deprecated and will be removed in a future release of PostgreSQL.
HINT:  Refer to the PostgreSQL documentation for details about migrating to another password type.
CREATE ROLE

Refusing plain-text seems pretty adjacent to that.

One concern is that while psql has the ability to construct an
encrypted password client-side, I'm not sure whether other clients
such as pgAdmin have grown equivalent features.  Putting in this
sort of restriction would move that from nice-to-have to a
virtual necessity, so it'd put some pressure on client authors.

            regards, tom lane

On 2025-02-21 Fr 11:08 AM, Tom Lane wrote:
> Matheus Alcantara <matheusssilv97@gmail.com> writes:
>> Attached a patch to redact the password value from pg_stat_statements_view when
>> executing:
>> { CREATE|ALTER} {USER|ROLE|GROUP } identifier { [WITH] [ENCRYPTED]
>> PASSWORD 'value' }
> Please see previous threads about hiding this sort of information,
> most recently [1].  It's a slippery slope for which there are no
> real fixes, and even partial fixes like this one are horrid kluges.
> One obvious objection to the direction you propose here is that it
> does nothing for pg_stat_activity, nor for the server log if
> log_statement is enabled.
>
> The right answer is to never send cleartext passwords to the server
> in the first place.
>
>             regards, tom lane
>
> [1] https://www.postgresql.org/message-id/flat/18817-771682052a364bfe%40postgresql.org
>
>


I don't think this is such a terrible kluge. I think it's different from 
the server log case, which after all requires access to the server file 
system to exploit.

I agree that people should not send passwords in cleartext, but I don't 
know that that means we should never try to ameliorate the risk of doing so.


cheers


andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:
> On 2025-02-21 Fr 11:08 AM, Tom Lane wrote:
>> Please see previous threads about hiding this sort of information,
>> most recently [1].  It's a slippery slope for which there are no
>> real fixes, and even partial fixes like this one are horrid kluges.

> I don't think this is such a terrible kluge. I think it's different from 
> the server log case, which after all requires access to the server file 
> system to exploit.

Well, pg_stat_statements requires pg_read_all_stats membership before
it will show you query text, so there is a permissions gate to pass
here too.  (I think the description of that role in user-manag.sgml
is perhaps not sufficiently explicit about how much power it has;
it's not apparent that it lets you see other sessions' queries.)

But the real reason that I'm allergic to this idea is that it sets
a precedent that we will attempt to hide such information.  Once
we do that, it becomes a lot harder to argue that leakage paths
like the postmaster log or pg_stat_activity aren't security bugs.

            regards, tom lane

On 21.02.25 17:38, Andrew Dunstan wrote:
> I don't think this is such a terrible kluge. I think it's different from 
> the server log case, which after all requires access to the server file 
> system to exploit.

To me, the mechanism by which this patch works is completely nonobvious 
and coincidental, and it might get broken by unrelated changes.

I think a possible, more robust approach would be to put a field, say, 
security_sensitive into DefElem (or maybe a higher node, maybe even 
Query), and drive decisions from that.

Thanks for all the comments on this folks! I probably underestimated
this change.

Thanks all.

-- 
Matheus Alcantara

On 2025-02-24 Mo 11:04 AM, Peter Eisentraut wrote:
> On 21.02.25 17:38, Andrew Dunstan wrote:
>> I don't think this is such a terrible kluge. I think it's different 
>> from the server log case, which after all requires access to the 
>> server file system to exploit.
>
> To me, the mechanism by which this patch works is completely 
> nonobvious and coincidental, and it might get broken by unrelated 
> changes.
>
> I think a possible, more robust approach would be to put a field, say, 
> security_sensitive into DefElem (or maybe a higher node, maybe even 
> Query), and drive decisions from that.


That's a fair comment, but I don't see any point in Matheus or anyone 
else working on it if we're going to reject it anyway. Probably nothing 
we could do is going to be completely leakproof (see Sami's case 
upthread abut DO blocks). If that means we avoid all attempts do lessen 
the danger here then I guess we are done.


cheers


andrew

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

> What about a more general solution, such as a flag to turn off logging of ALTER ROLE statements completely?

IMO, flags for a specific type of utility statement seems way too much
for pg_stat_statements, and this will also not completely prevent leaking
plain text passwords from all ways that CREATE/ALTER ROLE could be
run, i.e. DO blocks, inside functions/procs with track=all.

The clients that set passwords could simply turn off track_utility
on a user/transaction level while they are performing the action with
sensitive data.

--
Sami
Amazon Web Services (AWS)

Sami Imseih <samimseih@gmail.com> writes:
>> What about a more general solution, such as a flag to turn off logging of ALTER ROLE statements completely?

> IMO, flags for a specific type of utility statement seems way too much
> for pg_stat_statements, and this will also not completely prevent leaking
> plain text passwords from all ways that CREATE/ALTER ROLE could be
> run, i.e. DO blocks, inside functions/procs with track=all.

There is a fundamental conflict between the (understandable) desire
for a feature like this and the equally-understandable desire for
database command tracing and logging.  For better or worse, we've
pretty much cast our lot with the more-and-more-tracing-and-logging
side of that.  I don't want to get into a position where every
available introspection feature has to be aware of an obfuscation
rule and it's a security bug if it's not.

So I think the right path forward is to make it easier for
applications to do things in a more secure way.  (Maybe it's still
a security bug if they fail to, but it's their bug not ours.)
We already have things like \password in psql.  The most obvious
helper feature we could add for this on the server side is to allow
the password to be an out-of-line parameter:

    alter role joe set password $1;

That doesn't eliminate leakage to the server log, since we do log
parameter values.  But it'd solve the problem for most other
tracing features.  (And maybe we could suppress logging of
parameter values for certain command types?  Not sure how much
parsing happens before we log.)

I expressed misgivings about allowing parameters in utility
commands in another nearby thread, and I'm still not sure if
it's a totally great idea.  But perhaps it's worth looking into.

            regards, tom lane

>
> Well sure, but best effort is better than no effort at all. Preventing CREATE/ALTER will catch 99% of items, and as I
advocated,there really is no reason for them to be in pg_stat_statements in the first place.
 
>
>>
>> The clients that set passwords could simply turn off track_utility on a user/transaction level while they are
performingthe action with
 
>> sensitive data.
>
>
> Good point, but that relies on the client to do the right thing, and requires two extra steps.

Yes, I think relying on the client to do the right thing is a correct strategy.

> We already have things like \password in psql.  The most obvious
>helper feature we could add for this on the server side is to allow
> the password to be an out-of-line parameter:

>        alter role joe set password $1;

Giving the client to parametrize DDL commands seems like a good
idea overall, and it gives the client a more robust way to deal with
sensitive passwords. Of course, even if something like this becomes possible,
the responsibility is still on the client to ensure that they are not
logging the parameter values. So, the client doing the right thing
is still required.

--

Sami
Amazon Web Services (AWS)