r65085 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r65084‎ | r65085 | r65086 >
Date:19:28, 15 April 2010
Author:maxsem
Status:resolved (Comments)
Tags:
Comment:
new-installer: don't allow uploads if uploaded in some way scripts are executable
Modified paths:
  • /branches/new-installer/phase3/includes/installer/Installer.php (modified) (history)
  • /branches/new-installer/phase3/includes/installer/WebInstaller.php (modified) (history)
  • /branches/new-installer/phase3/languages/messages/MessagesEn.php (modified) (history)
  • /branches/new-installer/phase3/maintenance/language/messages.inc (modified) (history)

Diff [purge]

Index: branches/new-installer/phase3/maintenance/language/messages.inc
——@@ -3211,6 +3211,8 @@
32123212 'config-dir-not-writable',
32133213 'config-file-extension',
32143214 'config-shell-locale',
 3215+ 'config-uploads-safe',
 3216+ 'config-uploads-not-safe',
32153217 'config-db-type',
32163218 'config-db-host',
32173219 'config-db-host-help',
——@@ -3336,6 +3338,7 @@
33373339 'config-upload-settings',
33383340 'config-upload-enable',
33393341 'config-upload-help',
 3342+ 'config-upload-disabled',
33403343 'config-upload-deleted',
33413344 'config-upload-deleted-help',
33423345 'config-logo',
Index: branches/new-installer/phase3/includes/installer/WebInstaller.php
——@@ -1362,27 +1362,33 @@
13631363 $this->addHTML( $extHtml );
13641364 }
13651365
 1366+ # Uploading
 1367+ $this->addHTML( $this->parent->getFieldsetStart( 'config-upload-settings' ) );
 1368+ if ( $this->getVar( '_UploadsAreSafe' ) ) {
 1369+ $this->addHTML(
 1370+ $this->parent->getCheckBox( array(
 1371+ 'var' => 'wgEnableUploads',
 1372+ 'label' => 'config-upload-enable',
 1373+ 'attribs' => array( 'class' => 'showHideRadio', 'rel' => 'uploadwrapper' ),
 1374+ ) ) .
 1375+ $this->parent->getHelpBox( 'config-upload-help' ) .
 1376+ '<div id="uploadwrapper" style="display: none;">' .
 1377+ $this->parent->getTextBox( array(
 1378+ 'var' => 'wgDeletedDirectory',
 1379+ 'label' => 'config-upload-deleted',
 1380+ ) ) .
 1381+ $this->parent->getHelpBox( 'config-upload-deleted-help' ) .
 1382+ '</div>'
 1383+ );
 1384+ } else {
 1385+ $this->parent->showError( 'config-upload-disabled' );
 1386+ }
13661387 $this->addHTML(
1367 - # Uploading
1368 - $this->parent->getFieldsetStart( 'config-upload-settings' ) .
1369 - $this->parent->getCheckBox( array(
1370 - 'var' => 'wgEnableUploads',
1371 - 'label' => 'config-upload-enable',
1372 - 'attribs' => array( 'class' => 'showHideRadio', 'rel' => 'uploadwrapper' ),
1373 - ) ) .
1374 - $this->parent->getHelpBox( 'config-upload-help' ) .
1375 - '<div id="uploadwrapper" style="display: none;">' .
1376 - $this->parent->getTextBox( array(
1377 - 'var' => 'wgDeletedDirectory',
1378 - 'label' => 'config-upload-deleted',
1379 - ) ) .
1380 - $this->parent->getHelpBox( 'config-upload-deleted-help' ) .
13811388 $this->parent->getTextBox( array(
13821389 'var' => 'wgLogo',
13831390 'label' => 'config-logo'
13841391 ) ) .
13851392 $this->parent->getHelpBox( 'config-logo-help' ) .
1386 - '</div>' .
13871393 $this->parent->getFieldsetEnd()
13881394 );
13891395
——@@ -1500,7 +1506,7 @@
15011507
15021508 function submit() {
15031509 $this->parent->setVarsFromRequest( array( '_RightsProfile', '_LicenseCode',
1504 - 'wgEnableEmail', 'wgPasswordSender', 'wgEnableUpload', 'wgLogo',
 1510+ 'wgEnableEmail', 'wgPasswordSender', 'wgLogo',
15051511 'wgEnableUserEmail', 'wgEnotifUserTalk', 'wgEnotifWatchlist',
15061512 'wgEmailAuthentication', 'wgMainCacheType', '_MemCachedServers' ) );
15071513
——@@ -1532,6 +1538,10 @@
15331539 $this->setVar( 'wgRightsIcon', '' );
15341540 }
15351541
 1542+ $this->setVar( 'wgEnableUploads',
 1543+ $this->getVar( 'wgEnableUploads' ) && $this->getVar( '_UploadsAreSafe' )
 1544+ );
 1545+
15361546 $exts = $this->parent->getVar( '_Extensions' );
15371547 foreach( $exts as $key => $ext ) {
15381548 if( !$this->parent->request->getCheck( 'config_ext-' . $ext ) ) {
Index: branches/new-installer/phase3/includes/installer/Installer.php
——@@ -70,6 +70,7 @@
7171 '_CCDone' => false,
7272 '_Extensions' => array(),
7373 '_MemCachedServers' => '',
 74+ '_UploadsAreSafe' => false,
7475 );
7576
7677 /**
——@@ -123,6 +124,7 @@
124125 'envCheckWriteableDir',
125126 'envCheckExtension',
126127 'envCheckShellLocale',
 128+ 'envCheckUploadsDirectory',
127129 );
128130
129131 var $installSteps = array(
——@@ -699,8 +701,52 @@
700702 # Give up
701703 return true;
702704 }
 705+
 706+ function envCheckUploadsDirectory() {
 707+ global $IP, $wgServer;
 708+ $dir = $IP . '/images/';
 709+ $url = $wgServer . $this->getVar( 'wgScriptPath' ) . '/images/';
 710+ $safe = !$this->dirIsExecutable( $dir, $url );
 711+ if ( $safe ) {
 712+ $this->showMessage( 'config-uploads-safe' );
 713+ } else {
 714+ $this->showMessage( 'config-uploads-not-safe', $dir );
 715+ }
 716+ $this->setVar( '_UploadsAreSafe', $safe );
 717+ }
703718
704719 /**
 720+ * Checks if scripts located in the given directory can be executed via the given URL
 721+ */
 722+ function dirIsExecutable( $dir, $url ) {
 723+ $scriptTypes = array(
 724+ 'php' => array(
 725+ "<?php echo 'ex' . 'ec';",
 726+ "#!/var/env php5\n<?php echo 'ex' . 'ec';",
 727+ ),
 728+ );
 729+ // it would be good to check other popular languages here, but it'll be slow
 730+
 731+ wfSuppressWarnings();
 732+ foreach ( $scriptTypes as $ext => $contents ) {
 733+ foreach ( $contents as $source ) {
 734+ $file = 'exectest.' . $ext;
 735+ if ( !file_put_contents( $dir . $file, $source ) ) {
 736+ break;
 737+ }
 738+ $text = Http::get( $url . $file );
 739+ unlink( $dir . $file );
 740+ if ( $text == 'exec' ) {
 741+ wfRestoreWarnings();
 742+ return $ext;
 743+ }
 744+ }
 745+ }
 746+ wfRestoreWarnings();
 747+ return false;
 748+ }
 749+
 750+ /**
705751 * Convert wikitext $text to HTML.
706752 *
707753 * This is potentially error prone since many parser features require a complete
Index: branches/new-installer/phase3/languages/messages/MessagesEn.php
——@@ -4365,6 +4365,9 @@
43664366 </pre>",
43674367 'config-file-extension' => 'Installing MediaWiki with <tt>$1</tt> file extensions',
43684368 'config-shell-locale' => 'Detected shell locale, $1',
 4369+'config-uploads-safe' => 'Default uploads directory is safe from arbitrary scripts execution.',
 4370+'config-uploads-not-safe' => '<strong>Warning:</strong> Your default uploads directory <tt>$1</tt> is vulnerable to arbitrary scripts execution.
 4371+Uploads will be disabled.',
43694372 'config-db-type' => 'Database type:',
43704373 'config-db-host' => 'Database host:',
43714374 'config-db-host-help' => 'If your database server is on different server, enter the host name or IP address here.
——@@ -4552,6 +4555,7 @@
45534556 For more information, read the [http://www.mediawiki.org/wiki/Manual:Security security section] in the manual.
45544557
45554558 To enable file uploads, change the mode on the <tt>images</tt> subdirectory under MediaWiki's root directory so that the web server can write to it. Then enable this option.",
 4559+'config-upload-disabled' => 'Because your web server is configured to execute scripts from the default uploads directory, uploads will be disabled.',
45564560 'config-upload-deleted' => 'Directory for deleted files :',
45574561 'config-upload-deleted-help' => 'Choose a directory in which to archive deleted files.
45584562 Ideally, this should not be accessible from the web.',

Follow-up revisions

RevisionCommit summaryAuthorDate
r65403Per Brion, reverted part of r65085, now we just warn people instead of disabl...maxsem16:18, 21 April 2010

Comments

#Comment by Brion VIBBER (talk | contribs)   20:43, 15 April 2010

This is quite clever, but unfortunately it'll just kill the ability to use uploads for most installs; unless we add the ability to store uploads into DB as an alternative I'd probably recommend toning it down for the moment.

We should definitely check whether PHP code in a file *without* a ".php" extension is executable (a possible and very scary configuration).

Actually what might be a really good idea is to see whether we can add an .htaccess file to disable PHP execution and have it work. (The reason we don't just ship one that way is that an .htaccess that does things that aren't allowed in the Apache configuration will just kill all access to that directory.) Of course if PHP can write the .htaccess, an attacker might be able to change it again. ;)

#Comment by Simetrical (talk | contribs)   21:59, 28 April 2010

Having the ability to store uploads in the DB would also avoid safe mode problems, and allow uploads to be enabled by default for all installs. As well as avoiding all security problems. Would be great if someone did that at some point, it's probably one of the best cost/benefit ratio enhancements for third parties that we could do right now. (Unless it would be harder to implement than I think!)

#Comment by 😂 (talk | contribs)   18:54, 30 April 2010

Actual issue of blocking uploads unconditionally was fixed in r65403, so I'm marking this resolved. Storing uploads in the DB is a bigger issue outside the scope of this revision :)

#Comment by MaxSem (talk | contribs)   15:22, 29 April 2010

Hmm, last time I heard about a website storing images in DB was TheDailyWTF XD

#Comment by Simetrical (talk | contribs)   17:19, 29 April 2010

It's the only sane thing to do on cruddy shared hosting where you can't necessarily write in the web root, and certainly can't write outside it. vBulletin uses the DB by default for user-uploaded files, for example. I'm pretty sure most software does the same. Of course, this is crazy if you have an installer that can set up proper write access to both web-accessible and non-web-accessible upload directories, but that's not usually the case for web apps.

Status & tagging log