Page MenuHomePhabricator

Resolve GapFinder security issues
Closed, ResolvedPublic

Description

We need to resolve the issues discovered in T148133

  • 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

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 TranscriptJan 27 2017, 12:36 PM
schana added a comment.EditedFeb 16 2017, 12:32 AM

These issues should be resolved as the relevant parts of the codebase are touched in T153443

@schana can you pass this by Security to make sure they approve that the issues are addressed?

@schana can you pass this by Security to make sure they approve that the issues are addressed?

@leila They haven't been addressed yet - my comment was a note to explain that there is ongoing work that will likely incorporate fixing these issues when the relevant parts of the codebase are touched. Does this task need a higher priority than the UI work?

leila added a comment.Feb 16 2017, 4:56 PM

If Ellery doesn't /need/ more work on the readMore's UI, yes. If he needs it, no. :) I'd recommend checking with him as I don't know the requirements of that project.

schana moved this task from Next up to In Progress on the Recommendation-API board.

Change 338759 had a related patch set uploaded (by Nschaaf):
Add rel=noopener to target=_blank links

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

Change 338763 had a related patch set uploaded (by Nschaaf):
Fix url param escaping & add no results message

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

Change 338788 had a related patch set uploaded (by Nschaaf):
Not use find and replace for base href in preview

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

For reference, the specific revision of code that this refers to is available here: https://github.com/wikimedia/research-recommendation-api/tree/e7c58c39f3a4002d2b7947302892dd0557c290e3

schana updated the task description. (Show Details)Feb 20 2017, 4:46 PM

Change 338961 had a related patch set uploaded (by Nschaaf):
Use json instead of jsonp and encodeURIComponents

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

schana updated the task description. (Show Details)Feb 21 2017, 12:38 PM

Change 338759 merged by jenkins-bot:
Add rel=noopener to target=_blank links

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

Change 338763 merged by jenkins-bot:
Fix url param escaping & add no results message

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

Change 338788 merged by jenkins-bot:
Not use find and replace for base href in preview

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

Change 338961 merged by jenkins-bot:
Use json instead of jsonp and encodeURIComponents

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

Change 338969 had a related patch set uploaded (by Nschaaf):
Fix json dump and parse logic

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

Change 338969 merged by jenkins-bot:
Fix json dump and parse logic

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

@Bawolff I have a question regarding the following code and comment from the security review:

https://github.com/wikimedia/research-recommendation-api/blob/bbb9e2d35612c9e1bbff70e2dbe719ebf6cdd11e/recommendation/web/static/gf-preview.tag#L111-L121

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.

When setting the attribute using jQuery $(iframe).attr("srcdoc", data), the html text (data) is escaped. From what I can tell, setting the src attribute to the javascript URI is an established (https://www.github.com/jugglinmike/srcdoc-polyfill) method of adding support to browsers that don't support the srcdoc attribute, since it just returns the already-escaped value of the srcdoc attribute.

Are there other security implications that I'm not accounting for? Thanks!

@Bawolff I have a question regarding the following code and comment from the security review:

https://github.com/wikimedia/research-recommendation-api/blob/bbb9e2d35612c9e1bbff70e2dbe719ebf6cdd11e/recommendation/web/static/gf-preview.tag#L111-L121

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.

When setting the attribute using jQuery $(iframe).attr("srcdoc", data), the html text (data) is escaped. From what I can tell, setting the src attribute to the javascript URI is an established (https://www.github.com/jugglinmike/srcdoc-polyfill) method of adding support to browsers that don't support the srcdoc attribute, since it just returns the already-escaped value of the srcdoc attribute.

Are there other security implications that I'm not accounting for? Thanks!

The code as it currently exists for the fallback looks good.

Change 338987 had a related patch set uploaded (by Nschaaf):
Use url encoding instead of json for safety

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

Change 338987 merged by jenkins-bot:
Use url encoding instead of json for safety

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

schana updated the task description. (Show Details)Feb 21 2017, 4:32 PM
schana removed a project: Patch-For-Review.
schana moved this task from In Progress to For Review on the Recommendation-API board.

@Bawolff I have a question regarding the following code and comment from the security review:

https://github.com/wikimedia/research-recommendation-api/blob/bbb9e2d35612c9e1bbff70e2dbe719ebf6cdd11e/recommendation/web/static/gf-preview.tag#L111-L121

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.

When setting the attribute using jQuery $(iframe).attr("srcdoc", data), the html text (data) is escaped. From what I can tell, setting the src attribute to the javascript URI is an established (https://www.github.com/jugglinmike/srcdoc-polyfill) method of adding support to browsers that don't support the srcdoc attribute, since it just returns the already-escaped value of the srcdoc attribute.

Are there other security implications that I'm not accounting for? Thanks!

The code as it currently exists for the fallback looks good.

So looking back, it looks like that has always been the case. I think my eyes must have played tricks on me, and I thought the window.frameElement.getAttribute('srcdoc'); was not in the double quotes. Sorry about that.

leila closed this task as Resolved.Feb 23 2017, 6:05 PM