From 42333412833ab7f7515e193b83a909921c34887d Mon Sep 17 00:00:00 2001 From: csteipp Date: Thu, 16 May 2013 15:09:44 -0700 Subject: [PATCH] SECURITY: Do checks on all upload types Also, verify file before stashing it Change-Id: Ib2474cb778d53959a4f479e53d0392f916b18d83 --- includes/AutoLoader.php | 1 + includes/api/ApiUpload.php | 10 +++- includes/upload/UploadBase.php | 83 +++++++++++++++++++--------- includes/upload/UploadFromChunks.php | 41 +++++++++++++- includes/upload/UploadFromStash.php | 9 +-- includes/upload/UploadStash.php | 11 +++- 6 files changed, 117 insertions(+), 38 deletions(-) diff --git a/includes/AutoLoader.php b/includes/AutoLoader.php index 326fb5c7f0..3e08e74d22 100644 --- a/includes/AutoLoader.php +++ b/includes/AutoLoader.php @@ -996,6 +996,7 @@ $wgAutoloadLocalClasses = array( 'UnwatchedpagesPage' => 'includes/specials/SpecialUnwatchedpages.php', 'UploadChunkFileException' => 'includes/upload/UploadFromChunks.php', 'UploadChunkZeroLengthFileException' => 'includes/upload/UploadFromChunks.php', + 'UploadChunkVerificationException' => 'includes/upload/UploadFromChunks.php', 'UploadForm' => 'includes/specials/SpecialUpload.php', 'UploadSourceField' => 'includes/specials/SpecialUpload.php', 'UserrightsPage' => 'includes/specials/SpecialUserrights.php', diff --git a/includes/api/ApiUpload.php b/includes/api/ApiUpload.php index 5563087b25..e04c76214b 100644 --- a/includes/api/ApiUpload.php +++ b/includes/api/ApiUpload.php @@ -88,9 +88,10 @@ class ApiUpload extends ApiBase { if ( !$this->mUpload->getTitle() ) { $this->dieUsage( 'Invalid file title supplied', 'internal-error' ); } - } elseif ( $this->mParams['async'] ) { + } elseif ( $this->mParams['async'] && $this->mParams['filekey'] ) { // defer verification to background process } else { + wfDebug( __METHOD__ . 'about to verify' ); $this->verifyUpload(); } @@ -195,7 +196,12 @@ class ApiUpload extends ApiBase { $chunkPath = $request->getFileTempname( 'chunk' ); $chunkSize = $request->getUpload( 'chunk' )->getSize(); if ( $this->mParams['offset'] == 0 ) { - $filekey = $this->performStash(); + try { + $filekey = $this->performStash(); + } catch ( MWException $e ) { + // FIXME: Error handling here is wrong/different from rest of this + $this->dieUsage( $e->getMessage(), 'stashfailed' ); + } } else { $filekey = $this->mParams['filekey']; /** @var $status Status */ diff --git a/includes/upload/UploadBase.php b/includes/upload/UploadBase.php index 17da80e07a..2ed20c5603 100644 --- a/includes/upload/UploadBase.php +++ b/includes/upload/UploadBase.php @@ -356,8 +356,10 @@ abstract class UploadBase { } /** - * Verify the mime type + * Verify the mime type. * + * @note Only checks that it is not an evil mime. The does it have + * correct extension given its mime type check is in verifyFile. * @param string $mime representing the mime * @return mixed true if the file is verified, an array otherwise */ @@ -372,12 +374,6 @@ abstract class UploadBase { return array( 'filetype-badmime', $mime ); } - # XXX: Missing extension will be caught by validateName() via getTitle() - if ( $this->mFinalExtension != '' && !$this->verifyExtension( $mime, $this->mFinalExtension ) ) { - wfProfileOut( __METHOD__ ); - return array( 'filetype-mime-mismatch', $this->mFinalExtension, $mime ); - } - # Check IE type $fp = fopen( $this->mTempPath, 'rb' ); $chunk = fread( $fp, 256 ); @@ -398,17 +394,68 @@ abstract class UploadBase { return true; } + /** * Verifies that it's ok to include the uploaded file * * @return mixed true of the file is verified, array otherwise. */ protected function verifyFile() { + global $wgVerifyMimeType; + wfProfileIn( __METHOD__ ); + + $status = $this->verifyPartialFile(); + if ( $status !== true ) { + wfProfileOut( __METHOD__ ); + return $status; + } + + if ( $wgVerifyMimeType ) { + $this->mFileProps = FSFile::getPropsFromPath( $this->mTempPath, $this->mFinalExtension ); + $mime = $this->mFileProps['file-mime']; + + # XXX: Missing extension will be caught by validateName() via getTitle() + if ( $this->mFinalExtension != '' && !$this->verifyExtension( $mime, $this->mFinalExtension ) ) { + wfProfileOut( __METHOD__ ); + return array( 'filetype-mime-mismatch', $this->mFinalExtension, $mime ); + } + } + + + $handler = MediaHandler::getHandler( $mime ); + if ( $handler ) { + $handlerStatus = $handler->verifyUpload( $this->mTempPath ); + if ( !$handlerStatus->isOK() ) { + $errors = $handlerStatus->getErrorsArray(); + wfProfileOut( __METHOD__ ); + return reset( $errors ); + } + } + + wfRunHooks( 'UploadVerifyFile', array( $this, $mime, &$status ) ); + if ( $status !== true ) { + wfProfileOut( __METHOD__ ); + return $status; + } + + wfDebug( __METHOD__ . ": all clear; passing.\n" ); + wfProfileOut( __METHOD__ ); + return true; + } + + /** + * A verification routine suitable for partial files + * + * Runs the blacklist checks, but not any checks that may + * assume the entire file is present. + * + * @return Mixed true for valid or array with error message key. + */ + protected function verifyPartialFile() { global $wgAllowJavaUploads, $wgDisableUploadScriptChecks; wfProfileIn( __METHOD__ ); - # get the title, even though we are doing nothing with it, because - # we need to populate mFinalExtension + # getTitle() sets some internal parameters like $this->mFinalExtension $this->getTitle(); $this->mFileProps = FSFile::getPropsFromPath( $this->mTempPath, $this->mFinalExtension ); @@ -462,23 +509,6 @@ abstract class UploadBase { return array( 'uploadvirus', $virus ); } - $handler = MediaHandler::getHandler( $mime ); - if ( $handler ) { - $handlerStatus = $handler->verifyUpload( $this->mTempPath ); - if ( !$handlerStatus->isOK() ) { - $errors = $handlerStatus->getErrorsArray(); - wfProfileOut( __METHOD__ ); - return reset( $errors ); - } - } - - wfRunHooks( 'UploadVerifyFile', array( $this, $mime, &$status ) ); - if ( $status !== true ) { - wfProfileOut( __METHOD__ ); - return $status; - } - - wfDebug( __METHOD__ . ": all clear; passing.\n" ); wfProfileOut( __METHOD__ ); return true; } @@ -677,7 +707,6 @@ abstract class UploadBase { if ( $this->mTitle !== false ) { return $this->mTitle; } - /* Assume that if a user specified File:Something.jpg, this is an error * and that the namespace prefix needs to be stripped of. */ diff --git a/includes/upload/UploadFromChunks.php b/includes/upload/UploadFromChunks.php index 37db68848e..17462069bd 100644 --- a/includes/upload/UploadFromChunks.php +++ b/includes/upload/UploadFromChunks.php @@ -70,6 +70,8 @@ class UploadFromChunks extends UploadFromFile { // Stash file is the called on creating a new chunk session: $this->mChunkIndex = 0; $this->mOffset = 0; + + $this->verifyChunk(); // Create a local stash target $this->mLocalFile = parent::stashFile(); // Update the initial file offset (based on file size) @@ -131,9 +133,18 @@ class UploadFromChunks extends UploadFromFile { return $status; } wfDebugLog( 'fileconcatenate', "Combined $i chunks in $tAmount seconds.\n" ); + + $this->mTempPath = $tmpPath; // file system path + $this->mFileSize = filesize( $this->mTempPath ); //Since this was set for the last chunk previously + $ret = $this->verifyUpload(); + if ( $ret['status'] !== UploadBase::OK ) { + wfDebugLog( 'fileconcatenate', "Verification failed for chunked upload" ); + $status->fatal( $this->getVerificationErrorCode( $ret['status'] ) ); + return $status; + } + // Update the mTempPath and mLocalFile // (for FileUpload or normal Stash to take over) - $this->mTempPath = $tmpPath; // file system path $tStart = microtime( true ); $this->mLocalFile = parent::stashFile( $this->user ); $tAmount = microtime( true ) - $tStart; @@ -189,6 +200,15 @@ class UploadFromChunks extends UploadFromFile { if ( $preAppendOffset == $offset ) { // Update local chunk index for the current chunk $this->mChunkIndex++; + try { + # For some reason mTempPath is set to first part + $oldTemp = $this->mTempPath; + $this->mTempPath = $chunkPath; + $this->verifyChunk(); + $this->mTempPath = $oldTemp; + } catch ( UploadChunkVerificationException $e ) { + return Status::newFatal( $e->getMessage() ); + } $status = $this->outputChunk( $chunkPath ); if ( $status->isGood() ) { // Update local offset: @@ -312,7 +332,26 @@ class UploadFromChunks extends UploadFromFile { } return $this->mFileKey . '.' . $index; } + + /** + * Verify that the chunk isn't really an evil html file + * + * @throws UploadChunkVerificationException + */ + private function verifyChunk() { + // Rest mDesiredDestName here so we verify the name as if it were mFileKey + $oldDesiredDestName = $this->mDesiredDestName; + $this->mDesiredDestName = $this->mFileKey; + $this->mTitle = false; + $res = $this->verifyPartialFile(); + $this->mDesiredDestName = $oldDesiredDestName; + $this->mTitle = false; + if( is_array( $res ) ) { + throw new UploadChunkVerificationException( $res[0] ); + } + } } class UploadChunkZeroLengthFileException extends MWException {}; class UploadChunkFileException extends MWException {}; +class UploadChunkVerificationException extends MWException {}; diff --git a/includes/upload/UploadFromStash.php b/includes/upload/UploadFromStash.php index 9276b538bb..cb85fc63f0 100644 --- a/includes/upload/UploadFromStash.php +++ b/includes/upload/UploadFromStash.php @@ -137,14 +137,9 @@ class UploadFromStash extends UploadBase { return $this->mFileProps['sha1']; } - /** - * File has been previously verified so no need to do so again. - * - * @return bool + /* + * protected function verifyFile() inherited */ - protected function verifyFile() { - return true; - } /** * Stash the file. diff --git a/includes/upload/UploadStash.php b/includes/upload/UploadStash.php index 1ee462776a..8a6d766912 100644 --- a/includes/upload/UploadStash.php +++ b/includes/upload/UploadStash.php @@ -422,6 +422,7 @@ class UploadStash { * @return string */ public static function getExtensionForPath( $path ) { + global $wgFileBlacklist; // Does this have an extension? $n = strrpos( $path, '.' ); $extension = null; @@ -441,7 +442,15 @@ class UploadStash { throw new UploadStashFileException( "extension is null" ); } - return File::normalizeExtension( $extension ); + $extension = File::normalizeExtension( $extension ); + if ( in_array( $extension, $wgFileBlacklist ) ) { + // The file should already be checked for being evil. + // However, if somehow we got here, we definitely + // don't want to give it an extension of .php and + // put it in a web accesible directory. + return ''; + } + return $extension; } /** -- 2.20.1