| Grunny | |
| Sep 1 2015, 11:22 AM |
| F2535324: T111029-2.patch | |
| Sep 3 2015, 5:29 AM |
| F2525442: T111029.patch | |
| Sep 1 2015, 11:24 AM |
| F2525396: PageTriageXSS.png | |
| Sep 1 2015, 11:22 AM |
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:
| Subject | Repo | Branch | Lines +/- | |
|---|---|---|---|---|
| Add extra escaping in template | mediawiki/extensions/PageTriage | master | +1 -1 |
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | None | T108064 MediaWiki Security release 1.25.3 | |||
| Resolved | Catrope | T111029 XSS possible in PageTriage toolbar |
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, 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.
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).
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:
:)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.Change 250830 had a related patch set uploaded (by CSteipp):
Add extra escaping in template