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

feat: add hashed refresh tokens #663

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

hf
Copy link
Contributor

@hf hf commented Sep 5, 2022

Today refresh tokens are stored raw in the database. Should the database be stolen, leaked or misused by an external or internal party, it would be nearly trivial to issue a valid access token for any user.

This PR stores the refresh tokens (the token column in refresh_tokens) as a SHA256/224 hash. It is thus impossible to impersonate a user using this value, but it is possible to verify that the user has a valid token.

For backward compatibility purposes, a token lookup occurs using both the hashed and raw value of the token; but all new tokens will be using the hashed and secure form. To prevent hashed tokens being used instead of real tokens, they are stored with a H: prefix in the database, and backward-compatible lookups are done only after stripping the same prefix before hashing.

Furthermore, the old behavior with the reuse window of refresh tokens is now cleaned up. A new refresh token is generated if a revoked refresh token is issued within the revocation period. (This only happens if the browser / client has not synced up the refresh tokens they use for requests -- in a multi-tab or concurrent request environment.)

@hf hf requested a review from a team as a code owner September 5, 2022 16:25
Copy link
Member

@kangmingtay kangmingtay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm so we're just hashing the token in the refresh_tokens table? what happens when the user tries to use the refresh token in /token?grant_type=refresh_token ?

will we hash the refresh_token sent by the user to check if it matches the hashed_token in the db?

how do we also determine whether the refresh_token is an existing one (no corresponding hash in the db) or a newly issued one (has a corresponding hash in the db)?

@hf
Copy link
Contributor Author

hf commented Sep 7, 2022

hmm so we're just hashing the token in the refresh_tokens table? what happens when the user tries to use the refresh token in /token?grant_type=refresh_token ?

It's hashed and then looked up in the database.

func FindUserWithRefreshToken(tx *storage.Connection, token string) (*User, *RefreshToken, error) {
	hashedToken := crypto.HashSHA224Base64(token)

	refreshToken := &RefreshToken{}
	if err := tx.Where("token = ? OR token = ?", hashedToken, token).First(refreshToken); err != nil {

For backward compatibility the non-hashed version is also looked up. Note that since these strings are random and 128 bits or more, it is impossible to have the same value appearing in hashed or unhashed.

will we hash the refresh_token sent by the user to check if it matches the hashed_token in the db?

(Yes.)

how do we also determine whether the refresh_token is an existing one (no corresponding hash in the db) or a newly issued one (has a corresponding hash in the db)?

Well there is an interesting property. If you know the original value of the token (i.e. what the user has) and you successfully find it in the database, then you have access to both the hashed and original values. Thus, it does not matter if a token is one of the old ones or a new one. If you really needed to distinguish, you can always compare Token and HashedToken. If they're the same, it was an old refresh token.

@hf hf force-pushed the hf/add-hashed-refresh-tokens branch from 6dadf79 to 091725b Compare September 7, 2022 09:04
if err != nil {
return internalServerError("Error validating reuse interval").WithInternalError(err)
}
if time.Now().Before(refreshTokenReuseWindow) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully, this logic can be tested with a scenario I've encountered on the client side with supabase-js v2.
More details can be found here: https://github.com/supabase/supabase/issues/8959

Here's the gist, there's a multiTab situation, where the user's session is destroyed because there's a refresh timer conflict. Hopefully, the details provided give a better understanding of the problem

@pixtron
Copy link

pixtron commented Sep 18, 2022

For backward compatibility the non-hashed version is also looked up

@hf Couldn't an attacker just send the hashed token (from DB) as the non-hashed token (in Request) and mint new tokens with it?

@hf
Copy link
Contributor Author

hf commented Sep 18, 2022

For backward compatibility the non-hashed version is also looked up

@hf Couldn't an attacker just send the hashed token (from DB) as the non-hashed token (in Request) and mint new tokens with it?

Yes, that is a good catch (tho only valid in the backward compatibility period)... it's an easy fix though if the hashed tokens are stored with some prefix in the database. For example, H:<hash> then the lookup would fail because let's say your scenario happens:

  1. Attacker steals hashed value h = hash(r)
  2. GoTrue receives h and looks up token = h OR token = 'H:' + hash(h).

Since it's practically impossible to have an already existing token with the value of h and hash(h) can't ever be r, the attack wouldn't work.

I'll amend the code to include the prefix.


Obviously this only works if any H: prefix is stripped before doing the lookup.


Fixed and added test cases for it.

@hf hf force-pushed the hf/add-hashed-refresh-tokens branch from 091725b to d28a53d Compare September 18, 2022 14:36
@hf hf force-pushed the hf/add-hashed-refresh-tokens branch from d28a53d to d1605c0 Compare September 18, 2022 14:46
@hf hf requested a review from a team September 18, 2022 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants