Page MenuHomePhabricator
Files F2724623

T91205-REL1_23.patch
demon (Chad Horohoe)
Actions

Authored By
• demon
Oct 15 2015, 8:49 PM
Size
7 KB
Referenced Files
None
Subscribers
None

T91205-REL1_23.patch
View Options

From 2c554c1ab39a57f503e48fcd213dc0f806e4aee1 Mon Sep 17 00:00:00 2001
From: Chad Horohoe <chadh@wikimedia.org>
Date: Thu, 15 Oct 2015 13:47:16 -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 | 63 +++++++++++++++++++++++++++++++--------
5 files changed, 72 insertions(+), 14 deletions(-)
diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php
index cededac..4e5c8fd 100644
--- a/includes/DefaultSettings.php
+++ b/includes/DefaultSettings.php
@@ -670,6 +670,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
diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php
index 8941b97..ef6a53f 100644
--- a/includes/GlobalFunctions.php
+++ b/includes/GlobalFunctions.php
@@ -3942,12 +3942,13 @@ function wfMemoryLimit() {
* Converts shorthand byte notation to integer form
*
* @param string $string
+ * @param int $default Returned 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 b20ce14..0ce1733 100644
--- a/includes/Setup.php
+++ b/includes/Setup.php
@@ -295,6 +295,15 @@ if ( $wgResourceLoaderMaxQueryLength === false ) {
$wgResourceLoaderMaxQueryLength = $maxValueLength > 0 ? $maxValueLength : -1;
}
+// 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 b0e9bd2..97afe7e 100644
--- a/includes/api/ApiQuerySiteinfo.php
+++ b/includes/api/ApiQuerySiteinfo.php
@@ -247,6 +247,7 @@ class ApiQuerySiteinfo extends ApiQueryBase {
}
$data['maxuploadsize'] = UploadBase::getMaxUploadSize();
+ $data['minuploadchunksize'] = (int)$this->getConfig()->get( 'MinUploadChunkSize' );
$data['thumblimits'] = $GLOBALS['wgThumbLimits'];
$this->getResult()->setIndexedTagName( $data['thumblimits'], 'limit' );
diff --git a/includes/api/ApiUpload.php b/includes/api/ApiUpload.php
index 354bd0d..1f45ea8 100644
--- a/includes/api/ApiUpload.php
+++ b/includes/api/ApiUpload.php
@@ -82,7 +82,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' );
}
@@ -202,13 +202,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();
@@ -218,23 +235,29 @@ 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'
+ );
+ }
+
/** @var $status Status */
$status = $this->mUpload->addChunk(
$chunkPath, $chunkSize, $this->mParams['offset'] );
if ( !$status->isGood() ) {
$this->dieUsage( $status->getWikiText(), 'stashfailed' );
-
- 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( $filekey );
- if ( $progress && $progress['result'] === 'Poll' ) {
- $this->dieUsage( "Chunk assembly already in progress.", 'stashfailed' );
- }
UploadBase::setSessionStatus(
$filekey,
array( 'result' => 'Poll',
@@ -258,21 +281,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;
}
--
2.6.1

File Metadata

Mime Type
text/x-diff
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
2737038
Default Alt Text
T91205-REL1_23.patch (7 KB)

Event Timeline

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL