Auth Oddities / Vulnerabilities

[Benjamin-Dobell]

There's a (minor?) security issue and a few oddities I noticed with the way auth is being handled.

OAuth Callback Auth Session Vulnerability

The action in oauth.callback.tsx accepts a form data encoded AuthSession from the client and trusts its contents. Consequently, you can submit a real email with a fake access token etc. and side effects will be triggered.

If an account exists for the email, the server will commit (sign) the bogus auth session data and store it in a cookie used for future requests. If a user doesn't exist for the email, one will be created and we'll also end up with bogus auth session cookie as above.

Whilst not great, this is fairly safe at present since requireAuthSession() is being used on endpoints, and it calls through to verifyAuthSession(), and consequently it'll catch the bad access token. That does assume APIs are calling through to verifyAuthSession(). However, that leads into another concern, which is admittedly more of a query.

Why create our own auth session cookie at all?

The Supabase JS library is already creating its own sb-access-token cookie which contains all the same data. The main difference is it's created/verifiable with the Supabase instance's JWT secret key. Thus, in order to validate the token we need do one of:

1. Perform a round trip to Supabase as verifyAuthSession() does.
2. Grab the JWT secret from the Supabase dashboard and add it as a server-only environment variable. Then we can then validate the JWTs directly on our server without the round trip.

I guess the advantage of our own session cookie is that we can validate it ourselves on the server without needing to grab the Supabase JWT secret and save on a round trip i.e. we can trust its contents. However, we're not doing that, we are performing round trips via requireAuthSession(). Just as well because of the aforementioned OAuth Callback vulnerability. However, it does make the contents of the cookie redundant, since we can't trust the contents without verifying the access token, and in doing so, we're fetching the same data that's found in the cookie anyway.

Email Addresses as Unique Keys

Email addresses are being used as unique keys. This stack doesn't claim to support oauth beyond magic links, so it's fine. However, this approach becomes problematic when integrating social OAuth - which is what I was doing when I discovered all the above. Basically, users can change the email address associated with their OAuth provider. Say for example someone signs up via Github social auth whilst their email is name@example.com. If they proceed to change their primary email on Github to name@better.example.com, when that user returns to a site built on this stack and attempts to authenticate, no account will be found.

Again, the stack doesn't claim to support social auth, so it's by no means a bug. However, that part of the auth design could probably be shored up a bit to make it more robust/extensible.

Thank you

After me ranting about problems, just wanted to say thanks for all the hard work! Despite the above, this is my first time using Remix and this repo is, at the very least, super informative.

[Benjamin-Dobell]

Actually I think this may infact be quite severe. requireAuthSession does not validate the auth session matches the access token. It simply verifies that the access token is a valid access token.

So to impersonate someone else you can submit an auth session with a valid access token for your own account, but with the other user's email.

[rphlmr]

Hello,

First, thanks for your review, I really appreciate it.

The action in oauth.callback.tsx accepts a form data encoded AuthSession from the client and trusts its contents. Consequently, you can submit a real email with a fake access token etc. and side effects will be triggered.

You are absolutely right, I miss the first rule: never trust user input. I'll work on a fix asap, using verifyAuthSession to challenge the access token and compare the session it returns with the session received in formData.

Why create our own auth session cookie at all?

Maybe I'm wrong, but in previous work, we face this issue: https://github.com/mitchelvanbever/remix-auth-supabase/issues/15 Supabase tokens are really huge with some 3rd party providers (Twitch for example). So, to prevent cookie overflow, I choose to pick only what "I" need (the benefit of a Remix Stack is that you can customize It to add or remove what you want in mapAuthSession).

In the end, it seems that I do almost the same thing as Supabase. When looking at the code, it seems that they don't check what's in the request too 🤔

But again, you are right, there is an issue with oauth.callback and I'll work on a fix.

Email Addresses as Unique Keys Funny, It's what happens to me with the Supabase dashboard itself. Even with a new email, I didn't lose my account.

Supabase seems to work differently. In its private table auth.identities the primary key is not the email but an id provided by the social Oauth provider (at least for GitHub). Again, I can be wrong. It's hard to test (don't want to change my email 🤭)

Actually I think this may infact be quite severe. requireAuthSession does not validate the auth session matches the access token. It simply verifies that the access token is a valid access token.

If someone hacks this and set a stolen access token in our secure session cookie, we are roasted but I don't know if we can do something to prevent that. It's a common issue when a user has his token stolen. (and easy in a public wifi)

But I guess you still speak about oauth.callback for this?

Currently, outside of oauth.callback, we can trust requireAuthSession. If secure session cookies are compromised, it'll be a major world problem 😅.

It cost nothing to me to add a double check in requireAuthSession > verifyAuthSession: always be sure that we store/use the email and userId associated with the access token. ==> correction, some use cases doesn't rely on email (phone auth for exemple)

Maybe I'll end to remove email and userId from the session cookie and provide these values on the fly in requireAuthSession. In the end, It's useless to store that because we always call verifyAuthSession that returns supabase user (with email and userId associated with the access token) 🧐 ==> correction, after oauth.callback fix it'll be relatively safe (= we can trust cookie signing by our secret)

PS: Why I don't grab the JWT secret to check the access token is because I think it's Supabase's business and I want a "fresh" "user" from supabase auth api to be able to check various things, like if it's banned, if it has a verified email, etc.

When I'll have a PR, would you like to review it ?

Thank you again for the time you take to write your comment, and forgive me if my English is approximative 😇

[rphlmr]

Thanks, fixed here: https://github.com/rphlmr/supa-fly-stack/pull/46