2:33 p.m.
Had some spare time today and wanted to fix this issue.
The attached patch instruments nss and pam clients to use a pthread
mutex to prevent multiple threads from stomping on each other.
The patch is quite simple and basic testing seem to show no issues.
It may be worth of back-porting to older versions.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
3:46 a.m.
On Sat, Nov 20, 2010 at 03:33:31PM -0500, Simo Sorce wrote:
Had some spare time today and wanted to fix this issue.
The attached patch instruments nss and pam clients to use a pthread
mutex to prevent multiple threads from stomping on each other.
The patch is quite simple and basic testing seem to show no issues.
It may be worth of back-porting to older versions.
I think the mutexes cover too much code here, especially for PAM. The
PAM client does not use any global variables except the file handle to
to PAM responder of sssd. I would be enough to just protect the I/O to
sssd with the mutex to avoid other threads getting the response of a
request from a different thread. I here I would suggest that we reduce
the socket timeout considerably to avoid long delays if a request gets
stuck in sssd. We might be able to avoid mutexes here completely by
introducing request ids, but this can be seen as an enhancement for a
future version.
Protecting a whole PAM task like authentication with a mutex can lead
to DOS situation when a user logs into a threaded application with his
username but "forgets" to enter his password immediately because the
phone is ringing.
For NSS it might also be sufficient to protect the I/O to sssd.
Netgroups do not use a global variable to save the context and for
passwd and groups I think the global variables are not really protected.
E.g. while one thread is running a loop with getpwent() calls a second
thread can sneak in between in the calls and call setpwent() and resets
the global variable.
bye,
Sumit
Simo.
--
Simo Sorce * Red Hat, Inc * New York
7:25 a.m.
On Mon, 22 Nov 2010 10:46:02 +0100
Sumit Bose <sbose(a)redhat.com> wrote:
On Sat, Nov 20, 2010 at 03:33:31PM -0500, Simo Sorce wrote:
>
> Had some spare time today and wanted to fix this issue.
>
> The attached patch instruments nss and pam clients to use a pthread
> mutex to prevent multiple threads from stomping on each other.
>
> The patch is quite simple and basic testing seem to show no issues.
> It may be worth of back-porting to older versions.
I think the mutexes cover too much code here, especially for PAM.
I know they cover much, but as a first patch I think it is safer.
We can always better optimize. Remember that for single threaded apps
(or apps that do not use these interfaces from multiple threads)
nothing really changes.
The
PAM client does not use any global variables except the file handle to
to PAM responder of sssd. I would be enough to just protect the I/O to
sssd with the mutex to avoid other threads getting the response of a
request from a different thread. I here I would suggest that we reduce
the socket timeout considerably to avoid long delays if a request gets
stuck in sssd. We might be able to avoid mutexes here completely by
introducing request ids, but this can be seen as an enhancement for a
future version.
No you can't, The mutx is always needed because otherwise you risk
reading half of the reply from one thread and another half from another
breaking communication entirely.
Protecting a whole PAM task like authentication with a mutex can
lead
to DOS situation when a user logs into a threaded application with his
username but "forgets" to enter his password immediately because the
phone is ringing.
Is this better or worse than just breaking if 2 users authenticate at
the same time from 2 separate threads ?
For NSS it might also be sufficient to protect the I/O to sssd.
It might, I would consider it a performance enhancement though.
Netgroups do not use a global variable to save the context and for
passwd and groups I think the global variables are not really
protected. E.g. while one thread is running a loop with getpwent()
calls a second thread can sneak in between in the calls and call
setpwent() and resets the global variable.
setpwent() is mutex protected too. This way if another thread tries to
reset the memory cache, it will have to wait until the getpwent() cache
is done with it. Remember, sss_net_getpwent_data is a global variable
shared between all threads and needs protection.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
8:23 a.m.
On Mon, 22 Nov 2010 10:46:02 +0100
Sumit Bose <sbose(a)redhat.com> wrote:
Protecting a whole PAM task like authentication with a mutex can
lead
to DOS situation when a user logs into a threaded application with his
username but "forgets" to enter his password immediately because the
phone is ringing.
After discussing this point on IRC, moved the mutex to cover only
socket communication for the pam client.
New patch attached.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
10:25 a.m.
On Mon, Nov 22, 2010 at 09:23:01AM -0500, Simo Sorce wrote:
On Mon, 22 Nov 2010 10:46:02 +0100
Sumit Bose <sbose(a)redhat.com> wrote:
> Protecting a whole PAM task like authentication with a mutex can lead
> to DOS situation when a user logs into a threaded application with his
> username but "forgets" to enter his password immediately because the
> phone is ringing.
After discussing this point on IRC, moved the mutex to cover only
socket communication for the pam client.
New patch attached.
ACK
bye,
Sumit
Simo.
--
Simo Sorce * Red Hat, Inc * New York
11:19 a.m.
On Mon, 22 Nov 2010 17:25:38 +0100
Sumit Bose <sbose(a)redhat.com> wrote:
On Mon, Nov 22, 2010 at 09:23:01AM -0500, Simo Sorce wrote:
> On Mon, 22 Nov 2010 10:46:02 +0100
> Sumit Bose <sbose(a)redhat.com> wrote:
>
> > Protecting a whole PAM task like authentication with a mutex can
> > lead to DOS situation when a user logs into a threaded
> > application with his username but "forgets" to enter his password
> > immediately because the phone is ringing.
>
> After discussing this point on IRC, moved the mutex to cover only
> socket communication for the pam client.
>
> New patch attached.
ACK
Pushed to master.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
4903
days inactive
4905
days old
sssd-devel@lists.fedorahosted.org
5 comments
2 participants
participants (2)
-
Simo Sorce -
Sumit Bose