OAuth state query parameter can't be connected to a given user - state re-use (CSRF)
I found this while working on a different ~security issue and adding a CSRF nonce query parameter to a separate endpoint, /login/upgrade
We use TokenStateProvider to generate and verify a state token used in the GitHub OAuth process,
https://github.com/login/oauth/authorize?response_type=code&redirect_uri=http%3A%2F%2Flocalhost%3A5000%2Flogin%2Fcallback&scope=user%3Aemail%2Cread%3Aorg&state=xxx&client_id=xxxhttp://localhost:5000/login/callback?code=xxx&state=xxx
With TokenStateProvider, the state is made from the current time and a random salt (it doesn't matter that the string is encrypted) which we have no way to actually connect to the user who generated it. I am concerned that this isn't really adding any protection since an attacker can just generate their own state and re-use it in some CSRF (against some other users requests). All this seems to do currently is make sure a request was made within the maxTokenAge time period.
It seems like we need to add some additional information into the string that connects that string to the user later on. Since the user isn't signed in at this point, session makes sense
The alternative SessionStateProvider makes a bit more sense to me because you generate a state, then can verify it later down the road because we can compare what they have in their session to the query parameter
It looks like we moved to TokenStateProvider because we didn't want to rely on req.session. I'm not sure of root cause for switching but the following comment maybe hints at people not making it through the sign in fast enough,
The token state provider is useful if you want to use OAuth state verification without using session state on the client. It's also useful if you users take longer than your session timeout to complete the OAuth login.
I haven't looked into any actual attacks but something just seems a bit fishy.
Am I missing something?
Maintainer spotted this problem a while ago
Just saw this comment on the PR against the original project that comes to the same conclusion,
I've also removed the TokenStateProvider. I don't want the responsibility of maintenance for that code, so if you need it, feel free to package it as a separate module. I'd caution, however, that in its current implementation, it does not cryptographically bind the token to the current session, which leaves open the possibility of XSRF attacks.
@jaredhanson, https://github.com/jaredhanson/passport-oauth2/pull/24#issuecomment-181685311