Proposal: Logout user when they change their password.

[Arnoud van Heuvelen]

Hi,

I recently ran into a minor security issue with Django Auth.
Currently, when a user changes their password, the user will stay
logged in on all open sessions.

This is a problem when a password is compromised. The user will change
their password and be confident that the problem is solved. However,
if the compromised password has already been used to log in on another
browser session there are no changes to that session.

I understand that this could be seen as a responsibility for the
developer building the Django application. However, as far as I know
Django doesn't come with an out-of-the-box 'log out everything'
option. It does come with a change password feature. But with the
current implementation this feature is near-useless when the password
has already been used to log in by a malicious user.

With a default installation, it will not be possible to easily log out
your other sessions. I'm proposing that by default, Django Auth (Or at
least the admin system.) should log out all sessions, except the one
the user is currently changing the password in.

Thoughts?

Arnoud

[Karthik Abinav]

Hi,     

          Yes. I agree with Arnoud. I have always felt a need to have such a implementation with the default installation. I feel the auth system should provide an in-built logout all sessions feature otherwise as mentioned can comprimise heavily on the security if the developer doesnt take care of it explicitly.

Arnoud

--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To post to this group, send email to django-d...@googlegroups.com.
To unsubscribe from this group, send email to django-develop...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.

Karthik Abinav,

[Florian Apolloner]

And how could we do that? Sessions aren't linked to the user (well they are, but not in a queryable way).

Cheers,
Florian

[Arnoud van Heuvelen]

Well, as far as I see it, we have a number of options. These are some
I could come up with, but maybe someone else has a better idea:

1) Save the user_id with the session. This is probably not convenient,
because it might conflict with other applications that use the session
package. Of course we could give the user_id a default of null, but
that seems a bit sloppy to me. Upside is that we could invalidate
sessions on password change.

2) Build a relation table that links the sessions to the users. (Or
use a CSV in the user object.) This could be done completely in the
auth package. Downside is that we have to create a new table. Upside
is again, that we could invalidate sessions on password change.

3) Save the password hash (or part of it) in the session and compare
it against our data. If the hash is not the same, the user needs to be
logged out. This wouldn't change the database, but the downside is
that this causes overhead on every request.

4) Save the last_password_change_date on the user object and save the
session_creation_time inside the session. Compare these and we know if
we should log the user out. This is just a different approach to 3.

My favourite is option 2 for now. It seems the most resource friendly,
even though we need to save more data. I don't know what Django's
policy on adding tables is though.

Besides that, I have to admit that I am fairly new to Django. I have
build some small sites with it, but have only skimmed through some
parts of the source code so far. I am not completely familiar with the
inner workings of the framework.

Arnoud

[Jeremy Dunck]

On Sun, Jan 8, 2012 at 2:57 PM, Arnoud van Heuvelen
<avanhe...@gmail.com> wrote:
...

> 3) Save the password hash (or part of it) in the session and compare
> it against our data. If the hash is not the same, the user needs to be
> logged out. This wouldn't change the database, but the downside is
> that this causes overhead on every request.

+1.

1) We need a way to handle existing sessions when upgrading to the new
Django w/ this support. I think the most natural support for the
validation is
auth.get_user:
https://code.djangoproject.com/browser/django/trunk/django/contrib/auth/__init__.py?rev=16539#L94
to check the hash before returning.

2) I don't think the user should be logged out of their own session
when changing the password, and so we'd need to update the active
session on its way back to the user.

This is messier - we'd need to detect when the password changed
(User.set_password called?) and update the related request session, if
any. That may be best accomplished through a signal fired when the
password is changed and handled in session.

[Gabriel Hurley]

Options 2 and 4 from that list both involve database-level changes, and thus aren't feasible (our lack of schema migration tools being the biggest problem).

Of those, I'd go for option 3 as well. We definitely don't want to store the full hash in the session for obvious security reasons, but a small portion of the hash is probably enough to do the checking, be secure and provide a high degree of confidence that collisions would be unlikely.

I'll leave it to PaulM or someone else better versed in hashing to comment on what the appropriate subset might be, or if that's just totally off base.

Lastly, I'll add that it'd really be pushing it to get this into 1.4 at this point. I, personally, would be willing to allow it on the basis of it being a security concern, but we'd need to have a really solid patch for it in the next week or so to have time to review it, test it, etc. Once we release the beta it's definitely not making it into 1.4.

All the best,

    - Gabriel

[Carl Meyer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/08/2012 03:57 PM, Arnoud van Heuvelen wrote:
[snip]

> 2) Build a relation table that links the sessions to the users. (Or
> use a CSV in the user object.) This could be done completely in the
> auth package. Downside is that we have to create a new table. Upside
> is again, that we could invalidate sessions on password change.

[snip]

> My favourite is option 2 for now. It seems the most resource friendly,
> even though we need to save more data. I don't know what Django's
> policy on adding tables is though.

This is not an option because the database session backend is only one
of many session backends.

Carl
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk8LD1IACgkQ8W4rlRKtE2eHmgCbBx/7FlOHfi32Cr1pNWa4mKsW
Ca0An1rB+T9fwFwdlHzEmwj/PxTxIAd3
=Mj2V
-----END PGP SIGNATURE-----

[Carl Meyer]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 01/09/2012 09:01 AM, Carl Meyer wrote:
> This is not an option because the database session backend is only one
> of many session backends.

Oh, never mind - I misunderstood the proposal. This wouldn't be
dependent on using the database session backend if its done entirely in
the auth app and based on the session key. It would, however, require a
database check on every session access, meaning the value of using a
faster session backend would be lost. Thus I still think it's not
feasible, and (if anything is to be done here) a solution like #3 that
stores extra data in the session itself is preferable.

Carl
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk8LEGQACgkQ8W4rlRKtE2faiwCfUhQg9MouUnkGpYWec7WI1VJL
C7UAnjgEIiAgruPSPnlEZfucyRWmxbV3
=Jpx9
-----END PGP SIGNATURE-----

[Sergiy Kuzmenko]

I'm not sure that invalidating session based on last password change is the right thing to do. If the password has been compromised, this effectively enables an active attacker to deny access to the legitimate user. In case of Django admin site this can be quite disruptive as there is no password recovery option by default. And if superuser password has been stolen, it takes only few clicks to create another superuser account or to grant someone superuser privileges. Password change seems to be a rather weak defense in this case.

Session invalidation based on password change would only be effective is someone is passively spying using a compromised password.

Cheers
Sergiy

[Arnoud van Heuvelen]

Op dinsdag 10 januari 2012 03:56:04 UTC+1 schreef Sergiy het volgende:

I'm not sure that invalidating session based on last password change is the right thing to do. If the password has been compromised, this effectively enables an active attacker to deny access to the legitimate user. In case of Django admin site this can be quite disruptive as there is no password recovery option by default. And if superuser password has been stolen, it takes only few clicks to create another superuser account or to grant someone superuser privileges. Password change seems to be a rather weak defense in this case.

 

I see your point. However, if a malicious user would change the password, at least the original user would be logged out and not be able to log in again. Now, that user knows something is going on and can notify a server admin who can reset the password using the admin system or manage.py.

With the current logic, the password might get changed by a malicious user and the original user will not notice this until they log out. It is the same problem, but this makes it a lot harder to trace what happened since we don't know when the password was last changed.

I agree that this is a somewhat weak defense, but I think that it's more effective than the current logic. The plus side is that it would be fairly easy to implement. 

 

Session invalidation based on password change would only be effective is someone is passively spying using a compromised password.

This is not entirely true. The implementation would be a huge help when a user forgets to log out on a public computer. It would at least give us some control over the sessions being in use. Besides decoding all the sessions in the database and deleting the ones that are tied to our account.

I agree that it would be better to have a more feature-rich session management system that allows us to invalidate sessions based on user, IP, log in date etc. But that would be a much bigger change to the code-base and almost certainly involve some database changes.

[Tom Evans]

On Tue, Jan 10, 2012 at 4:18 PM, Arnoud van Heuvelen
<avanhe...@gmail.com> wrote:
> I see your point. However, if a malicious user would change the password, at
> least the original user would be logged out and not be able to log in again.
> Now, that user knows something is going on and can notify a server admin who
> can reset the password using the admin system or manage.py.
>
> With the current logic, the password might get changed by a malicious user
> and the original user will not notice this until they log out. It is the
> same problem, but this makes it a lot harder to trace what happened since we
> don't know when the password was last changed.
>
> I agree that this is a somewhat weak defense, but I think that it's more
> effective than the current logic. The plus side is that it would be fairly
> easy to implement.
>
>>
>> Session invalidation based on password change would only be effective is
>> someone is passively spying using a compromised password.
>
>
> This is not entirely true. The implementation would be a huge help when a
> user forgets to log out on a public computer. It would at least give us some
> control over the sessions being in use. Besides decoding all the sessions in
> the database and deleting the ones that are tied to our account.
>
> I agree that it would be better to have a more feature-rich session
> management system that allows us to invalidate sessions based on user, IP,
> log in date etc. But that would be a much bigger change to the code-base and
> almost certainly involve some database changes.
>

This is achievable with very little work, and no changes to core. We
implemented this feature for different reasons, but I see no reason
why you cannot make it do what you wanted it to do.

Basically:

Use a database session backend.

Define a new model for holding session references:

class SessionAudit(models.Model):
user = models.ForeignKey(User)
session = models.ForeignKey(django.contrib.sessions.models.Session)
ip_address = models.IPAddressField()
user_agent = models.TextField()
modified = models.DateTimeField(auto_now=True,)
created = models.DateTimeField(auto_now_add=True,)

Hook into django.contrib.auth.signals.{user_logged_in,user_logged_out}
and {create,remove} SessionAudit objects appropriately.

You now have, linked to the user, a list of their active sessions. If
you need to log out all but the current session, you simply need to
destroy the appropriate Sessions, as found from the SessionAudit
model.

As a final bit of clean up, when you destroy your sessions, you should
also destroy the related SessionAudit.

If you want to talk more about this approach, I think we are firmly in
django-users@ ground now, as this is simple BI.

Cheers

Tom