Add session methods to handle destruction and regeneration

[twolfson]

We have been using flask-session for our sessions and noticed that there were no easy methods on flask or flask-session to handle session destruction/regeneration. These are practical as they aid in preventing session fixation attacks (e.g. when someone knows your session id and it doesn't get changed on login so they can reuse your session):

https://www.owasp.org/index.php/Session_fixation

We saw that flask-kvsession has some logic for this:

https://github.com/mbr/flask-kvsession/blob/0.6.2/flask_kvsession/__init__.py#L89-L124

but we were already invested into flask-session (added sha256 signing support) that we decided to implement similar logic there as well:

pallets-eco/flask-session#27

However, I'm not sure that's the ideal interface. I think the final product would look something more like

flask.session.destroy()
flask.session.regenerate()

which in turn call flask.session_interface.destroy_session(self, app, session) and flask.session_interface.regenerate_session(self, app, session).

What are your thoughts on this?

[davidism]

This seems to be specific to session interfaces that need a session id, and those two examples you gave seem to be handling it already. What do we gain by adding this to Flask? The default session doesn't use a session id, and can be wiped with session.clear().

[twolfson]

Ah, good point. I think the benefit of implementing it in Flask (even as NotImplementedError) would lead to more consistent APIs for sessions (e.g. everyone's already using session_interface and session.clear()). It feels like session.destroy and session.regenerate would be a natural extension.

On a side note, session.clear and session.destroy are similar but not the same. clear erases the contents of the session but still persists a cookie. destroy empties session contents, remove it from the store (if there is a non-cookie one), and delete the cookie entirely.

[davidism]

Removing the cookie doesn't actually seem to be part of the security, only clearing the data or changing the id matter. The cookie is removed if the dict is empty, so the subclass could just override clear to also clear the server side storage. Regenerate just becomes "change the value of the id key", or "remove the id, and let it be generated when save_session doesn't find it". I guess that could use a standard name. I'm not sure about encouraging this though, it seems more secure to clear the session when logging in, and make sure anything that needs to persist for a user is persisted to the database, not the cookie.

[twolfson]

In a session id context, regenerate does more than change the value of the id key. It also corresponding session data in the store.

https://github.com/underdogio/flask-session/blob/52b3d16149d01db2642f04f9164a0b71789ba4a5/flask_session/sessions.py#L133-L142

We perform this so that if the session id and its HMAC are known by an attacker, then they cannot access anything about the old session as well (e.g. maybe there's a tracking id in it).

• Attacker somehow knows visitor's session id (either by setting or snooping)
• Visitor has session id ABCD
• Upon login, visitor gets new session id GHIJ and we destroy old store data under ABCD
• Attacker tries to make request with header Cookie: session=ABCD -- has no access to past session information before login
• Visitor continues to use GHIJ session id until it expires or logout

[twolfson]

I feel like I should mention why I am opposed to using cookie based sessions (i.e. storing session as a cookie JSON blob).

• It leaves sessions in user's control (e.g. I can't wipe invalidate a session without the user visiting the site)
• If a user's session is compromised, we can't perform anything like a regeneration since typically user-specific data (e.g. user id) would be stored there. We could change server salt but that is a huge change for a single user session compromise

[davidism]

OK, thanks for the detailed report. I don't feel that I understand the issue enough to make a change. If you'd be willing to create a PR including a short summary in the docs and a test case, I'd be happy to review it.

I still vaguely feel like there's better solutions that don't require changes, such as basing the hash for a session id on a secret that can be changed for each user, or tracking a list of valid ids for each user, but I can't think of any strong arguments against this.

[twolfson]

Ah, damn =( Please re-review the OWASP page -- it should clarify the topic some more:

https://www.owasp.org/index.php/Session_fixation

I will create a PR in a bit. I guess I am so adamant about this because a framework should be forward thinking for users -- similar to how we have app.secret_key (e.g. a novice should feel safe/secure on all fronts).

With respect to changing the session id hash, people could use things like an IP address as part of the salt. But the generic salt will likely do the trick. Usually these compromises occur less from guessing via brute force (since rate limiting would block that) and more from somehow setting the session id (e.g. via a compromised subdomain which allows setting a .domain.com cookie).

[twolfson]

Started on a PR and I see a few approaches but none seem appealing.

• Update SessionMixin signature to accept app as a parameter during __init__
  • This would allow us save self.app on the session and later have a destroy method call self.app.destroy_session(self)
    • In turn app.destroy_session calls self.session_interface.destroy_session(self, session)
  • session.destroy() would be how we invoke a destroy
• Update SessionMixin signature to accept session_interface in its __init__
  • Not possible since we need to pass app to session_interface so it has the proper config attrs
• Define only app.destroy_session(session)
  • Invokes self.session_interface.destroy_session(self, session)
  • Works but seems verbose and not session oriented

[ThiefMaster]

Why not use current_app if you need access to the app from within the session class?

[twolfson]

It might be a strawman but I think there's a reason that Flask has app as a parameter in open_session/save_session. My guess is to allow for multiple apps running side by side.

[ThiefMaster]

True, maybe it can run outside an app context (or it's from the time where there was no app context but just a request context).

Anyway, self.app = ... in SessionMixin seems dangerous. You might want to json-encode or pickle the session object and both would fail with a reference to the app (pickle might actually work, but then result in a mess when unpickling some old app object)

[twolfson]

Ah, good point. JSON serialization seems fine but pickle is polluted.

# Output from `python` REPL
>>> class MyDict(dict):
...   def __init__(self):
...     self.foo = 'bar'
...     super(dict, self).__init__()
... 
>>> 
>>> x = MyDict()
>>> import json
>>> json.dumps(x)
'{}'
>>> x['hello'] = 'world'
>>> json.dumps(x)
'{"hello": "world"}'
>>> import pickle
"ccopy_reg\n_reconstructor\np0\n(c__main__\nMyDict\np1\nc__builtin__\ndict\np2\n(dp3\nS'hello'\np4\nS'world'\np5\nstp6\nRp7\n(dp8\nS'foo'\np9\nS'bar'\np10\nsb."

I guess that forces us to always use:

app.regenerate_session(session)
app.destroy_session(session)

[twolfson]

@davidism I have opened up #1603 as requested