Background information

This was originally reported as a private security issue, but has been approved for handling in the public queue by the Drupal Security Team.

Problem/Motivation

This check in ajax.js can be bypassed if the URL corresponds to an existing property of the ajaxTrustedUrl object (for example .constructor) :

if (drupalSettings.ajaxTrustedUrl[originalUrl]) {

In order to exploit this an attacker would need the following permissions:

  • Create redirects
  • Upload files (any extension would work)
  • Insert HTML with data attributes (or be able to set the URL of some existing AJAX call)

Steps to reproduce

1. Upload a .txt file containing containing this:

[{"command":"insert","method":"html","data":"<img src=x onerror=alert(0)>", "selector": "body"}]

2. Create a new redirect from /constructor to the URL of the uploaded file (for example with the redirect module).
3. Insert this HTML into a page:

<a class="use-ajax" href="constructor" data-ajax-http-method="GET">Click me</a>

4. When the link is clicked, the uploaded JSON is used as a trusted response and the JS is executed.

Proposed resolution

I suppose the easiest fix would be for core to add something like this:

drupalSettings.ajaxTrustedUrl.hasOwnProperty(originalUrl)

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3600777

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

prudloff created an issue. See original summary.

prudloff opened merge request !16060

kieran.cott made their first commit to this issue’s fork.

kieran.cott opened merge request !16061

kieran.cott’s picture

kieran.cott
Location Manila 🇵🇭
commented

I've opened an MR for this issue here - https://git.drupalcode.org/project/drupal/-/merge_requests/16061. It's similar in function to https://git.drupalcode.org/project/drupal/-/merge_requests/16060, but instead resolves the issue by use of a helper method and adds test coverage by:

  1. Adding an ajax_test regression page with a use-ajax link to constructor.
  2. Adding an untrusted JSON response route that returns an AJAX insert command without the verification header.
  3. Adding a functional JavaScript regression test asserting the untrusted response is rejected and not processed.

I had already started working on this when MR 16060 was opened, so I've committed the work separately and will leave it to the maintainers/community to choose their preferred path forward.

prudloff’s picture

prudloff commented

Maybe wait a bit before jumping on new issues with your LLM...
Anyway, I finished adding test coverage to my MR so now the two are very similar and we need to decide which one to keep.

kieran.cott’s picture

kieran.cott
Location Manila 🇵🇭
commented

Happy to keep yours - closing MR.

kieran.cott closed merge request !16061

prudloff’s picture

prudloff commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Ran the test-only job since I can't in the pipeline

A status message of type "error" containing "Oops, something went wrong. Check your browser's developer console for more details." does not appear on this page, but it should.
Failed asserting that null is not null.
/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php:651
/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/Ajax/AjaxTest.php:384

Was curious if failOnJavascriptConsoleErrors was needed but test fails without it failOnJavascriptConsoleErrors

Change LGTM.

godotislate made their first commit to this issue’s fork.

  • godotislate committed 996580c9 on 11.x 
    fix: #3600777 Redirects can be used to bypass AJAX trusted URLs

By:...

  • godotislate committed 2aea80d1 on main 
    fix: #3600777 Redirects can be used to bypass AJAX trusted URLs

By:...
godotislate’s picture

godotislate
he/him
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

The new routing entries could have been attributes on the methods in the Controller class, but I don't think that's worth blocking this on. We can convert them together with the rest of the module routes in a separate issue.

Committed and pushed 2aea80d to main and 996580c to 11.x. Thanks!

This probably can go to 11.4.x as well, but will consult other committers first.

  • godotislate committed 2ef86a51 on 11.4.x 
    fix: #3600777 Redirects can be used to bypass AJAX trusted URLs

By:...
godotislate’s picture

godotislate
he/him
commented
Version: 11.x-dev » 11.4.x-dev
Status: Patch (to be ported) » Fixed

Consulted with @longwave on Slack, and this is good for 11.4. Committed 2ef86a5 and pushed to 11.4.x.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.