From 85bb59e2b638254cfac96241c9a2c67f52acc1c1 Mon Sep 17 00:00:00 2001 From: Timothy Cyrus Date: Thu, 4 Oct 2018 06:43:36 -0400 Subject: [PATCH 1/3] Change usage of basename where possible (#1480) Change usage of basename where possible (#1480) --- classes/admin.php | 14 +++++++------- classes/adminbasecontroller.php | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/classes/admin.php b/classes/admin.php index 4c53f8d4..efc079c7 100644 --- a/classes/admin.php +++ b/classes/admin.php @@ -170,11 +170,11 @@ class Admin /** @var \DirectoryIterator $directory */ foreach (new \DirectoryIterator($path) as $file) { - if ($file->isDir() || $file->isDot() || Utils::startsWith($file->getBasename(), '.')) { + if ($file->isDir() || $file->isDot() || Utils::startsWith($file->getFilename(), '.')) { continue; } - $lang = basename($file->getBasename(), '.yaml'); + $lang = $file->getBasename('.yaml'); $languages[$lang] = LanguageCodes::getNativeName($lang); @@ -202,7 +202,7 @@ class Admin if ($file->isDir() || !preg_match('/^[^.].*.yaml$/', $file->getFilename())) { continue; } - $configurations[] = basename($file->getBasename(), '.yaml'); + $configurations[] = $file->getBasename('.yaml'); } return $configurations; @@ -670,10 +670,10 @@ class Admin $obj->file = $file; $obj->page = $this->grav['pages']->get(dirname($obj->path)); - $filename = pathinfo($obj->title)['filename']; - $filename = str_replace(['@3x', '@2x'], '', $filename); - if (isset(pathinfo($obj->title)['extension'])) { - $filename .= '.' . pathinfo($obj->title)['extension']; + $fileInfo = pathinfo($obj->title); + $filename = str_replace(['@3x', '@2x'], '', $fileInfo['filename']); + if (isset($fileInfo['extension'])) { + $filename .= '.' . $fileInfo['extension']; } if ($obj->page && isset($obj->page->media()[$filename])) { diff --git a/classes/adminbasecontroller.php b/classes/adminbasecontroller.php index 4ed8817a..2d736cdb 100644 --- a/classes/adminbasecontroller.php +++ b/classes/adminbasecontroller.php @@ -368,7 +368,7 @@ class AdminBaseController // Generate random name if required if ($settings->random_name) { // TODO: document - $extension = pathinfo($upload->file->name)['extension']; + $extension = pathinfo($upload->file->name, PATHINFO_EXTENSION); $upload->file->name = Utils::generateRandomString(15) . '.' . $extension; } From ca1062443bf4d219355957e190ae26548c19e2d7 Mon Sep 17 00:00:00 2001 From: Matias Griese Date: Thu, 4 Oct 2018 13:46:10 +0300 Subject: [PATCH 2/3] Changelog update --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ebbbd3f..f31a0f48 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +# v1.8.11 +## mm/dd/2018 + +1. [](#improved) + * Change usage of basename where possible [#1480](https://github.com/getgrav/grav-plugin-admin/pull/1480) + # v1.8.10 ## 10/01/2018 From 42c8b63520b33e3025bdd0818c09049391aaae66 Mon Sep 17 00:00:00 2001 From: Matias Griese Date: Thu, 4 Oct 2018 15:42:59 +0300 Subject: [PATCH 3/3] Improved file uploads --- CHANGELOG.md | 4 +++ blueprints.yaml | 2 +- classes/adminbasecontroller.php | 30 ++++++++++++++------ classes/admincontroller.php | 50 ++++++++++++++++++++++++--------- 4 files changed, 62 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f31a0f48..87166f28 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/blueprints.yaml b/blueprints.yaml index ec0c1a6a..866fd61d 100644 --- a/blueprints.yaml +++ b/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' } diff --git a/classes/adminbasecontroller.php b/classes/adminbasecontroller.php index 2d736cdb..a8eeb2a3 100644 --- a/classes/adminbasecontroller.php +++ b/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 (!$match) { - $message = $isMime ? 'The MIME type "' . $upload->file->type . '"' : 'The File Extension'; - $errors[] = $message . ' for the file "' . $upload->file->name . '" is not an accepted.'; - $accepted |= false; + if ($isMime) { + $match = preg_match('#' . $find . '$#', $mime); + if (!$match) { + $errors[] = 'The MIME type "' . $mime . '" for the file "' . $upload->file->name . '" is not an accepted.'; + } else { + $accepted = true; + break; + } } else { - $accepted |= true; + $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; + } } } diff --git a/classes/admincontroller.php b/classes/admincontroller.php index 41785b3c..c67ac6ff 100644 --- a/classes/admincontroller.php +++ b/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']); - - $fileExt = ''; - if (isset($fileParts['extension'])) { - $fileExt = strtolower($fileParts['extension']); - } + $extension = strtolower(pathinfo($filename, PATHINFO_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']; - $filename = str_replace(['@3x', '@2x'], '', $filename); + $basename = str_replace(['@3x', '@2x'], '', pathinfo($filename, PATHINFO_BASENAME)); $metadata = []; - if ($include_metadata && isset($media[$filename])) { - $img_metadata = $media[$filename]->metadata(); + if ($include_metadata && isset($media[$basename])) { + $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; + } }