Merge pull request #3022 from tractorcow/pulls/3.2-upload-restrictions

API Remove html,htm,xhtml,xml as default allowed uploadable file types

docs/en/changelogs/3.2.0.md

@@ -31,6 +31,15 @@ use `setDisplayFolderName()` with a folder path relative to `assets/`:
 
 	UploadField::create('MyField')->setDisplayFolderName('Uploads');
 
+### File.allowed_extensions restrictions
+
+Certain file types such as swf, html, htm, xhtml and xml have been removed from the list
+of allowable file uploads. If your application requires the ability to upload these,
+you will need to append these to the `File.allowed_extensions` config as necessary.
+Also if uploading other file types, it's necessary to ensure that `File.allowed_extensions`
+includes that extension, as extensions passed to `[api:UploadField]` will be filtered against
+this list.
+
 ### Removed format detection in i18n::$date_format and i18n::$time_format
 
 Localized dates cause inconsistencies in client-side vs. server-side formatting

docs/en/reference/uploadfield.md

@@ -143,9 +143,15 @@ to known file categories.
 	:::php
 	// This will limit files to the following extensions:
 	// "bmp" ,"gif" ,"jpg" ,"jpeg" ,"pcx" ,"tif" ,"png" ,"alpha","als" ,"cel" ,"icon" ,"ico" ,"ps"
-	// 'doc','docx','txt','rtf','xls','xlsx','pages', 'ppt','pptx','pps','csv', 'html','htm','xhtml', 'xml','pdf'
+	// 'doc','docx','txt','rtf','xls','xlsx','pages', 'ppt','pptx','pps','csv','pdf'
 	$uploadField->setAllowedFileCategories('image', 'doc');
 
+<div class="notice" markdown='1'>
+Note: File types such as SWF, XML and HTML are excluded by default from uploading as these types are common
+security attack risks. If necessary, these types may be allowed as uploads (at your own risk) by adding each
+extension to the `File.allowed_extensions` config or setting `File.apply_restrictions_to_admin` to false.
+See [the security topic](/topics/security#user-uploaded-files) for more information.
+</div>
 
 ### Limit the maximum file size
 

docs/en/topics/security.md

@@ -417,6 +417,34 @@ you need to serve directly.
 See [Apache](/installation/webserver) and [Nginx](/installation/nginx) installation documentation for details 
 specific to your web server
 
+### User uploaded files
+
+Certain file types are by default excluded from user upload. html, xhtml, htm, and xml files may have embedded,
+or contain links to, external resources or scripts that may hijack browser sessions and impersonate that user.
+Even if the uploader of this content may be a trusted user, there is no safeguard against these users being
+deceived by the content source.
+
+Flash files (swf) are also prone to a variety of security vulnerabilities of their own, and thus by default are
+disabled from file upload. As a standard practice, any users wishing to allow flash upload to their sites should
+take the following precautions:
+
+ * Only allow flash uploads from trusted sources, preferably those with available source.
+ * Make use of the [AllowScriptAccess](http://helpx.adobe.com/flash/kb/control-access-scripts-host-web.html)
+   parameter to ensure that any embedded Flash file is isolated from its environments scripts. In an ideal
+   situation, all flash content would be served from another domain, and this value is set to "sameDomain". If this
+   is not feasible, this should be set to "never". For trusted flash files you may set this to "sameDomain" without
+   an isolated domain name, but do so at your own risk.
+ * Take note of any regional cookie legislation that may affect your users. See
+   [Cookie Law and Flash Cookies](http://eucookiedirective.com/cookie-law-and-flash-cookies/).
+
+See [the Adobe Flash security page](http://www.adobe.com/devnet/flashplayer/security.html) for more information.
+
+ADMIN privileged users may be allowed to override the above upload restrictions if the
+`File.apply_restrictions_to_admin` config is set to false. By default this is true, which enforces these
+restrictions globally.
+
+Additionally, if certain file uploads should be made available to non-privileged users, you can add them to the
+list of allowed extensions by adding these to the `File.allowed_extensions` config.
 
 ## Passwords
 

filesystem/File.php

@@ -127,10 +127,10 @@ class File extends DataObject {
 	 */
 	private static $allowed_extensions = array(
 		'','ace','arc','arj','asf','au','avi','bmp','bz2','cab','cda','css','csv','dmg','doc','docx',
-		'flv','gif','gpx','gz','hqx','htm','html','ico','jar','jpeg','jpg','js','kml', 'm4a','m4v',
+		'flv','gif','gpx','gz','hqx','ico','jar','jpeg','jpg','js','kml', 'm4a','m4v',
 		'mid','midi','mkv','mov','mp3','mp4','mpa','mpeg','mpg','ogg','ogv','pages','pcx','pdf','pkg',
-		'png','pps','ppt','pptx','ra','ram','rm','rtf','sit','sitx','swf','tar','tgz','tif','tiff',
-		'txt','wav','webm','wma','wmv','xhtml','xls','xlsx','xml','zip','zipx',
+		'png','pps','ppt','pptx','ra','ram','rm','rtf','sit','sitx','tar','tgz','tif','tiff',
+		'txt','wav','webm','wma','wmv','xls','xlsx','zip','zipx',
 	);
 
 	/**

filesystem/Folder.php

@@ -144,27 +144,33 @@ class Folder extends File {
 
 		if(file_exists($baseDir)) {
 			$actualChildren = scandir($baseDir);
-			$ignoreRules = Config::inst()->get('Filesystem', 'sync_blacklisted_patterns');
+			$ignoreRules = Filesystem::config()->sync_blacklisted_patterns;
+			$allowedExtensions = File::config()->allowed_extensions;
+			$checkExtensions = $this->config()->apply_restrictions_to_admin || !Permission::check('ADMIN');
 
 			foreach($actualChildren as $actualChild) {
-				if($ignoreRules) {
-					$skip = false;
-
-					foreach($ignoreRules as $rule) {
-						if(preg_match($rule, $actualChild)) {
-							$skip = true;
-
-							break;
-						}
-					}
-
-					if($skip) {
-						$skipped++;
-
-						continue;
+				$skip = false;
+				
+				// Check ignore patterns
+				if($ignoreRules) foreach($ignoreRules as $rule) {
+					if(preg_match($rule, $actualChild)) {
+						$skip = true;
+						break;
 					}
 				}
+				
+				// Check allowed extensions, unless admin users are allowed to bypass these exclusions
+				if($checkExtensions
+					&& ($extension = self::get_file_extension($actualChild))
+					&& !in_array(strtolower($extension), $allowedExtensions)
+				) {
+					$skip = true;
+				}
 
+				if($skip) {
+					$skipped++;
+					continue;
+				}
 
 				// A record with a bad class type doesn't deserve to exist. It must be purged!
 				if(isset($hasDbChild[$actualChild])) {
@@ -176,7 +182,6 @@ class Folder extends File {
 					}
 				}
 				
-				
 				if(isset($hasDbChild[$actualChild])) {
 					$child = $hasDbChild[$actualChild];
 					unset($unwantedDbChildren[$actualChild]);

tests/filesystem/FolderTest.php

@@ -303,7 +303,8 @@ class FolderTest extends SapphireTest {
 			'.git',
 			'web.config',
 			'.DS_Store',
-			'_my_synced_file.txt'
+			'_my_synced_file.txt',
+			'invalid_extension.xyz123'
 		);
 
 		$folders = array(
@@ -325,7 +326,7 @@ class FolderTest extends SapphireTest {
 		$folder = Folder::find_or_make('/FolderTest/sync');
 		$result = $folder->syncChildren();
 
-		$this->assertEquals(10, $result['skipped']);
+		$this->assertEquals(11, $result['skipped']);
 		$this->assertEquals(2, $result['added']);
 
 		// folder with a path of _test should exist
@@ -336,6 +337,10 @@ class FolderTest extends SapphireTest {
 		$this->assertEquals(1, File::get()->filter(array(
 			'Name' => '_my_synced_file.txt'
 		))->count());
+
+		$this->assertEquals(0, File::get()->filter(array(
+			'Name' => 'invalid_extension.xyz123'
+		))->count());
 	}
 	
 	public function testIllegalFilenames() {

tests/forms/uploadfield/UploadFieldTest.php

@@ -161,6 +161,8 @@ class UploadFieldTest extends FunctionalTest {
 	public function testAllowedExtensions() {
 		$this->loginWithPermission('ADMIN');
 
+		// Test invalid file
+		// Relies on Upload_Validator failing to allow this extension
 		$invalidFile = 'invalid.php';
 		$_FILES = array('AllowedExtensionsField' => $this->getUploadFile($invalidFile));
 		$response = $this->post(
@@ -170,6 +172,7 @@ class UploadFieldTest extends FunctionalTest {
 		$this->assertTrue($response->isError());
 		$this->assertContains('Extension is not allowed', $response->getBody());
 
+		// Test valid file
 		$validFile = 'valid.txt';
 		$_FILES = array('AllowedExtensionsField' => $this->getUploadFile($validFile));
 		$response = $this->post(
@@ -178,6 +181,17 @@ class UploadFieldTest extends FunctionalTest {
 		);
 		$this->assertFalse($response->isError());
 		$this->assertNotContains('Extension is not allowed', $response->getBody());
+		
+		// Test that setAllowedExtensions rejects extensions explicitly denied by File.allowed_extensions
+		// Relies on File::validate failing to allow this extension
+		$invalidFile = 'invalid.php';
+		$_FILES = array('AllowedExtensionsField' => $this->getUploadFile($invalidFile));
+		$response = $this->post(
+			'UploadFieldTest_Controller/Form/field/InvalidAllowedExtensionsField/upload',
+			array('InvalidAllowedExtensionsField' => $this->getUploadFile($invalidFile))
+		);
+		$this->assertTrue($response->isError());
+		$this->assertContains('Extension is not allowed', $response->getBody());
 	}
 
 	/**
@@ -1005,6 +1019,9 @@ class UploadFieldTestForm extends Form implements TestOnly {
 		$fieldAllowedExtensions = new UploadField('AllowedExtensionsField');
 		$fieldAllowedExtensions->getValidator()->setAllowedExtensions(array('txt'));
 
+		$fieldInvalidAllowedExtensions = new UploadField('InvalidAllowedExtensionsField');
+		$fieldInvalidAllowedExtensions->getValidator()->setAllowedExtensions(array('txt', 'php'));
+
 		$fields = new FieldList(
 			$fieldRootFolder,
 			$fieldNoRelation,
@@ -1022,7 +1039,8 @@ class UploadFieldTestForm extends Form implements TestOnly {
 			$fieldSubfolder,
 			$fieldCanUploadFalse,
 			$fieldCanAttachExisting,
-			$fieldAllowedExtensions
+			$fieldAllowedExtensions,
+			$fieldInvalidAllowedExtensions
 		);
 		$actions = new FieldList(
 			new FormAction('submit')