Page MenuHomePhabricator

Overall security review of Popups/Hovercards extension
Closed, ResolvedPublic

Description

Overall security of the extension before we make it the default behavior on Chinese, Greek and Catalan wikipedia.

Event Timeline

Prtksxna raised the priority of this task from to High.
Prtksxna updated the task description. (Show Details)
Prtksxna added a project: Page-Previews.
Prtksxna added subscribers: Aklapper, Prtksxna.
Se4598 set Security to None.
Aklapper renamed this task from Overall security review to Overall security review of Popups/Hovercards extension.Feb 2 2015, 6:30 PM

@Jaredzimmerman-WMF, did you get a chance to schedule this?

Just to confirm, this is Extension:Popups (https://gerrit.wikimedia.org/r/p/mediawiki/extensions/Popups.git), right?

Yup!

Looks much better this time around!

  • ext.popups.renderer.article.js

In createImgThumbnail, it's unlikely (but not guaranteed) that the thumbnail url will include \, ), or " characters, which would allow someone to inject css, which could inject javascript. Either bail out if you find those, or urlenode them.

  • ext.popups.settings.js

In renderOption, you're passing content[1] into .html without validating it. Do you need to generate html there? If not, use .text. If you do need html, document that the parameter is html, and make sure that you're escaping any inputs-- e.g., in settings.render you should pass mw.message( ... ).escaped() instead of .text().

Change 199210 had a related patch set uploaded (by Prtksxna):
settings: Use .text() instead of .html() for option's label

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

Change 199211 had a related patch set uploaded (by Prtksxna):
renderer.article: Ignore thumnail if the URL has suspicious characters

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

Looks much better this time around!

Thanks for reviewing @csteipp!

In createImgThumbnail, it's unlikely (but not guaranteed) that the thumbnail url will include \, ), or " characters, which would allow someone to inject css, which could inject javascript. Either bail out if you find those, or urlenode them.

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

I now check the thumbnail.source at article.createThumbnail and return a <span> if those characters are found. Neither article.createSvgImageThumbnail nor article.createImgThumbnail is called with such a URL now. Would you rather have me returning the <span> from the respective functions instead?

In renderOption, you're passing content[1] into .html without validating it. Do you need to generate html there? If not, use .text. If you do need html, document that the parameter is html, and make sure that you're escaping any inputs-- e.g., in settings.render you should pass mw.message( ... ).escaped() instead of .text().

https://gerrit.wikimedia.org/r/199210
Done!

Change 199211 merged by jenkins-bot:
renderer.article: Ignore thumnail if the URL has suspicious characters

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

Change 199210 merged by jenkins-bot:
settings: Use .text() instead of .html() for option's label

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

Prtksxna moved this task from Next Up to For Review on the Page-Previews board.Mar 27 2015, 12:31 AM

@csteipp, can we close this issue now?

csteipp closed this task as Resolved.Apr 1 2015, 8:50 PM
csteipp claimed this task.
Quiddity moved this task from For Review to Done on the Page-Previews board.Jun 21 2015, 10:49 PM

Change 486844 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Popups@master] Add missing HTML escaping to all existing page preview types

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

Change 486844 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Add missing HTML escaping to all existing page preview types

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