Last modified: 2013-08-18 19:04:52 UTC
Using the pre-alpha JavaScript by Rillke which allows chunked uploads to commons one can upload any kind of file to the servers. Steps to reproduce: #Add importScript('User:Rillke/bigChunkedUpload.js'); to https://commons.wikimedia.org/wiki/Special:MyPage/common.js ( https://commons.wikimedia.org/wiki/User:Rillke/bigChunkedUpload.js ) #Append "&chunkedupload=1" to any commons-url and select the file to upload.
Any is a strong word... are you saying this gets around security checks?
(In reply to comment #1) > Any is a strong word... are you saying this gets around security checks? seems so. I just dropped some Code here: https://commons.wikimedia.org/wiki/File:48306_bugzilla.jpg
Well that's not good...
This is all about using API action=upload at Wikimedia Commons. Indeed, as soon as chunked upload comes into place, no matter whether async or not (my script makes use of async while it is also reproducible with UpWiz[1] with files > 5MB, which does not make use of async). I was able to upload a windows executable[2] using my tool with PNG file extension but not with exe[3] extension. [1] https://commons.wikimedia.org/wiki/File:Istgleic2h.jpg [2] https://commons.wikimedia.org/wiki/File:%27_onmouseover%3D%27alert%28%22There_is_a_security_vulnerable._Please_contact_the_next_admin_if_you_see_this_in_a_message_box.%22%29%27_target%3D%27.png [3] "error":{"code":"internal-error","info":"Invalid file title supplied"}
On my dev setup using Rillke's script, I'm not able to upload gifar-style images or php scripts. It looks like action=upload does have some different handling for file validation for async / chunk uploads, so it may be missing the step where it compares the detected mine to the file extension.
(In reply to comment #5) > On my dev setup using Rillke's script, I'm not able to upload gifar-style > images or php scripts. It looks like action=upload does have some different > handling for file validation for async / chunk uploads, so it may be missing > the step where it compares the detected mine to the file extension. Bug is only present if the test file is more than 0.5mb big. Was your test file big enough? (Aka only the code path where the file is in more than one chunk skips the verify checks) In my test I was able to successfully upload a php file. MW had no problem recognizing it as a php file, it appears the code to reject things if they are the wrong mime type is simply missing. Snippet from my debug log: MimeMagic::doGuessMimeType: recognized /tmp/chunkedupload_a85b5f072fc9-1 as application/x-php (And it just continued on its merry way after that)
I think Aaron and I figured out where the problem is coming from, and are trying to get a patch on the cluster asap. This is pretty bad. Bawolff, correct, I hadn't realized that script didn't chunk smaller files when I tested it the first time.
After looking at the code a little futher, I think there's 2 vulnerable code paths: *Chunked uploading in general *Any form of async uploading (Set $wgEnableAsyncUploads = true, which is what it is on commons but not by default, then do any form of uploading with async=1 parameter). I think this is where some of the confusion stems, since commons has this option enabled, but its disabled by default in MediaWiki. Rillke's script will use async uploading, so this case is hit on commons with that script for files < 0.5 mb, but not on our local test wikis, unless we enable $wgEnableAsyncUploads)
Created attachment 12294 [details] patch UploadFromChunks to verify the uploaded file Patch that calls the missing verifyUpload() when the chunks are combined into the final file.
Created attachment 12295 [details] Updated patch to catch all async issues Brian was correct. This patch seems to fix this case. I'll try and get some test cases put together to make sure we're catching all of the cases.
The patch seems to break uploading from the api in general. For the non-chunked upload I got: [Fri May 10 22:44:01 2013] [notice] child pid 7015 exit signal Segmentation fault (11) with the patch, which I'm not even sure how the patch could cause that...
I think the issue was that the patch caused UploadFromStash::verifyFile to call parent::verifyUpload, where really it should be calling parent::verifyFile. Calling parent::verifyUpload triggers an infinite loop, and hence php segfault.
Created attachment 12296 [details] Modified version of Chris's patch to fix the indef loop Attached is a fixed version of Chris's patch to fix the loop issue. I've tested it, and it appears to work fine. (Testing was made difficult because runJobs.php --type AssembleUploadChunks appears to do weird stuff with session which doesn't quite work and logs out the uploading user on my setup...) ---- However other issue discovered that this patch does not fix: *The individual chunks should be checked. In default MW config the temp directory where these chunks are held is publicly readable. Furthermore, if you upload a filetype that has an extension in mime.types, the temp file has the right extension. Hence in default config an attacker could upload an html file in chunks (or even just the first chunk be an html file). This html file is now publically accesible from the same server as the wiki, with a .html extension (99% of third parties have uploads on same). Thus giving the attacker an XSS (Even worse, the only thing stopping that from being an arbitrary code execution vulnerability is no other evil extensions except html being in mime.types). Not an issue for Wikimedia where the temp directory is publicly accessible, but pretty bad for third parties.
Created attachment 12299 [details] Attempt at a more complete patch I did an attempt at a more complete patch based on Chris's patch (I say attempt as I wouldn't exactly say "succesful". It works, but the part to make it verify the individual chunks is really really ugly. I also haven't tested the case where it gets published async due to issues in my install with losing session data whenever cli executed) The patch does the following: *The stuff from Chris's original patch involving verifying as stashed files are published. *Each individual chunk is verified before being stored in the temp zone (The way I did that is not very good) *Making sure we never add a blacklisted extension to a filename, even if its in the temp zone. Otherwise if you upload an html file in chunks (and you're not verifying each chunk), each individual chunk gets added to the temp zone with a .htm extension, which is web accesible (albeit with hard to figure out urls, but in your average MW install with directory indexes enabled, that obfuscation of the url isn't worth much). Note in comment 13 I thought that wmf doesn't expose the temp directory, I'm not sure if that statement was correct.
Created attachment 12320 [details] disable chunked uploads I'd like to release MW with this patch. It would disable chunked uploads by default. Most MW users don't use them, so i don't think this would be a problem.
I've been working on getting the (marked as @broken) tests for the upload api working, and trying to get through testing all permutations of of stash, chunked, and async, both to find any more cases that are vulnerable and to make sure we don't accidentally break too much with the final patch. I think I the case that you're concerned about. I'm uploading a file with the extension .jpg, but it's actually an html file. Using stash=1, async=1. The file gets saved in the stash as with a .html extension, since html mimetype was the detected. While the file is sitting in the stash (assuming I don't call upload with the filekey), the html is returned via a url like http://localhost/wiki/index.php/Special:UploadStash/file/11fkde79h6r0.dfxrl8.3.html, and they are stored in core/images/temp, so probably directly accessible if you can work out the obfuscation. With my patch, it would also have to be uploaded as chunked, and the last chunk would have to be omitted so that the verification wasn't called, but that's easy to do. At the very least, this gives someone xss. I don't think any default installs would be able to execute php, but it would be nice to not even come close. I'll try and finish testing your second patch tomorrow. I think that's headed in the right direction.
Mark, your patch combined with the first patch on this bug would be sufficient. Your patch alone disables chunked uploads, but as brian found out (comment 8), the vulnerability can also be triggered with the async flag.
Thanks. Unfortunately, I don't have any more time to work on this at the moment, so I am delaying the release until Saturday.
Hi Brian, I did more testing on your second patch. One thing I noticed in review is that you took out the call to getTitle in verifyFile, which the comments said was used to set mFinalExtension. Is that no longer needed? In testing, it looks like I'm still able to upload a gifar-style image (the first chunk is a .jpg, the second chunk is a .jar) chunks to the stash when I do chunked uploads. When I try to move it from the stash, it triggers "The file is a corrupt or otherwise unreadable ZIP file." I'm guessing that because even though it calls UploadBase::verifyMimeType with "application/zip" for the .jar chunk, it doesn't make sure that matches the .jpg extension of the file. The same gifar image is caught (with "This file did not pass file verification") when I upload it without chunks (stash 1 or 0, async 1 or 0).
I should also add that the gifar that I can upload to the stash has a .jpg and .jar concatenated right at the chunk boundary. If it isn't right at the boundary, it correctly returns an error. It looks like my test harness was also thinking it was correctly moved from the stash to the final location, but when I try it with the uploadwizard and Rillke's script, it doesn't get moved out of the stash. So we may be pretty close. Just some weird issue with detecting jars in some chunks.
And I should have checked sooner, but it in both Special:UploadStash and the file in images/temp/, only the first (non-vulnerable) chunk was stored in the above case. The chunk with the .jar was not saved anywhere in the images/ path. So I think for that case, we're ok with the current patch. However, in retesting with UploadWizard, I found that uploading an html file that requires chunking results in the first chunk (of html) being written into images/temp/, and also creating and entry in Special:UploadStash. Even though in the debug log I get: ApiUpload::performStash Stashing temporary file failed: UploadChunkVerificationException filetype-badmime Ah, that because a few lines before that in the logs we have: UploadStash::stashFile key for '/tmp/phps0zVzU': 11fpk2dcbnfc.2xbrrm.1. UploadStash::stashFile inserting mwrepo://local/temp/e/e6/20130516194658!phps0zVzU. under 11fpk2dcbnfc.2xbrrm.1. So I *think* we just need to clean up the wmrepo file if verification fails?
Ideally it would verify the /tmp file and not even upload it to shared storage (images/temp) if the check fails.
Created attachment 12329 [details] Check chunks, don't store before verification New version of bawolff's patch. I only moved where he called $this->verifyChunk() in UploadFromChunks::stashFile() to before where the file is stashed into the temp area. So a failure on the first chunk shouldn't be saved to disk anywhere.
(In reply to comment #19) > Hi Brian, I did more testing on your second patch. One thing I noticed in > review is that you took out the call to getTitle in verifyFile, which the > comments said was used to set mFinalExtension. Is that no longer needed? Its still needed. I had moved it to verifyPartialFile, which got called from verifyFile (I really don't like all these weird dependencies in the upload class). > > In testing, it looks like I'm still able to upload a gifar-style image (the > first chunk is a .jpg, the second chunk is a .jar) chunks to the stash when I > do chunked uploads. When I try to move it from the stash, it triggers "The > file > is a corrupt or otherwise unreadable ZIP file." I'm guessing that because > even > though it calls UploadBase::verifyMimeType with "application/zip" for the > .jar > chunk, it doesn't make sure that matches the .jpg extension of the file. > It should be caught since application/zip shouldn't be allowed in general. I didn't do the matching step since a chunk might be an incomplete file, so the individual chunks might have a different mime type then the file as a whole. ----- >New version of bawolff's patch. I only moved where he called >$this->verifyChunk() in UploadFromChunks::stashFile() to before where the file >is stashed into the temp area. So a failure on the first chunk shouldn't be >saved to disk anywhere. I got confused with the difference between parent::stashFile and $this->outputChunk. That change looks right.
Jan, Aaron and/or Brain, could you guys look at giving this patch a pretty thorough test/review today, in hopes that we can deploy on the cluster today, and get 1.20.6 out on Monday? This is also blocking Mark on 1.21, so it would be good to get this finalized soon. I'm pretty confident this closes the security issues (although if anyone sees anything suspicious, please do say so), so my main concern is it breaking existing functionality, since this did refactor how UploadBase works. As soon as someone as we get a +2, I'll deploy it and start working on the release.
Comment on attachment 12296 [details] Modified version of Chris's patch to fix the indef loop Obsoleting my first patch
Created attachment 12339 [details] Check chunks, don't store before verification Updated patch on Aaron's recommendation to do the mDesiredDestName overriding in UploadFromChunks, instead of passing the FileKey to VerifyPartial and doing the override there. Since if mTitle wasn't reset to false after overriding, strange things could happen.
I started to make a release today, but after Tyler asked me for a justification for https://gerrit.wikimedia.org/r/#/c/64482/ I realized that more is involved here than just providing fixes without some explanation. So, while I was planning on making a release today with a minimal fix, I'm giving up because this hasn't been made public yet. Mea culpa.
(In reply to comment #25) > Jan, Aaron and/or Brain, could you guys look at giving this patch a pretty > thorough test/review today, in hopes that we can deploy on the cluster today, > and get 1.20.6 out on Monday? This is also blocking Mark on 1.21, so it would > be good to get this finalized soon. > > I'm pretty confident this closes the security issues (although if anyone sees > anything suspicious, please do say so), so my main concern is it breaking > existing functionality, since this did refactor how UploadBase works. > > As soon as someone as we get a +2, I'll deploy it and start working on the > release. Sorry for the late reply, ive got some things happening in real life and don't have much time at the moment. Would someone else be able to review the patch?
Created attachment 12353 [details] Check chunks, don't store before verification Missed one $this->mDesiredDestName = $oldDesiredDestName;
I think this is fine now (it also works locally for me).
Chris, when you commit this patch (per Aaron's review) could you add me to the gerritt review so I can get it integrated into 1.21 ASAP. Since I've been waiting on this to release 1.21, I'd like to get this out the door.
I'll do that Mark. I patched the cluster about an hour ago. I'll be sending the pre-release announcement out this afternoon, and making the 1.20.6/1.19.7 release tomorrow.
Created attachment 12356 [details] 1.20 backport 1.20 didn't skip the verify check based on the async param, and one variable name change, but otherwise the patch is identical.
Created attachment 12357 [details] 1.19 backport
Released as 1.19.7/1.20.6
Mark, the 1.21 patch will be somewhere between these. Let me know if you need any help! (master) https://gerrit.wikimedia.org/r/#/c/64848/ (REL1_20) https://gerrit.wikimedia.org/r/#/c/64849/
Related URL: https://gerrit.wikimedia.org/r/64857 (Gerrit Change Icd8e799aaa2cca06495c690187283dd2913a17e3)
Related URL: https://gerrit.wikimedia.org/r/64860 (Gerrit Change I6e7fcbc511b8bdaebaa374bf5787039bdc797786)
Related URL: https://gerrit.wikimedia.org/r/64863 (Gerrit Change I6e7fcbc511b8bdaebaa374bf5787039bdc797786)
Using CVE-2013-2114 for this issue
Change 64860 abandoned by MarkAHershberger: Disable chunked uploads by default. Reason: MZ makes some good points and this has sat for a while. Abandoning since I'm not willing to spend time arguing in favor of this. https://gerrit.wikimedia.org/r/64860
(In reply to comment #41) > Using CVE-2013-2114 for this issue This appears to be reserved, but not published/publicized. Is that right? What's the status of this?
