Refresh token validation appears broken

[IvanAnishchuk]

There are two things that I noticed around validate_refresh_token:

1. It ignores REFRESH_TOKEN_EXPIRE_SECONDS entirely, regardless of whether it's set refresh tokens seem to work until the end of time (unless revoked or rotated).
2. It always checks revoked timestamp against REFRESH_TOKEN_GRACE_PERIOD_SECONDS which is valid logic when tokens are revoked by rotation but not so much in case of voluntary revocation (logging out).
3. clear_expired has two ways of determining which tokens to remove and both of them seem incorrect: revoked__lt=refresh_expire_at -- revoked is set to now on revocation or rotation but clear_expired won't get them until after additional EXPIRE_SECONDS, access_token__expires__lt=refresh_expire_at -- lets them sit in the database for EXPIRE_SECONDS after access token expiration, not since the issue time (also if clear_expired is called often enough it's possible that it might remove the expired access token before it considers deleting the corresponding refresh token and the condition would never be met, leaving the refresh token in the db forever and causing other trouble).

My proposal:

1. Add an additional filter to check that refresh token wasn't created before now() - REFRESH_TOKEN_EXPIRE_SECONDS (possibly - REFRESH_TOKEN_GRACE_PERIOD_SECONDS as well, not sure). Alternatively, see (4)
2. On token rotation, instead of setting revoked = now() set it to now() + GRACE_PERIOD (and remove grace period subtraction everywhere, always compare revoked with now) -- this way tokens revoked voluntarily would be revoked immediately but rotated tokens would continue to live until the grace period. Probably by adding an optional argument to the revoke() method (either timedelta/seconds to add or just boolean so that value is fetched from settings instead).
3. Remove expired refresh tokens based on their created time, not (only) the others.
4. Possibly use RefreshToken.revoked the same way AccessToken.expires is used: if REFRESH_TOKEN_EXPIRE_SECONDS is set, set revoked = now + EXPIRE_SECONDS on issuing a new token, to simplify the following validation. It would've looked better if the field was called expires though, but renaming is probably not worth it now. This might be superior to checking anything against created because this way it would be possible to extend expiration time if desired (e.g. on renewal) without having to break the audit trail automagic created timestamp provides.
5. Possibly add a separate settings variable (and possibly an overriding command argument) for configuring clear_expired: instead of removing everything that was expired or using expire_seconds settings it can only remove tokens that were expired for longer than a given period (could be a day or even a year, depending on the application's load, we could also try to set some sensible default). it would probably be a good idea not to remove access tokens if they still have a valid refresh token linked to them (required for determining scopes and some other things on renewal). Also, it might make sense to allow introspecting expired tokens, but that's probably not a priority, can't think of a use case for that right now.

Thoughts?

[ryanpetrello]

I'm able to reproduce this in another open source project I maintain which uses DOT, and have added code in our project to work around it and properly validate REFRESH_TOKEN_EXPIRE_SECONDS when users attempt to use refresh tokens.

https://github.com/ansible/awx/issues/6630

@synasius @jleclanche this seems like a pretty severe security vulnerability in DOT. Any thoughts?

[cristian-home]

I'm able to reproduce this in another open source project I maintain which uses DOT, and have added code in our project to work around it and properly validate REFRESH_TOKEN_EXPIRE_SECONDS when users attempt to use refresh tokens.

ansible/awx#6630

@synasius @jleclanche this seems like a pretty severe security vulnerability in DOT. Any thoughts?

I agree, I use DOT to authenticate against a Rest API, and I have a SPA to which I must pass the access_token and the refresh_token, I pass the refresh_token through an httponly cookie and set the expiration time on it. But for the other scenarios where cookies are not used, such as a mobile app or server-to-server communication, the refresh_token will never expire.