Last modified: 2014-03-04 18:17:37 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 61743 - Security review for 'Popups' Extension
Security review for 'Popups' Extension
Status: RESOLVED FIXED
Product: MediaWiki extensions
Classification: Unclassified
Popups (Other open bugs)
unspecified
All All
: Normal normal (vote)
: ---
Assigned To: Chris Steipp
:
Depends on:
Blocks: 61167
  Show dependency treegraph
 
Reported: 2014-02-21 09:58 UTC by Prateek Saxena
Modified: 2014-03-04 18:17 UTC (History)
7 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Comment 1 Prateek Saxena 2014-02-25 12:00:05 UTC
Instances of `.html()` - 

1. In the `createBox` method I do something like:

    $el.html( $el.html() );

Its to refresh the DOM and display the SVG elements (see comments in the method)  that were added in the `createThumbnail` method. The elements created there follow [1] and thus are escaped.


2. To create the SVG element that masks the popup to create the triangle I do:

    $svg.html( '<svg width="0" height="0">...</svg>' );

Making this through jQuery methods was becoming to verbose. This a plain string with no concatenation from anywhere so I guess its safe.


3. There is an i18n string if the page redirects, it needs to read like "redirects to OtherPage". As in certain languages it could be "OtherPage…" and not "…OtherPage", Mark suggested that I add a $1 to it [2]. As I need those elements to be styled a certain way, the i18n strings will end up having an <h3> and thus my code looks something like this

    $( '<div>' ).html( mw.message( 'popups-redirects', redirects[ 0 ].to ).text() );

I am not sure if this is safe.

[1] https://www.mediawiki.org/wiki/Manual:Coding_conventions/JavaScript#Creating_elements
[2] https://gerrit.wikimedia.org/r/#/c/111983/3/Popups.i18n.php
Comment 2 Chris Steipp 2014-03-03 19:39:08 UTC
(In reply to Prateek Saxena from comment #1)
> Instances of `.html()` - 
> 
> 1. In the `createBox` method I do something like:
> 
>     $el.html( $el.html() );
> 
> Its to refresh the DOM and display the SVG elements (see comments in the
> method)  that were added in the `createThumbnail` method. The elements
> created there follow [1] and thus are escaped.
> 

I'm mostly concerned about the $contentbox portion, since that is generated from user content.

> 
> 2. To create the SVG element that masks the popup to create the triangle I
> do:
> 
>     $svg.html( '<svg width="0" height="0">...</svg>' );
> 
> Making this through jQuery methods was becoming to verbose. This a plain
> string with no concatenation from anywhere so I guess its safe.

Yes, this part is fine.

> 3. There is an i18n string if the page redirects, it needs to read like
> "redirects to OtherPage". As in certain languages it could be "OtherPage…"
> and not "…OtherPage", Mark suggested that I add a $1 to it [2]. As I need
> those elements to be styled a certain way, the i18n strings will end up
> having an <h3> and thus my code looks something like this
> 
>     $( '<div>' ).html( mw.message( 'popups-redirects', redirects[ 0 ].to
> ).text() );
> 
> I am not sure if this is safe.
> 
> [1]
> https://www.mediawiki.org/wiki/Manual:Coding_conventions/
> JavaScript#Creating_elements
> [2] https://gerrit.wikimedia.org/r/#/c/111983/3/Popups.i18n.php

Is there a working version of this in labs somewhere that I can test with? Or can you list out what dependencies this has? I'm not able to get it working locally.
Comment 3 Prateek Saxena 2014-03-04 04:21:27 UTC
(In reply to Chris Steipp from comment #2)
> I'm mostly concerned about the $contentbox portion, since that is generated 
> from user content.

We are using .text() when placing the extract in the Popup[1]. Are there any other measures that need to be taken? The other elements are being created in jQuery (how the code convention link explains)


> Yes, this part is fine.

Alright!


> Is there a working version of this in labs somewhere that I can test with?
> Or can you list out what dependencies this has? I'm not able to get it
> working locally.

There is a test instance[2] where the latest code lives. A couple of people have had the same issue and I am not sure what is wrong. I'll talk to Yuvi and resolve this. Are you using the vagrant role (popups) to set it up?

[1] https://git.wikimedia.org/blob/mediawiki%2Fextensions%2FPopups/2b021ef048aac6bfcbd0c1944bccc9ba2d7db040/resources%2Fext.popups.core.js#L53
[2] http://chicken.wmflabs.org/wiki/TestNavPopUps
Comment 4 Chris Steipp 2014-03-04 16:52:54 UTC
(In reply to Prateek Saxena from comment #3)
> (In reply to Chris Steipp from comment #2)
> > I'm mostly concerned about the $contentbox portion, since that is generated 
> > from user content.
> 
> We are using .text() when placing the extract in the Popup[1]. Are there any
> other measures that need to be taken? The other elements are being created
> in jQuery (how the code convention link explains)

No, .text() doesn't stop several attacks. For example:
$i = $( "<div>asdf&lt;script&gt;alert(1)&lt;/script&gt</div>" );
$o = $( "<div/>" );
$o.html( $i.text() );

You may be able to santize it with mw.html.escape, but I'm not entirely sure what markup you're trying to pass through.

> > Yes, this part is fine.
> 
> Alright!
> 
> 
> > Is there a working version of this in labs somewhere that I can test with?
> > Or can you list out what dependencies this has? I'm not able to get it
> > working locally.
> 
> There is a test instance[2] where the latest code lives. A couple of people
> have had the same issue and I am not sure what is wrong. I'll talk to Yuvi
> and resolve this. Are you using the vagrant role (popups) to set it up?

As soon as I install it, ResourceLoader complains that it can't find the class ResourceLoaderSchemaModule. I'm not sure if that's a typo, or if you're pulling that in from another extension.

> 
> [1]
> https://git.wikimedia.org/blob/mediawiki%2Fextensions%2FPopups/
> 2b021ef048aac6bfcbd0c1944bccc9ba2d7db040/resources%2Fext.popups.core.js#L53
> [2] http://chicken.wmflabs.org/wiki/TestNavPopUps
Comment 5 Chris Steipp 2014-03-04 17:13:21 UTC
(In reply to Chris Steipp from comment #4)
> > We are using .text() when placing the extract in the Popup[1]. Are there any
> > other measures that need to be taken? The other elements are being created
> > in jQuery (how the code convention link explains)
> 
> No, .text() doesn't stop several attacks. For example:
> $i = $( "<div>asdf&lt;script&gt;alert(1)&lt;/script&gt</div>" );
> $o = $( "<div/>" );
> $o.html( $i.text() );
> 
> You may be able to santize it with mw.html.escape, but I'm not entirely sure
> what markup you're trying to pass through.

Sorry, I should have looked at your reference first. Yeah, setting the text like that should work for that case. I'm digging through the TextExtracts section to make sure it can't return anything else dangerous.
Comment 6 Chris Steipp 2014-03-04 18:17:37 UTC
(In reply to Chris Steipp from comment #5)
> Sorry, I should have looked at your reference first. Yeah, setting the text
> like that should work for that case. I'm digging through the TextExtracts
> section to make sure it can't return anything else dangerous.

Ok, it should be fine as is. It would be helpful for you to document around the lines where you do .text( page.extract ) and .html( $box.html() ) what the expectations are, so that someone doesn't change those in the future and open up an issue.

(In reply to Prateek Saxena from comment #1)
> 3. There is an i18n string if the page redirects, it needs to read like
> "redirects to OtherPage". As in certain languages it could be "OtherPage…"
> and not "…OtherPage", Mark suggested that I add a $1 to it [2]. As I need
> those elements to be styled a certain way, the i18n strings will end up
> having an <h3> and thus my code looks something like this
> 
>     $( '<div>' ).html( mw.message( 'popups-redirects', redirects[ 0 ].to
> ).text() );
> 
> I am not sure if this is safe.

The title should be fine, due to the page naming rules. If the message contains scripts, that would cause an issue, but we accept that risk in many places in MediaWiki, so this isn't much different.


So in general, as if 2b021ef, this extension looks ok for security. Ori should review it for performance next.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links