Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore "end specific session" functionality? #104

Open
Bouke opened this issue Jan 24, 2020 · 7 comments
Open

Restore "end specific session" functionality? #104

Bouke opened this issue Jan 24, 2020 · 7 comments

Comments

@Bouke
Copy link
Collaborator

Bouke commented Jan 24, 2020

In versions before 1.7.1 there was a functionality to end specific sessions. This functionality worked by exposing session keys, which is a security concern. The functionality could be restored, by introducing a different way of identifying sessions between the view and the template. For example adding a field in the database, or encrypting the session key.

@dwasyl
Copy link

dwasyl commented Jan 27, 2020

@Bouke It would be great to restore this functionality as it seems like a handy function. Is it possible to show the current session without exposing session keys?

That said on my sites, I have often added a custom view/button let users "delete all but the current session" which seems to be the functionality I see most often on other sites. Security wise I decided that was a reasonable compromise to avoid displaying session keys (other than the current one).

@dwasyl
Copy link

dwasyl commented May 17, 2021

What about adding a uuid slug to each Session that could be used for a web link instead of exposing the session key? Or would an encrypted session key be simpler?

@blag
Copy link
Contributor

blag commented Oct 15, 2023

Proposal using asymmetric encryption

In the list sessions view, encrypt each session key for the user, use that in the value of the form used to delete a specific session.

In the SessionDeleteView, iterate through all of a user's sessions, encrypting each session key and comparing it to the (encrypted) session key in the request. Once a match is made, delete that session and stop iteration.

Good:

  • Client never has a cleartext session ID (besides their current one, obviously).
  • Server doesn't have to touch provided (encrypted) session key from client, so lower chance of something going wrong

Bad:

  • A lot of database traffic to encrypt each session key for the session list
  • A lot of database traffic to find the single session to delete
  • DOS vector if a user has a lot of sessions, or provides a fake encrypted session key (because the server will have to search through all of the users' session objects to ensure the provided one doesn't exist)

Proposal using symmetric encryption

In the list sessions view, encrypt-and-sign each session key for the user, use that in the value of the form used to delete a specific session.

In the SessionDeleteView, verify signed session key from the request, decrypt the (encrypted) session key from the request, query the user's sessions for that session key, and delete the session object.

Good:

  • Client never has a cleartext session ID (besides their current one, obviously).
  • Server doesn't have to iterate through multiple session objects to find the one it wants to delete, so very little database traffic and no DOS attack vector

Bad:

  • A lot of database traffic to encrypt each session key for the session list
  • Server has to carefully handle the submitted signed-and-encrypted session key

Thoughts?

@WhyNotHugo
Copy link
Member

Aren't session_key themselves signed with the SECRET_KEY?

We could expose the unsigned session_keys to views and template. When a deletion request is received,we just need to sign the selected session and we have the original session_key again, right?

@blag
Copy link
Contributor

blag commented Oct 16, 2023

Session data is signed, but session keys are not AFAIK.

But either way, we should not expose cleartext session keys other than the request's current one even if they are signed.

@dwasyl
Copy link

dwasyl commented Oct 16, 2023

Just in the interest of keeping things simple, is there a particular security issue created by just having a secondary key that we use for deleting session entries?

It isn't useful as a session key, doesn't have encryption we wouldn't want displayed, and just enables deletion.

@blag
Copy link
Contributor

blag commented Oct 22, 2023

Not a security issue, no. But I would be worried about a performance issue.

There are at least as many session objects stored as their are currently connected users (CCUs) for a site.

I considered temporary indexes (eg: offsets, not DB indexes) - sort a user's sessions, display them in a list, and then submit the display index of a user's session to delete it. But this leads to concurrency issues with listing a users sessions, one or more sessions being added or expiring, and then processing a delete request. The wrong session could be deleted.

I don't know that a secondary display ID column in the database is a great move here.

If we use an auto-incrementing column, it becomes more difficult to scale horizontally. PostgreSQL users would have one more centralized sequence they need to manage. The nice thing about using Django's _get_new_session_key function (see here) is that each ID is entirely random and guaranteed to be unique. That means there's no centralized authority to hand out session IDs - every Django server can do it independently.

Using UUIDs (thinking UUIDv4 here) is complicated, and there's a lot of FUD about how well they can be indexed. The only database that natively supports UUIDs is PostgreSQL, all others will store UUIDs as 32-character strings. So we'd have an index on a 32-character primary key column from Django and another index on a 32-character display ID column. That is far from ideal.

None of that is impossible, and this project doesn't necessarily have to aim for the broadest userbase, but...is all of that really worth this one feature of being able to selectively end other sessions? I kinda don't think so.

That's how I arrived at doing some cryptography to blind users to their other session IDs. And to be perfectly clear, I'm not even sure if this one feature is worth that complexity. Users already have a big red "destruct all other sessions" button. Is the ability to kill of specific sessions worth that much more than that?

Maybe it might be worth moving the functionality to delete other sessions to something like a contrib app (possibly including an extra package dependency group) of this project? That way users who want it can easily enable it, and nobody else has to deal with the extra complexity.

Edit: Clarification

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants