Page MenuHomePhabricator

Potential XSS on korma.wmflabs.org
Closed, ResolvedPublic

Description

The site korma.wmflabs.org apparently allows XSS injections through mailing list post subjects.

See on http://korma.wmflabs.org/browser/mls.html the thread #1 showed as «1 [Wikitech-l] tags are a usability nightmare for editing on mediawiki.org», where "<translate>" was inserted as an html tag, ie.

<tbody><tr><td>1</td><td><a target="_blank" href="http://www.google.com/search?output=search&amp;q=[Wikitech-l] <translate> tags are a usability nightmare for editing on mediawiki.org%20site%3Ahttp://lists.wikimedia.org/pipermail/wikitech-l&amp;btnI=1">
[Wikitech-l] <translate> tags are a usability nightmare for editing on mediawiki.org</translate></a>&nbsp;
<i class="fa fa-external-link"></i></td><td>Brion Vibber</td><td>25</td></tr>

(the </translate> was probably added by a sanitizer)

Credit goes to @bd808 and @Krenair for figuring out that http://korma.wmflabs.org is really contributors-metrics.contributors.eqiad.wmflabs and thus belongs to the contributors project.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 18 2016, 8:32 PM
Platonides assigned this task to Qgil.Apr 18 2016, 8:32 PM
Platonides added a subscriber: Acs.

@Qgil Is source code for this publically available somewhere?

Aklapper claimed this task.Apr 19 2016, 4:40 AM
Aklapper added subscribers: Dicortazar, Lcanasdiaz, Qgil.

The downstream code of that very page is https://github.com/Bitergia/mediawiki-dashboard/blob/master/browser/mls.html , its upstream source code is https://github.com/Bitergia/grimoire-dashboard/blob/master/browser/mls.html , and the backend code for "mls" stuff should be mostly in https://github.com/VizGrimoire/GrimoireLib

@Dicortazar and @Lcanasdiaz (upstream developers) know way better and can correct me here.

csteipp triaged this task as Medium priority.Apr 19 2016, 9:11 PM
csteipp moved this task from Backlog / Other to External (Non-WMF) Issues on the acl*security board.

@Aklapper Just to clarify, based on your comments, I assume upstream has been notified of this. Is that correct?

Dicortazar added a comment.EditedJun 12 2016, 11:37 AM

Hi there, sorry for the delay.

We were notified @Bawolff and we're still deciding how to fix this.

Another point is that we're starting to migrate the platform to the new toolchain, and this is close to be 'legacy' at some point, so we do not expect to spend a lot of effort on the old version.

In any case data-related issues and maintenance needs are always welcome :).

Aklapper reassigned this task from Aklapper to Dicortazar.Jun 15 2016, 3:58 PM

Based on the bug we will perform two actions:

  1. we will sanitize the subject of the mails
  2. we will avoid this error in the new dashboard already being cooked

I'll do it next week. Thanks for the feedback guys.

Bug fixed in VizGrimoireJS version 16.04-3-g74622e3. It is already deployed in production.

https://github.com/VizGrimoire/VizGrimoireJS-lib/commit/74622e3c0bda1bc8889ea088ce7a88d4081e4e4d

Feel free to review it @Platonides

Does that mean it's been deployed to korma.wmflabs.org?
If so, is this task ready for release?

¡Gracias, Luis!

@Krenair: seems the deployment would have to have been done by either Ryan Lane, @Qgil, @Acs, @jgbarah or @Andrew

Why did the one updating korma not comment here? :(

Does that mean it's been deployed to korma.wmflabs.org?
If so, is this task ready for release?

Yep, it is already deployed. I did it myself :) (I've been maintaining this dashboard for more than a year so I have the access).

The url parameter doesn't look escaped. What if someone wrote an email with the subject

"><script>alert(1)</script><

Or what about the case where opts.links_enabled is false?

Also what about threads_data.initiator_name[i] (I assume that's just the From header of the email and thus user controlled).

There's a large number of other examples of making html with string concatenation, where its unclear (to me) whether or not the variables could potentially be controlled by an attacker. It would be nicer if there was escaping on all of them, just to be safe.

The url parameter doesn't look escaped. What if someone wrote an email with the subject

"><script>alert(1)</script><

Or what about the case where opts.links_enabled is false?

Yeah. You are absolutely right.

Commit was reverted and pushed with the fix of the fix :P

https://github.com/VizGrimoire/VizGrimoireJS-lib/commit/c736a66b264377042990d24996807322029625b1

Also what about threads_data.initiator_name[i] (I assume that's just the From header of the email and thus user controlled).

This is managed by the data collection tool, which does not allow HTML in this field.

There's a large number of other examples of making html with string concatenation, where its unclear (to me) whether or not the variables could potentially be controlled by an attacker. It would be nicer if there was escaping on all of them, just to be safe.

Yeah. There are more potential security holes, but I'm reluctant to patch them because our intention is to replace the whole dashboard with the new one (based on Kibana) during the next month.

In any case, if you guys are worried about the topic, we are open to discuss it.

Data is being generated now, the new version will be pushed automatically as soon as it finishes.

Yeah. There are more potential security holes, but I'm reluctant to patch them because our intention is to replace the whole dashboard with the new one (based on Kibana) during the next month.

That sounds reasonable to me. The dashboard is on a *.wmflabs.org domain, and not a domain that has sensitive data on it, so I'm not super-worried.

Commit was reverted and pushed with the fix of the fix :P
Data is being generated now, the new version will be pushed automatically as soon as it finishes.

@Lcanasdiaz: Does that mean this task is resolved by now? If so, feel free to update its status. :)

Ping?

Sorry for the delay. Yes, we can close the ticket.

Aklapper closed this task as Resolved.EditedAug 30 2016, 9:51 AM

This is deployed and resolved.

Making this task public.

Aklapper changed the visibility from "Custom Policy" to "Public (No Login Required)".Aug 30 2016, 9:52 AM
Aklapper changed Security from Software security bug to None.