Page MenuHomePhabricator

Kartographer has an XSS using magic javascript __proto__ property in GeoJson description
Closed, ResolvedPublic

Description

Perhaps not really kartographer's fault, this is something that ResourceLoader should probably be sanitizing. But Kartographer should maybe be more careful in its sanitizing unrecognized input too.

Consider the following Kartographer invocation:

<mapframe width="350" height="350" zoom="13" longitude="-122.39902496337889" latitude="37.80151060070086">
{
  "type": "FeatureCollection",
  "features": [
    {
      "type": "Feature",
      "properties": { "__proto__": {
        "marker-symbol": "hospital",
        "marker-color": "302060",
        "title": "",
        "description": "<img onerror=\"alert('xss!')\" src=x>"
}
      },
      "geometry": {
        "type": "Point",
        "coordinates": [
          -122.41816520690917,
          37.79097260220947
        ]
      }
    },
  ]
}
</mapframe>

Then click on the plus icon.

For some reason this doesn't seem to work when using <script> tags directly. I can't for the life of me figure out why. If anyone knows why it doesn't work with script, please tell me.

Kartographer does not recurse into property names it doesn't recognize during sanitation. Thus everything in "__proto__" is untouched. But browsers notice __proto__ and use it to override the object's prototype. Thus the property is taken into account as if it was directly set on the object, due to js inheritance.

  • The Javascript escaping functions in Xml class, should probably ban property names starting with double underscore
  • Maybe SimpleStyleSanitizer in Kartographer should rethink its decision to allow unrecognized properties through.

Event Timeline

Bawolff created this task.May 9 2016, 5:18 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 9 2016, 5:18 AM
MaxSem claimed this task.May 9 2016, 5:12 PM
MaxSem moved this task from Unsorted to General on the Maps (Kartographer) board.
MaxSem added a comment.May 9 2016, 6:43 PM

MaxSem added a comment.May 9 2016, 6:55 PM

Maybe SimpleStyleSanitizer in Kartographer should rethink its decision to allow unrecognized properties through.

We're at the point where we can mostly painlessly do that, however json-schema can't report in a reasonable way what exactly is wrong, as it spams all failures in oneOf branches even though the condition eventually succeeds. For the same reason you can't filter out the non-compliant properties.

Yurik added a comment.May 9 2016, 9:34 PM

@MaxSem the patch looks ok

csteipp triaged this task as High priority.May 10 2016, 9:34 PM
csteipp added a project: Patch-For-Review.

Both Brian and I looked at the patch, and it seemed like it should fix the immediate problem. Max is going to deploy it.

Deployed by @MaxSem,

23:59 logmsgbot: maxsem@tin Synchronized php-1.27.0-wmf.23/extensions/Kartographer/: Security patch (duration: 00m 25s)
23:58 logmsgbot: maxsem@tin Synchronized php-1.28.0-wmf.1/extensions/Kartographer/: Security patch (duration: 00m 26s)

@Bawolff, Max has more patches in Kartographer that will conflict with this patch, so he would like to make it public soon. Do you want to look at doing a core patch for this before then?

@Bawolff, Max has more patches in Kartographer that will conflict with this patch, so he would like to make it public soon. Do you want to look at doing a core patch for this before then?

Yep, here's my patch:

Yep, here's my patch:

It makes me a little nervous to deploy this without review from some of the more frontend developers. Maybe @Krinkle or @matmarex?

Is it likely Kartographer was the only instance of interpreting the json as javascript, instead of using JSON.parse()? If so, we could release that with the security patch, and do this as a public hardening patch.

@Bawolff's patch looks correct to me. The regex check makes me uneasy, but it looks correct, and probably faster than trying to walk the array to detect bad keys. FormatJson looked like a weird place to put it, but it's probably the best one. I can't think of any legitimate reason to use _‌_proto_‌_ in JSON.

Throwing an exception is a bit drastic, but removing the value would require walking the array… actually, maybe we can replace it with something harmless? If we keep the exception, we'll probably want to add some try…catch in JsonContent (and maybe a special error message too) and probably other places.

Is it likely Kartographer was the only instance of interpreting the json as javascript, instead of using JSON.parse()? If so, we could release that with the security patch, and do this as a public hardening patch.

FormatJson takes special care to make the JSON it produces also valid JavaScript (U+2028 and U+2029 are escaped), so I think this is too optimistic. We even do this everywhere in core with Xml::encodeJsVar() and Xml::encodeJsCall(). That said, it's unlikely to result in security vulnerabilities like this one, since it would still require the caller to interpret some data as raw HTML (and the attacker to be able to control object keys).

@Bawolff's patch looks correct to me. The regex check makes me uneasy, but it looks correct, and probably faster than trying to walk the array to detect bad keys. FormatJson looked like a weird place to put it, but it's probably the best one. I can't think of any legitimate reason to use _‌_proto_‌_ in JSON.
Throwing an exception is a bit drastic, but removing the value would require walking the array… actually, maybe we can replace it with something harmless? If we keep the exception, we'll probably want to add some try…catch in JsonContent (and maybe a special error message too) and probably other places.

I guess we could replace it with something else, but then the question becomes what. Ideally we'd just remove it, but i dont feel confident doing that with a regex.

Maybe we could use the regex as a first stage detector - if it triggers only then walk through all keys.

Restricted Application added a project: Discovery. · View Herald TranscriptMay 17 2016, 8:34 PM

Alt version of patch that replaces proto with "proto is not allowed" -

this is something that ResourceLoader should probably be sanitizing

This isn't related to ResourceLoader. RL provides an interface to register and load those modules client-side. It creates a bundle with the JavaScript and CSS files specified in said module.

It has no relation to WikiPage content storage. No relation to arbitrary script tags extensions may create and put in the HTML via OutputPage.

I don't think __proto__ should be excluded by FormatJson. Not by default and not optional. I'm 100% on-board with creating intuitive interfaces that are secure by default, but it's imho more damaging and confusing in the long run to have this handled in FormatJson than to have it handled centrally at all.

From a JSON perspective, there might be no impact. In Chrome/Opera/V8/Node.js, __proto__ doesn't affect the inheritance chain when parsed through JSON.parse() (it stays a literal key as specified, no magic behaviour triggered).

From a JavaScript perspective (and for JSON.parse in browsers other than Chrome/Opera), there is very little impact. It just causes user input to change itself in ways the user input could've already done in other ways (e.g. it could make an object inherit from array instead of object, or make keys appear at a different level within the same structure).

To me it seems the bug is that Kartographer is not doing very well at validating the JSON in the first place. It should make the JSON conform to a known schema when sending it to the client. Especially because it's exported as a JavaScript argument (not as JSON).

If it wants to allow unknown properties to exist in the JSON/wikitext content, that's fine. But then it must silently ignore and remove (or throw on) any unknown property keys when preparing the blob for the client-side.

See also:

this is something that ResourceLoader should probably be sanitizing

This isn't related to ResourceLoader. RL provides an interface to register and load those modules client-side. It creates a bundle with the JavaScript and CSS files specified in said module.
It has no relation to WikiPage content storage. No relation to arbitrary script tags extensions may create and put in the HTML via OutputPage.
I don't think __proto__ should be excluded by FormatJson. Not by default and not optional. I'm 100% on-board with creating intuitive interfaces that are secure by default, but it's imho more damaging and confusing in the long run to have this handled in FormatJson than to have it handled centrally at all.

I think that'd make sense if FormatJson was just a general json formatter, however it also advertises support for sanitizing input in such a way that its safe for inclusion directly as javascript inside HTML. I feel like allowing __proto__ conflicts with the claim that the class is safe for that use.

From a JSON perspective, there might be no impact. In Chrome/Opera/V8/Node.js, __proto__ doesn't affect the inheritance chain when parsed through JSON.parse() (it stays a literal key as specified, no magic behaviour triggered).

Yes, but MediaWiki does not use JSON.parse() generally. Making MediaWiki use JSON.parse() for all its mw.config.set() statements would be just as valid a fix in my opinion.

From a JavaScript perspective (and for JSON.parse in browsers other than Chrome/Opera), there is very little impact. It just causes user input to change itself in ways the user input could've already done in other ways (e.g. it could make an object inherit from array instead of object, or make keys appear at a different level within the same structure).

Allowing data to be interpreted one way in one context but another way in a different context, is fundamentally what causes most security issues.

To me it seems the bug is that Kartographer is not doing very well at validating the JSON in the first place. It should make the JSON conform to a known schema when sending it to the client. Especially because it's exported as a JavaScript argument (not as JSON).

I agree. But I also think its an understandable for someone to think doing what kartographer did was safe, and I think its likely someone else could make the same mistake again.

MaxSem moved this task from Backlog to In progress on the Maps-Sprint board.May 26 2016, 10:12 PM

Ping. We need to decide this.

Krinkle added a comment.EditedJun 2 2016, 7:57 PM

Maybe SimpleStyleSanitizer in Kartographer should rethink its decision to allow unrecognized properties through.

We're at the point where we can mostly painlessly do that, however json-schema can't report in a reasonable way what exactly is wrong

I don't see how error reporting is relevant here. Whether to disallow saving of insecure content is secondary to the whether or not we transfer insecure content from the server to the client.

We don't have error reporting for insecure input for Markdown or wikitext either. And that's common in other applications as well. Rather, it's stripped as part of sanitisation.

As a first step, just filter out unknown properties and send the rest to the client. That solves the security problem and doesn't regress in any other way.

LGTM.

this is something that ResourceLoader should probably be sanitizing

This isn't related to ResourceLoader. RL provides an interface to register and load those modules client-side. It creates a bundle with the JavaScript and CSS files specified in said module.
It has no relation to WikiPage content storage. No relation to arbitrary script tags extensions may create and put in the HTML via OutputPage.
I don't think __proto__ should be excluded by FormatJson. Not by default and not optional. I'm 100% on-board with creating intuitive interfaces that are secure by default, but it's imho more damaging and confusing in the long run to have this handled in FormatJson than to have it handled centrally at all.

I think that'd make sense if FormatJson was just a general json formatter, however it also advertises support for sanitizing input in such a way that its safe for inclusion directly as javascript inside HTML. I feel like allowing __proto__ conflicts with the claim that the class is safe for that use.

I support a patch to core for this reason.

Guys, this is a serious issue and I'm uncomfortable to keep it in a patch for a long time and risk it being misapplied to a new branch. Unless we have a definite plan for addressing the general problem, I'd like to disclose and commit this.

@MaxSem, I don't think we're going to come to consensus in short order. Go ahead and disclose and release so that you are not held up further.

MaxSem closed this task as Resolved.Jun 14 2016, 8:27 PM
MaxSem changed Security from Software security bug to None.
MaxSem changed the visibility from "Custom Policy" to "Public (No Login Required)".Jun 14 2016, 8:30 PM
Restricted Application added a subscriber: Malyacko. · View Herald TranscriptJun 14 2016, 8:30 PM

Change 294412 had a related patch set uploaded (by MaxSem):
SECURITY: Fix XSS via proto

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

Change 294412 merged by jenkins-bot:
SECURITY: Fix XSS via proto

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

Change 294650 had a related patch set uploaded (by Mattflaschen):
SECURITY: Fix XSS via proto

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

Change 294650 merged by jenkins-bot:
SECURITY: Fix XSS via proto

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