Psifi-Solutions / csrf-csrf

A utility package to help implement stateless CSRF protection using the Double Submit Cookie Pattern in express.
Other
113 stars 17 forks source link

Why forcing httpOnly cookie flag? #57

Open pbryant-xag opened 5 months ago

pbryant-xag commented 5 months ago

https://github.com/Psifi-Solutions/csrf-csrf/blob/14c1f0e0821e53ca3ade99d2ef3d0cdd8904183e/src/index.ts#L97

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.

psibean commented 5 months ago

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.

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 csrf-sync instead.

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.

pbryant-xag commented 5 months ago

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.

psibean commented 5 months ago

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.

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 csrf-csrf, just explain how it came to be.

I'm happy for the following changes to be made for the next major version:

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.

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).

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.

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.

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.

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 csurf so that it isn't insecure (as it's default implementation is insecure, but it can still be configured securely if you know how), the intention here was to provide the best options to people who aren't necessarily as well-informed as yourself and who may misconfigure things.

The linked discussion is regarding a very specific and circumstantial instance where the httpOnly flag would have made a difference due to the vulnerability being on a compromised subdomain. I'll cc @davidgonmar on that. You can dive into that specific case from the linked discussion.

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).

Browser apps can't access a cookie that has the httpOnly flag set.

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 csrf-csrf has always been that you decide how to provide the token to the frontend, this is documented in the readme and it is also documented in the examples in the repository, it is also mentioned multiple times in the linked discussion. csrf-csrf provides the plain token value with which you can deliver to the frontend by cookie (or any other means) if you so wish to do so. I'll agree that's a negligible and arbitrary overhead, I'm just saying that this is how it had been intended to use. So whilst the limitation is there, with the way it's intended to be used, the limitation is not there, you just need to provide the token yourself, and until the updates / changes and a major version release, that is how it is needed to be used.

pbryant-xag commented 5 months ago

Thanks @psibean. We'll stand by for the next major version.

davidgonmar commented 5 months ago

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 domain.com, evil.domain.com will be able to see it. If CORS is correctly configured, then the attacker will not be able to request a CSRF token from the vulnerable subdomain (evil.domain.com). By vulnerable I mean the attacker can inject content, including JS (that is, they have control over it). However, if the cookie is not set to httpOnly, they will be able to read the token-hash pair, and, since the cookie gets sent automatically, they just need to read the token from the token-hash pair obtained from the cookie and include it in a header. If httpOnly is on, they will not be able to do that. Moreover, in the way the library was designed, there is no need to access it from the cookie, and disabling httpOnly can introduce some small details like the one mentioned, so we decided to force it. Do you have an specific use case, just out of curiosity?

Just my insight since I was mentioned and saw this hehe, but I am fine with whatever decision you take:)

psibean commented 5 months ago

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 httpOnly to true. This way the token won't necessarily be exposed via an exposed cookie.

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.

psibean commented 5 months ago

I have a PR open to address the changes outlined in this issue.

https://github.com/Psifi-Solutions/csrf-csrf/pull/61

There will be some additional testing, examples, and README updates to come before the next major release (v4) with these changes.

psibean commented 5 months ago

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

The recommendation is to have your client call an endpoint on your backend to get the CSRF token. The endpoint must be super vigilant about confirming the caller’s origin and keeping that CORS allowlist very strict.

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:

const generateCsrfToken = (req, res, options) => {
    const generatedToken = generateToken(req, options);
    res.cookie('__Host-xsrf-token', generatedToken, {
        sameSite = "lax",
        path = "/",
        secure = true,
        httpOnly = false
    });
}

And now you can use your own generateCsrfToken wrapper to generate the token and have an additional cookie set from which it can be extracted. I'll still update csrf-csrf to re-expose the flag, but it could be a little bit before that release.

pbryant-xag commented 5 months ago

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

psibean commented 5 months ago

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.

It doesn't, but if you read the full quote, specifically the parts I've put in bold:

For SPAs, getting that CSRF token from the server is the difficult part. In traditional web apps, it’s not a problem. But for SPAs, you don’t call your back-end API until after you load the application. You also want CSRF protection before logging in if you’re not delegating authentication to a third-party authentication provider. 😎

The recommendation is to have your client call an endpoint on your backend to get the CSRF token. The endpoint must be super vigilant about confirming the caller’s origin and keeping that CORS allowlist very strict.

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.

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.

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.

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

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 generateToken and sets an additional cookie that the frontend can extract the token from. It's a workaround until the changes are released.

pbryant-xag commented 5 months ago

Noted re your last point - the work-around, thanks.