[CVE-2020-26138] Validate custom multi-file uploads

silverstripe · Jun 8, 2021 · 06dbd52 · 06dbd52
1 parent 3bb435c
commit 06dbd52
Show file tree

Hide file tree

Showing 2 changed files with 98 additions and 4 deletions.
diff --git a/src/Forms/FileField.php b/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;
     }
 

diff --git a/tests/php/Forms/FileFieldTest.php b/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
      */