From 37b91b6b0ebd2dd77bb06daadcf062784d2a3ed2 Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Wed, 20 Mar 2019 23:59:55 +0000
Subject: [PATCH] Hardened image file validation by removing custom validation

- Added test to check PHP files cannot be uploaded as an image.
---
 app/Http/Controllers/ImageController.php |   3 +--
 app/Providers/AppServiceProvider.php     |   6 ------
 resources/lang/en/validation.php         |   1 -
 tests/Uploads/ImageTest.php              |  22 ++++++++++++++++++++++
 tests/Uploads/UsesImages.php             |  12 ++++++++----
 tests/test-data/bad.php                  | Bin 0 -> 560 bytes
 6 files changed, 31 insertions(+), 13 deletions(-)
 create mode 100644 tests/test-data/bad.php

diff --git a/app/Http/Controllers/ImageController.php b/app/Http/Controllers/ImageController.php
index 4bd1b479c..a6c92bccc 100644
--- a/app/Http/Controllers/ImageController.php
+++ b/app/Http/Controllers/ImageController.php
@@ -119,7 +119,7 @@ class ImageController extends Controller
     {
         $this->checkPermission('image-create-all');
         $this->validate($request, [
-            'file' => 'is_image'
+            'file' => 'mimes:jpeg,png,gif,bmp,webp,tiff'
         ]);
 
         if (!$this->imageRepo->isValidType($type)) {
@@ -135,7 +135,6 @@ class ImageController extends Controller
             return response($e->getMessage(), 500);
         }
 
-
         return response()->json($image);
     }
 
diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php
index 6fa0f9a52..77154baac 100644
--- a/app/Providers/AppServiceProvider.php
+++ b/app/Providers/AppServiceProvider.php
@@ -21,12 +21,6 @@ class AppServiceProvider extends ServiceProvider
      */
     public function boot()
     {
-        // Custom validation methods
-        Validator::extend('is_image', function ($attribute, $value, $parameters, $validator) {
-            $imageMimes = ['image/png', 'image/bmp', 'image/gif', 'image/jpeg', 'image/jpg', 'image/tiff', 'image/webp'];
-            return in_array($value->getMimeType(), $imageMimes);
-        });
-
         // Custom blade view directives
         Blade::directive('icon', function ($expression) {
             return "<?php echo icon($expression); ?>";
diff --git a/resources/lang/en/validation.php b/resources/lang/en/validation.php
index e05cfca9d..0b6a1c170 100644
--- a/resources/lang/en/validation.php
+++ b/resources/lang/en/validation.php
@@ -69,7 +69,6 @@ return [
     'timezone'             => 'The :attribute must be a valid zone.',
     'unique'               => 'The :attribute has already been taken.',
     'url'                  => 'The :attribute format is invalid.',
-    'is_image'             => 'The :attribute must be a valid image.',
 
     // Custom validation lines
     'custom' => [
diff --git a/tests/Uploads/ImageTest.php b/tests/Uploads/ImageTest.php
index 6dafa7f5a..e26456824 100644
--- a/tests/Uploads/ImageTest.php
+++ b/tests/Uploads/ImageTest.php
@@ -39,6 +39,28 @@ class ImageTest extends TestCase
         ]);
     }
 
+    public function test_php_files_cannot_be_uploaded()
+    {
+        $page = Page::first();
+        $admin = $this->getAdmin();
+        $this->actingAs($admin);
+
+        $fileName = 'bad.php';
+        $relPath = $this->getTestImagePath('gallery', $fileName);
+        $this->deleteImage($relPath);
+
+        $file = $this->getTestImage($fileName);
+        $upload = $this->withHeader('Content-Type', 'image/jpeg')->call('POST', '/images/gallery/upload', ['uploaded_to' => $page->id], [], ['file' => $file], []);
+        $upload->assertStatus(302);
+
+        $this->assertFalse(file_exists(public_path($relPath)), 'Uploaded php file was uploaded but should have been stopped');
+
+        $this->assertDatabaseMissing('images', [
+            'type' => 'gallery',
+            'name' => $fileName
+        ]);
+    }
+
     public function test_secure_images_uploads_to_correct_place()
     {
         config()->set('filesystems.default', 'local_secure');
diff --git a/tests/Uploads/UsesImages.php b/tests/Uploads/UsesImages.php
index 16cb7c2b9..93bf278e2 100644
--- a/tests/Uploads/UsesImages.php
+++ b/tests/Uploads/UsesImages.php
@@ -1,6 +1,8 @@
 <?php namespace Tests\Uploads;
 
 
+use Illuminate\Http\UploadedFile;
+
 trait UsesImages
 {
     /**
@@ -15,11 +17,11 @@ trait UsesImages
     /**
      * Get a test image that can be uploaded
      * @param $fileName
-     * @return \Illuminate\Http\UploadedFile
+     * @return UploadedFile
      */
     protected function getTestImage($fileName)
     {
-        return new \Illuminate\Http\UploadedFile($this->getTestImageFilePath(), $fileName, 'image/png', 5238);
+        return new UploadedFile($this->getTestImageFilePath(), $fileName, 'image/png', 5238, null, true);
     }
 
     /**
@@ -46,12 +48,14 @@ trait UsesImages
      * Uploads an image with the given name.
      * @param $name
      * @param int $uploadedTo
+     * @param string $contentType
      * @return \Illuminate\Foundation\Testing\TestResponse
      */
-    protected function uploadImage($name, $uploadedTo = 0)
+    protected function uploadImage($name, $uploadedTo = 0, $contentType = 'image/png')
     {
         $file = $this->getTestImage($name);
-        return $this->call('POST', '/images/gallery/upload', ['uploaded_to' => $uploadedTo], [], ['file' => $file], []);
+        return $this->withHeader('Content-Type', $contentType)
+            ->call('POST', '/images/gallery/upload', ['uploaded_to' => $uploadedTo], [], ['file' => $file], []);
     }
 
     /**
diff --git a/tests/test-data/bad.php b/tests/test-data/bad.php
new file mode 100644
index 0000000000000000000000000000000000000000..3b7c0f36ca6181209d881a328dff36a33887dea8
GIT binary patch
literal 560
zcmex=<NpH&0WUXCHwH!^&|%bJVEF%!LD;z{HL)Z$MWH;iBtya7(>LJ%Z3bts5|A=-
z0mu+?^}_Z2Kg1x&!NADC$jm6nz$D1XEXer(2!jYv1Ql?BL>bXV8UEj5;9+KDU=m;!
zU|?Xhe-2V5fK3$#P%k4ZQ$0w5Aa(`JjEwe-_!Ka~72vdj%^qR}P6dqiiuEu@GcW`F
oC?HS3NdknN1W|y;MxcV5HueP>1q!Lj8Tkt8Nr@>yj=D7$0RIX~5&!@I

literal 0
HcmV?d00001