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

Rememberable race condition causing users to be incorrectly logged out #376

Open
JonRCahill opened this issue Sep 12, 2018 · 4 comments
Open

Comments

@JonRCahill
Copy link
Contributor

I believe I have found a race condition in the rememberable feature.

I have a pretty straightforward page which has links to 36 images on it. These images are not links to static images but are instead routed through the Phoenix itself (where I look up the image and return it). So my links look something like:

/for/123/image
/for/124/image

The routes for these links are piped through the browser pipeline, set up with the Coherence.Authentication.Session as specified in the docs so everything is being authenticated through Coherence.

When I log into my system for the first time everything is fine, selecting the remember me checkbox. When I close the browser and open the site again I appear to be logged in (the menus show logged in) but I am in fact logged out and if I refresh the page it shows I am logged out (menus show logged out).

It appears that the page request is working correctly and keeping me logged in. However, when processing one of the 36 image requests it is logging me out, I can see in the logs I get an "Invalid token. Potential Fraud." warning.

I believe what is happening is when requesting the images Coherence is validating the series_hash and token_hash to ensure it is valid, then updates these before returning the request. If a second image request starts before the first image request completes then it won't have the updated series_hash and token_hash and when validating it will fail, logging the user out as fraudulent.

I have worked around this for the moment by making another pipeline which doesn't include Coherence.Authentication.Session and piping my images only through this pipeline.

I believe you would also be able to have this same problem is someone refreshed multiple windows/tabs in their browser at the same time as well.

I think there are a couple of issues at the heart of the problem, the first being that the validating of series_hash/token_hash and updating them is not an atomic action. Second being the series_hash/token_hash is being changed when there could be another request being processed which will now have the incorrect keys.

At the moment I don't know where to get started on resolving this issue as I don't know enough detail on how the rememberable feature is implemented.

When I get a chance, I will try and put together an example repo which replicates this issue.

@nmbrone
Copy link

nmbrone commented Sep 12, 2018

We have the same issue. After home page loads two widgets on it initiates two requests to the API endpoints which are piped through Coherence.Authentication.Session plug. Next happens the same as described above.

@smpallen99
Copy link
Owner

I have seen similar issues in one of my apps but have never taken the time to investigate. Thanks for raising this issue!!

My first thought on a fix is to serialize the requests through a GenServer (Or Agent), ensuring that only one can be done at a time. I'll see if I can come up with a fix today.

@smpallen99
Copy link
Owner

I just reviewed the code. First, the validation and generation of a new rememberable token IS an atomic action. It is done through the RememberableServer GenServer. Its a synchronous operation (call), but I don't think that's an important detail.

So, I wonder if the issue is that the client is making simultaneous async requests so they are all landing with the same session. We had a similar issue with AJAX requests.

For the AJAX case, we added code to bypass the token validation for AJAX requests.

I'm at a bit of loss on how to solve the issue if we are getting async requests from the server. Some thoughts are:

  • add a timer (say a couple seconds) to skip the token validation of subsequent requests in the timer period. We should however, update the token so later requests will pass (bit of a hack).
  • add a new insecure_rememberable configuration option to bypass the series/token algorithm (not really solving the issue and exposing a security hole.

@BobaFaux and @nmbrone what are your thoughts??

@mbosco
Copy link

mbosco commented Nov 14, 2018

I also ran into a similar issue, where users were sporadically being logged off and seeing the invalid token message. After experimenting a bit, I was able to replicate the bug.

When the initial session cookie expires or is deleted, authentication is handled by the coherence cookie, which causes the token to be reset. However, a new session cookie is never set, and the credential store is never updated when logging in with the coherence cookie. I don't know if this is a bug or by design, but it means that the token is reset with every request. If two requests are made simultaneously, the first will update the token, and the second will cause the user to be logged out because it is still using the old token.

Making it so a new session cookie is set doesn't fix the race condition, but it does reduce the likelihood of a race condition happening because the token is reset much less often (and it doesn't require a database hit on every request). Fixing this so far seems to have fixed the issue for me.

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