| From 641c2572681d1cec93d45df9055b90f018c7e9fe Mon Sep 17 00:00:00 2001 | |
| From: Chad Horohoe <[email protected]> | |
| Date: Thu, 15 Oct 2015 13:20:10 -0700 | |
| Subject: [PATCH] SECURITY: API: Improve validation in chunked uploading | |
| This fixes a few shortcomings in the chunked uploader: | |
| * Raises an error if offset + chunksize > filesize. | |
| * Enforces a minimum chunk size for non-final chunks. | |
| * Refuses additional chunks after seeing a final chunk. | |
| * Status of a chunked upload in progress is now available with | |
| 'checkstatus'. | |
| Bug: T91203 | |
| Bug: T91205 | |
| Change-Id: If33cc99f304aab2486507c7500b4abb06b6b5d70 | |
| --- | |
| includes/DefaultSettings.php | 8 ++++ | |
| includes/GlobalFunctions.php | 5 ++- | |
| includes/Setup.php | 9 +++++ | |
| includes/api/ApiQuerySiteinfo.php | 1 + | |
| includes/api/ApiUpload.php | 82 ++++++++++++++++++++++++++++++++------- | |
| 5 files changed, 89 insertions(+), 16 deletions(-) | |
| diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php | |
| index 05d447d..1b9e0f9 100644 | |
| --- a/includes/DefaultSettings.php | |
| +++ b/includes/DefaultSettings.php | |
| @@ -703,6 +703,14 @@ $wgCopyUploadAsyncTimeout = false; | |
| $wgMaxUploadSize = 1024 * 1024 * 100; # 100MB | |
| /** | |
| + * Minimum upload chunk size, in bytes. When using chunked upload, non-final | |
| + * chunks smaller than this will be rejected. May be reduced based on the | |
| + * 'upload_max_filesize' or 'post_max_size' PHP settings. | |
| + * @since 1.26 | |
| + */ | |
| +$wgMinUploadChunkSize = 1024; # 1KB | |
| + | |
| +/** | |
| * Point the upload navigation link to an external URL | |
| * Useful if you want to use a shared repository by default | |
| * without disabling local uploads (use $wgEnableUploads = false for that). | |
| diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php | |
| index 45cd7ea..e717c12 100644 | |
| --- a/includes/GlobalFunctions.php | |
| +++ b/includes/GlobalFunctions.php | |
| @@ -3860,12 +3860,13 @@ function wfMemoryLimit() { | |
| * Converts shorthand byte notation to integer form | |
| * | |
| * @param string $string | |
| + * @param int $default Return if $string is empty | |
| * @return int | |
| */ | |
| -function wfShorthandToInteger( $string = '' ) { | |
| +function wfShorthandToInteger( $string = '', $default = -1 ) { | |
| $string = trim( $string ); | |
| if ( $string === '' ) { | |
| - return -1; | |
| + return $default; | |
| } | |
| $last = $string[strlen( $string ) - 1]; | |
| $val = intval( $string ); | |
| diff --git a/includes/Setup.php b/includes/Setup.php | |
| index 1ab07cc..1b6d66c 100644 | |
| --- a/includes/Setup.php | |
| +++ b/includes/Setup.php | |
| @@ -368,6 +368,15 @@ if ( $wgResourceLoaderMaxQueryLength === false ) { | |
| unset($suhosinMaxValueLength); | |
| } | |
| +// Ensure the minimum chunk size is less than PHP upload limits or the maximum | |
| +// upload size. | |
| +$wgMinUploadChunkSize = min( | |
| + $wgMinUploadChunkSize, | |
| + $wgMaxUploadSize, | |
| + wfShorthandToInteger( ini_get( 'upload_max_filesize' ), 1e100 ), | |
| + wfShorthandToInteger( ini_get( 'post_max_size' ), 1e100) - 1024 # Leave room for other parameters | |
| +); | |
| + | |
| /** | |
| * Definitions of the NS_ constants are in Defines.php | |
| * @private | |
| diff --git a/includes/api/ApiQuerySiteinfo.php b/includes/api/ApiQuerySiteinfo.php | |
| index b81e993..b7b1084 100644 | |
| --- a/includes/api/ApiQuerySiteinfo.php | |
| +++ b/includes/api/ApiQuerySiteinfo.php | |
| @@ -245,6 +245,7 @@ class ApiQuerySiteinfo extends ApiQueryBase { | |
| $data['misermode'] = (bool)$config->get( 'MiserMode' ); | |
| $data['maxuploadsize'] = UploadBase::getMaxUploadSize(); | |
| + $data['minuploadchunksize'] = (int)$this->getConfig()->get( 'MinUploadChunkSize' ); | |
| $data['thumblimits'] = $config->get( 'ThumbLimits' ); | |
| ApiResult::setArrayType( $data['thumblimits'], 'BCassoc' ); | |
| diff --git a/includes/api/ApiUpload.php b/includes/api/ApiUpload.php | |
| index 6a5fd9e..7661625 100644 | |
| --- a/includes/api/ApiUpload.php | |
| +++ b/includes/api/ApiUpload.php | |
| @@ -81,7 +81,7 @@ class ApiUpload extends ApiBase { | |
| // Check if the uploaded file is sane | |
| if ( $this->mParams['chunk'] ) { | |
| - $maxSize = $this->mUpload->getMaxUploadSize(); | |
| + $maxSize = UploadBase::getMaxUploadSize(); | |
| if ( $this->mParams['filesize'] > $maxSize ) { | |
| $this->dieUsage( 'The file you submitted was too large', 'file-too-large' ); | |
| } | |
| @@ -203,13 +203,30 @@ class ApiUpload extends ApiBase { | |
| private function getChunkResult( $warnings ) { | |
| $result = array(); | |
| - $result['result'] = 'Continue'; | |
| if ( $warnings && count( $warnings ) > 0 ) { | |
| $result['warnings'] = $warnings; | |
| } | |
| + | |
| $request = $this->getMain()->getRequest(); | |
| $chunkPath = $request->getFileTempname( 'chunk' ); | |
| $chunkSize = $request->getUpload( 'chunk' )->getSize(); | |
| + $totalSoFar = $this->mParams['offset'] + $chunkSize; | |
| + $minChunkSize = $this->getConfig()->get( 'MinUploadChunkSize' ); | |
| + | |
| + // Sanity check sizing | |
| + if ( $totalSoFar > $this->mParams['filesize'] ) { | |
| + $this->dieUsage( | |
| + 'Offset plus current chunk is greater than claimed file size', 'invalid-chunk' | |
| + ); | |
| + } | |
| + | |
| + // Enforce minimum chunk size | |
| + if ( $totalSoFar != $this->mParams['filesize'] && $chunkSize < $minChunkSize ) { | |
| + $this->dieUsage( | |
| + "Minimum chunk size is $minChunkSize bytes for non-final chunks", 'chunk-too-small' | |
| + ); | |
| + } | |
| + | |
| if ( $this->mParams['offset'] == 0 ) { | |
| try { | |
| $filekey = $this->performStash(); | |
| @@ -221,6 +238,18 @@ class ApiUpload extends ApiBase { | |
| } | |
| } else { | |
| $filekey = $this->mParams['filekey']; | |
| + | |
| + // Don't allow further uploads to an already-completed session | |
| + $progress = UploadBase::getSessionStatus( $this->getUser(), $filekey ); | |
| + if ( !$progress ) { | |
| + // Probably can't get here, but check anyway just in case | |
| + $this->dieUsage( 'No chunked upload session with this key', 'stashfailed' ); | |
| + } elseif ( $progress['result'] !== 'Continue' || $progress['stage'] !== 'uploading' ) { | |
| + $this->dieUsage( | |
| + 'Chunked upload is already completed, check status for details', 'stashfailed' | |
| + ); | |
| + } | |
| + | |
| $status = $this->mUpload->addChunk( | |
| $chunkPath, $chunkSize, $this->mParams['offset'] ); | |
| if ( !$status->isGood() ) { | |
| @@ -229,18 +258,12 @@ class ApiUpload extends ApiBase { | |
| ); | |
| $this->dieUsage( $status->getWikiText(), 'stashfailed', 0, $extradata ); | |
| - | |
| - return array(); | |
| } | |
| } | |
| // Check we added the last chunk: | |
| - if ( $this->mParams['offset'] + $chunkSize == $this->mParams['filesize'] ) { | |
| + if ( $totalSoFar == $this->mParams['filesize'] ) { | |
| if ( $this->mParams['async'] ) { | |
| - $progress = UploadBase::getSessionStatus( $this->getUser(), $filekey ); | |
| - if ( $progress && $progress['result'] === 'Poll' ) { | |
| - $this->dieUsage( "Chunk assembly already in progress.", 'stashfailed' ); | |
| - } | |
| UploadBase::setSessionStatus( | |
| $this->getUser(), | |
| $filekey, | |
| @@ -260,21 +283,37 @@ class ApiUpload extends ApiBase { | |
| } else { | |
| $status = $this->mUpload->concatenateChunks(); | |
| if ( !$status->isGood() ) { | |
| + UploadBase::setSessionStatus( | |
| + $this->getUser(), | |
| + $filekey, | |
| + array( 'result' => 'Failure', 'stage' => 'assembling', 'status' => $status ) | |
| + ); | |
| $this->dieUsage( $status->getWikiText(), 'stashfailed' ); | |
| - | |
| - return array(); | |
| } | |
| // The fully concatenated file has a new filekey. So remove | |
| // the old filekey and fetch the new one. | |
| + UploadBase::setSessionStatus( $this->getUser(), $filekey, false ); | |
| $this->mUpload->stash->removeFile( $filekey ); | |
| $filekey = $this->mUpload->getLocalFile()->getFileKey(); | |
| $result['result'] = 'Success'; | |
| } | |
| + } else { | |
| + UploadBase::setSessionStatus( | |
| + $this->getUser(), | |
| + $filekey, | |
| + array( | |
| + 'result' => 'Continue', | |
| + 'stage' => 'uploading', | |
| + 'offset' => $totalSoFar, | |
| + 'status' => Status::newGood(), | |
| + ) | |
| + ); | |
| + $result['result'] = 'Continue'; | |
| + $result['offset'] = $totalSoFar; | |
| } | |
| $result['filekey'] = $filekey; | |
| - $result['offset'] = $this->mParams['offset'] + $chunkSize; | |
| return $result; | |
| } | |
| @@ -384,6 +423,10 @@ class ApiUpload extends ApiBase { | |
| // Chunk upload | |
| $this->mUpload = new UploadFromChunks(); | |
| if ( isset( $this->mParams['filekey'] ) ) { | |
| + if ( $this->mParams['offset'] === 0 ) { | |
| + $this->dieUsage( 'Cannot supply a filekey when offset is 0', 'badparams' ); | |
| + } | |
| + | |
| // handle new chunk | |
| $this->mUpload->continueChunks( | |
| $this->mParams['filename'], | |
| @@ -391,6 +434,10 @@ class ApiUpload extends ApiBase { | |
| $request->getUpload( 'chunk' ) | |
| ); | |
| } else { | |
| + if ( $this->mParams['offset'] !== 0 ) { | |
| + $this->dieUsage( 'Must supply a filekey when offset is non-zero', 'badparams' ); | |
| + } | |
| + | |
| // handle first chunk | |
| $this->mUpload->initialize( | |
| $this->mParams['filename'], | |
| @@ -766,8 +813,15 @@ class ApiUpload extends ApiBase { | |
| ), | |
| 'stash' => false, | |
| - 'filesize' => null, | |
| - 'offset' => null, | |
| + 'filesize' => array( | |
| + ApiBase::PARAM_TYPE => 'integer', | |
| + ApiBase::PARAM_MIN => 0, | |
| + ApiBase::PARAM_MAX => UploadBase::getMaxUploadSize(), | |
| + ), | |
| + 'offset' => array( | |
| + ApiBase::PARAM_TYPE => 'integer', | |
| + ApiBase::PARAM_MIN => 0, | |
| + ), | |
| 'chunk' => array( | |
| ApiBase::PARAM_TYPE => 'upload', | |
| ), | |
| -- | |
| 2.6.1 | |
File Metadata
File Metadata
- Mime Type
- text/x-diff
- Storage Engine
- blob
- Storage Format
- Raw Data
- Storage Handle
- 2736995
- Default Alt Text
- T91205-REL1_25.patch (8 KB)