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