Re: [HACKERS] Change authentication error message (patch)
Bruce Momjian br...@momjian.us writes: On Thu, Jan 23, 2014 at 10:39:34PM -0500, Tom Lane wrote: I'm not convinced that this improves anything. The problem might not in fact be either of the things you mention, in which case the new message is outright misleading. Also, what of the policy stated in the header comment for the function you're hacking, ie we intentionally don't reveal the precise cause of the failure to the client? Well, the only solution then would be to add some weasel words like perhaps expired password, but that seems so rare that I doubt it would apply very often and seems like an odd suggestion. We could go with: password authentication failed for user \%s\: perhaps invalid or expired password We did have two threads on this issue in the past 12 months so I figured we should try to do something. I agree with doing *something*, but this particular thing seems to violate our very long-standing policy on how to deal with authentication failures, as well as being too vague to be really useful. What would be well within that policy is to log additional information into the postmaster log. I see that md5_crypt_verify knows perfectly well whether the problem is no password set, wrong password, or expired password. I don't see anything wrong with having it emit a log entry --- maybe not in the second case for fear of log-spam complaints, but certainly the third case and maybe the first one. Or possibly cleaner, have it return additional status so that auth_failed() can include the info in the main ereport using errdetail_log(). regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Change authentication error message (patch)
I wrote:
I agree with doing *something*, but this particular thing seems to violate
our very long-standing policy on how to deal with authentication failures,
as well as being too vague to be really useful.
What would be well within that policy is to log additional information
into the postmaster log. I see that md5_crypt_verify knows perfectly
well whether the problem is no password set, wrong password, or expired
password. I don't see anything wrong with having it emit a log entry
--- maybe not in the second case for fear of log-spam complaints, but
certainly the third case and maybe the first one. Or possibly cleaner,
have it return additional status so that auth_failed() can include the
info in the main ereport using errdetail_log().
Attached is a proposed patch that does exactly that. This just addresses
the cases mentioned above; once the infrastructure is in place, there
might be more things that would be worth logging using this mechanism.
regards, tom lane
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 882dc8f..1974b57 100644
*** a/src/backend/libpq/auth.c
--- b/src/backend/libpq/auth.c
***
*** 38,46
*
*/
static void sendAuthRequest(Port *port, AuthRequest areq);
! static void auth_failed(Port *port, int status);
static char *recv_password_packet(Port *port);
! static int recv_and_check_password_packet(Port *port);
/*
--- 38,46
*
*/
static void sendAuthRequest(Port *port, AuthRequest areq);
! static void auth_failed(Port *port, int status, char *logdetail);
static char *recv_password_packet(Port *port);
! static int recv_and_check_password_packet(Port *port, char **logdetail);
/*
*** ClientAuthentication_hook_type ClientAut
*** 207,216
* in use, and these are items that must be presumed known to an attacker
* anyway.
* Note that many sorts of failure report additional information in the
! * postmaster log, which we hope is only readable by good guys.
*/
static void
! auth_failed(Port *port, int status)
{
const char *errstr;
int errcode_return = ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION;
--- 207,217
* in use, and these are items that must be presumed known to an attacker
* anyway.
* Note that many sorts of failure report additional information in the
! * postmaster log, which we hope is only readable by good guys. In
! * particular, if logdetail isn't NULL, we send that string to the log.
*/
static void
! auth_failed(Port *port, int status, char *logdetail)
{
const char *errstr;
int errcode_return = ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION;
*** auth_failed(Port *port, int status)
*** 273,286
}
if (port-hba)
! ereport(FATAL,
! (errcode(errcode_return),
! errmsg(errstr, port-user_name),
! errdetail_log(Connection matched pg_hba.conf line %d: \%s\, port-hba-linenumber, port-hba-rawline)));
! else
! ereport(FATAL,
! (errcode(errcode_return),
! errmsg(errstr, port-user_name)));
/* doesn't return */
}
--- 274,294
}
if (port-hba)
! {
! char *cdetail;
!
! cdetail = psprintf(_(Connection matched pg_hba.conf line %d: \%s\),
! port-hba-linenumber, port-hba-rawline);
! if (logdetail)
! logdetail = psprintf(%s\n%s, logdetail, cdetail);
! else
! logdetail = cdetail;
! }
!
! ereport(FATAL,
! (errcode(errcode_return),
! errmsg(errstr, port-user_name),
! logdetail ? errdetail_log(%s, logdetail) : 0));
/* doesn't return */
}
*** void
*** 294,299
--- 302,308
ClientAuthentication(Port *port)
{
int status = STATUS_ERROR;
+ char *logdetail = NULL;
/*
* Get the authentication method to use for this frontend/database
*** ClientAuthentication(Port *port)
*** 507,518
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
errmsg(MD5 authentication is not supported when \db_user_namespace\ is enabled)));
sendAuthRequest(port, AUTH_REQ_MD5);
! status = recv_and_check_password_packet(port);
break;
case uaPassword:
sendAuthRequest(port, AUTH_REQ_PASSWORD);
! status = recv_and_check_password_packet(port);
break;
case uaPAM:
--- 516,527
(errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION),
errmsg(MD5 authentication is not supported when \db_user_namespace\ is enabled)));
sendAuthRequest(port, AUTH_REQ_MD5);
! status = recv_and_check_password_packet(port, logdetail);
break;
case uaPassword:
sendAuthRequest(port, AUTH_REQ_PASSWORD);
! status = recv_and_check_password_packet(port, logdetail);
Re: [HACKERS] Change authentication error message (patch)
On Fri, Jan 24, 2014 at 10:10:00AM -0500, Tom Lane wrote: Bruce Momjian br...@momjian.us writes: On Thu, Jan 23, 2014 at 10:39:34PM -0500, Tom Lane wrote: I'm not convinced that this improves anything. The problem might not in fact be either of the things you mention, in which case the new message is outright misleading. Also, what of the policy stated in the header comment for the function you're hacking, ie we intentionally don't reveal the precise cause of the failure to the client? Well, the only solution then would be to add some weasel words like perhaps expired password, but that seems so rare that I doubt it would apply very often and seems like an odd suggestion. We could go with: password authentication failed for user \%s\: perhaps invalid or expired password We did have two threads on this issue in the past 12 months so I figured we should try to do something. I agree with doing *something*, but this particular thing seems to violate our very long-standing policy on how to deal with authentication failures, as well as being too vague to be really useful. What would be well within that policy is to log additional information into the postmaster log. I see that md5_crypt_verify knows perfectly well whether the problem is no password set, wrong password, or expired password. I don't see anything wrong with having it emit a log entry --- maybe not in the second case for fear of log-spam complaints, but certainly the third case and maybe the first one. Or possibly cleaner, have it return additional status so that auth_failed() can include the info in the main ereport using errdetail_log(). I was afraid that PGOPTIONS='-c client_min_messages=log' would allow clients to see the log messages, but in testing I found we don't show them during authentication, and I found this C comment: * client_min_messages is honored only after we complete the * authentication handshake. This is required both for security * reasons and because many clients can't handle NOTICE messages * during authentication. I like the 'LOG' idea very much, and liked your patch too. :-) -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Change authentication error message (patch)
On Wed, Jun 19, 2013 at 01:27:39PM -0700, Joshua D. Drake wrote: On 06/19/2013 01:18 PM, Markus Wanner wrote: Authentication failed or password has expired for user \%s\ Authentication failed covers any combination of a username/password being wrong and obviously password expired covers the other. Works for me. Considering the password to be the thing that expires (rather than the account) is probably more accurate as well. It is also how it is worded in the docs (which is why I used it). Patch below. I have developed the attached patch to fix this problem. Do I need to say invalid user or invalid or expired password? --- -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c new file mode 100644 index 882dc8f..fa96238 *** a/src/backend/libpq/auth.c --- b/src/backend/libpq/auth.c *** auth_failed(Port *port, int status) *** 245,251 break; case uaPassword: case uaMD5: ! errstr = gettext_noop(password authentication failed for user \%s\); /* We use it to indicate if a .pgpass password failed. */ errcode_return = ERRCODE_INVALID_PASSWORD; break; --- 245,251 break; case uaPassword: case uaMD5: ! errstr = gettext_noop(password authentication failed for user \%s\: invalid or expired password); /* We use it to indicate if a .pgpass password failed. */ errcode_return = ERRCODE_INVALID_PASSWORD; break; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Change authentication error message (patch)
Bruce Momjian br...@momjian.us writes: I have developed the attached patch to fix this problem. Do I need to say invalid user or invalid or expired password? I'm not convinced that this improves anything. The problem might not in fact be either of the things you mention, in which case the new message is outright misleading. Also, what of the policy stated in the header comment for the function you're hacking, ie we intentionally don't reveal the precise cause of the failure to the client? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Change authentication error message (patch)
On Thu, Jan 23, 2014 at 10:39:34PM -0500, Tom Lane wrote: Bruce Momjian br...@momjian.us writes: I have developed the attached patch to fix this problem. Do I need to say invalid user or invalid or expired password? I'm not convinced that this improves anything. The problem might not in fact be either of the things you mention, in which case the new message is outright misleading. Also, what of the policy stated in the header comment for the function you're hacking, ie we intentionally don't reveal the precise cause of the failure to the client? Well, the only solution then would be to add some weasel words like perhaps expired password, but that seems so rare that I doubt it would apply very often and seems like an odd suggestion. We could go with: password authentication failed for user \%s\: perhaps invalid or expired password We did have two threads on this issue in the past 12 months so I figured we should try to do something. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Change authentication error message (patch)
On 06/20/2013 12:51 AM, Jeff Janes wrote: I think we need to keep the first password. Password authentication is a single thing, it is the authentication method attempted. It is the password method (which includes MD5) which failed, as opposed to the LDAP method or the Peer method or one of the other methods. That's against the rule of not revealing any more knowledge than a potential attacker already has, no? For that reason, I'd rather go with just authentication failed. Without this level of explicitness, it might be hard to figure out which row in pg_hba.conf was the one that PostgreSQL glommed onto to use for authentication. As argued before, that should go into the logs for diagnosis by the sysadmin, but should not be revealed to an attacker. Regards Markus Wanner -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Change authentication error message (patch)
On 20/06/2013 08:47, Markus Wanner wrote: On 06/20/2013 12:51 AM, Jeff Janes wrote: I think we need to keep the first password. Password authentication is a single thing, it is the authentication method attempted. It is the password method (which includes MD5) which failed, as opposed to the LDAP method or the Peer method or one of the other methods. That's against the rule of not revealing any more knowledge than a potential attacker already has, no? For that reason, I'd rather go with just authentication failed. My understanding is that the attacker would already have that information since the server would have sent an AuthenticationMD5Password message to get to the error in the first place. And we still reveal the authentication method to the frontend in all other cases (peer authentication failed, for example). Without this level of explicitness, it might be hard to figure out which row in pg_hba.conf was the one that PostgreSQL glommed onto to use for authentication. As argued before, that should go into the logs for diagnosis by the sysadmin, but should not be revealed to an attacker. Isn't the point of this patch exactly that we didn't want to go down that road? I.e. password authentication failed didn't say that the password might've expired, but some people thought just logging a WARNING/LOG wasn't enough. Regards, Marko Tiikkaja -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Change authentication error message (patch)
On 06/20/2013 12:27 PM, Marko Tiikkaja wrote: My understanding is that the attacker would already have that information since the server would have sent an AuthenticationMD5Password message to get to the error in the first place. And we still reveal the authentication method to the frontend in all other cases (peer authentication failed, for example). Oh, right, I wasn't aware of that. Never mind, then. +1 for keeping it mention password authentication explicitly. However, thinking about this a bit more: Other authentication methods may also provide password (or even account) expiration times. And may fail to authenticate a user for entirely different reasons. Given that, I wonder if password expired is such a special case worth mentioning in case of the password auth method. If we go down that path, don't we also have to include auth server unreachable as a possible cause for authentication failure for methods that use an external server? Regards Markus Wanner -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Change authentication error message (patch)
On 06/18/2013 02:25 AM, Markus Wanner wrote: On 06/16/2013 06:02 PM, Joshua D. Drake wrote: Instead of pushing extra info to the logs I decided that we could without giving away extra details per policy. I wrote the error message in a way that tells the most obvious problems, without admitting to any of them. Please see attached: +1 for solving this with a bit of word-smithing. However, the proposed wording doesn't sound like a full sentence to my ears, because a password or username cannot fail per-se. I believe it actually can. The error message that is returned for a bad password, bad user or expired password is all the same. Which is why I put the username in there. How about: password authentication failed or account expired for user \%s\ It's a bit longer, but sounds more like a full sentence, no? Yes but I don't think it is accurate, what about: Authentication failed or password has expired for user \%s\ Authentication failed covers any combination of a username/password being wrong and obviously password expired covers the other. Sincerely, Joshua D. Drake Regards Markus Wanner -- Command Prompt, Inc. - http://www.commandprompt.com/ 509-416-6579 PostgreSQL Support, Training, Professional Services and Development High Availability, Oracle Conversion, Postgres-XC, @cmdpromptinc For my dreams of your image that blossoms a rose in the deeps of my heart. - W.B. Yeats -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Change authentication error message (patch)
This probably is nit-picking, but it interests me in terms of how the language is used and understood. On 06/19/2013 08:55 PM, Joshua D. Drake wrote: I believe it actually can. The error message that is returned for a bad password, bad user or expired password is all the same. Which is why I put the username in there. Sure, the authentication can fail for all these reasons. What I stumbled over was the formulation of a failed username. If an engine fails, it might literally fall apart. The username itself - even if it doesn't pass authentication - is not falling apart in the same sense. But does the username (or the password) fail if authentication with it (in combination with password and account expiration time) is not possible? After all, it might still a valid and complete username for another cluster or another service. You can probably say: that username failed when you actually mean it failed to authenticate together with the provided password. Or how do English native speakers perceive this? Authentication failed or password has expired for user \%s\ Authentication failed covers any combination of a username/password being wrong and obviously password expired covers the other. Works for me. Considering the password to be the thing that expires (rather than the account) is probably more accurate as well. Regards Markus Wanner -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Change authentication error message (patch)
On 06/19/2013 01:18 PM, Markus Wanner wrote: Authentication failed or password has expired for user \%s\ Authentication failed covers any combination of a username/password being wrong and obviously password expired covers the other. Works for me. Considering the password to be the thing that expires (rather than the account) is probably more accurate as well. It is also how it is worded in the docs (which is why I used it). Patch below. JD diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index 415b614..f129fe1 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -270,7 +270,7 @@ auth_failed(Port *port, int status) break; case uaPassword: case uaMD5: - errstr = gettext_noop(password authentication failed for user \%s\); + errstr = gettext_noop(Authentication failed or password has expired for user \%s\); /* We use it to indicate if a .pgpass password failed. */ errcode_return = ERRCODE_INVALID_PASSWORD; break; -- Command Prompt, Inc. - http://www.commandprompt.com/ 509-416-6579 PostgreSQL Support, Training, Professional Services and Development High Availability, Oracle Conversion, Postgres-XC, @cmdpromptinc For my dreams of your image that blossoms a rose in the deeps of my heart. - W.B. Yeats -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Change authentication error message (patch)
On Wed, Jun 19, 2013 at 11:55 AM, Joshua D. Drake j...@commandprompt.comwrote: On 06/18/2013 02:25 AM, Markus Wanner wrote: On 06/16/2013 06:02 PM, Joshua D. Drake wrote: How about: password authentication failed or account expired for user \%s\ It's a bit longer, but sounds more like a full sentence, no? Yes but I don't think it is accurate, what about: Authentication failed or password has expired for user \%s\ I think we need to keep the first password. Password authentication is a single thing, it is the authentication method attempted. It is the password method (which includes MD5) which failed, as opposed to the LDAP method or the Peer method or one of the other methods. Without this level of explicitness, it might be hard to figure out which row in pg_hba.conf was the one that PostgreSQL glommed onto to use for authentication. (Although by this argument, I don't know why MD5 doesn't get its own message specific to it, rather than sharing plain password) Cheers, Jeff
Re: [HACKERS] Change authentication error message (patch)
On 06/16/2013 06:02 PM, Joshua D. Drake wrote: Instead of pushing extra info to the logs I decided that we could without giving away extra details per policy. I wrote the error message in a way that tells the most obvious problems, without admitting to any of them. Please see attached: +1 for solving this with a bit of word-smithing. However, the proposed wording doesn't sound like a full sentence to my ears, because a password or username cannot fail per-se. How about: password authentication failed or account expired for user \%s\ It's a bit longer, but sounds more like a full sentence, no? Regards Markus Wanner -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Change authentication error message (patch)
Hello, Instead of pushing extra info to the logs I decided that we could without giving away extra details per policy. I wrote the error message in a way that tells the most obvious problems, without admitting to any of them. Please see attached: diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index 415b614..a775534 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -270,7 +270,7 @@ auth_failed(Port *port, int status) break; case uaPassword: case uaMD5: - errstr = gettext_noop(password authentication failed for user \%s\); + errstr = gettext_noop(password, username or password expiry failed for user \%s\); /* We use it to indicate if a .pgpass password failed. */ errcode_return = ERRCODE_INVALID_PASSWORD; break; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
