Protect against Session Fixation and session data leakage when crossing privilege boundaries

[dstufft]

This will invalidate the session in the SessionAuthenticationPolicy().remember(), ensuring that all the data is copied over to the new session. This will ensure that for server side sessions which have a session ID, a new session ID is granted when going from an unauthenticated user to an authenticated user. For client side sessions this will simply do a little extra work but ultimately be a no-op.

Fixes #1569

[mmerickel]

I think we may wish to backport this to 1.5-branch as well. I'm +1 on this PR and will merge it after it has baked another day or so for anyone else to comment.

[dstufft]

A more comprehensive change, but one with two possible backwards incompatibilities for users would be changing this to:

    def remember(self, request, userid, **kw):
        """ Store a userid in the session."""

        # To protect against session fixation attacks we want to ensure that
        # we have a new session identifier (where applicable) when remembering
        # a user authentication.
        if (self.userid_key in request.session
                and request.session[self.userid_key] != userid):
            # If we already have an authenticated user, we're going to just
            # invalidate the session without copying data. We'll do this
            # because going authenticated -> authenticated might leak sensitive
            # information and we don't want to re-use another user's session.
            request.session.invalidate()
        else:
            # We don't already have an authenticated user, or if we do it's
            # the same user we're trying to remember. Since this is either
            # already this user's session or we're going from
            # unauthenticated -> authenticated we'll just copy the data from
            # the session, invalidate it to create a new session, and restore
            # the data.
            data = request.session.copy()
            request.session.invalidate()
            request.session.update(data)

        # Actually store the user in the session to remember which user has
        # been authenticated.
        request.session[self.userid_key] = userid

        return []

    def forget(self, request):
        """ Remove the stored userid from the session."""

        # When crossing an authentication boundry we want to create a new
        # session identifier. We don't want to keep any information in the
        # session when going from authenticated to unauthenticated because
        # user's generally expect that logging out is a desctructive action
        # that erases all of their private data. However if we don't clear the
        # session then another user can use the computer after them, log in to
        # their account, and then gain access to anything sensitive stored in
        # the session for the original user.
        request.session.invalidate()

        return []

This will make sure that:

1. unauthenticated -> authenticated keeps session data but will still invalidate the session to get a new session identifier.
2. authenticated -> authenticated with the same userid keeps session data but will still invalidate the session to get a new session identifier.
3. authenticated -> authenticated with a different userid will throw away all of the session data and invalidate to get a new session identifier.
4. authenticated -> unauthenticated will throw away all of the session data and invalidate to get a new session identifier.

1 and 2 are more or less backwards compatible as long as no one is relying on retaining the same session identifier across remember() calls. However 3 and 4 are backwards incompatible if someone is relying on keeping data in the session when crossing those authentication boundaries. I do not believe that those are likely to be very major backwards incompatibilities though but I may be wrong.

If you'd prefer to get the more complete fix I can update this PR to do that, or I can open a new PR.

[digitalresistor]

ISession extends IDict and neither has an explicit requirement that they have a .copy() function. It also looks like IDict was originally only intended for renderers, looking at the docstring on IDict.update(d).

[dstufft]

Good catch, I can switch it to using data = dict(request.session.items()).

[dstufft]

Updated with the more comprehensive changes above, and to fix the reliance on a method that IDict doesn't promise that @bertjwregeer noticed.

[digitalresistor]

I am a +1 on this PR as is. From a security standpoint, I would +1 your extended fix. It might be backwards incompatible, but if anyone is relying on that I want to know what apps they are writing ;-)

[mmerickel]

I thought you'd be implementing this more generally in pyramid.security.remember and pyramid.security.forget. Did you intentionally want to only add these security fixes if using the SessionAuthenticationPolicy? Since session != auth in pyramid, I'm thinking I'd prefer a more general fix where pyramid.security.remember checks if authenticated_userid(request) == userid so that it works in cases as well.

[dstufft]

I implemented it in SessionAuthenticationPolicy because originally the attack I was worried about is really only relevant if you're storing your authentication inside of the session. IOW if you're using a different cookie to store your authentication information, then someone fixating your session doesn't really matter because that session isn't going to get the ability to log in as you.

On the other hand now that this pull request also covers attempting to stop data leakage via the session that can occur when crossing privilege boundaries you're right in that it really is more general than just session fixation and it would be appropriate to move it out of the SessionAuthenticationPolicy and into the top level API itself.

[digitalresistor]

You can't have the top-level API's doing anything with a session because there is no guarantee that the user has registered a session or is using one. Also, in my apps I have the authentication be a separate cookie from the session, so these protections are not required.

[dstufft]

I'm not sure it's not required. You can still leak data between authenticated and unauthenticated if you're storing any data in the session.

[digitalresistor]

Sure, you can, it all depends on what type of data you store on the session. I don't agree with unilaterally clearing out the session in the authentication API's. I would find it a very hard pill to swallow that my session would get cleared upon remember/forget when I am not using an authentication policy that uses session to store it's information.

[digitalresistor]

Ugh, I just realised that if we don't add it to the authentication API's essentially anyone writing an authentication policy would have to write the logic themselves. I can see the merit...

I withdraw my previous complaints, and would +1 such a change. If a session policy is enabled, let's go ahead and clear it.

[dstufft]

What's the best way to determine if a session policy is enabled? Should I just try to access request.session?

[mgrbyte]

@dstufft ~~The way I would do it would be:

from pyramid.interfaces import ISessionFactory
session_factory = request.registry.queryUtility(ISessionFactory)

session_factory will be None if not configured (and accessing request.session will raise AttributeError)~~

Whilst the above is correct, it's overly complex. and not necessary.
Just checking request.session w/hasattr should be fine.
Sorry for the noise.

[dstufft]

Ok, I've reworked this so that it's implemented inside of pyramid.security.remember() and pyramid.security.forget() instead of being specific to SessionAuthenticationPolicy.

[digitalresistor]

LGTM!

[sontek]

Can you add the PR link here?

[dstufft]

Added!

[sontek]

ship it!

[mmerickel]

LGTM

[dstufft]

Just following up on whether this is still waiting for more eyes or if it can be merged.

[mmerickel]

The reason I can't merge this atm is a hairy one. The remember and forget apis do not currently have any side-effects. They return headers that you may (or may not) add to a response (which one? dunno). We almost changed this in 1.5 such that request.remember() would affect request.add_response_callback and @mcdonc decided it wasn't gonna happen. If we had then this would be a no-brainer but right now it directly conflicts with that explicit decision made during the 1.5 cycle.

[dstufft]

Makes sense. Technically though they have whatever side effects the AuthenticationPolicy has, which for the session backed authentication policy means modifying the session and returning an empty list for the headers.

[mgrbyte]

I think this Could patch could implement in pyramid.authentication.SesssionAuthenticatioPolicy's remember and forget instead of pyramid.security?

[dstufft]

It was originally, if you read up through the comments you can see the rationale for moving out of there and to the high level functions.

[mgrbyte]

@dstufft doh, my bad 👓

[mmerickel]

Certainly this is worthwhile to add to the SessionAuthenticationPolicy either way but this part of the auth api I think is unfortunate and should be solvable going forward. I think I have a way that is bw-compatible.

Update pyramid.security.remember to the following signature pyramid.security.remember(request, userid, response=None, **kwargs). We could then document that passing the response will mutate it directly. I think this would actually solve some subtle bugs in the way people currently use remember as well by requiring you to have a response object first. Traditionally examples show something like HTTPFound(..., headers=remember()) which overwrites some headers you may not expect. With this API you could do things like

remember(request, 'raydeo', response=request.response)
# or
response = HTTPSeeOther(...)
remember(request, 'raydeo', response=response)
return response

I like this but it may be lipstick on a pig or missing some other apis like this.

[dstufft]

So the idea then is that you'd gate this particular change on pyramid.security.remember() getting the response kwarg? I think the biggest thing with that is it means that people have to remember to pass an argument in order to protect against session fixation and leakage. We could implement the protection in both the high level APIs and in SessionAuthenticationPolicy which would mean that if you're using the SessionAuthenticationPolicy you're always protected, and if you're using the response kwarg to the high level APIs you're protected, so then you'd only not be protected if you're using sessions, not using SessionAuthenticationPolicy, and not passing a response object to the top level APIs.

That's obviously a gap that the current PR doesn't have in what's protected, however I don't know the Pyramid ecosystem well enough to know if that is a significant gap and how bad the bw-compat break would be as this is currently written.

[tilgovi]

Is it the case that these protections are universally applicable? Is this mitigating the impact of session fixation? What if, say, I run my application on SSL with HSTS and secure cookies? Of course, there are always going to be possible vulnerabilities, but is it the responsibility of the auth system to worry about session fixation?

I wonder if this isn't driven by the need of a particular application to have more paranoid security but lacking hooks into some auth module. Maybe subscribers for remember and forget events would be useful?

I may be completely off base, but I just thought I'd toss out some questions that might help reason through this.

[digitalresistor]

I recommended adding some events to remember/forget, however @mmerickel did not seem particularly enthusiastic about that idea when I proposed this to him on IRC.

As for adding the hooks, while you might not be able to add them directly on the remember/forget, you can of course add the code to your custom auth policy.

[dstufft]

The answer to that question is a little complex.

They are not universally applicable if you consider situations where there is no session configured, however this PR is effectively a no-op if you don't have sessions enabled so that case doesn't really matter. So the question then becomes, are these universally applicable if you're using a session? For that the answer is more or less yes unless you're using sessions but not storing user specific data in them.

The attacks this protects against are not particular effected by TLS or secure cookies. You can set a cookie via a HTTP connection (even secure cookies!) and the app hosted via TLS won't know the difference. It can be mitigated if your entire site (aka the root domain and all subdomains using the includeSubdomains flag) is protected by HSTS, however the protection isn't complete since HSTS relies on either getting pre-loaded or the user having visited your site within the max-age.

This isn't really a "paranoid" security change, this is a fairly basic protection that protects against one the OWASP top 10 attacks (It was #2 on the 2013 list), it's the same sort of protection as something like having auto escaping turned on by default in templates. I can easily fix this in my own application, but it's something that I believe Pyramid should really do on it's own. It's a fiddly security bit that super easy to get wrong if you remember to do it at all. I've written this kind of code before and it took me roughly an hour to sit there and convince myself that I had the right security properties once I had already written the code, this sort of thing is exactly the kind of thing that I think a framework should provide, fiddly hard to get right code that most everyone is going to need.

To be clear too, the impact of merging this is:

Common Cases:

• When a user changes from unauthenticated to authenticated (the common sign in case) they will get a new session but with all of the existing data inside it. This should have no practical impact on anyone unless they were depending on session IDs or something else to stay the same that isn't contained inside the session data.
• When a user changes from authenticated to unauthenticated (the sign out case) they will get a new session without any existing data inside it. This could possibly impact someone if they were relying on the session data to travel when moving to a logged out state, however I do not believe this is likely to be a major use case, most users I think would expect logging out to clear any personal data they have saved in the session.

Uncommon Cases:

• When a user changes from authenticated to authenticated and it's the same user, this is treated the same as going from unautheticated to authenticated, they get a new session with all of the data copied over, same impact.
• When a user changes from authenticated to authenticated and it's a different user, this is treated the same as authenticated to unauthenticated and then back to authenticated (essentially), so you get a new session ID but all of the data is gone from it.

I personally feel like the negative impact of this are fairly small corner cases, and the positive impact is closing up a flaw that affects most web applications and can trivially be used to steal someone's user account.

[dstufft]

Sorry, slightly wrong above, in the first uncommon case, authenticated to authenticated with the same user id there is no new session ID, this change is a strict no-op in that case.

[tilgovi]

Thank you so much for the detailed response @dstufft. I defintely learned some things.

I also just took a moment to convince myself that invalidate() should, and at least one implementation in the wild (pyramid_redis_sessions) does so, actually ensure we have a new session identifier and that doing update after invalidate will work. I was concerned that, e.g., an invalidate call might set a response callback that removes the cookie without checking whether there was a subsequent update.

👍 from me!

[tilgovi]

Do you think it might be wise to call session.changed() after the update on the off chance that, say, some implementation doesn't actually bother to clear the session (expecting to just delete cookie on egress) and is tricked into thinking the session hasn't changed? Probably not, since invalidate should clear the session, but I'm crossing all the t's and dotting all the i's.

[dstufft]

I don't care much if we call session.changed() except that the ISession documentation says that invalidate() should clear the session and create a brand new one, and modifying it should trigger a new session, so for any session interface that correctly implements the interface it should work, but I don't think there's a harm in calling that method extraneously.

[tilgovi]

Fine to leave it out. No further comments. Thanks for being so patient.

[almet]

Are we waiting on anything specific to merge this?

[mmerickel]

Yeah my comments about the design of the API are the main blocker if you look at some of the previous comments. I didn't write the API but it's clear it's intended to have no side-effects from the way it is implemented and the way the auth policies work (their values are not cached, etc). This PR is adding a side-effect and thus not in the spirit of the API.

We may be able to justify it though in terms of the security benefits and the fact that the side-effects are idempotent (you could call remember multiple times and the session would be affected in a stable way). However that's not true because if you called remember with a different userid each time then the order of the remember calls matters. I realize calling remember multiple times in one request is probably something no one does, but as far as API design it's a wrinkle that matters.

[dstufft]

We're waiting on @mcdonc to make a decision about that right?

[mmerickel]

His API design prowess would be much appreciated. :-) He gave a talk on API design at pycon, so you know he's good.

[mcdonc]

I'm going to say -1 to making remember() or forget() have side effects, sorry. I'd suggest a couple of things:

• hang a method off the request named something like clone_session() which does what the PR does.
• I'd change docs to recommend calling clone_session any time it's necessary, and define what necessary means in the docs by example (lots of good info in this pull request to steal from).

[dstufft]

@mcdonc What about moving it back into the SessionAuthenticationPolicy and having the side effects occur there? (which already has side effects since it operates on the session directly and doesn't return values).

[dstufft]

The concern is that detecting the absence of the "make me safe" function is hard to notice, so if there's a place we can have it on by default, that would be ideal.

[mcdonc]

I think maybe we should do a combination of those two things:

• Make it easy to clone the session by hanging a method off the request
• Have the SessionAuthenticationPolicy call that method in the appropriate place.

Then if someone happens to be using sessions but not the SessionAuthenticationPolicy and they do need to invalidate the session at boundaries, they can do it easily.

[dstufft]

Makes sense!

Would we just document that you should do something like this:

if (request.unauthenticated_userid is not None
        and request.unauthenticated_userid != userid):
    request.session.invalidate()
else:
    request.clone_session()  # Maybe this should be on session itself? IDK

Or would we have clone_session handle the check for the already logged in user being a different user and doing an invalidate instead of a copying of the session data?

I suppose we'd also want to document that you should explicitly invalidate the session when you log out the user too if you're not using the SessionAuthenticationPolicy.

[mcdonc]

Maybe there are two methods added to the request:

• clone_session() (better name requested) which does the work of invalidating and
  reconstituting the session so that it gets reserialized.
• some_other_method_name(userid) which compares the userid passed in to the unauthenticated userid, and calls clone_session() if they are not the same

Then the SessionAuthenticationPolicy calls some_other_method_name(userid)

[mcdonc]

Er, have that backwards, but you know what I mean.

[mcdonc]

FWIW, the reason I say that these methods should be added to the request and not to the session is because we'd need to chang the ISession interface to include the methods, and it would break any existing session impls (e.g. pyramid_redis_sessions, etc)

[mcdonc]

And FWIW, if these methods are called when a session factory is not configured, I wouldn't try to protect the caller from seeing an exception. I'd just let the normal session-retrieval machinery raise whatever exception it already does.

[mcdonc]

In a stunning reversal of opinion in IRC, we decided that we would not merge this PR at all.

Instead, we'll add some docs about OWASP best practices to the cookbook or possibly to a "hardening" section of the Pyramid docs (see #1591 (comment))