Update: [CSRF] Improving the new Double Submit Cookie sections from #1110

[advename]

What is missing or needs to be updated?

My previous PR to fix #1110 reintroduced an overview of the HMAC CSRF Token and added a Naive Double Submit Cookie and Encrypt Cookie sections.

The PR is merged into main, but there are some improvements left I believe should be addressed.

How should this be resolved?

Improvement 1

Uncertain if the pseudocode should be marked with code. Maybe we should use python since it's close to python syntax, taking advantage of highlighting.

Improvement 2

Personally, the new "Signed Double Submit Cookie" section does still not provide full mitigation against CSRF attacks, since it doesn't involve by default the idea of using session-dependent value. I would suggest instead of the header "Signed Double Submit Cookie", to use "Session-dependent Signed Double Submit Cookie", since a "Session-independent Signed Double Submit Cookie" is only a mitigation technique when combined with Referer validation to protect against:

• Vulnerable subdomains or sibling domains
• Man-in-the-middle (MITM) attack
• Open redirect vulnerability

Actually, the "Double Submit Cookie" & "Signed Double Submit Cookie" pattern without a session-dependent value is only a defense-in-depth (DiD) technique.

Only the following two are true mitigation techniques:

• Signed, session-independent, CSRF Token is combined with Referer validation. This is the CSRF mitigation technique of Django, as they have always utilized a signed, session independent, Double Submit Cookie) which by itself without Referer validation is vulnerable to both previously mentioned attacks.
• Signed, session-dependent, CSRF Token

I'm uncertain how to include easily the idea of "session-dependent" value without overwhelming the less experienced developers.

Furthermore, the "Naive Double Submit Cookie" is a defense-in-depth technique and should probably be moved into the Defense in Depth Techniques section? This would mean we have to restructure the "Double Submit Cookie" to introduce "Signed Double Submit Cookie" as the default from the beginning.

Improvement 3

There is actually a bug I just noticed in the pseudocode. The request.setCookie() should actually be response.setCookie() since we set the CSRF cookie in the response.

[mackowski]

Awesome @advename I agree.
I am not sure how much python will help in Improvement 1 but it is worth checking

[jmanico]

I think the double-submit cookie section is really "over built," and would love to see it simplified.

[advename]

@jmanico, do you have any suggestions?
I mentioned in #1101 about Collapsible Sections?
But how do you see Improvement 2 of my issue? Do you agree that naive Double Submit Cookie should be moved to DiD or what do you suggest?

[kiara-riley]

I like the new double-submit cookie section as someone that is attempting to learn about this. There are a lot of different approaches for different reasons and more information is certainly appreciated.

However, I have a question about the pseudocode. It sets a crsfToken with the sessionID in plaintext so that the server can verify the HMAC hash later. However, wouldn't this add a new vulnerability where the sessionID is now exposed to XSS attacks?

Wouldn't it be better to only expose the random value in the csrfToken and recreate the message with the sessionID from the (hopefully) httpOnly session cookie?

[jmanico]

I think the signed double-cookie submit is very confusing and would like it to go away, I think its very confusing and not necessary.

[jmanico]

I like the new double-submit cookie section as someone that is attempting to learn about this. There are a lot of different approaches for different reasons and more information is certainly appreciated.

However, I have a question about the pseudocode. It sets a crsfToken with the sessionID in plaintext so that the server can verify the HMAC hash later. However, wouldn't this add a new vulnerability where the sessionID is now exposed to XSS attacks?

Wouldn't it be better to only expose the random value in the csrfToken and recreate the message with the sessionID from the (hopefully) httpOnly session cookie?

If you care to submit a PR that removed that section, I'd take it!

[advename]

@jmanico
I think the signed double-cookie submit is very confusing and would like it to go away, I think its very confusing and not necessary.

That's what I outlined in this PR, and am still waiting for feedback on PR 1149. Collapsibles should help us to make it more straightforward and "stow away" the extensive contexts.

---

@kiara-riley
However, I have a question about the pseudocode. It sets a crsfToken with the sessionID in plaintext so that the server can verify the HMAC hash later. However, wouldn't this add a new vulnerability where the sessionID is now exposed to XSS attacks?
Wouldn't it be better to only expose the random value in the csrfToken and recreate the message with the sessionID from the (hopefully) httpOnly session cookie?

I have not yet had the chance to see any XSS vulnerabilities because of the sessionID knowledge. Nevertheless, I agree. My notes, from which I wrote the new sections from, also don't send the sessionID in the CSRF Cookie, but only, as you point out, the random value.
Would you mind opening a new issue or PR that fixes this in the pseudocode?

[jmanico]

Hey @advename can you give me a PR for one small thing at a time, maybe on PR for just the removal of signed double submit for start? It's a lot easier for me to process small changes as opposed to big design changes.

And when your collapsable is ready to go live let me know!

[advename]

@jmanico would love to. But i have still not received a reply to my previous questions.

[jmanico]

Hey @advename what questions are not answered? I don't have any thoughts on collapsable but am happy to answer questions about content. I don't think we need the signed cookie section at all, I'd rather just keep it simple and explain the basic ideas.

If there are pending questions you want me to answer, please ask me again here. I'm sorry to have missed them!

[siradam7th]

I like the new double-submit cookie section as someone that is attempting to learn about this. There are a lot of different approaches for different reasons and more information is certainly appreciated.

However, I have a question about the pseudocode. It sets a crsfToken with the sessionID in plaintext so that the server can verify the HMAC hash later. However, wouldn't this add a new vulnerability where the sessionID is now exposed to XSS attacks?

Wouldn't it be better to only expose the random value in the csrfToken and recreate the message with the sessionID from the (hopefully) httpOnly session cookie?

I was about to open an issue about this, then decided to see if anyone else mentioned it, so here I am.

As you mentioned, this way of generating a csrf token leaks the SessionID since it is recommended to set it to HttpOnly, but doing it this way, it makes such action useless, since it will be accessible by javascript easily from the csrf Cookie, by just spliting the csrf token string.

In summary, including the SessionID in the CSRF token generation by concatenating it defies the protections afforded by the actual SessionID cookie being set to HttpOnly.

[jmanico]

I also agree the sessionId derived token is a bad idea. Would someone like to remove this and submit a PR?

[murshex]

I like the new double-submit cookie section as someone that is attempting to learn about this. There are a lot of different approaches for different reasons and more information is certainly appreciated.
However, I have a question about the pseudocode. It sets a crsfToken with the sessionID in plaintext so that the server can verify the HMAC hash later. However, wouldn't this add a new vulnerability where the sessionID is now exposed to XSS attacks?
Wouldn't it be better to only expose the random value in the csrfToken and recreate the message with the sessionID from the (hopefully) httpOnly session cookie?

I was about to open an issue about this, then decided to see if anyone else mentioned it, so here I am.

As you mentioned, this way of generating a csrf token leaks the SessionID since it is recommended to set it to HttpOnly, but doing it this way, it makes such action useless, since it will be accessible by javascript easily from the csrf Cookie, by just spliting the csrf token string.

In summary, including the SessionID in the CSRF token generation by concatenating it defies the protections afforded by the actual SessionID cookie being set to HttpOnly.

Just saw your post now, and I ended up noticing the same thing and created an issue #1493

The implementation of the "signed double submit cookie" is fundamentally flawed. I'm genuinely surprised this was even approved.

Do you have anything to say for your broken implementation of csrf protection @advename ?

[advename]

@c0nd3v - I think we all agreed in this discussion that the session id should go. However, at the time of when I initially discussed this issue, I was waiting for some updates from the maintainers on collapsible sections for improved structure. Since that moved quite slowly and a crunch-time at work happened, I didn't spare much attention to this since then.

[murshex]

@c0nd3v - I think we all agreed in this discussion that the session id should go. However, at the time of when I initially discussed this issue, I was waiting for some updates from the maintainers on collapsible sections for improved structure. Since that moved quite slowly and a crunch-time at work happened, I didn't spare much attention to this since then.

Eh, its your pull request, isn't it? It's been up for a whole year and some more already. You have some responsibility in association with that. I sure hope nobody actually followed what you recommended in the mean time. Sorry to be harsh, but this is embarrassing and hurts the reputation of OWASP as an organization.

[advename]

No harshness taken!
I felt that I invested quite a lot of time then already and hoped that someone else would be able to remove those lines of code. Anyway, you are right, I'll make a PR ASAP to fix that particular issue.

[advename]

@c0nd3v feel free to chip in: #1513

[mackowski]

Thank you all!

[advename]

@mackowski , this issue is not completed. It mainly discussed the session id issue, but the initial topics are still open.