CSRF Prevention Cheat Sheet Pseudo code example exposes session ID in the case of XSS

[murshex]

https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#pseudo-code-for-implementing-hmac-csrf-tokens

If the website was vulnerable to XSS, the example here would make the session id exposed. Bad example honestly.

[szh]

@c0nd3v can you please walk through how this would be vulnerable? Assuming this code is running on the server side of course (since anything client side is insecure), all I see is that the session id is included in the cookie. But is that necessarily a problem? The CSRF token can't be recreated without knowing the CSRF secret key, so this appears no different than most other forms of cookie based session handling. Am I missing something?

[murshex]

@szh The problem I see is that the cookie is not HttpOnly. So if the website has a XSS vulnerability, they could read the cookie using javascript. Which means they would have the session id.

If the synchronizer pattern was used instead, and the website has a XSS vulnerability, there is only a chance to leak the CSRF token, and not the session id.

[murshex]

And obviously the session id leaking is pretty bad.

A hacker could send you a link that has XSS in query params or something, and that XSS could read the cookie and send it to the hacker using fetch or something. And then the hacker could hijack the session.

[murshex]

TLDR; the session id should always be HttpOnly and not exposed anywhere else

[szh]

Ah absolutely. Do you want to submit a PR to make it HttpOnly?

[murshex]

Sure but, if we make the cookie http only, the double submit csrf token thats a hidden form value or inside the request header (not the cookie) must not contain the session id. I guess it should be just the hmac. I think that should be more clear, I don't even think that its mentioned.

[jmanico]

Cookie isolation per domain prevents other sites from reading or setting that cookie. And XSS is irrelevant, because XSS defeats ANY CSRF defense.

[murshex]

@jmanico XSS is not irrelevant. Yes, XSS defeats any CSRF Defense. But as I said above, this CSRF pseudo code example EXPOSES the SESSION ID if the website is vulnerable to XSS. Which means the website would also be vulnerable to SESSION HIJACKING.

[jmanico]

Your point is well taken, I got you now. :) I only mean XSS is not relevant to CSRF defense. I hear you, good point.,

[murshex]

Update: @advename Stated that due to crunch-time at their work and other reasons, he has been unable to update his implementation of double submit cookie with HMAC and fix its vulnerabilities. I think that's pretty reasonable and understandable. After that, they've even offered to submit a PR ASAP to fix it.

Honestly, @jmanico if you are unfamiliar with CSRF, please, don't approve PRs like this. I don't know what your role is here, but I will say in my opinion, you should 1. Be stripped of your ability to approve PRs or 2. No longer allowed to contribute to the CSRF cheat sheet. This should have never been approved and it's honestly laughable. It's no surprise that so many websites are vulnerable to CSRF when even the "experts" get it wrong. How did you miss this? Why did you approve that PR if you don't understand CSRF?

In addition to all this, if I have time I'll contribute to the cheat sheet myself.

[jmanico]

This has been fixed. Thank you for your other editorial comments. We added a better review process since then.