search

aekasitt / fastapi-csrf-protect

Stateless implementation of Cross-Site Request Forgery (XSRF) Protection by using Double Submit Cookie mitigation pattern
https://pypi.org/project/fastapi-csrf-protect
MIT License
74 stars 14 forks source link

Support for additional factors in token #10

Closed mkiesel closed 2 years ago

mkiesel commented 2 years ago

The CSRF token generated is only dependent on time by default, i.e., without additional work, generated tokens are valid for all users in multi user sites.

However, tokens should be unique per user session as stated in https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#synchronizer-token-pattern

A workaround would be to use different secret_keys per user (e.g., append the user id to the site wide secret), but this seems clumsy (and should probably get documented). @aekasitt

https://github.com/aekasitt/fastapi-csrf-protect/blob/81bbece1dc214de0b0a7395fc21b53433c196214/fastapi_csrf_protect/core.py#L39-L41

aekasitt commented 2 years ago

I'll look into this.

aekasitt commented 2 years ago

The assertion is that the CSRF Token generated is dependent on time by default; But I'm not sure that is observed by the code snippet given above. The serializer (itsdangerous's URLSafeTimedSerializer) is only dependent on time by default, but then the token is generated by a secondary process of hashing sha1(urandom(64)).hexdigest() which I think qualifies as session independent unless it can be credibly observed as timestamp dependent where two sessions generated at the exact same timestamp (a rarity but not an impossibility) can cause a collision with the same CSRF Token

mkiesel commented 2 years ago

The following attack is trivial since the CSRF token isn't really checked against anything (but time validity):

The lib should either send a random value (no signing necessary) and actually store this in a session, and check the submit data/header against that value (in OWASP this is the "Synchronizer Token Pattern"), or it should set a random value in a cookie, and check whether the cookie value and the submit data/header info matches (the OWASP "Double Submit Cookie" approach).

fastapi-jwt-auth uses the Double Submit Cookie approach - if fastapi-csrf-protect wants to do the same, it's missing the actual value check. In fastapi-jwt-auth, that's the following code. https://github.com/IndominusByte/fastapi-jwt-auth/blob/a6c06193319da0e4976c7472966f3a2891e0d50c/fastapi_jwt_auth/auth_jwt.py#L516-L521

Note all the JWT/HMAC/encrypting things are not strictly necessary for CSRF. The Double Submit Cookie approach doesn't rely on encryption/HMAC but on the fact that an attacker cannot control or know the CSRF token value stored in the cookie but has to supply that value in headers/form data.

aekasitt commented 2 years ago

This would defeat the purpose of the library. You're looking for a Session solution. (big S) The scope of Cross-Site Reference Protection has been met with current implementation.

mkiesel commented 2 years ago

The library doesn't have any concept of a user session; it doesn't actually store the token in a (server-side) session but only in a cookie; and the CSRF token (value) stored in that cookie is never checked against the token (value) that gets submitted via headers or form data. The only thing that's checked is basically well-formedness of the token (HMAC, max_age) which is not enough.

@aekasitt Did you read https://github.com/IndominusByte/fastapi-jwt-auth/blob/a6c06193319da0e4976c7472966f3a2891e0d50c/fastapi_jwt_auth/auth_jwt.py#L516-L521 ?

if not hmac.compare_digest(csrf_token,decoded_token['csrf']):
                    raise CSRFError(status_code=401,message="CSRF double submit tokens do not match")

The key point there is that csrf_token comes from request headers/form data, and decoded_token is obtained from the CSRF cookie.

This check is missing completely in fastapi-csrf-protect.

aekasitt commented 2 years ago

I agree with you that fastapi-jwt-auth's csrf token is more sophisticated since it depends on a session id (client-side and stored in cookies, but session id nonetheless)

I am telling you that it goes beyond the scope of this simple library.

It is in fact preferable if session integrity is coveted to use alternative library such as the one you mentioned.

To prevent the attack you mentioned, with Scammer A building a real-time cloned page with stolen CSRF token for User B; What is being attacked there is session integrity, which this library never claimed to protect against.

Such attack can be prevented using SameSite cookie, shortening Max-Age and storing CSRF Token in Cookies for enhanced secrecy.

mkiesel commented 2 years ago

The attacker doesn't need to "steal" any CSRF token. He can just use a token that our site gave to him, and use that token in a POST request he makes user B submit.

That is no session integrity attack, there's no session involved.

Perhaps there's a misunderstanding that a "session" is?

E.g., I was wondering in this code snippet https://github.com/aekasitt/fastapi-csrf-protect/blob/3682f9d82b24d66e059e379d9a67904416099096/fastapi_csrf_protect/core.py#L32-L41 it says "multiple calls to this function will generate the same token" which clearly is not the case.

In any case, as it is, this library doesn't provide protection against CSRF, at least not in the sense OWASP etc. use those terms.

In Cross Site Request Forgery protection, the point is checking form data/header info against a CSRF (server side) session token or CSRF cookie value, which this library doesn't do.

aekasitt commented 2 years ago

You have a misunderstanding of the OWASP requirements, and I used the word "session" because of it was used in the document.

It satisfies the OWASP requirements as I previously explained.

The cached comment I can remove. Not sure how that got there. Thanks!

Please refer to the Defense in Depth Techniques section of the attached OWASP document Does your attack vector make sense with SameSite Cookie Attribute properly configured?

mkiesel commented 2 years ago

Of course the token has to be unique per user session and it has to be validated against the user session (on POST). Every other CSRF library does this. Look at the code I mentioned a few postings above, and just for fun, https://github.com/wtforms/flask-wtf/blob/72419060c0ce71c002098e304121f7b61c7ec985/src/flask_wtf/csrf.py#L118-L119 is the corresponding code in flask-wtf.

Sorry, but as it is, fastapi-csrf-protect is completely broken.

aekasitt commented 2 years ago

Yeah please help with SameSite :) I've been confused as to why you raised this issue and not that.

aekasitt commented 2 years ago

Token generation satisfies OWASP requirements. Session integrity preservation is outside of Cross-Site Reference Forgery Protection. Code breaking side-problem found, SameSite not yet implemented in version 0.2.0. Closed.

mkiesel commented 2 years ago

validate_csrf_in_cookies() only looks at the cookie. It doesn't check against form data/headers. Any attacker doesn't need to guess any token at all and can just make the victim POST any form data, it'll check out.

https://github.com/aekasitt/fastapi-csrf-protect/blob/3682f9d82b24d66e059e379d9a67904416099096/fastapi_csrf_protect/core.py#L120-L126

Token generation satisfies OWASP requirements.

"Generation" yes, but not validation.

aekasitt commented 2 years ago

Can you please open another issue for this? Hard to track.