CSRF token not always updated with XF.KeepAlive.refresh()

[digitalpoint]

Affected version

2.2.12

Ran into this for something else, but it's also the same reason for this bug report:

Logout bug when Guest caching is enabled

The issue described here https://xenforo.com/community/threads/guest-page-caching.164816/ has been existing for a long time with no resolution .. I tried doing some troubleshooting to see the reason behind it because it was really annoying , I found the bug is very simple at method...

xenforo.com

XF.KeepAlive.refresh() updates XF.config.csrf and hidden input fields containing csrf, but it does not update URLs with t={csrf_token}. Things like Logout button, the advanced cookie consent buttons, language selector, style selector and a few other things.

XF.KeepAlive is an anonymous function so there wasn't a real way to extend it to fix it for now. However I did come up with an extremely hacky way to do it (using JavaScript Proxy() to monitor for XF.config.csrf changes).

Either way, if XenForo is supposed to update csrf with XF.KeepAlive.refresh(), it should do it in all the places, not just some.

 

[K a M a L]

This is not the same reason for https://xenforo.com/community/threads/logout-bug-when-guest-caching-is-enabled.200977/ ..because user gets logged out actullay (so CSRF token is valid and works ) but the cached version of page showing user avatar and username is still displayed instead of the logged out page.

 

[digitalpoint]

Oh okay… well another issue I guess.

 

[Kirby]

Hmm .. updating tokens in links could be problematic, it would have to take care to not update "wrong" links (like links that did not have a valid token initially which seems complicated as the client cannot validate old tokens) - otherwise this could create a vulnerability and / or break links.

[DigitalPoint] App for Cloudflare®

I should have been clear... not a realistic way to do it at the edge. Cache Keys still wouldn't be ideal because you are limited in scope as to what you can logic on (even for Enterprise customers which probably no one here is). It could be done with Workers, but yes... that gets cost...

xenforo.com

IMHO GET requests should not change state and thus should not require tokens to begin with.

So instead of updating tokens on links a different approach might be better:

• Get rid of the t parameter
• Add a click handler like data-xf-click="submit-as-post" that turns those links into POST when clicked
• Add a fallback confirmation if the link is accessed as GET like is is already being done for changing style:
  April Style

 

[digitalpoint]

Similar to one of @Kirby's ideas, I've been thinking the simplest way to handle it (without fundamental changes) is to have a click handler that simply adds the token to the URL when the URL is clicked. Something like, data-xf-click="csrf", which then simply adds t={XF.config.csrf}. Even better would be to use the same parameter that is used for AJAX requests, _xfToken={XF.config.csrf}, for sake of consistency.

So instead of hard-coding the t parameter in HTML source, give those links a click handler instead that handles it. Afterall, XF.config.csrf is available via JavaScript (and then you also don't need to actually do anything else to handle changing CSRF tokens via XF.KeepAlive).

 

[Kirby]

I've been thinking the simplest way to handle it (without fundamental changes) is to have a click handler that simply adds the token to the URL when the URL is clicked. Something like, data-xf-click="csrf", which then simply adds t={XF.config.csrf}

Hmm, not sure if I like that approach

• I does not degrade nicely if JavaScript is not available
  While it is somewhat debatable if requiring JavaScript for "basic" things is acceptable or not, I think if it isn't too difficult to provide a fallback it should be done
• It does not work if the user chooses to open the link in a new tab or window
  This does work with standard XenForo
  IMHO this is a more serious limitation than requiring JavaScript
• It still seems conceptionally wrong to me - GET should not modify state

 

[digitalpoint]

Hmm, not sure if I like that approach

• I does not degrade nicely if JavaScript is not available
  While it is somewhat debatable if requiring JavaScript for "basic" things is acceptable or not, I think if it isn't too difficult to provide a fallback it should be done

Yep, although this guy made a good point.

Like it or not, expecting JavaScript to be enabled is reasonable in 2020.

• It does not work if the user chooses to open the link in a new tab or window
  This does work with standard XenForo
  IMHO this is a more serious limitation than requiring JavaScript

Yep, agreed.

• It still seems conceptionally wrong to me - GET should not modify state

Also agree, just trying to think about working around without fundamental changes, that's all.

 

[digitalpoint]

The best solution imo is simply to convert the few places that use CSRF in URLs/GET requests to POST requests. No need for JavaScript trickery, no need for one-off handling of tokens passing in as t variable in some controllers and it also fixes the underlying issue of this bug report.

 

[tristan9]

That would certainly make everyone's life easier indeed, though (and maybe there's something else that protects against this here) it seems like it might not be as good a solution as it seems, from https://owasp.org/www-community/attacks/csrf#only-accepting-post-requests ?

 

[Steffen]

CSRF-checking for POST requests is possible without CSRF tokens nowadays: https://xenforo.com/community/threa...re-full-html-page-caching.202315/post-1609573

 

[digitalpoint]

That would certainly make everyone's life easier indeed, though (and maybe there's something else that protects against this here) it seems like it might not be as good a solution as it seems, from https://owasp.org/www-community/attacks/csrf#only-accepting-post-requests ?

Well I wasn’t talking about removing CSRF token, but switching to POST makes the normal CSRF system work like all the other POST requests in XenForo.

 

[digitalpoint]

Just ran into this again for something else. Why exactly are we mutating state with GET requests? These really should be POST requests, not GET requests, imo:

• Logout
• Language chooser
• Style chooser
• Advanced cookie consent
• Profile post moderation

If we stop using GET requests and instead use POST requests, the underlying issue of the CSRF token not being updated when XF.KeepAlive.refresh() is called fixes itself (no longer need to do kludgey CSRF token insertion into URLs).