CSRF Token is refreshed on every request

[ia3andy]

Describe the bug

As far as I know the token expiry should be extended but not the token itself shouldn't be changed on new request. Currently this makes requests with htmx fails as after the first request the token is wrong.

Expected behavior

The token expiry should be increased but the token should be the same

Actual behavior

it changes the token

How to Reproduce?

clone https://github.com/ia3andy/renotes/
start with quarkus dev
load the page and refresh many times, keep a look on the body tag, you will see the token getting updated

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

3.6.5

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

[quarkus-bot]

/cc @pedroigor (bearer-token), @sberyozkin (bearer-token,jwt,security)

[ia3andy]

This is a big issue AFAIK

[maxandersen]

did this ever work or is it a long standing issue?

[ia3andy]

Yes it's working in 3.6.3, but there were other issues, it seems to have been broken in #37725

[maxandersen]

@FroMage @sberyozkin I notice #37725 got in without testing - not sure if we could have caught it then but now we know about this issue is there some testing we can add to avoid breaking in future?

[FroMage]

The original idea of #37725 was to make sure that AJAX request would extend the life-time of CSRF tokens, which so far only get renewed by regular HTTP calls, leading to AJAX apps failing due to token non-refreshment.

The problem is that we should have extended their life-time without changing their value. Just renewed the cookie with a further expiry. Changing the CSRF token value itself invalidates other parts of the HTML forms that have the original token, if they're not also refreshed.

We might be able to mitigate this by returning the new token value in a header, unencrypted, so that ajax calls can update it.

Note that none of these ajax issues would have happened if we'd gone with a fixed CSRF header with no value, as OWASP recommends. The value of the CSRF token is only advised for HTML forms, not for AJAX requests, where any special CSRF header with any random or fixed value is enough: https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#employing-custom-request-headers-for-ajaxapi

[sberyozkin]

Changing the CSRF token value itself invalidates other parts of the HTML forms that have the original token, if they're not also refreshed.

This is what I missed - it should not be a problem with simple forms, pages. The unit tests are there, but from now on, indeed, the practitioners like Steph and Andy should verify the CSRF PRs before there are merged since I'm not practicing CSRF related setups with HTMX etc, it is hard to get it right with some basic unit tests.

@FroMage Steph, FYI, the double cookie submit pattern is sound, specifically we also implement an optional signing feature.

OK, let me give it a try and I'll ask yourself and Andy to verify it, thanks

[ia3andy]

I will implement an integration test in Renarde ecosystem-ci with Playwright to make sure CRSF and htmx works properly with Renarde. It won't detect errors in the PR, but it will check the main branch before it gets released...

[ia3andy]

Also we will be able to run it locally with the PR if needed

[maxandersen]

@ia3andy does it really need a full renarde setup to verify? just curious as seems like something we should have test for closer to the code.

[ia3andy]

here is my take on this:

1. I implement some test using playwright in Renarde
2. @sberyozkin have a look at what is happening on the network and add some equivalent unit test in Quarkus core

I am saying this because the use cases are strongly tied to the sequence it is used in the app and it's always something new breaking. So better have the full integration test to make sure it actually works in the browser...

[sberyozkin]

@ia3andy even with HtmlUnit test it is hard to verify at the Quarkus level it really works with HTMX containing multiple forms, so like you said having some integration tests in Renarde would complement it.

[maxandersen]

yeah, i 'm not saying don't do it in renarde - just wondering if can get it closer as renarde test runs aren't run all the time.

[ia3andy]

Yeah for example in this case, it works in the first form and breaks on the second...

[ia3andy]

ecosystem-ci is run everyday AFAIK

[FroMage]

@FroMage Steph, FYI, the double cookie submit pattern is sound, specifically we also implement an optional signing feature.

Sure, but read the OWASP article I linked. Double cookie submit pattern is for regular HTML forms. For AJAX, a single special header with a fixed value is enough.

[FroMage]

@BarDweller Hey, how're your CSRF skills? :)

[sberyozkin]

@FroMage

For AJAX, a single special header with a fixed value is enough.

Possibly, it feels to me though that cookie comparison adds an extra safety dimension even in this case. I don't know how we can prove for certain that a single header only without having to compare it with anything is sufficient in various setups. I think we should take even OWASP published content as not necessarily 100% correct, possibly applicable to certain deployments only.

[sberyozkin]

@FroMage @ia3andy @BarDweller In any case, I think in this case only a minor tweak is needed to resolve it, if the cookie is already available then reuse its value when refreshing a cookie, instead of always generating a new value - IMHO it is a technical bug which does not loosen the security cover, only breaks the complex UIs unfortunately

[BarDweller]

Heh.. familiar with the basic idea, but I wouldn't trust my current understanding enough to declare if anything has been 'fixed' =) I will however use this one as a reason to go learn more & make sure I understand any changes that come as a result of this !!

…

On Tue, 16 Jan 2024 at 11:41, Stéphane Épardaud ***@***.***> wrote: @BarDweller <https://github.com/BarDweller> Hey, how're your CSRF skills? :) — Reply to this email directly, view it on GitHub <#38225 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGCKMWASWEV3KYHJXVZQALYO2UVPAVCNFSM6AAAAABB5AIFVOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJUGEYTIMBRGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[FroMage]

Possibly, it feels to me though that cookie comparison adds an extra safety dimension even in this case. I don't know how we can prove for certain that a single header only without having to compare it with anything is sufficient in various setups. I think we should take even OWASP published content as not necessarily 100% correct, possibly applicable to certain deployments only.

Well, my understanding is that if there's proof that a user could set a custom header (any header, of any value) then it can't be a CSRF attack in the browser.

I am, of course, skeptical of my understanding, but I'm also skeptical about your assertion that OWASP would get things wrong more than we would. I am also skeptical of any security benefit of requiring more checks than necessary, as this introduces usability issues inevitably.

[ia3andy]

As announced, here is the integration test in Renarde: quarkiverse/quarkus-renarde#191 and it fails with 3.6.5

[takesson]

I ran into this issue a couple of days ago when using HTMX. I doubted my code/template for a while before finding this issue.

I can confirm that 3.6.7 resolved the issue for my use-case.

Not sure I am qualified but I made a couple of comments in #38229 related to the concerns raised by @FroMage.