Merge pull request #9969 from creative-commoners/480-tag

Security fixes from 4.8.0

_config/dev.yml

@@ -17,3 +17,6 @@ SilverStripe\Dev\DevelopmentAdmin:
       controller: Silverstripe\Dev\DevConfigController
       links:
         config: 'View the current config, useful for debugging'
+
+SilverStripe\Dev\CSSContentParser:
+  disable_xml_external_entities: true

docs/en/02_Developer_Guides/19_GraphQL/04_security_and_best_practices/01_authentication.md

@@ -43,11 +43,13 @@ the [CSRF Middleware](csrf_protection) enabled. (It is by default).**
 ### HTTP basic authentication
 
 Silverstripe CMS has built-in support for [HTTP basic authentication](https://en.wikipedia.org/wiki/Basic_access_authentication).
-There is a `BasicAuthAuthenticator` which is configured for GraphQL by default, but
+
+There is a `BasicAuthAuthenticator` which can be configured for GraphQL that
 will only activate when required. It is kept separate from the SilverStripe CMS
 authenticator because GraphQL needs to use the successfully authenticated member
 for CMS permission filtering, whereas the global `BasicAuth` does not log the
-member in or use it for model security.
+member in or use it for model security.  Note that basic auth will bypass MFA authentication
+so if MFA is enabled it is not recommended that you also use basic auth for graphql.
 
 When using HTTP basic authentication, you can feel free to remove the [CSRF Middleware](csrf_protection),
 as it just adds unnecessary overhead to the request.
@@ -95,7 +97,7 @@ is applicable in the current request context (provided as an argument).
 Here's an example for implementing HTTP basic authentication:
 
 [notice]
-Note that basic auth is enabled by default.
+Note that basic authentication for graphql will bypass Multi-Factor Authentication (MFA) if that's enabled. Using basic authentication for graphql is considered insecure if you are using MFA .
 [/notice]
 
 ```yaml

src/Dev/CSSContentParser.php

@@ -2,6 +2,7 @@
 
 namespace SilverStripe\Dev;
 
+use SilverStripe\Core\Config\Configurable;
 use SilverStripe\Core\Injector\Injectable;
 use SimpleXMLElement;
 use tidy;
@@ -25,6 +26,7 @@
 class CSSContentParser
 {
     use Injectable;
+    use Configurable;
 
     protected $simpleXML = null;
 
@@ -56,6 +58,13 @@ public function __construct($content)
             $tidy = $content;
         }
 
+        // Prevent loading of external entities to prevent XXE attacks
+        // Note: as of libxml 2.9.0 entity substitution is disabled by default so this won't be required
+        if ($this->config()->get('disable_xml_external_entities')) {
+            libxml_set_external_entity_loader(function () {
+                return null;
+            });
+        }
         $this->simpleXML = @simplexml_load_string($tidy, 'SimpleXMLElement', LIBXML_NOWARNING);
         if (!$this->simpleXML) {
             throw new Exception('CSSContentParser::__construct(): Could not parse content.'

src/Forms/FileField.php

@@ -177,13 +177,46 @@ public function Value()
 
     public function validate($validator)
     {
-        if (!isset($_FILES[$this->name])) {
+        // FileField with the name multi_file_syntax[] or multi_file_syntax[key] will have the brackets trimmed in
+        // $_FILES super-global so it will be stored as $_FILES['mutli_file_syntax']
+        // multi-file uploads, which are not officially supported by Silverstripe, though may be
+        // implemented in custom code, so we should still ensure they are at least validated
+        $isMultiFileUpload = strpos($this->name, '[') !== false;
+        $fieldName = preg_replace('#\[(.*?)\]$#', '', $this->name);
+
+        if (!isset($_FILES[$fieldName])) {
             return true;
         }
 
-        $tmpFile = $_FILES[$this->name];
+        if ($isMultiFileUpload) {
+            $isValid = true;
+            foreach (array_keys($_FILES[$fieldName]['name']) as $key) {
+                $fileData = [
+                    'name' => $_FILES[$fieldName]['name'][$key],
+                    'type' => $_FILES[$fieldName]['type'][$key],
+                    'tmp_name' => $_FILES[$fieldName]['tmp_name'][$key],
+                    'error' => $_FILES[$fieldName]['error'][$key],
+                    'size' => $_FILES[$fieldName]['size'][$key],
+                ];
+                if (!$this->validateFileData($validator, $fileData)) {
+                    $isValid = false;
+                }
+            }
+            return $isValid;
+        }
+
+        // regular single-file upload
+        return $this->validateFileData($validator, $_FILES[$this->name]);
+    }
 
-        $valid = $this->upload->validate($tmpFile);
+    /**
+     * @param Validator $validator
+     * @param array $fileData
+     * @return bool
+     */
+    private function validateFileData($validator, array $fileData): bool
+    {
+        $valid = $this->upload->validate($fileData);
         if (!$valid) {
             $errors = $this->upload->getErrors();
             if ($errors) {
@@ -193,7 +226,6 @@ public function validate($validator)
             }
             return false;
         }
-
         return true;
     }
 

tests/php/Dev/CSSContentParserTest.php

@@ -50,4 +50,14 @@ public function testGetBySelector()
         $result = $parser->getBySelector('#A .other');
         $this->assertEquals("result", $result[0] . '');
     }
+
+    public function testXmlEntitiesDisabled()
+    {
+        // CSSContentParser uses simplexml to parse html
+        // Ensure XML entities are not substituted in to prevent XXE attacks
+        $xml = '<!DOCTYPE html [<!ENTITY myentity "World">]><html><div>Hello &myentity;</div></html>';
+        $parser = new CSSContentParser($xml);
+        $div = $parser->getBySelector('div')[0]->asXML();
+        $this->assertEquals('<div>Hello &amp;myentity;</div>', $div);
+    }
 }

tests/php/Forms/FileFieldTest.php

@@ -3,6 +3,7 @@
 namespace SilverStripe\Forms\Tests;
 
 use ReflectionMethod;
+use SilverStripe\Assets\Upload_Validator;
 use SilverStripe\Dev\FunctionalTest;
 use SilverStripe\Control\Controller;
 use SilverStripe\Forms\FileField;
@@ -39,6 +40,67 @@ public function testUploadRequiredFile()
         $this->assertTrue($form->validationResult()->isValid());
     }
 
+    /**
+     * Test that FileField::validate() is run on FileFields with both single and multi-file syntax
+     * By default FileField::validate() will return true early if the $_FILES super-global does not contain the
+     * corresponding FileField::name. This early return means the files was not fully run through FileField::validate()
+     * So for this test we create an invalid file upload on purpose and test that false was returned which means that
+     * the file was run through FileField::validate() function
+     */
+    public function testMultiFileSyntaxUploadIsValidated()
+    {
+        $names = [
+            'single_file_syntax',
+            'multi_file_syntax_a[]',
+            'multi_file_syntax_b[0]',
+            'multi_file_syntax_c[key]'
+        ];
+        foreach ($names as $name) {
+            $form = new Form(
+                Controller::curr(),
+                'Form',
+                new FieldList($fileField = new FileField($name, 'My desc')),
+                new FieldList()
+            );
+            $fileData = $this->createInvalidUploadedFileData($name, "FileFieldTest.txt");
+            // FileFields with multi_file_syntax[] files will appear in the $_FILES super-global
+            // with the [] brackets trimmed e.g. $_FILES[multi_file_syntax]
+            $_FILES = [preg_replace('#\[(.*?)\]#', '', $name) => $fileData];
+            $fileField->setValue($fileData);
+            $validator = $form->getValidator();
+            $isValid = $fileField->validate($validator);
+            $this->assertFalse($isValid, "$name was run through the validate() function");
+        }
+    }
+
+    protected function createInvalidUploadedFileData($name, $tmpFileName): array
+    {
+        $tmpFilePath = TEMP_PATH . DIRECTORY_SEPARATOR . $tmpFileName;
+
+        // multi_file_syntax
+        if (strpos($name, '[') !== false) {
+            $key = 0;
+            if (preg_match('#\[(.+?)\]#', $name, $m)) {
+                $key = $m[1];
+            }
+            return [
+                'name' => [$key => $tmpFileName],
+                'type' => [$key => 'text/plaintext'],
+                'size' => [$key => 0],
+                'tmp_name' => [$key => $tmpFilePath],
+                'error' => [$key => UPLOAD_ERR_NO_FILE],
+            ];
+        }
+        // single_file_syntax
+        return [
+            'name' => $tmpFileName,
+            'type' => 'text/plaintext',
+            'size' => 0,
+            'tmp_name' => $tmpFilePath,
+            'error' => UPLOAD_ERR_NO_FILE,
+        ];
+    }
+
     /**
      * @skipUpgrade
      */