Redact user password on pg_stat_statements

The idea and the patch looks good to me at first glance, +1.

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?

Cheers,
Greg

--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support

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]/messages/by-id/18817-771682052a364bfe@postgresql.org. 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]: /messages/by-id/18817-771682052a364bfe@postgresql.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] /messages/by-id/18817-771682052a364bfe@postgresql.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? Does anyone really need to know the
standard deviation of the timings for "ALTER ROLE alice SET
work_mem='50MB'"? Let's be honest, there are a lot of things that go into
pg_stat_statements that don't need to. Removing ALTER ROLE entirely would
have a bonus security side-effect, without it being the primary driver.

Cheers,
Greg

--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support

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

On Tue, Feb 25, 2025 at 10:12 AM Sami Imseih <samimseih@gmail.com> wrote:

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.

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 performing the action with
sensitive data.

Good point, but that relies on the client to do the right thing, and
requires two extra steps.

Cheers,
Greg

--
Crunchy Data - https://www.crunchydata.com
Enterprise Postgres Software Products & Tech Support

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 performing the 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)