Last modified: 2014-01-14 07:26:57 UTC
Steps to reproduce: *Add <span class="PopUpMediaTransform" data-videopayload="<script>alert(1)</script>">[[foo]]</span> to a page *Hit save *Click on link (Important note, only present on page save, it appears there is a bug in how TimedMediaHandler loads javascript on page preview). The vulnrability is in TimedMediaHandler/resources/mw.PopUpThumbVideo.js: Specificly the line: var $videoContainer = $( unescape( $(this).parent().attr('data-videopayload') ) ); and later line mw.addDialog({ ... 'title' : $videoContainer.find('video,audio').attr('data-mwtitle'), 'content' : $videoContainer, Suggested solution - Easy workaround for right now would be setting $wgMinimumVideoPlayerSize to 0 and stop TimedMediaHandler/resources/mw.PopUpThumbVideo.js from being served. Longer term, I guess would be to rewrite how PopUpThumbVideo, so it is given the information not as a string of html, but as data it can turn into html safely. Alternatively, maybe instead of putting the html as a string, display:none ing the html, and then retrieving it for the pop up. --- Discovered well investigating bug 56538. That bug has the obvious issue of unescaping something with the non-unicode aware unescape function when the input isn't even url escaped in the first place, but I haven't mentioned on bug since that quite obviously leads to discovering this.
Working exploit on commons would be: <span class="PopUpMediaTransform" data-videopayload="<script>alert(1)</script>">[[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 [details] Rename vulnerable attribute
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)
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.
$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.
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.
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.
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 [details] 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.
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.
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. ***
