Page MenuHomePhabricator

XSS in object descriptions from tabular data
Closed, ResolvedPublic

Description

Discovered while investigating T155216: Wikimarkup is shown as raw text instead of html on marker click for externaldata page. Minimal repro, easiest to achieve with commons_datasets Vagrant role:

On shared wiki, create a page called Data:XSS.map:

{
    "license": "CC0-1.0",
    "description": {
        "en": "blah"
    },
    "sources": "blah",
    "zoom": 13,
    "latitude": -34.75315,
    "longitude": 149.71575,
    "data": {
        "type": "FeatureCollection",
        "features": [
            {
                "type": "Feature",
                "properties": {
                    "title": {
                        "en": "<span onclick='javascript:alert(document.cookie)'>Click me</span>"
                    }
                },
                "geometry": {
                    "type": "Point",
                    "coordinates": [
                        149.713056,
                        -34.7475
                    ]
                }
            }
        ]
    }
}

Then, either on the same or client wiki, create the following page:

<mapframe width=500 height=500>
{
  "type": "ExternalData",
  "service": "page",
  "title": "Test.map"
}
</mapframe>

Upon saving and clicking on affected marker then on "click me", XSS is triggered. Doesn't seem to work with crude <script> injection.

Event Timeline

MaxSem created this task.Apr 17 2017, 11:17 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 17 2017, 11:17 PM
MaxSem added subscribers: dpatrick, Bawolff.

OK, so fixing it with local (client wiki) parsing is really easy (swap two lines). However, T155216 calls for parsing on central wiki which would require some serious mucking around. Question for @Bawolff or @dpatrick: from a security standpoint, does it make sense to always sanitize on client wikis, closer to output and not trust blindly whatever was received from another wiki?

MaxSem claimed this task.Apr 17 2017, 11:32 PM
MaxSem moved this task from Backlog to In progress on the Maps-Sprint board.
MaxSem updated the task description. (Show Details)Apr 17 2017, 11:36 PM

For reference, GlobalUserPage does parsing on the central wiki and the local wiki blindly trusts the HTML it's given. At that time, Chris said it was OK provided it was explicitly documented (https://www.mediawiki.org/wiki/Extension:GlobalUserPage#Caveats).

Ideal case is to sanitize on client wiki as close to output as possible. I consider it ok to sanitize/parse on the central wiki provided there is a reasonable rationale to do so (In the case of GlobalUserPages or file descriptions on commons, the rationale is that users expect templates/etc to render like the central wiki). The central wiki of course must be a WMF controlled wiki with raw html disabled, etc.

Ugh, that also invokes other types of sanitization, like T134719: Kartographer has an XSS using magic javascript __proto__ property in GeoJson description. I propose that we plug the hole with a simple patch that implements sanitization on client wikis and then work on a proper cross-repository patch to move parsing to the central wiki.

I'd prefer to keep parsing on the central wiki for the reasons noted in T155216, so unless there are issues I think @MaxSem's proposal is the way to go.

MaxSem updated the task description. (Show Details)Apr 18 2017, 10:00 PM

Upon saving and clicking on affected marker then on "click me", XSS is triggered. Doesn't seem to work with crude <script> injection.

For reference, for these types of things, often <img src=x onerror="alert(1)"> will work in places where a <script> won't.

Patches for Kartographer and JsonConfig, please review:

MaxSem added a subscriber: Yurik.Apr 24 2017, 8:47 PM
Yurik added a comment.Apr 24 2017, 8:52 PM

@MaxSem, the code seems to be ok, but i haven't fully tested it.

Patches deployed. Can someone from security OK the publication of patches/declassification of this bug?

dpatrick triaged this task as High priority.Apr 25 2017, 9:02 PM

It looks like JsonConfig now cannot be used separately from Kartographer. Is this just temporary @MaxSem?

Only specific configurations that sysadmins decided to use are affected.

dpatrick added a comment.EditedApr 27 2017, 1:48 AM

I'm speaking in terms of the second patch above, to JsonConfig, where a dependency on Kartographer\SimpleStyleParser is introduced in includes/JCMapDataContent.php. The fix looks good, but do we need to notify users of this new coupling, or was it already assumed and understood that anyone using JsonConfig would also be using Kartographer? The docs at https://www.mediawiki.org/wiki/Extension:JsonConfig make it seem like it's a general purpose tool.

Only specific configurations that sysadmins decided to use are affected.

I'm taking a second pass at your comment. I think I understand what you're getting at, which is that JCMapDataContent is only used in context of Kartographer anyway. Am I understanding that correctly?

Yes. JC doesn't do anything by default, so to ever execute these code paths, a conscious decision to configure it that way is required.

What is outstanding here? The task that depends on this fix is fairly urgent. :-)

@MaxSem, you can go ahead and release and open this bug. I have no further comments. The fix looks good.

MaxSem changed the visibility from "Custom Policy" to "Public (No Login Required)".May 2 2017, 9:44 PM

Change 351522 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/Kartographer@master] SECURITY: fix XSS in map feature title/description via tabular data

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

Change 351523 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/JsonConfig@master] SECURITY: fix XSS in map feature title/description

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

Change 351523 merged by jenkins-bot:
[mediawiki/extensions/JsonConfig@master] SECURITY: fix XSS in map feature title/description

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

Change 351522 merged by jenkins-bot:
[mediawiki/extensions/Kartographer@master] SECURITY: fix XSS in map feature title/description via tabular data

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

MaxSem closed this task as Resolved.May 2 2017, 10:25 PM

Change 351536 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/JsonConfig@REL1_29] SECURITY: fix XSS in map feature title/description

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

Change 351538 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/Kartographer@REL1_29] SECURITY: fix XSS in map feature title/description via tabular data

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

Change 351536 merged by jenkins-bot:
[mediawiki/extensions/JsonConfig@REL1_29] SECURITY: fix XSS in map feature title/description

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

Change 351538 merged by jenkins-bot:
[mediawiki/extensions/Kartographer@REL1_29] SECURITY: fix XSS in map feature title/description via tabular data

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

Change 351916 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/Kartographer@wmf/1.29.0-wmf.21] SECURITY: fix XSS in map feature title/description via tabular data

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

Change 351916 merged by jenkins-bot:
[mediawiki/extensions/Kartographer@wmf/1.29.0-wmf.21] SECURITY: fix XSS in map feature title/description via tabular data

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

Change 351919 had a related patch set uploaded (by MaxSem; owner: MaxSem):
[mediawiki/extensions/JsonConfig@wmf/1.29.0-wmf.21] SECURITY: fix XSS in map feature title/description

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

Change 351919 merged by jenkins-bot:
[mediawiki/extensions/JsonConfig@wmf/1.29.0-wmf.21] SECURITY: fix XSS in map feature title/description

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