Security fix to ensure file uploads are not manipulated

getgrav · Dec 5, 2017 · 73478f6 · 73478f6
1 parent a2cb2e6
commit 73478f6
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 27 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -17,6 +17,7 @@
     * Catch ValidationException to avoid potential fatal error
     * Fixed regression issue on reset fields
     * Removed `required` attribute in individual checkboxes as it forces all to be checked
+    * Security fix to ensure file uploads are not manipulated mid-post - thnx @FLH!
 
 # v2.10.0
 ## 10/26/2017

diff --git a/classes/form.php b/classes/form.php
@@ -395,37 +395,21 @@ public function uploadFiles()
             ];
         }
 
-        // Remove the error object to avoid storing it
-        unset($upload->file->error);
-
-        // we need to move the file at this stage or else
-        // it won't be available upon save later on
-        // since php removes it from the upload location
-        $tmp_dir = $grav['locator']->findResource('tmp://', true, true);
-        $tmp_file = $upload->file->tmp_name;
-        $tmp = $tmp_dir . '/uploaded-files/' . basename($tmp_file);
-
-        Folder::create(dirname($tmp));
-        if (!move_uploaded_file($tmp_file, $tmp)) {
-            // json_response
-            return [
-                'status' => 'error',
-                'message' => sprintf($grav['language']->translate('PLUGIN_FORM.FILEUPLOAD_UNABLE_TO_MOVE', null, true), '', $tmp)
-            ];
-        }
-
-        $upload->file->tmp_name = $tmp;
-
-        // Handle file size limits
-        $settings->filesize *= self::BYTES_TO_MB; // 1024 * 1024 [MB in Bytes]
-        if ($settings->filesize > 0 && $upload->file->size > $settings->filesize) {
-            // json_response
-            return [
+        // Handle bad filenames.
+        $filename = $upload->file->name;
+        if (strtr($filename, "\t\n\r\0\x0b", '_____') !== $filename || rtrim($filename, ". ") !== $filename || preg_match('|\.php|', $filename)) {
+            $this->admin->json_response = [
                 'status'  => 'error',
-                'message' => $grav['language']->translate('PLUGIN_FORM.EXCEEDED_GRAV_FILESIZE_LIMIT')
+                'message' => sprintf($this->admin->translate('PLUGIN_ADMIN.FILEUPLOAD_UNABLE_TO_UPLOAD', null),
+                    $filename, 'Bad filename')
             ];
+
+            return false;
         }
 
+        // Remove the error object to avoid storing it
+        unset($upload->file->error);
+
 
         // Handle Accepted file types
         // Accept can only be mime types (image/png | image/*) or file extensions (.pdf|.jpg)
@@ -459,6 +443,36 @@ public function uploadFiles()
             ];
         }
 
+
+        // Handle file size limits
+        $settings->filesize *= self::BYTES_TO_MB; // 1024 * 1024 [MB in Bytes]
+        if ($settings->filesize > 0 && $upload->file->size > $settings->filesize) {
+            // json_response
+            return [
+                'status'  => 'error',
+                'message' => $grav['language']->translate('PLUGIN_FORM.EXCEEDED_GRAV_FILESIZE_LIMIT')
+            ];
+        }
+
+
+        // we need to move the file at this stage or else
+        // it won't be available upon save later on
+        // since php removes it from the upload location
+        $tmp_dir = $grav['locator']->findResource('tmp://', true, true);
+        $tmp_file = $upload->file->tmp_name;
+        $tmp = $tmp_dir . '/uploaded-files/' . basename($tmp_file);
+
+        Folder::create(dirname($tmp));
+        if (!move_uploaded_file($tmp_file, $tmp)) {
+            // json_response
+            return [
+                'status' => 'error',
+                'message' => sprintf($grav['language']->translate('PLUGIN_FORM.FILEUPLOAD_UNABLE_TO_MOVE', null, true), '', $tmp)
+            ];
+        }
+
+        $upload->file->tmp_name = $tmp;
+
         // Retrieve the current session of the uploaded files for the field
         // and initialize it if it doesn't exist
         $sessionField = base64_encode($uri);