-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Why forcing httpOnly cookie flag? #57
Comments
It isn't supposed to be accessed by JavaScript. By asking this question you're indicating you don't understand how the Double Submit Pattern works, and you might be wanting the Synchronized Token Pattern via The cookie isn't supposed to be accessed by JavaScript, here is what is supposed to happen:
The only reason the original token is also included in the cookie with the hashed version of the cookie is to support token re-use where server side session state isn't a thing. The Double Submit Pattern works by having you submit the value via two different methods. One is the http only cookie, which the browser will automatically send, and it contains the token hash. Then there's the actual token value, which you retrieve through a different means. As for the enforcement of the httpOnly flag, there's an entire discussion on another issue here where a subdomain vulnerability was able to compromise the main domain and could have been avoided if they followed this pattern. |
Thanks for the quick reply @psibean. I'm quite familiar with the DSC pattern, so let's go deeper to check that we both are. Please search for "without httpOnly flag" at https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#signed-double-submit-cookie-recommended, and let me know if you think OWASP has this wrong. The DSC pattern doesn't care whether the token is returned only in the cookie or also in the some other response data. It also doesn't care whether the cookie is flagged as httpOnly. And for the record it also doesn't care if they are hashed or not (they should be, but that's not fundamental to the pattern). The pattern just requires that:
The DSC middleware is (amongst other things) just checking those things are the same as each other. The protection comes from the assertion that the attacker didn't have access to the previously-returned token (either just in the cookie, or also in some non-cookie response data). The DSC pattern doesn't say anything about the token only being accessible to the browser app from a non-cookie response property/payload. Per the link above, the OWASP advice actually implies that the cookie is the only way the token is returned to the browser app. And if the token is going to be accessible to the browser app from the cookie, then the cookie must not have the httpOnly flag set. Regarding XSS - no CSRF protections are immune to XSS. See "IMPORTANT: Remember that Cross-Site Scripting (XSS) can defeat all CSRF mitigation techniques!" in the same OWASP cheat sheet linked above. So it doesn't matter if the cookie is httpOnly or not. The DSC pattern absolutely does not require the cookie to be httpOnly (in fact the authoritative commentaries imply the opposite), and by forcing it to be so you are applying an unnecessary limitation that clearly affects applications that (legitimately) want to access the cookie value (evidence the other issue thread you linked above). No matter how or where the token is passed to the browser application, successful XSS has access to that token value, just like the browser app's legitimate HTML/code does. Forcing the httpOnly flag on the cookie in no way reduces the XSS risk. This package forcing the httpOnly flag wrongly implies that this gives some XSS protection against XSS defeats (against CSRF protections), which it doesn't. I think that outweighs your comments in the other thread about protecting uninformed users from themselves. It doesn't help protect them from anything as far as CSRF is concerned. Also see https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#naive-double-submit-cookie-pattern-discouraged, and search for "Since an attacker is unable to access the cookie". Although this package doesn't implement the naive pattern, this OWASP para does (like the one linked above) clearly suggest that the browser app needs to be able to access the cookie in the DSC pattern. Browser apps can't access a cookie that has the httpOnly flag set. If you still disagree, it would be reasonable to provide links to authoritative/consensus commentaries confirming your (and as you said in the other issue thread "kind of arbitrary") choice here, such as from OWASP, Mozilla, NIST, etc. PS - nonetheless I do appreciate your efforts on this package, thank you. |
I'll start off by saying that I concede and that I don't disagree at all, just so there's no misunderstanding with the points I'm making below, as I'm not trying to defend the current state of I'm happy for the following changes to be made for the next major version:
Interesting, there's a particular part underneath the STP pattern which advises not to send the token by cookie, and I could have sworn there was a similar recommendation under the DSC pattern too. Either I misread, or the page has since been updated (I can't check right now).
As per my previous paragraph, I could have sworn it did/used to, either it has been updated since, or I very much got confused when reading the page and other various sources.
You've mentioned XSS at multiple points throughout your paragraphs so I'll reply to it all generally here. I'm aware XSS bypasses any CSRF protections. The intention behind having the flag set to true was never intended to do anything for XSS, in the same way people don't understand how to configure The linked discussion is regarding a very specific and circumstantial instance where the
Whilst I will agree it is not currently correct (and my dot points above should resolve that and bring it in line with intended behaviours), the intention with |
Thanks @psibean. We'll stand by for the next major version. |
Hey! Some time ago I thought about this. The thing is that if I set a cookie for Just my insight since I was mentioned and saw this hehe, but I am fine with whatever decision you take:) |
I think I'll go ahead with the changes outlined in this issue. I'll expose the httpOnly flag and I'll set it to true by default. The readme will recommend leaving it to true and link to the appropriate discussions and also mention the potential for subdomain vulnerabilities to have additional access if it's set to false. Additionally, I'll make it so that the original token also being stored in the cookie is a configurable option, where it is included by default, but the docs will recommend disabling that if changing If a subdomain is vulnerable, this should be considered a separate problem in the first place. Whilst it CAN provide additional protection, it's not the purpose of CSRF protection to protect against other vulnerabilities (subdomain takeover) which should be appropriately handled and avoided. |
I have a PR open to address the changes outlined in this issue. There will be some additional testing, examples, and README updates to come before the next major release (v4) with these changes. |
Just a heads up: From okta - also a very reputable security source, the OWASP pages are often outdated, and the CSRF ones in particular don't mention anything about SPA's. Okta is an extremely reputable Identity and Access Management (IAM) service provider
In cases where you aren't using an SPA, exposing the cookie isn't really necessary, because you can render the token in a meta tag in the head element of the server side rendered HTML, or directly as a hidden field inside forms. Additionally, even if the cookie is directly exposed, your frontend is going to have to split it in order to get just the token value (as the cookie value is a combination of the token + the signed value). With the versions of csrf-csrf so far, if you do want to use a cookie as means of receiving the csrf token on the client side, for that particular use case it has otherwise been recommended to do something like this:
And now you can use your own |
Just noting that SPA, SSR and returning HTML from the back-end aren't the only or even necessarily the dominant patterns. For example, a Node+Express app that serves an API to the front-end - that isn't an SPA, and there is no SSR or HTML being returned from the back-end to the front-end. Also noting that that Okta article is only about SPAs and doesn't mention the httpOnly cookie attribute at all as far as I can see. Regarding the rest of your post, can you please clarify? Do you mean the force-httpOnly override in csrf-csrf has now been removed, and/or the consuming code can now override generateCsrfToken() without that being subsequently overridden (whereas it couldn't before)? I haven't looked at the commits yet - just seeking to understand what you're saying at this point. Thanks |
It doesn't, but if you read the full quote, specifically the parts I've put in bold:
If you could just get the token from the cookie, then what would be difficult and why would they recommend to do something different? It is implied that the recommended approach here is something that isn't getting it out of the cookie.
If the frontend is just a completely static site (that isn't an SPA) that is served from it's own web server (separate from the backend), then this is the same as the SPA case in terms of the retrieving of the token, as far as the logic in the okta article is concerned.
The update hasn't been made yet, the changes are in progress, there's some testing and documentation stuff that needs to be done before I get everything merged and release. I'm saying that it could be some time before the changes get released. What I'm saying is, even in the current state, where the httpOnly flag is forced to true, you can use the code snipped I provided as an example to create your own token generator which wraps the provided |
Noted re your last point - the work-around, thanks. |
I ended up re-enabling the ability to override the Whilst there are some extremely rare use cases (discussed previously and in other issues) where the |
csrf-csrf/src/index.ts
Line 97 in 14c1f0e
Could you please clarify why you are forcing the
httpOnly
cookie flag here? If the cookie is to be accessed by JavaScript, in order to be able to add that value to a subsequent request header, it needs to not be a httpOnly cookie. Thanks.The text was updated successfully, but these errors were encountered: