Review our CSRF policy for better UX

[AdrienClairembault]

There are two ways to handle CSRF tokens:

• Generate single token per session (a bit less secure but better for UX).
• Generate a new token for each POST action (more secure, but worse for UX as user that press the "back" button on their browser won't be able to submit a form).

Source: https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#token-based-mitigation

CSRF tokens should be generated on the server-side and they should be generated only once per user session or each request. Because the time range for an attacker to exploit the stolen tokens is minimal for per-request tokens, they are more secure than per-session tokens. However, using per-request tokens may result in usability concerns.

For example, the "Back" button browser capability can be hindered by a per-request token as the previous page may contain a token that is no longer valid. In this case, interaction with a previous page will result in a CSRF false positive security event on the server-side. If per-session token implementations occur after the initial generation of a token, the value is stored in the session and is used for each subsequent request until the session expires.

The problem with GLPI is that we implement both of these solutions, meaning we get both drawbacks (less secure and worse UX) and none of the benefits.

Indeed, we have a specific per token form that will result in the "bad UX" downside of a broken "back" action (I think we've all experienced it) but it doesn't come with the benefits of extra protection because we also have reusable tokens for AJAX requests that are easily retrieved and could be used without limits on any forms by a malicious user.

Since we have no extra security benefits, maybe we could switch to a single per session token to improve our UX at no security cost?

I'm opening this discussion because I've noticed that this behavior make the new "Import form" feature less convenient to use. Indeed, this action is based on multiple chained POST actions and as a user it is very tempting to press the browser's back button to go back to the previous step (which mean you lose all progress and have to start your import again as you will fail CSRF check). Obviously this applies everywhere on GLPI, the benefits would not be limited to this feature as this is a very common issue.

This could be done quite easily on the 11.0 branch, as the change to make this effective would be very minimal and completely safe:

diff --git a/src/Session.php b/src/Session.php
index cd6cae44c5..d57b564cc0 100644
--- a/src/Session.php
+++ b/src/Session.php
@@ -1711,9 +1711,6 @@ class Session
         }
         $requestToken = $data['_glpi_csrf_token'];
         if (isset($_SESSION['glpicsrftokens'][$requestToken])) {
-            if (!$preserve_token) {
-                unset($_SESSION['glpicsrftokens'][$requestToken]);
-            }
             return true;
         }

This would bring a UX improvement right away.

Then on the main branch the code could be reworked more efficiently to remove the useless preserve_token parameter and make sure only a single token is generated (i.e. delay riskier changes to the next major to avoid any bugs).

[cedric-anne]

Having per-request tokens prevent the user to accidentally submit the same form twice, but, as we have a data-submit-once attribute in most of ours forms, I guess it will have an impact only on specific forms.

Anyway, as long as we can use a token in any form (i.e. we can get token from the simplified interface and use it in any admin form), our tokens cannot be considered as per-request tokens and are giving no more security than per-session tokens.

So I am not against this change.

[AdrienClairembault]

Anyway, as long as we can use a token in any form (i.e. we can get token from the simplified interface and use it in any admin form), our tokens cannot be considered as per-request tokens and are giving no more security than per-session tokens.

Exactly, this is totally what I meant. We get the downside of the broken "back" action but no additional protection compared to a single token mode.

Still, even if this seems completely risk-free I am afraid of doing it now that we are already in RC. I guess it depends on how much we think improving this UX side is an important issue, if we don't care much about it then we can wait for GLPI 11.1.

What's your opinion @orthagh?

[orthagh]

we are already in RC

Indeed, not the time to make this kind of changes So not against it but later.