GenerateTokenHash is highly questionable

[Manouchehri]

https://github.com/supabase/gotrue/blob/577e3207aab8ee4c4661f5a8148f02296210f1d8/internal/crypto/crypto.go#L41-L43

This function gives the false security to developers that there's 224-bits of entropy when they see the TokenHash in an email.

e.g. when I saw TokenHash in an email, I thought 3f4cef3ebadfa21c8a4559e025966e5806dcc6081d3cf657444c0054 definitely couldn't be brute forced, so it would be safe to have a high OTP token lifetime.

Then... I realized that if you know the email address...

dave@mbp yep % printf "gotrue@testing.email.ai.moda131337" | sha224sum
3f4cef3ebadfa21c8a4559e025966e5806dcc6081d3cf657444c0054  -

And there you have it, what I thought was 224-bits of entropy, is actually less than 20-bits.

[Manouchehri]

Ah, found our internal ticket on this from a few months ago. Here's the URL to brute-force:

curl -v "https://your_instance_id_here.supabase.co/auth/v1/verify?token=calculated_hash_here&type=magiclink&redirect_to=http://127.0.0.1:31337"

You will hit rate limiting. I personally don't feel comfortable with rate limiting being the primary layer of defense, the OTP token should not be <20-bits to begin with.

[kangmingtay]

Hi @Manouchehri, thanks for raising this concern up! There is a tradeoff between security and UX here that we had to make. To provide more context, this should also support email and phone OTPs which require the user to enter a 6-10 digit OTP. Obviously, it would be pretty annoying to have your users enter a hash manually (yes they can copy and paste but we have had customers who reported that their users view the OTP on a separate device / don't know how to copy and paste the OTP).

We like the idea of having a defence in-depth solution, which is also why we allow you to customise the length of the OTP for a slightly higher entropy. However, we don't want it to compromise the UX too. I don't think that these are mutually exclusive choices and I think we can definitely improve on this solution by separating the logic for generating the OTPs and the token hash embedded in the email links. That way, developers who are developing more security-sensitive applications can opt to use the email link over the OTP. At this point, we feel that rate limiting protection is pretty sufficient considering that the OTPs expire after 24hrs too.

Given our existing rate limits and an attacker who attempts to brute force the OTP, they would have a less than 1% chance of doing so successfully: (360 req / hr) * (24 hrs) / (1000000 possible OTPs).

[Manouchehri]

I'm not sure why I understand the point of hashing at all here?

[kangmingtay]

@Manouchehri you don't want to have the raw values stored in the database - this is akin to hashing a user's password

[Manouchehri]

With the way you're doing the hash, it's pointless; it can always be cracked in seconds.

[kangmingtay]

@Manouchehri i think the assumption you're making here is that the attacker has access to the database system already, at which point, there are more severe issues at hand than compromising a single user's account.

i definitely do see where you're coming from, and one of the benefits of an open-source community is that we can leverage different expertise to make the product better. It would be nice if you also supplemented your points with potential solutions too, whilst noting the tradeoffs i mentioned above.

We also have a security policy here and if you do find any other potential vulnerabilities, we would really appreciate it if you communicate them through that channel.

[Manouchehri]

I'm not sure I see how this is similar to hashing a password; you hash a password so the plaintext version is difficult to recover. This is more like obfuscation, but like I mentioned at the beginning, it ends up being obfuscation for the developer, not to an attacker.

with potential solutions

Change TokenHash to be a uniquely random (i.e. not a hash of known value).

From a quick glance, it looks like you're more or less using TokenHash like a code in a Authorization Code Flow.

we would really appreciate it if you communicate them through that channel.

As you correctly pointed out in #1245 (comment), this is hopefully mitigated by rate limiting. (I am slightly concerned that the rate limiting is per-IP and not per target user.)

I don't think this is really a security bug. There is no increased risk in using TokenHash over Token; the problem is that TokenHash is basically still Token, but gives developers the false impression that it's something else.

[hf]

@Manouchehri Thanks for the issue. I think the discussion is getting a bit sidetracked, so could I please ask you to provide a list of short steps outlining an "attack" scenario so we can focus on identifying the severity and class of this issue.

If you feel uncomfortable doing it in public, you can always use security@supabase.com to responsibly disclose security issues. This is outlined in our security.txt file.

As for the following:

the problem is that TokenHash is basically still Token, but gives developers the false impression that it's something else.

I don't fully agree with you about the impression bit, but I do see the point in that better docs or possibly name should have been chosen. My disagreement comes from the fact that the word hash in this case means hash value from a one-way (non-password) hashing function. It's not wise to expect everyone to have a deep knowledge of hash functions, but it's generally widely known that hash functions don't change the "entropy" of the hashed value. Therefore, if TokenHash is important to you, in the context of the email message, then it's safe to assume that it's coming from the Token value.

We'll try to plan for a change in the Supabase Dashboard and docs that this makes this more prominent, but we do also accept community contributions if you have some ideas!

[Manouchehri]

Using the word "hash" is any security related context (like logging in) is fairly going to be assumed to be a cryptographic hash. The comment at #1245 (comment) seems to be saying that this hashing was incorrectly intended to provide a security benefit.

[Manouchehri]

To simplify the discussion, can you clarify these two questions:

1. What is the purpose of TokenHash?

2. What is the intended reason to use TokenHash over Token?

[hf]

Yes, hashing provides a security benefit. There are multiple layers of security here, entropy is just one aspect of the issue.

Inside GoTrue / the database, token hash provides these layers of security:

1. Prevents read-only users to the database from impersonating real users.
2. Prevents large-scale account takeover attacks by binding the email address with the OTP code, so that a separate dictionary of hashes of the OTP code must be used per account, not just one set of hashes for all possible OTP codes. This also makes it more difficult to use rainbow tables techniques to "reverse" the hash function and guess the OTP code.

(Again, the method of "attack" in the above scenario is an attack from within, such as: disgruntled employee / staff member, partial or total data leak.)

Outside of GoTrue, when sent out in an email:

1. Does not have a different security benefit than just using the Token, both can be used interchangably depending on the application's use case (as pointed out by @kangmingtay).

This is why I'm not super clear what is questionable in the function, or the hashing, and would like to see an attack outline before we can have a reasonable discussion about the security of the system.

What is the intended reason to use TokenHash over Token?

When you use Token, you need to build a UI that takes in the email in addition to the OTP value -- as this is the only way that GoTrue can verify the token (since it holds a hash of those two values).

This does not fit well with certain applications, such as those who intend to use Server-Side Rendering, where the "token verification" has to be done on the backend instead (especially with PKCE). By allowing the use of TokenHash, we give applications the ability (which is not by default) to build out those flows as well.

[kangmingtay]

closing this issue since it's more of a discussion rather than a bug with gotrue

[Manouchehri]

The solution is to remove GenerateTokenHash.

I think you meant to close this as won’t fix, not complete?

[tomekit]

I am with @Manouchehri on this.

In my opinion if you really aim to "build the best Auth solution" not just shiny play tool you need to focus on security pretty seriously.

Valid points were raised. There are couple possible resolutions here and likely longer discussion is required, but closing it doesn't solves anything and only discourages people from taking their time to report potential security issues.

[kangmingtay]

@tomekit @Manouchehri we will fix this by separating the logic for TokenHash and OTP. The TokenHash flow will be generated with much higher entropy and the OTP will continue to be a 6-digit code returned.

@tomekit i agree fully, if you feel that there are any security concerns that should be raised, the team would really appreciate making the disclosure through the appropriate channel (https://supabase.com/.well-known/security.txt) .