Problem/Motivation

Originally reported to the Drupal Security Team. We discussed internally and we could find no way to exploit this remotely - the progress bar is controlled from JavaScript or server side PHP - so we agreed this can be fixed in public.

Thanks to Tom Konda for reporting this originally.

Steps to reproduce

You can see this vulnerability by:
1. A developer write following JavaScript code. (This code is based on batch.js)
2. Display any page which loads this library.
3. An alert dialog is appeared.

  // This value is assumed to be retrieved from drupalSettings.
  const paramFromDrupalSettings = `"><img src="#" onerror="alert('XSS');" /></div><div id="mal`;

  const $xssTest = $('#target');
  const pb = new Drupal.ProgressBar(paramFromDrupalSettings);
  $xssTest.empty();
  $xssTest.append(pb.element);

Proposed resolution

Escape the ID in Drupal.theme.progressBar().

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3426514

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:

Comments

longwave created an issue. See original summary.

longwave’s picture

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

magaki’s picture

Status: Active » Needs review

I changed the assigned string in the progress bar id attribute to the escaped strings.
And, I checked the work using javascript similar to the steps to reproduce, no dialog appeared.

The test fails, but it doesn't seem to be related to the changes.
https://git.drupalcode.org/issue/drupal-3426514/-/jobs/1233839#L778
Reran the pipeline again, and the test passed.

Please review it.

smustgrave’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs Review Queue Initiative

Based on the issue summary seems like something that a test can be written for.

Tagging for issue summary as the proposed solution appears to be empty

longwave’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update

Given #2972776: [policy, no patch] Better scoping for bug fix test coverage I am not sure it is worth the effort of writing a test for this. The fix is trivial and constructing test coverage for it is going to be much more work. The security improvement is minimal - someone explicitly has to inject a weird ID for it to be exploitable - so I think we just fix this and move on, unless anyone disagrees?

  • nod_ committed 1c9e2cff on 11.x
    Issue #3426514 by magaki, Tom Konda, longwave: Drupal.theme.progressBar...

  • nod_ committed 8464ab1a on 10.3.x
    Issue #3426514 by magaki, Tom Konda, longwave: Drupal.theme.progressBar...

  • nod_ committed e28809ff on 10.2.x
    Issue #3426514 by magaki, Tom Konda, longwave: Drupal.theme.progressBar...
nod_’s picture

Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 1c9e2cf and pushed to 11.x. Thanks!

If it was more serious it wouldn't be public, so +1 to tests not needed.

Status: Fixed » Closed (fixed)

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