Page MenuHomePhabricator

Resolve security issues
Closed, ResolvedPublic

Description

We need to resolve the issues discovered in T148133

Overall, this looks good. By and large the code seems safe, and most of the issues are either low severity, or very difficult to trigger.
There are two issues I saw come up multiple times. Both are relatively minor as far as security issues go, but still should be fixed:

  • Inconsistent encodeURIComponent-ing of url parts in js. Even if the data is from what's probably a safe source, encodeURIComponent should be used to catch special characters, and so we can tell at a glance that it is safe. This is especially true when using JSONP, where there is even the remotest possibility of overriding the callback parameter.
  • Using target=_blank for external links without specifying rel=noopener The full list of issues are as follows:

Normal issues

  • The escaping of js values inserted into web/index.html is incorrect. They are being escaped for html, but should be escaped for javascript. In particular, ?s=%0A or ?s=foo\ can both cause syntax errors, and other special characters will be escaped in ways that are not correct (albeit not a security issue). This cannot be exploited at present, but if two values were inserted on the same line it would be, so this should definitely be fixed, in case the js ever gets run through a minimizer, or something like that.
  • All target=_blank links should have rel=noopener.
  • Line 93 gf-preview.js - The find and replace of </head> could lead to XSS. Consider if someone could add html like <div title="</head><img src=x onerror=alert(1)"> to the page. This is perfectly valid and safe prior to the regex (albeit MW usually doesn't allow that sort of thing, but extensions might), but unsafe after the regex. (In practice this would be very hard to pull off as you would have to be able to insert the tag prior to the actual <head/>, but that's not something we should rely on)
  • line 106 of gf-preview.js. This doesn't put the html text inside quote marks (And also doesn't escape quotes). This will cause the page to be executed as javascript, which is not what is desired.
  • line 46 of gf-preview.js, encodeURIComponent missing from jsonp queries.

Minor

  • It would be nice if trusted data from other servers would be escaped (langauge_pairs, language_to_domain_mappings) or failing that, checked if they contain an apostrophe and throw an exception.
  • Since you're already loading mw.RegExp.escape, the writeCookie() function in web/static/cookies.js should probably use it. (This is very minor)
  • line 70 of gf-input.js - Please encodeURIComponent s and t.
  • line 63-ish of gf-preview.js - encodeURIComponent of all the parts of the url.
    • ditto gf-create.js
  • wm-typeahead.js doesn't seem to escape titles properly when doing syntax highlighting. (Perhaps this is more something I should bring up with the portal people)

Other comments

  • You may want to consider using CORS instead of JSONP for api communication. The mediawiki api supports anonymous CORS if you provide an &origin=* url parameter. From what I understand this is more reliable than jsonp, and also less scary as it doesn't rely on script execution.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 21 2016, 11:07 AM
Jay8g added a subscriber: Jay8g.Jan 7 2017, 4:28 AM
leila triaged this task as High priority.Jan 18 2017, 8:59 PM
leila added a subscriber: leila.Jan 18 2017, 9:02 PM

@schana I've set the priority of this task to high given that productization of the recommender api is a carry-over goal from the last quarter that we should aim to finish this quarter.

schana lowered the priority of this task from High to Medium.Jan 27 2017, 12:37 PM
schana added a project: Epic.
leila closed this task as Resolved.Feb 23 2017, 6:04 PM