Add a warning message when using unencrypted passwords
Hello,
We've got a long tradition of telling people not to use unencrypted
passwords in CREATE ROLE and ALTER ROLE because the queries may be logged.
We try to encourage them to use \password in psql, and related techniques
on other tools. Users usually want us to stop logging passwords, but this
is not that easy to do (if it's at all possible and interesting). A few
days ago, I read Divya Sharma's interview on postgresql.life [1]https://postgresql.life/post/divya_sharma/ and she
said:
Whenever log_statement is set to all (which I understand should be done
for a short period of time for troubleshooting purposes only), if we change
the password for a user, or create a new user, the passwords would be
logged in plain text. From a security point of view, this should not be
allowed. Ideally, It should error out (or at least throw a warning) saying
“while log_statement is set to ‘all’, you shouldn’t change passwords/create
new user with passwords”.
While I dislike the idea of throwing an error, I found the idea of a
warning message really great. So kudos to her for the idea!
I thought about it, and tried to write a patch. I've mostly copied the
"Deprecate MD5 passwords" patch/commit from Nathan Bossart. My patch works
on current HEAD. Documentation and tests are dealt with.
Here is a quick demo:
postgres=# show plaintext_password_warnings;
plaintext_password_warnings
-----------------------------
on
(1 row)
postgres=# create user foo password 'bar';
WARNING: using a plaintext password in a query
DETAIL: plaintext password may be logged.
HINT: Refer to the PostgreSQL documentation for details about using
encrypted password in queries.
CREATE ROLE
postgres=# alter role foo password 'bar2';
WARNING: using a plaintext password in a query
DETAIL: plaintext password may be logged.
HINT: Refer to the PostgreSQL documentation for details about using
encrypted password in queries.
ALTER ROLE
postgres=# set plaintext_password_warnings to off;
SET
postgres=# alter role foo password 'bar3';
ALTER ROLE
postgres=# set plaintext_password_warnings to on;
SET
postgres=# \password foo
Enter new password for user "foo":
Enter it again:
As I'm writing this email, I'm thinking we could transform the boolean GUC
into an enum GUC, allowing the user to get an error or a log message, or no
message at all (old behaviour), whatever fits better for him/her.
I'm interested in any comments about this. I didn't create a commitfest
entry yet, I'm mostly waiting on your comments.
Thanks.
Regards.
[1]: https://postgresql.life/post/divya_sharma/
--
Guillaume.
Attachments:
0001-Add-a-warning-when-using-plain-text-passwords.patchtext/x-patch; charset=US-ASCII; name=0001-Add-a-warning-when-using-plain-text-passwords.patchDownload+58-1
On Sat, 7 Dec 2024 at 15:40, Guillaume Lelarge <guillaume@lelarge.info> wrote:
Whenever log_statement is set to all (which I understand should be done for a short period of time for troubleshooting purposes only), if we change the password for a user, or create a new user, the passwords would be logged in plain text. From a security point of view, this should not be allowed. Ideally, It should error out (or at least throw a warning) saying “while log_statement is set to ‘all’, you shouldn’t change passwords/create new user with passwords”.
While I dislike the idea of throwing an error, I found the idea of a warning message really great. So kudos to her for the idea!
+1 for more clearly letting people know that what they're doing is not
recommended from a security standpoint.
Regarding warning vs error, I agree that a WARNING is probably the
right choice generally. But I think that Divya is correct: When
log_statement = 'all', an error should be thrown instead. Because when
that is the case, we know for sure that the password will be leaked to
the logs. And that error should contain something like: You should
consider this password compromised.
Throwing an error always actually has an interesting downside: We then
automatically log the statement, and thus the password to the log.
When I change the level to ERROR in your code, I get the following
(but with WARNING the STATEMENT line is not there):
2024-12-08 22:59:50.967 CET [104900] ERROR: using a plaintext
password in a query
2024-12-08 22:59:50.967 CET [104900] DETAIL: plaintext password may be logged.
2024-12-08 22:59:50.967 CET [104900] HINT: Refer to the PostgreSQL
documentation for details about using encrypted password in queries.
2024-12-08 22:59:50.967 CET [104900] STATEMENT: ALTER ROLE jelte
PASSWORD 'abc';
PS. I created a commit fest entry here:
https://commitfest.postgresql.org/51/5426/
On 7/12/2024 21:39, Guillaume Lelarge wrote:
I'm interested in any comments about this. I didn't create a commitfest
entry yet, I'm mostly waiting on your comments.
My first impression is that such a GUC may be generalised - something
like a 'security_warnings' parameter.
Also, you can check the value of the log_statement and not report at all
if it is set to off.
--
regards, Andrei Lepikhov
Overall +1 to the idea of a warning.
Regarding warning vs error, I agree that a WARNING is probably the right
choice generally. But I think that Divya is correct: When
log_statement = 'all', an error should be thrown instead.
First, it should be for 'all' AND 'ddl'. And obviously glossing over
log_min_duration_statement entirely. But -1 to throwing an ERROR - that's
not really an error, and not our call to make, so a WARNING is sufficient.
Cheers,
Greg
On 9 Dec 2024, at 14:26, Greg Sabino Mullane <htamfids@gmail.com> wrote:
-1 to throwing an ERROR - that's not really an error, and not our call to make, so a WARNING is sufficient.
Agreed, regardless of how bad it's considered, it's not an error. There are
many ways sensitive data can end up in the logs and offering the impression
there is a safety switch offers a false sense of security.
--
Daniel Gustafsson
Hi,
Le lun. 9 déc. 2024 à 14:40, Daniel Gustafsson <daniel@yesql.se> a écrit :
On 9 Dec 2024, at 14:26, Greg Sabino Mullane <htamfids@gmail.com> wrote:
-1 to throwing an ERROR - that's not really an error, and not our call
to make, so a WARNING is sufficient.
Agreed, regardless of how bad it's considered, it's not an error. There
are
many ways sensitive data can end up in the logs and offering the impression
there is a safety switch offers a false sense of security.
I'm fine with adding a test on whether or not we log statements. But that
completely hides the fact that people listening on the network could also
get to the password if the server doesn't use SSL. Isn't it weird to warn
about one potential leak and not the other one?
--
Guillaume.
On Sat, Dec 07, 2024 at 03:39:55PM +0100, Guillaume Lelarge wrote:
I thought about it, and tried to write a patch. I've mostly copied the
"Deprecate MD5 passwords" patch/commit from Nathan Bossart. My patch works
on current HEAD. Documentation and tests are dealt with.
I've been thinking about this one for a couple of days, and I'm finding
myself leaning -1. I certainly didn't intend for the MD5 password
deprecation warnings to set a precedent of warning for every potentially
undesired behavior, and given that there's no plan to remove the ability to
specify a clear-text password in queries, I worry that this will generate
far more noise than is warranted. I also worry that users may misinterpret
the warning as saying that the clear-text password is stored in the
catalogs.
I know the topic of "password leakage" comes up a lot (e.g., [0]/messages/by-id/b75955f7-e8cc-4bbd-817f-ef536bacbe93@joeconway.com). It'd be
good to find a path forward for that, but IMHO it will require more than
warnings.
[0]: /messages/by-id/b75955f7-e8cc-4bbd-817f-ef536bacbe93@joeconway.com
--
nathan
On 09/12/2024 15:58, Guillaume Lelarge wrote:
Hi,
Le lun. 9 déc. 2024 à 14:40, Daniel Gustafsson <daniel@yesql.se
<mailto:daniel@yesql.se>> a écrit :On 9 Dec 2024, at 14:26, Greg Sabino Mullane <htamfids@gmail.com
<mailto:htamfids@gmail.com>> wrote:
-1 to throwing an ERROR - that's not really an error, and not our
call to make, so a WARNING is sufficient.
Agreed, regardless of how bad it's considered, it's not an error.
There are
many ways sensitive data can end up in the logs and offering the
impression
there is a safety switch offers a false sense of security.I'm fine with adding a test on whether or not we log statements. But
that completely hides the fact that people listening on the network
could also get to the password if the server doesn't use SSL. Isn't it
weird to warn about one potential leak and not the other one?
Here is patch v2. The only addition is a check to warn the user only if
the query has been logged:
(log_statement == LOGSTMT_ALL || log_statement == LOGSTMT_DDL ||
log_min_duration_statement > -1)
v2 is attached.
Regards.
--
Guillaume Lelarge
Consultant
https://dalibo.com
Attachments:
v2-0001-Add-a-warning-when-using-plain-text-passwords.patchtext/x-patch; charset=UTF-8; name=v2-0001-Add-a-warning-when-using-plain-text-passwords.patchDownload+60-1
Guillaume Lelarge <guillaume.lelarge@dalibo.com> writes:
v2 is attached.
This seems pretty much entirely useless to me. The password
has already been leaked to the log (*and* the network, if
session is unencrypted), so what's the point of a warning?
And as already noted, this ignores several other hazards of
the same sort, so it's more likely to create a false sense of
security than anything else.
(In addition to the points noted, what of event triggers?
Or ~/.psql_history?)
regards, tom lane
On 04/02/2025 17:59, Tom Lane wrote:
Guillaume Lelarge <guillaume.lelarge@dalibo.com> writes:
v2 is attached.
This seems pretty much entirely useless to me. The password
has already been leaked to the log (*and* the network, if
session is unencrypted), so what's the point of a warning?
And as already noted, this ignores several other hazards of
the same sort, so it's more likely to create a false sense of
security than anything else.(In addition to the points noted, what of event triggers?
Or ~/.psql_history?)
I agree that the warning itself doesn't make the password secure. But it
never pretends to do that. If I, as a user, see a message like this, my
next move will be to search for a way to change my password in a secure way.
Warning users won't save everyone, but it may help some. Doing nothing
helps no one.
--
Guillaume Lelarge
Consultant
https://dalibo.com
On 04/02/2025 19:14, Guillaume Lelarge wrote:
On 04/02/2025 17:59, Tom Lane wrote:
Guillaume Lelarge <guillaume.lelarge@dalibo.com> writes:
v2 is attached.
This seems pretty much entirely useless to me. The password
has already been leaked to the log (*and* the network, if
session is unencrypted), so what's the point of a warning?
And as already noted, this ignores several other hazards of
the same sort, so it's more likely to create a false sense of
security than anything else.(In addition to the points noted, what of event triggers?
Or ~/.psql_history?)I agree that the warning itself doesn't make the password secure. But it
never pretends to do that. If I, as a user, see a message like this, my
next move will be to search for a way to change my password in a secure
way.Warning users won't save everyone, but it may help some. Doing nothing
helps no one.
FWIW, I just set my patch to the "Withdrawn" status on the commitfest
app. Greg's patch is pretty much the same, and offers more options, I
reviewed it, and it has my vote.
--
Guillaume Lelarge
Consultant
https://dalibo.com