You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Hi, I appreciate the library and the excellent developer experience! I'm really a fan.
However, I think there are a few issues with the CSRF implementation; I just want to clarify my understanding and see if it really is an issue or not. I'd be willing to contribute changes if you agree and we can agree on a better approach.
You encourage developers to set the CSRF token in a cookie, presumably to avoid needing to send the CSRF token in every loader. However this greatly reduces the effectiveness of CSRF tokens. If the attacker either A) steals the user's cookies, or B) crafts a Form submission forgery that tricks the user into taking an unwanted action, then your CSRF has added no extra protection. The number of cookies needed by the attacker for both of these types of attacks has just gone from 1 to 2 (session cookie + CSRF cookie). The attacker will have none or all, so it's no difference in the end. Transmitting CSRF tokens in cookies is a major no-no.
The best way to address this, if I'm understanding it, would be to NOT encourage developers to set the CSRF token in a cookie. Any page that is rendering a Form will need to include the CSRF token in the loader data. Then, at least, a somehow leaked or stolen cookie won't be the entire key to an attacker - they would still need to get a CSRF token from a form on a rendered page, which means they would essentially have to be logged in as the end user and view the page source - which would be a total account takeover already.
(This is why traditional old style server-rendered web frameworks always communicated the CSRF token to the client in the form of a hidden form input - we're assuming a compromised session cookie here, so we communicate this secondary information through another channel that's not a cookie.)
This one is less severe, and much much harder to address (in fact, many SPA frameworks get this wrong). It looks like your implementation uses no server-side state, and just relies on a valid signature for the passed-in token. This is susceptible to replay / reuse attacks, and makes it impossible to expire or invalidate a token. Usually, you want to tie a CSRF token to a specific instance of a mutable action (a single client-side rendering of a Form, for example) and only let them be used once, ideally within a short time window after their creation.
As it is currently implemented, one single leaked or compromised CSRF token can be used indefinitely because it has a valid signature. The only mitigation that I know of for this is to store tokens server side and validate them according to expiration / one-time-use rules. This obviously necessitates integration with a back-end data store, which is more involved (and would require a lot more work to create a clean API that consumers of this library would have to implement). And it's even worse than that: Because of the way Remix revalidates and rerenders parts of a page, it's near impossible to know which exact instance of a CSRF token should be associated with which exact form submission.
The best solution I can think of for this would be something like storing a collection of one-time use CSRF tokens with time-to-live that's longer than the expected dwell time on your average page, and burning tokens as they are used, while allowing any token to satisfy any action as long as it's valid, not used, and not expired.
Needless to say, this is a big problem and the most attractive solution is to do what the Remix developers did: throw up your hands and say "You need to use SameSite=lax" (I agree with them, actually - there are no perfect anti-CSRF measures that I know of for SPAs).
If you want to explore how to address either of these items, I'm open to discussion. But I don't recommend folks rely on this utility as it's written today for anti-CSRF functionality.
If you get rid of the cookie aspect of this, then this library will at least add some protection.
I'm happy to discuss further, as it's also totally possible I've misinterpreted some code!
reacted with thumbs up emoji reacted with thumbs down emoji reacted with laugh emoji reacted with hooray emoji reacted with confused emoji reacted with heart emoji reacted with rocket emoji reacted with eyes emoji
-
Hi, I appreciate the library and the excellent developer experience! I'm really a fan.
However, I think there are a few issues with the CSRF implementation; I just want to clarify my understanding and see if it really is an issue or not. I'd be willing to contribute changes if you agree and we can agree on a better approach.
The best way to address this, if I'm understanding it, would be to NOT encourage developers to set the CSRF token in a cookie. Any page that is rendering a Form will need to include the CSRF token in the loader data. Then, at least, a somehow leaked or stolen cookie won't be the entire key to an attacker - they would still need to get a CSRF token from a form on a rendered page, which means they would essentially have to be logged in as the end user and view the page source - which would be a total account takeover already.
(This is why traditional old style server-rendered web frameworks always communicated the CSRF token to the client in the form of a hidden form input - we're assuming a compromised session cookie here, so we communicate this secondary information through another channel that's not a cookie.)
As it is currently implemented, one single leaked or compromised CSRF token can be used indefinitely because it has a valid signature. The only mitigation that I know of for this is to store tokens server side and validate them according to expiration / one-time-use rules. This obviously necessitates integration with a back-end data store, which is more involved (and would require a lot more work to create a clean API that consumers of this library would have to implement). And it's even worse than that: Because of the way Remix revalidates and rerenders parts of a page, it's near impossible to know which exact instance of a CSRF token should be associated with which exact form submission.
The best solution I can think of for this would be something like storing a collection of one-time use CSRF tokens with time-to-live that's longer than the expected dwell time on your average page, and burning tokens as they are used, while allowing any token to satisfy any action as long as it's valid, not used, and not expired.
Needless to say, this is a big problem and the most attractive solution is to do what the Remix developers did: throw up your hands and say "You need to use SameSite=lax" (I agree with them, actually - there are no perfect anti-CSRF measures that I know of for SPAs).
If you want to explore how to address either of these items, I'm open to discussion. But I don't recommend folks rely on this utility as it's written today for anti-CSRF functionality.
If you get rid of the cookie aspect of this, then this library will at least add some protection.
I'm happy to discuss further, as it's also totally possible I've misinterpreted some code!
Thanks for the open source work, keep it up!
Beta Was this translation helpful? Give feedback.
All reactions