Page MenuHomePhabricator

Reflected XSS on https://cluebotng.toolforge.org?page=View&id=
Closed, ResolvedPublicSecurity

Description

Summary

The ClueBot NG Report Interface is susceptible to reflected XSS attacks by way of the id parameter wherein HTML is returned unescaped in the report subtitle.

<div id="top">
  <div class="floatleft"><h1 id="title">ClueBot NG Report Interface</h1></div>
  <div class="floatright">
    <h1 id="subtitle">
      // Viewing 12345"&gt;<img src="x" onerror="alert(document.cookie)" />
    </h1>
  </div>
</div>

An attacker can execute arbitrary JavaScript in the context of another user on https://cluebotng.toolforge.org and perform actions as that user by fetching their session cookie.

In this case, I imagine the impact is limited to posting comments on ClueBot false positive reports and toggling the "redirect on review" setting for that user.

Relevant pointer: https://github.com/cluebotng/report/blob/397a2fc5dc6d480d59f91b0bc6523e84164d367b/pages/View.page.php#L55

public function writeHeader()
    {
        echo 'Viewing ' . $this->id;
    }

Environment

  • Firefox 93.0a1 (2021-08-23)

Steps to Reproduce

  1. Browse to https://cluebotng.toolforge.org/?page=View&id=12345%22%3E%3Cimg%20src=x%20onerror=alert(document.cookie)%3E.
  1. Observe the alert() dialog with the user's PHPSESSID cookie (flags: HttpOnly false, Secure false).

Details

Risk Rating
Medium
Author Affiliation
Wikimedia Communities

Event Timeline

Hi, just reporting a small finding which I wanted to flag after looking around WMF Toolforge. Please let me know if you require any additional info.

sbassett edited projects, added Toolforge; removed Cloud-Services.
sbassett changed Risk Rating from N/A to Medium.
sbassett edited projects, added Cloud-Services, SecTeam-Processed; removed Toolforge.
sbassett added subscribers: Cobi, DamianZaremba, RichSmith.

Thanks for this, unless Damian beats me to it I will attempt to sort this tonight

I didn't audit the rest of the code, but this patch should fix the immediate issue around escaping $this->id:

(this could easily be applied and submitted as a GH pull request, I just didn't want to go that route since I can't merge such requests and did not want to immediately and publicly disclose a security vulnerability.)

I've just seen a GitHub commit by Damian, he's on it :)

I deployed https://github.com/cluebotng/report/releases/tag/v1.0.3 which should sort it.

$ fab deploy-report
Moving report to v1.0.3
HEAD is now at 397a2fc Merge pull request #6 from cluebotng/move-to-oauth
From https://github.com/cluebotng/report
   397a2fc..5d7cd9e  main       -> origin/main
 * [new tag]         v1.0.3     -> v1.0.3
Previous HEAD position was 397a2fc Merge pull request #6 from cluebotng/move-to-oauth
HEAD is now at 5d7cd9e Explicitly cast id to int & escape in header
Upgrading to version 2.1.6 (stable channel).
   
Use composer self-update --rollback to return to version 2.1.5
Installing dependencies from lock file (including require-dev)
Verifying lock file contents can be installed on current platform.
Nothing to install, update or remove
Generating autoload files
16 packages you are using are looking for funding.
Use the `composer fund` command to find out more!

That logic hasn't changed in (literally) 10 years so I wouldn't be overly concerned with a pull request sitting there for a couple of days.

sbassett changed Author Affiliation from N/A to Wikimedia Communities.
sbassett changed the visibility from "Custom Policy" to "Public (No Login Required)".
sbassett changed the edit policy from "Custom Policy" to "All Users".