Improved file uploads

CHANGELOG.md

@@ -3,6 +3,10 @@
 
 1. [](#improved)
     * Change usage of basename where possible [#1480](https://github.com/getgrav/grav-plugin-admin/pull/1480)
+    * Improved filename validation (requires Grav 1.5.3)
+1. [](#bugfix)
+    * File Uploads: Do not trust mimetype sent by the browser
+    * Fixed file extension detection
 
 # v1.8.10
 ## 10/01/2018

blueprints.yaml

@@ -13,7 +13,7 @@ docs: https://github.com/getgrav/grav-plugin-admin/blob/develop/README.md
 license: MIT
 
 dependencies:
-    - { name: grav, version: '>=1.5.2' }
+    - { name: grav, version: '>=1.5.3' }
     - { name: form, version: '>=2.14.0' }
     - { name: login, version: '>=2.7.0' }
     - { name: email, version: '>=2.7.0' }

classes/adminbasecontroller.php

@@ -227,10 +227,10 @@ class AdminBaseController
 
         $upload = $this->normalizeFiles($_FILES['data'], $settings->name);
 
-        $filename = trim($upload->file->name);
+        $filename = $upload->file->name;
 
         // Handle bad filenames.
-        if (strtr($filename, "\t\n\r\0\x0b", '_____') !== $filename || rtrim($filename, '. ') !== $filename || preg_match('|\.php|', $filename)) {
+        if (!Utils::checkFilename($filename)) {
             $this->admin->json_response = [
                 'status'  => 'error',
                 'message' => sprintf($this->admin->translate('PLUGIN_ADMIN.FILEUPLOAD_UNABLE_TO_UPLOAD', null),
@@ -287,6 +287,9 @@ class AdminBaseController
         $accepted = false;
         $errors   = [];
 
+        // Do not trust mimetype sent by the browser
+        $mime = Utils::getMimeByFilename($upload->file->name);
+
         foreach ((array)$settings->accept as $type) {
             // Force acceptance of any file when star notation
             if ($type === '*') {
@@ -295,15 +298,24 @@ class AdminBaseController
             }
 
             $isMime = strstr($type, '/');
-            $find   = str_replace('*', '.*', $type);
+            $find   = str_replace(['.', '*'], ['\.', '.*'], $type);
 
-            $match = preg_match('#' . $find . '$#', $isMime ? $upload->file->type : $upload->file->name);
+            if ($isMime) {
+                $match = preg_match('#' . $find . '$#', $mime);
                 if (!$match) {
-                $message  = $isMime ? 'The MIME type "' . $upload->file->type . '"' : 'The File Extension';
+                    $errors[] = 'The MIME type "' . $mime . '" for the file "' . $upload->file->name . '" is not an accepted.';
-                $errors[] = $message . ' for the file "' . $upload->file->name . '" is not an accepted.';
-                $accepted |= false;
                 } else {
-                $accepted |= true;
+                    $accepted = true;
+                    break;
+                }
+            } else {
+                $match = preg_match('#' . $find . '$#', $upload->file->name);
+                if (!$match) {
+                    $errors[] = 'The File Extension for the file "' . $upload->file->name . '" is not an accepted.';
+                } else {
+                    $accepted = true;
+                    break;
+                }
             }
         }
 

classes/admincontroller.php

@@ -1685,6 +1685,19 @@ class AdminController extends AdminBaseController
                 return false;
         }
 
+        $filename = $_FILES['file']['name'];
+
+        // Handle bad filenames.
+        if (!Utils::checkFilename($filename)) {
+            $this->admin->json_response = [
+                'status'  => 'error',
+                'message' => sprintf($this->admin->translate('PLUGIN_ADMIN.FILEUPLOAD_UNABLE_TO_UPLOAD'),
+                    $filename, 'Bad filename')
+            ];
+
+            return false;
+        }
+
         $grav_limit = $config->get('system.media.upload_limit', 0);
         // You should also check filesize here.
         if ($grav_limit > 0 && $_FILES['file']['size'] > $grav_limit) {
@@ -1698,18 +1711,13 @@ class AdminController extends AdminBaseController
 
 
         // Check extension
-        $fileParts = pathinfo($_FILES['file']['name']);
+        $extension = strtolower(pathinfo($filename, PATHINFO_EXTENSION));
-
-        $fileExt = '';
-        if (isset($fileParts['extension'])) {
-            $fileExt = strtolower($fileParts['extension']);
-        }
 
         // If not a supported type, return
-        if (!$fileExt || !$config->get("media.types.{$fileExt}")) {
+        if (!$extension || !$config->get("media.types.{$extension}")) {
             $this->admin->json_response = [
                 'status'  => 'error',
-                'message' => $this->admin->translate('PLUGIN_ADMIN.UNSUPPORTED_FILE_TYPE') . ': ' . $fileExt
+                'message' => $this->admin->translate('PLUGIN_ADMIN.UNSUPPORTED_FILE_TYPE') . ': ' . $extension
             ];
 
             return false;
@@ -1735,7 +1743,7 @@ class AdminController extends AdminBaseController
 
         // Upload it
         if (!move_uploaded_file($_FILES['file']['tmp_name'],
-            sprintf('%s/%s', $path, $_FILES['file']['name']))
+            sprintf('%s/%s', $path, $filename))
         ) {
             $this->admin->json_response = [
                 'status'  => 'error',
@@ -1747,13 +1755,12 @@ class AdminController extends AdminBaseController
 
         // Add metadata if needed
         $include_metadata = Grav::instance()['config']->get('system.media.auto_metadata_exif', false);
-        $filename = $fileParts['basename'];
+        $basename = str_replace(['@3x', '@2x'], '', pathinfo($filename, PATHINFO_BASENAME));
-        $filename = str_replace(['@3x', '@2x'], '', $filename);
 
         $metadata = [];
 
-        if ($include_metadata && isset($media[$filename])) {
+        if ($include_metadata && isset($media[$basename])) {
-            $img_metadata = $media[$filename]->metadata();
+            $img_metadata = $media[$basename]->metadata();
             if ($img_metadata) {
                 $metadata = $img_metadata;
             }
@@ -1796,6 +1803,11 @@ class AdminController extends AdminBaseController
 
         $filename = !empty($this->post['filename']) ? $this->post['filename'] : null;
 
+        // Handle bad filenames.
+        if (!Utils::checkFilename($filename)) {
+            $filename = null;
+        }
+
         if (!$filename) {
             $this->admin->json_response = [
                 'status'  => 'error',
@@ -1884,7 +1896,7 @@ class AdminController extends AdminBaseController
     protected function taskProcessMarkdown()
     {
         if (!$this->authorizeTask('process markdown', ['admin.pages', 'admin.super'])) {
-            return;
+            return false;
         }
 
         try {
@@ -2274,6 +2286,16 @@ class AdminController extends AdminBaseController
             }
 
             $file_path = $_FILES['uploaded_file']['tmp_name'];
+
+            // Handle bad filenames.
+            if (!Utils::checkFilename($file_path)) {
+                $this->admin->json_response = [
+                    'status'  => 'error',
+                    'message' => $this->admin->translate('PLUGIN_ADMIN.UNKNOWN_ERRORS')
+                ];
+
+                return false;
+            }
         }