From 2e73dcb8912223abb88354e09b13aafa902af8af Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Wed, 9 Apr 2014 17:37:25 +1200 Subject: [PATCH] API Remove swf,html,htm,xhtml,xml as default allowed upload able file types --- docs/en/changelogs/3.2.0.md | 9 +++++ docs/en/reference/uploadfield.md | 8 ++++- docs/en/topics/security.md | 28 +++++++++++++++ filesystem/File.php | 6 ++-- filesystem/Folder.php | 39 ++++++++++++--------- tests/filesystem/FolderTest.php | 9 +++-- tests/forms/uploadfield/UploadFieldTest.php | 20 ++++++++++- 7 files changed, 95 insertions(+), 24 deletions(-) diff --git a/docs/en/changelogs/3.2.0.md b/docs/en/changelogs/3.2.0.md index 685663538..470002d75 100644 --- a/docs/en/changelogs/3.2.0.md +++ b/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 diff --git a/docs/en/reference/uploadfield.md b/docs/en/reference/uploadfield.md index ed852989e..52e038034 100644 --- a/docs/en/reference/uploadfield.md +++ b/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'); +
+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. +
### Limit the maximum file size diff --git a/docs/en/topics/security.md b/docs/en/topics/security.md index 44493a9f0..0ebbef7bd 100644 --- a/docs/en/topics/security.md +++ b/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 diff --git a/filesystem/File.php b/filesystem/File.php index a47a2317b..dea381bd2 100644 --- a/filesystem/File.php +++ b/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', ); /** diff --git a/filesystem/Folder.php b/filesystem/Folder.php index bad6d0d0f..64b39139a 100644 --- a/filesystem/Folder.php +++ b/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]); diff --git a/tests/filesystem/FolderTest.php b/tests/filesystem/FolderTest.php index d37d69b2f..8f2f9fba5 100644 --- a/tests/filesystem/FolderTest.php +++ b/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() { diff --git a/tests/forms/uploadfield/UploadFieldTest.php b/tests/forms/uploadfield/UploadFieldTest.php index eb5849c07..058483267 100644 --- a/tests/forms/uploadfield/UploadFieldTest.php +++ b/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')