MarkAHershberger has uploaded a new change for review.
https://gerrit.wikimedia.org/r/64857
Change subject: Chunked uploads allow arbitrary data to be dropped on the server
......................................................................
Chunked uploads allow arbitrary data to be dropped on the server
Bug: 48306
Change-Id: Icd8e799aaa2cca06495c690187283dd2913a17e3
---
M RELEASE-NOTES-1.21
M includes/AutoLoader.php
M includes/api/ApiUpload.php
M includes/upload/UploadBase.php
M includes/upload/UploadFromChunks.php
M includes/upload/UploadFromStash.php
M includes/upload/UploadStash.php
7 files changed, 112 insertions(+), 33 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/57/64857/1
diff --git a/RELEASE-NOTES-1.21 b/RELEASE-NOTES-1.21
index ef82ac4..d1fb8cb 100644
--- a/RELEASE-NOTES-1.21
+++ b/RELEASE-NOTES-1.21
@@ -124,6 +124,7 @@
quotmarks, order and whitespace in the attribute list).
=== Bug fixes in 1.21 ===
+* (bug 48306) Chunked uploads allow arbitrary data to be dropped on the server
* (bug 47271) $wgContentHandlerUseDB should be set to false during the upgrade
* (bug 46084) Sanitize $limitReport before outputting.
* (bug 46859) Disable external entities in XMLReader.
diff --git a/includes/AutoLoader.php b/includes/AutoLoader.php
index 4248178..3555e2c 100644
--- a/includes/AutoLoader.php
+++ b/includes/AutoLoader.php
@@ -997,6 +997,7 @@
'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 2dcf392..98833ab 100644
--- a/includes/api/ApiUpload.php
+++ b/includes/api/ApiUpload.php
@@ -195,7 +195,12 @@
$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 1b4d411..5a82362 100644
--- a/includes/upload/UploadBase.php
+++ b/includes/upload/UploadBase.php
@@ -355,6 +355,8 @@
/**
* 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
*/
@@ -367,12 +369,6 @@
if ( $this->checkFileExtension( $mime,
$wgMimeTypeBlacklist ) ) {
wfProfileOut( __METHOD__ );
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
@@ -401,6 +397,56 @@
* @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__ );
@@ -459,23 +505,6 @@
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;
}
diff --git a/includes/upload/UploadFromChunks.php
b/includes/upload/UploadFromChunks.php
index e784e51..4b331e9 100644
--- a/includes/upload/UploadFromChunks.php
+++ b/includes/upload/UploadFromChunks.php
@@ -70,6 +70,8 @@
// 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 @@
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 @@
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 @@
}
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 fd2416d..c82103c 100644
--- a/includes/upload/UploadFromStash.php
+++ b/includes/upload/UploadFromStash.php
@@ -137,14 +137,9 @@
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 cfa3879..089bd8b 100644
--- a/includes/upload/UploadStash.php
+++ b/includes/upload/UploadStash.php
@@ -422,6 +422,7 @@
* @return string
*/
public static function getExtensionForPath( $path ) {
+ global $wgFileBlacklist;
// Does this have an extension?
$n = strrpos( $path, '.' );
$extension = null;
@@ -441,7 +442,15 @@
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;
}
/**
--
To view, visit https://gerrit.wikimedia.org/r/64857
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icd8e799aaa2cca06495c690187283dd2913a17e3
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: REL1_21
Gerrit-Owner: MarkAHershberger <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
