elog(FATAL)ing non-existent roles during client authentication
Hi all,
I noticed that during client authentication by HBA, some times we will
necessarily determine whether or not a role exists. For example, password,
crypt and md5 auth methods call get_role_line() which tells the caller
whether the role exists. If it doesn't (or if the authentication fails due
to a password mismatch) we error out.
I wonder if we should check if the role exists for the other
authentication methods too? get_role_line() should be very cheap and it
would prevent unnecessary authentication work if we did it before
contacting, for example, the client ident server. Even with trust, it
would save work because otherwise we do not check if the user exists until
InitializeSessionUserId(), at which time we're set up our proc entry etc.
This might seem overly pessimistic, I know, but it seemed to me that a
malicious user on a local network might be able to tie up a system in
interesting ways by launching lots of connections without necessarily
knowing any usernames/passwords.
Thanks,
Gavin
Gavin Sherry <swm@linuxworld.com.au> writes:
I wonder if we should check if the role exists for the other
authentication methods too? get_role_line() should be very cheap and it
would prevent unnecessary authentication work if we did it before
contacting, for example, the client ident server. Even with trust, it
would save work because otherwise we do not check if the user exists until
InitializeSessionUserId(), at which time we're set up our proc entry etc.
This only saves work if the supplied ID is in fact invalid, which one
would surely think isn't the normal case; otherwise it costs more.
I could see doing this in the ident path, because contacting a remote
ident server is certainly expensive on both sides. I doubt it's a good
idea in the trust case.
regards, tom lane
On Thu, 30 Nov 2006, Tom Lane wrote:
Gavin Sherry <swm@linuxworld.com.au> writes:
I wonder if we should check if the role exists for the other
authentication methods too? get_role_line() should be very cheap and it
would prevent unnecessary authentication work if we did it before
contacting, for example, the client ident server. Even with trust, it
would save work because otherwise we do not check if the user exists until
InitializeSessionUserId(), at which time we're set up our proc entry etc.This only saves work if the supplied ID is in fact invalid, which one
would surely think isn't the normal case; otherwise it costs more.
Yes.
I could see doing this in the ident path, because contacting a remote
ident server is certainly expensive on both sides. I doubt it's a good
idea in the trust case.
Agreed. How about Kerberos too, applying the same logic?
Gavin
On Tue, 5 Dec 2006, Gavin Sherry wrote:
On Thu, 30 Nov 2006, Tom Lane wrote:
Gavin Sherry <swm@linuxworld.com.au> writes:
I wonder if we should check if the role exists for the other
authentication methods too? get_role_line() should be very cheap and it
would prevent unnecessary authentication work if we did it before
contacting, for example, the client ident server. Even with trust, it
would save work because otherwise we do not check if the user exists until
InitializeSessionUserId(), at which time we're set up our proc entry etc.This only saves work if the supplied ID is in fact invalid, which one
would surely think isn't the normal case; otherwise it costs more.Yes.
I could see doing this in the ident path, because contacting a remote
ident server is certainly expensive on both sides. I doubt it's a good
idea in the trust case.Agreed. How about Kerberos too, applying the same logic?
Attached is a patch check adds the checks.
Gavin
Attachments:
auth_check_role.difftext/plain; charset=US-ASCII; name=auth_check_role.diffDownload
Index: src/backend/libpq/auth.c
===================================================================
RCS file: /usr/local/cvsroot/pgsql/src/backend/libpq/auth.c,v
retrieving revision 1.146
diff -c -p -r1.146 auth.c
*** src/backend/libpq/auth.c 6 Nov 2006 01:27:52 -0000 1.146
--- src/backend/libpq/auth.c 4 Dec 2006 13:47:05 -0000
*************** pg_krb5_recvauth(Port *port)
*** 216,221 ****
--- 217,225 ----
krb5_ticket *ticket;
char *kusername;
+ if (get_role_line(port->user_name) == NULL)
+ return STATUS_ERROR;
+
ret = pg_krb5_init();
if (ret != STATUS_OK)
return ret;
Index: src/backend/libpq/hba.c
===================================================================
RCS file: /usr/local/cvsroot/pgsql/src/backend/libpq/hba.c,v
retrieving revision 1.157
diff -c -p -r1.157 hba.c
*** src/backend/libpq/hba.c 5 Nov 2006 22:42:08 -0000 1.157
--- src/backend/libpq/hba.c 4 Dec 2006 13:47:05 -0000
*************** authident(hbaPort *port)
*** 1589,1594 ****
--- 1589,1597 ----
{
char ident_user[IDENT_USERNAME_MAX + 1];
+ if (get_role_line(port->user_name) == NULL)
+ return STATUS_ERROR;
+
switch (port->raddr.addr.ss_family)
{
case AF_INET:
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.
---------------------------------------------------------------------------
Gavin Sherry wrote:
On Tue, 5 Dec 2006, Gavin Sherry wrote:
On Thu, 30 Nov 2006, Tom Lane wrote:
Gavin Sherry <swm@linuxworld.com.au> writes:
I wonder if we should check if the role exists for the other
authentication methods too? get_role_line() should be very cheap and it
would prevent unnecessary authentication work if we did it before
contacting, for example, the client ident server. Even with trust, it
would save work because otherwise we do not check if the user exists until
InitializeSessionUserId(), at which time we're set up our proc entry etc.This only saves work if the supplied ID is in fact invalid, which one
would surely think isn't the normal case; otherwise it costs more.Yes.
I could see doing this in the ident path, because contacting a remote
ident server is certainly expensive on both sides. I doubt it's a good
idea in the trust case.Agreed. How about Kerberos too, applying the same logic?
Attached is a patch check adds the checks.
Gavin
Content-Description:
[ Attachment, skipping... ]
---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match
--
Bruce Momjian bruce@momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +
Patch applied. Thanks.
---------------------------------------------------------------------------
Gavin Sherry wrote:
On Tue, 5 Dec 2006, Gavin Sherry wrote:
On Thu, 30 Nov 2006, Tom Lane wrote:
Gavin Sherry <swm@linuxworld.com.au> writes:
I wonder if we should check if the role exists for the other
authentication methods too? get_role_line() should be very cheap and it
would prevent unnecessary authentication work if we did it before
contacting, for example, the client ident server. Even with trust, it
would save work because otherwise we do not check if the user exists until
InitializeSessionUserId(), at which time we're set up our proc entry etc.This only saves work if the supplied ID is in fact invalid, which one
would surely think isn't the normal case; otherwise it costs more.Yes.
I could see doing this in the ident path, because contacting a remote
ident server is certainly expensive on both sides. I doubt it's a good
idea in the trust case.Agreed. How about Kerberos too, applying the same logic?
Attached is a patch check adds the checks.
Gavin
Content-Description:
[ Attachment, skipping... ]
---------------------------(end of broadcast)---------------------------
TIP 9: In versions below 8.0, the planner will ignore your desire to
choose an index scan if your joining column's datatypes do not
match
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +