I originally reported this to the security team, but they cleared it to be discussed in public.

Problem/Motivation

The routing system of Drupal Core offers a CSRF protection mechanism (mainly) for GET requests. This mechanism adds a CSRF token as a query parameter to the URL. There are two (related) problems:

1. Sensitive information within URLs may be logged in various locations, including the user's browser, the web server, and any forward or reverse proxy servers between the two endpoints. URLs may also be displayed on-screen, bookmarked or emailed around by users. They may be disclosed to third parties via the Referer header when any off-site links are followed. Placing session tokens into the URL increases the risk that they will be captured by an attacker. Therefore, it is also an OWASP recommendation not to place CSRF tokens in the URL. If an attacker is able to intercept the CSRF token of the user, the attacker is able to conduct a CSRF attack on the specific user for the specific protected action.

2. GET requests should (by specification) never be used for server-side state-changing actions in the first place. Nevertheless, Core and many contributed modules do so, although even Drupal's own documentation stated that this is bad practice in the past.

CSRF tokens in GET requests are therefore not only somewhat insecure (1.), but shouldn't also never be necessary (2.) if strictly following the HTTP specification and secure coding guidelines.

Therefore, it is in my opinion questionable whether this core feature should even exist in its current form since it encourages the usage of GET requests for server-side state-changing actions. Currently, the feature only seems to exist to make a bad practice a bit (or much) "less bad".

Proposed resolution

tbd

Remaining tasks

  • Discuss if this feature should still exist
  • Discuss potential impact of removal of feature
  • ...

Comments

nico.b created an issue. See original summary.

prudloff’s picture

prudloff commented
Issue tags: +Security improvements
msankhala’s picture

msankhala commented

I had a security scan with Burp Suite for a Drupal 10 website and Burp suite also reported the same issue with Medium severity. Below is one sample of such issue reported by Burp suite.

1. Medium severity issues
Next
1.1. Session token in URL
Next

There are 3 instances of this issue:

/
/user/dashboard
/user/edit-profile

Issue background
Sensitive information within URLs may be logged in various locations, including the user's browser, the web server, and any forward or reverse proxy servers between the two endpoints. URLs may also be displayed on-screen, bookmarked or emailed around by users. They may be disclosed to third parties via the Referer header when any off-site links are followed. Placing session tokens into the URL increases the risk that they will be captured by an attacker.

Issue remediation
Applications should use an alternative mechanism for transmitting session tokens, such as HTTP cookies or hidden fields in forms that are submitted using the POST method.

Vulnerability classifications
CWE-200: Information Exposure
CWE-384: Session Fixation
CWE-598: Information Exposure Through Query Strings in GET Request
CAPEC-593: Session Hijacking


1.1.1. /
Next
Summary
Severity:  	Medium
Confidence:  	Firm
Host:  	
Path:  	/
Issue detail
The response contains the following links that appear to contain session tokens:
/admin/flush/plugin?token=byJZOaxpUgF1dZRzuNCjxHdTmTU5y_YOkn1fFcdDGVQ
/admin/flush/rendercache?token=dlTbfFgJ2GZEybPTTMH57iaBdzIYTdJG7UNenPdSzrQ
/admin/flush/theme_rebuild?token=reqwFvxP-LTFodvzyPG6t-sXN9wb3QAofyFFjHV3WWY
/run-cron?token=QF0VrLooJ81Z_0mRNoxnS3SamlcyMD0inN2NChxxnkk
/admin/flush/menu?token=eALtewSxjWVO7SCEn0GZxu8LUJaxkXlo20OKKabBaJs
/admin/flush?token=xIXNfpSObi4TNVGsoIYiUr13531hbaNpq6qhKBNiYbc
/admin/flush/twig?token=qzFQkY40jfUxponYKRl0gj3FnZXiKdqZCnOy_fO0pOo
/admin/flush/static-caches?token=dTWgAZ27g-hR_D4Lb2MrFXSyZATqMG_Vjd2Y0JC6u1s
/admin/flush/views?token=XLrOpUmAxNMPG2xrjvDawQPgLfLKGSL99_9YOhpzYsg
/admin/flush/cssjs?token=2KlTAMSzJcSa3XFs4gh5hvn186leaGHNVushmCSydR8
joachim’s picture

joachim commented

I've attempted to make a way of doing this with POST instead of GET -- it uses a button which is styled to look like a link, and the CSRF token is in a hidden input value element.

Proof-of-concept module at https://github.com/joachim-n/link_csrf_post_poc. Feedback welcome!

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

prudloff’s picture

prudloff commented

While I agree that having state-changing GET requests is not a best practice, I don't think it would be realistic to remove the existing CSRF token feature anytime soon.

I think we would need to come up with a long term plan and handle this in multiple followup issues.
Maybe something like this:

  1. Provide an easy way to protect a POST route with CSRF (something like @joachim suggested). IMHO it should be as easy to use as the _csrf_token requirement.
  2. Avoid setting a bad example in core and replace state-changing GET routes with POST routes.
  3. Maybe log a warning if a CSRF token is used on a GET route (suggesting to use another HTTP method instead)?
  4. Re-evaluate deprecating the _csrf_token requirement.

The main risk of doing this too quickly would be to have modules that keep using state-changing GET requests but simply don't protect them against CSRF (which would be way worse than the current situation).

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

I appreciate the effort here. I think the ideas from #6 are a strong proposal.