Working exploit on commons would be:

<span class="PopUpMediaTransform" data-videopayload="&lt;script&gt;alert(1)&lt;/script&gt;">[[File:Shock_wave_above_A320_wing_(1).webm|100px]]</span>

And click on the thumbnail. And please click "Preview" instead of save, so we don't have these in recent changes..

I think we'll set $wgMinimumVideoPlayerSize = 0, and basically blank mw.PopUpThumbVideo.js so we prevent this from running. I'm not seeing an easy fix to this that we could deploy immediately.

A slightly less drastic measure would be to disable the vector that's being used, and change the attribute name that is storing the html to one that can't be inserted by other users. So if we change the payload to sit in 'videopayload' as the attribute, and the javsacript checks for a div with that attribute, there shouldn't be a way for users to inject a fake one on the page.

Created attachment 13720
Rename vulnerable attribute

Attached:

mdale wrote:

Fix looks good to me.

Probably would not hurt to add in:
$videoContainer = $videoContainer.filter( 'div,video,track,audio' );

Before mw.addDialog({ call as well.

That seems like a reasonable short term fix. Long term it would be nice if we avoided stuffing html in attributes (separating code and data is good, validation, etc)

mdale wrote:

Yea... we should just package that in JSON instead of HTML for the respective embed build out / pop-up.

Will add it to the TODO list for the player update ;)

It just occured to me the old version of the html would be in parser cache, so if we disabled reading from data-payload, it would break pages with old cache.

mdale wrote:

$videoContainer = $videoContainer.filter( 'div,video,track,audio' );

Would be safe for existing cached pages?

And or check both places as we transition?

Thanks for bringing that up Bawolff. Thankfully with the cluster break this morning I didn't deploy :)

We don't want to check for the old value, since that will allow the xss for another 30 days while the caches expire. And although the filter is probably a good start, it won't quite work since it won't catch <video onerror="alert(1)"><source></source></video> if someone can add that to the attribute.

Is it possible to deploy our patch as it, and use a maintenance script to edit or purge all the articles using video files? I'm not sure if we keep a reference around to where we use all the files..

We could keep the old value for 30 days, and add data-videopayload as a temporary exception to the Sanitizer class.

(In reply to comment #9)

Is it possible to deploy our patch as it, and use a maintenance script to
edit
or purge all the articles using video files? I'm not sure if we keep a
reference around to where we use all the files..

We do, sort of. You can look at everything with img_media_type = VIDEO, and join on globalimagelinks. Doesn't sound like the most fun thing to do though, might get complicated with cross-wiki stuff.

(In reply to comment #10)

We could keep the old value for 30 days, and add data-videopayload as a
temporary exception to the Sanitizer class.

Temp blacklisting data-videopayload sounds fine to me

Also if worst comes to worst, we just need to make sure the cached pages aren't totally broken, if there is some sane fallback (like going to the file description page on click), that would probably be fine for the transition. Most popular pages end up getting edited or otherwise have their cache purged before the 30 days are up.

good start, it won't quite work since it won't catch <video
onerror="alert(1)"><source></source></video> if someone can add that to the
attribute.

I'm not sure I like the filter idea, seems like it would give a false sense of security. At the very least, even if the filter stripped all js, the filter wouldn't prevent someone making a fake video tag that referenced a video on another website, potentially exposing user IPs.

Also if worst comes to worst, we just need to make sure the cached pages
aren't
totally broken, if there is some sane fallback (like going to the file
description page on click), that would probably be fine for the transition.
Most popular pages end up getting edited or otherwise have their cache purged
before the 30 days are up.

Actually, current behaviour if the js doesn't run, is to go directly to video file - which isn't exactly horrid (but not great), as many web browsers would probably just play the file.

mdale wrote:

Yea seems like filter is not a good idea. Even serialized JSON if in the data-* attribute could result in leaking IPs to other domains if full urls are taken as is. So we are back to non-standard HTML attribute since we let users control the data-* attributes in HTML output.

The final solution may be an api request based solely on a title attribute instead of packing in all resource data inline. Assuming we care about "standard" HTML.

Tisza idea of restricting data-videopayload in santization sounds reasonable, if not too much work, would prevent you from saving new pages with this attribute?

Or just deploy the change as proposed, and old pages will just end up linking to the file of the respective asset.

IE, Safari users will get a prompt to play in VLC or what have you but in-browser cortado is bad experience anyway / often java is disabled.

(In reply to comment #13)

Tisza idea of restricting data-videopayload in santization sounds reasonable,
if not too much work, would prevent you from saving new pages with this
attribute?

If we had global abuse filters, we could prevent it coming in, but we don't. So I think the only way we could filter this would be a temporary hack in the Sanitizer, which we don't typically do. So I'm thinking no, unless Gergő, were you thinking of another way to filter those?

So if we change the attribute name (my patch), I think the fallback behavior for when the user gets a cached page, but current javascript, would be they are taken to the file page and could play it there, right? I don't have this setup in a way I can confirm that, but if so, then initially a lot of users would click through, but then pages would get recached and the popup would work.

Does that sounds realistic? Or am I off on that?

(In reply to comment #14)

(In reply to comment #13)

Tisza idea of restricting data-videopayload in santization sounds reasonable,
if not too much work, would prevent you from saving new pages with this
attribute?

If we had global abuse filters, we could prevent it coming in, but we don't.
So
I think the only way we could filter this would be a temporary hack in the
Sanitizer, which we don't typically do. So I'm thinking no, unless Gergő,
were
you thinking of another way to filter those?

That's (patch Sanitizer) what I was assuming he meant.

So if we change the attribute name (my patch), I think the fallback behavior
for when the user gets a cached page, but current javascript, would be they
are
taken to the file page and could play it there, right?

Just tested, and that's not what happens. The behaviour is that an empty dialog is popped up (the if statement for if to put the pop-up is based only on class name, and doesn't check if there is a valid (data-)videopayload attribute.

mdale wrote:

yea you would need to add:
if( $videoContainer.length == 0 ){

return true;

}

For it to follow the link if the attribute was not found.

(In reply to comment #16)

yea you would need to add:
if( $videoContainer.length == 0 ){

return true;

}

For it to follow the link if the attribute was not found.

There's the additional problem in that the link is to the actual file, not the file page. We probably want to direct users to the former.

mdale wrote:

Unfortunately that would create an infinite loop on non-purged file pages at the moment, since the popup was recently broadly enabled on commons.

Created attachment 13782
Rename the attribute with graceful degradation

(In reply to comment #18)

Unfortunately that would create an infinite loop on non-purged file pages at
the moment, since the popup was recently broadly enabled on commons.

Well the main image on the file description page isn't included in the parser cache (One could still have issues with browser cache, and perhaps squid/varnish for logged out users).

Anyways, included is a patch which I think would degrade relatively gracefully in all cases. If the element is not present, it redirects user to image page, unless already on image page, in which case user goes directly to the file.

Attached:

This issue was assigned CVE-2013-4574. Since we don't have a patch on the cluster yet, we'll wait to release this publicly with 1.21.4.

Bawolff, sorry for the delay on this, but I think that patch looks good. I'll get it deployed onto the cluster this week.

Deployed onto the cluster:
20:21 logmsgbot: csteipp synchronized php-1.23wmf4/extensions/TimedMediaHandler 'bug56699'
20:16 logmsgbot: csteipp synchronized php-1.23wmf5/extensions/TimedMediaHandler 'bug56699'

It looks like there are on a few non WMF sits running the extension (http://wikiapiary.com/wiki/Extension:TimedMediaHandler). Michael, we can either keep the patch a secret until the next security release and do an official release of it then, or we can put it into gerrit now and give it a brief mention of it when we do the release. Let me know your preference.

mdale wrote:

Standard practice would be keep a secret until the next security release no?

When you say release, you mean for the full mediaWiki package and all extensions running on WMF wikis correct?

I don't know if folks would be accustom to checking for TMH releases, since that has not been a regular thing.

(In reply to comment #23)

Standard practice would be keep a secret until the next security release no?

Yes

When you say release, you mean for the full mediaWiki package and all
extensions running on WMF wikis correct?

Yes

I don't know if folks would be accustom to checking for TMH releases, since
that has not been a regular thing.

Ok, I think then we'll wait for the next tarball release, and we'll mention this at that time. We'll probably need one early next month.

• Bug 58263 has been marked as a duplicate of this bug. ***