prevent session fixation for external authentication #908

Merged
merged 1 commit into from Sep 18, 2015

Projects

None yet

3 participants

@mrjoel
Collaborator
mrjoel commented Aug 20, 2015

I found that there is an existing session fixation issue when using external (non-standard as defined by Constants.AuthenticationType.isStandard()).

The issue only currently affects deployments using container and certificate based authentication, when two users are using the same browser session which results in the same session cookie being passed. When the external authentication token changes, the UserModel in the session is never updated to reflect the new user, and the new external user ends up using the first user's session, along with accompanying rights.

I initially tried to address this by updating the UserModel in the session from AuthenticationManager processing of authenticate(username, password). However, since that is called by the filter chain processing, it does not have access to the wicket session, since it's not yet bound to a page (exception below, line numbers may differ from local changes in testing). I'm not very familiar with Wicket, if there's a mechanism to do this I couldn't find it.

java.lang.IllegalStateException: you can only locate or create sessions in the context of a request cycle
at org.apache.wicket.Session.findOrCreate(Session.java:209)
at org.apache.wicket.Session.get(Session.java:253)
at com.gitblit.wicket.GitBlitWebSession.get(GitBlitWebSession.java:160)
at com.gitblit.manager.AuthenticationManager.authenticate(AuthenticationManager.java:351)
at com.gitblit.manager.AuthenticationManager.authenticate(AuthenticationManager.java:193)
at com.gitblit.servlet.EnforceAuthenticationFilter.doFilter(EnforceAuthenticationFilter.java:87)

Even if the above were possible with Wicket, there is still an issue with using an inherently stale session attribute for determining whether a user is logged in and who that user is. As part of this fix, I've instead migrated to use a request attribute for the authentication, since that is cleared and starts fresh for processing of every request. Upon successful authentication, the filter mechanism sets the updated UserModel into the userManager, and sets the request attribute. SessionPage then looks at the request attribute and retrieve the corresponding UserModel from the userManager. If external authentication is not done, it falls back to trusting the existing session. The new request attribute asserts the username that has been authenticated in a less error-prone way (since it's not persisted between individual requests).

There may be a better way to assert from a filter to a page that authentication has successfully been performed and for what user, but the request attribute works for now. If there's another more wicket-friendly way to do it I'm fine with that too.

@mrjoel Joel Johnson prevent session fixation for external authentication
  + use request instead of session to flag authentication status
    and user, for external authentication types
62e0259
@gitblit
Owner
gitblit commented Aug 20, 2015

when two users are using the same browser session

How do I reproduce this scenario?

@mrjoel
Collaborator
mrjoel commented Aug 20, 2015

It is definitely a corner case since multiple accounts in a single browser session (the cookie is a session scoped cookie) can be uncommon, however when using team development or other use cases, logging out from one user and logging in as another user with both present causes this to occur. For the issue to occur, requires two different users with valid credentials to login using the same browser session.

There is currently no relation enforced between the username validated in AuthenticationManager.java (session flag), and the session id provided, so when a session id is sent, it is trusted even if the external username has changes. While gitblit invalidates a session when using local authentication in the logout mechanism, gitblit [rightly] doesn't offer a logout link for external auth, and can't do anything about clearing a session on an external authentication change since it's unaware of it. It does, however, need to check each request to ensure that the authenticated username does in fact match the session id provided, and discard the session if not.

I discovered it when creating a new HTTP header authentication module (#273) which exposes it a bit more easily. It can be reproduced without that as well, however. The easiest way to reproduce it in the existing state is using certificate authentication, and using two client certs on Firefox, unchecking the box to remember the certificate selection for the site. Login first with one user's certificate, and then login with the other user's certificate and notice that the username, menu, and associated rights continue to be reflecting the first user, even though the second user is now logged in.

@gitblit gitblit merged commit 0322351 into gitblit:develop Sep 18, 2015
@wangliyu
wangliyu commented Oct 2, 2015

Please revert this commit, the change cause issue when relogin the different user, to reproduce the problem, you have logout the current user and try relogin with different user, it will still remain the previous user.

@gitblit
Owner
gitblit commented Oct 2, 2015

This was reported in #936. I don't observe the behavior you describe. Please give more detail on how to reproduce the issue.

@mrjoel mrjoel deleted the mrjoel:mrjoel-authrequestnotsession branch Dec 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment