CSRF cookies

[SelfhostedPro]

Hi, I think it would be beneficial to include csrf cookies along with the standard jwt cookies in order to help prevent your jwt tokens from getting stolen.

You can see what flask-jwt-extended does here: https://github.com/vimalloc/flask-jwt-extended/blob/5bd8b1ed08ea64d23869a629af3c3c868816b8a8/flask_jwt_extended/utils.py#L260

[SelfhostedPro]

If you're able to, it may be beneficial to add documentation on manually adding a user via the Databases package in a similar way to how you do in the /register endpoint for people who would like to automatically generate an admin user if there currently are none.

[SelfhostedPro]

As far as the db stuff goes, this is what I have for creating a new user in a similar way to how the /register endpoint does. Please let me know if there's something wrong with doing things this way.

from .auth import fastapi_users, get_auth_router, cookie_authentication, database, users, user_create, UserDB, get_password_hash #this is the file that fastapi-users is initialized in.

@app.on_event("startup")
async def startup():
    await database.connect()
    users_exist = await database.fetch_all(query=users.select())
    if users_exist:
        print("users exist")
    else:
        print("no users")
        hashed_password = get_password_hash(settings.ADMIN_PASSWORD)
        base_user = UserDB(
            id = uuid.uuid4(),
            email = settings.ADMIN_EMAIL,
            hashed_password = hashed_password,
            is_active = True,
            is_superuser = True
        )
        user_created = await user_create(base_user)

This is a function in the auth.py file that just exposes the same function that you use for the /register endpoint.

async def user_create(UD):
    await fastapi_users.db.create(UD)

[frankie567]

Hi @SelfhostedPro!

I don't see anything wrong with the code you are showing ; even if I still personally think it's better to set the superuser flag manually from the database ; but that's okay!

As for the CSRF, I'll look further into it, thanks!

[frankie567]

I just discovered this project: https://github.com/simonw/asgi-csrf

It's an ASGI middleware that handles CSRF protection. I'll look into this, but I think this would allow to implement this security without having to implement it in fastapi-users. If so, I'll add some documentation about how to setup it.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[SelfhostedPro]

Is there a way to disable the stale bot on a specific issue? That looks like it would work. It's unfortunate that they haven't responded to your issue though.

[frankie567]

Sorry! I didn't pay attention!

[Harrison88]

This got me interested in reading up on CSRF, and I thought it might be helpful to share what I learned as far as FastAPI-Users is concerned.

In short, it seems that FastAPI-Users is mostly secure against CSRF already.

Routes that are potential targets:

1. /logout -- but all the attacker would be able to do is cause your user to logout. Since logging out basically consists of deleting the session cookie, I'm not even sure if a browser would delete the cookie when the request originated in a different website.
2. PATCH /me PATCH/DELETE /{user_id} -- these would be my main concern. It could allow an attacker to update a user's password. Some barriers exist, though: the attacker wouldn't know the username, only the password that they set. So they would have to get a list of usernames and try the new password against all of them to find out which user's password they changed. Beyond that, I'm not sure that there's a way to do a CSRF attack on the PATCH method, at least in modern browsers. If you wanted to be sure, you could require the user to enter their password in order to update their account. The user update model would then have a "current_password" field, and that would be checked along with the session cookie.

Using CSRF Tokens would also protect these routes, but could be a bit tricky to implement. Unfortunately, the asgi-csrf middleware wouldn't work for this case. The problem is that you have to have a way of communicating the CSRF Token to the client that the attacker can't see, and then all forms/api clients will have to send the token. As mentioned, flask-jwt-extended accomplishes this by setting a cookie. The client would then have to read the cookie and submit the token in a separate header (that's what double submit means: the csrf token is sent back to the server both as a cookie, and manually in a custom header). Since a third-party attacker couldn't read the token in the cookie, only JS that came from the same origin would be able to submit the custom header.

The answer to this question goes over several ways that the CSRF Token can safely be communicated between the client and server, which I found helpful in understanding how it works: https://stackoverflow.com/questions/20504846/why-is-it-common-to-put-csrf-prevention-tokens-in-cookies

Here are the routes that are definitely already secure against CSRF.
Routes that don't require a session cookie to be sent:

1. /login
2. /register
3. /forgot-password
4. /reset-password
5. /authorize
6. /callback

Routes that don't change state on the server:

1. GET /me
2. GET /{user_id}

[frankie567]

Thank you @Harrison88 for the detailed explanations, it helps a lot ; sorry for not having reacted to it sooner.

I think the issue with CSRF here comes actually from the routes the end-developer will implement themself using our dependencies like get_current_active_user which will be sensible then (I believe) to this kind of attacks.

I also came across the double submit pattern (I noticed LinkedIn is doing it this way). I think this will be the way to go.

[Harrison88]

Thanks for the response, and no problem about the delay. I've been offline for awhile, myself.

Definitely, routes using the dependencies could be vulnerable to CSRF. I feel like adding CSRF Tokens would be a lot of effort with little payoff, though. Considering that the cookies are already being set with the SameSite=Lax attribute, it's only really a concern for browsers that don't support that. According to caniuse.com, over 93% of users are on browsers that support the SameSite attribute.

Beyond that, even if CSRF Tokens are implemented in FastAPI-Users, the end-developer will still have to do the work on the frontend to support it. And if they're already doing that, it should be easy to just drop in something like the asgi-csrf middleware.

I'm not saying it would be a bad idea to add this feature, or that it wouldn't be useful to anybody, only that it might not be worth your time. But only you can really decide that, of course. If you still want to do it, I'd be happy to help in any way I can.

[Harrison88]

It looks like fastapi-jwt-auth does use a header for receiving the token from the client. You can see that it gets the CSRF token from a header here: https://github.com/IndominusByte/fastapi-jwt-auth/blob/master/fastapi_jwt_auth/auth_jwt.py#L504

And the config value that determines which header the token is sent as is here: https://github.com/IndominusByte/fastapi-jwt-auth/blob/master/fastapi_jwt_auth/auth_config.py#L43

Setting the cookie is purely to communicate the token to the client.

[SelfhostedPro]

From my understanding, the http only setting for the cookie is what denotes it as protected from malicious javascript. With the initial submit you're providing the backend with the csrf token and once that matches it talks directly to the backend (hence the http only setting) to provide the actual jwt token.

You can see where the cookies are set here:
https://github.com/IndominusByte/fastapi-jwt-auth/blob/a6c06193319da0e4976c7472966f3a2891e0d50c/fastapi_jwt_auth/auth_jwt.py#L393

You can see how the backend authenticates the cookies here:
https://github.com/IndominusByte/fastapi-jwt-auth/blob/a6c06193319da0e4976c7472966f3a2891e0d50c/fastapi_jwt_auth/auth_jwt.py#L693

Sure, if you're using headers to communicate, you'll have to make some changes but cookies are more secure (as localstorage is susceptible to XSS attacks and csrf cookies prevent csrf attacks). This blog has a good writeup on it: https://stormpath.com/blog/where-to-store-your-jwts-cookies-vs-html5-web-storage

[Harrison88]

I think you may be misunderstanding how the CSRF attack works. Malicious javascript doesn't access your cookies; the attacker tricks the browser into sending a request with the cookies. So, if a user is logged in (they have a session cookie), the browser will send that session cookie with the request, and it looks like the user is the one who sent it. The attacker is never able to see the cookie in this case, so the fact that it's HttpOnly doesn't matter.
Now, add a CSRF cookie, so the user has both a CSRF cookie and a session cookie. The attacker tricks the browser into making a request, and the browser sends the cookies. The server receives both the CSRF cookie and the session cookie and believes that it's actually the user making the request. The attacker can't see either of the cookies. They don't even know that there is a CSRF cookie. They rely on the browser sending the cookies with the request. For that matter, the attacker also doesn't know whether there is a session cookie. They just try to attack as many people as possible, and hope some of them are logged in to the website they're attacking.

One solution to CSRF is to convince the browser not to send the cookies with those requests. That's what the SameSite attribute does. It tells the browser, "don't send this cookie if the request is coming from a different domain." That prevents the attack, but there may be a way around it, and it only works if the user is on a modern browser. So for defence in depth, CSRF tokens are still helpful.

The double submit pattern looks like this:
When the user logs in, the server sets two cookies. 1. The session cookie (which should be HttpOnly, for the reasons you gave). 2. The CSRF cookie. For subsequent requests, the browser will send the session cookie, which identifies the user. However, the CSRF cookie needs to be accessible from javascript, and needs to be sent with the request in a different way, such as in a header. The attacker wouldn't be able to access the cookie, so any malicious requests that look like they're from the user (because they have the session cookie) can be discarded (because the CSRF token isn't in the header).

As for fastapi-jwt-auth, I admit that there are a lot of functions in that one file, and I still don't really know how they all interact. As you pointed out, the CSRF cookie is set in lines 405-416. However, the cookie isn't actually accessed in line 693. Another function is called, _verify_and_get_jwt_in_cookies(), which is defined in line 523. Within that function, you can see that the access cookie is retrieved in line 547: cookie = request.cookies.get(cookie_key). Two lines after that, line 549, you can see that the CSRF token is retrieved from a header: csrf_token = request.headers.get(self._access_csrf_header_name).

[SelfhostedPro]

I think you're right about that. The frontend(vuejs) code looks like this:

  [AUTH_REQUEST]: ({ commit }, credentials) => {
    return new Promise((resolve, reject) => {
      commit(AUTH_REQUEST);
      const url = "/api/auth/login";
      axios
        .post(url, credentials, { withCredentials: true })
        .then(resp => {
          localStorage.setItem("username", resp.data.username);
          axios.defaults.withCredentials = true;
          axios.defaults.xsrfCookieName = "csrf_access_token";
          axios.defaults.xsrfHeaderName = "X-CSRF-TOKEN";
          commit(AUTH_SUCCESS, resp);
          resolve(resp);
        })
        .catch(err => {
          commit(AUTH_ERROR, err);
          commit("snackbar/setErr", err, { root: true });
          localStorage.removeItem("username");
          reject(err);
        });
    });
  },

It's been a while since I've written this so I guess I had since forgotten that it sends it in a header.

[frankie567]

So! I've read your discussions there, very interesting, thanks 😄

Now, I must admit I'm still not sure if we need to implement CSRF protection or not. Sounds like the SameSite cookie policy is the easy and future-proof solution. I don't really realize if the 8% of incompatible browsers is a major security threat or if we can consider that it's okay 🤔

[frankie567]

Hey there!

A long time since we discussed this subject 😄 After some extensive research, it appears that it's still strongly recommended nowadays to have a CSRF protection besides Same-Site: Lax (in a "soon" future, this should be enough however).

I gave a shot at implementing a middleware for Starlette/FastAPI implementing the double-submit cookie technique: https://github.com/frankie567/starlette-csrf I wanted to make it easy to understand and configure. Would be glad to have your feedbacks 😄

[Harrison88]

Looks good to me! Thank you for making that available!

[kyrtrotsenko]

@frankie567 If I use both fastapi-users and starlette-csrf, I encounter the error "CSRF token verification failed" because I'm not sending the x-csrftoken.

I cannot find a ready-made solution in fastapi-users.
Are there any plans to implement this feature in the future?|
Or perhaps I'm overlooking something?

[frankie567]

@kyrtrotsenko If you're using the Swagger UI, then yes, it won't work. It's more a limitation of Swagger than FastAPI Users. If you make requests yourself in your own app, you'll be able to send the token and everything should work smoothly.

[kyrtrotsenko]

@frankie567 Sure, thank you. All good.