From f5fe524e6c466e483e424cb70fe1c461e7a4c2b5 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Thu, 21 Mar 2019 19:43:15 +0000 Subject: [PATCH] Added extension whitelist for image uploads - A continuation of the security issues addressed in v0.25.3 --- app/Http/Controllers/ImageController.php | 2 +- app/Providers/AppServiceProvider.php | 8 ++++++ resources/lang/en/validation.php | 1 + tests/Uploads/ImageTest.php | 34 +++++++++++++++++++---- tests/test-data/bad.phtml | Bin 0 -> 560 bytes 5 files changed, 38 insertions(+), 7 deletions(-) create mode 100644 tests/test-data/bad.phtml diff --git a/app/Http/Controllers/ImageController.php b/app/Http/Controllers/ImageController.php index a6c92bccc..fdcb3aba4 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' => 'mimes:jpeg,png,gif,bmp,webp,tiff' + 'file' => 'image_extension|mimes:jpeg,png,gif,bmp,webp,tiff' ]); if (!$this->imageRepo->isValidType($type)) { diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 77154baac..1d9a8736e 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -8,6 +8,7 @@ use BookStack\Entities\Page; use BookStack\Settings\Setting; use BookStack\Settings\SettingService; use Illuminate\Database\Eloquent\Relations\Relation; +use Illuminate\Http\UploadedFile; use Illuminate\Support\ServiceProvider; use Schema; use Validator; @@ -21,6 +22,13 @@ class AppServiceProvider extends ServiceProvider */ public function boot() { + // Custom validation methods + Validator::extend('image_extension', function ($attribute, $value, $parameters, $validator) { + $validImageExtensions = ['png', 'jpg', 'jpeg', 'bmp', 'gif', 'tiff', 'webp']; + return in_array(strtolower($value->getClientOriginalExtension()), $validImageExtensions); + }); + + // Custom blade view directives Blade::directive('icon', function ($expression) { return ""; diff --git a/resources/lang/en/validation.php b/resources/lang/en/validation.php index 0b6a1c170..3de4c54b0 100644 --- a/resources/lang/en/validation.php +++ b/resources/lang/en/validation.php @@ -33,6 +33,7 @@ return [ 'filled' => 'The :attribute field is required.', 'exists' => 'The selected :attribute is invalid.', 'image' => 'The :attribute must be an image.', + 'image_extension' => 'The :attribute must have a valid & supported image extension.', 'in' => 'The selected :attribute is invalid.', 'integer' => 'The :attribute must be an integer.', 'ip' => 'The :attribute must be a valid IP address.', diff --git a/tests/Uploads/ImageTest.php b/tests/Uploads/ImageTest.php index e26456824..4091bec57 100644 --- a/tests/Uploads/ImageTest.php +++ b/tests/Uploads/ImageTest.php @@ -61,13 +61,35 @@ class ImageTest extends TestCase ]); } + public function test_php_like_files_cannot_be_uploaded() + { + $page = Page::first(); + $admin = $this->getAdmin(); + $this->actingAs($admin); + + $fileName = 'bad.phtml'; + $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'); $this->asEditor(); - $galleryFile = $this->getTestImage('my-secure-test-upload'); + $galleryFile = $this->getTestImage('my-secure-test-upload.png'); $page = Page::first(); - $expectedPath = storage_path('uploads/images/gallery/' . Date('Y-m-M') . '/my-secure-test-upload'); + $expectedPath = storage_path('uploads/images/gallery/' . Date('Y-m-M') . '/my-secure-test-upload.png'); $upload = $this->call('POST', '/images/gallery/upload', ['uploaded_to' => $page->id], [], ['file' => $galleryFile], []); $upload->assertStatus(200); @@ -83,9 +105,9 @@ class ImageTest extends TestCase { config()->set('filesystems.default', 'local_secure'); $this->asEditor(); - $galleryFile = $this->getTestImage('my-secure-test-upload'); + $galleryFile = $this->getTestImage('my-secure-test-upload.png'); $page = Page::first(); - $expectedPath = storage_path('uploads/images/gallery/' . Date('Y-m-M') . '/my-secure-test-upload'); + $expectedPath = storage_path('uploads/images/gallery/' . Date('Y-m-M') . '/my-secure-test-upload.png'); $upload = $this->call('POST', '/images/gallery/upload', ['uploaded_to' => $page->id], [], ['file' => $galleryFile], []); $imageUrl = json_decode($upload->getContent(), true)['url']; @@ -106,9 +128,9 @@ class ImageTest extends TestCase { config()->set('filesystems.default', 'local_secure'); $this->asEditor(); - $galleryFile = $this->getTestImage('my-system-test-upload'); + $galleryFile = $this->getTestImage('my-system-test-upload.png'); $page = Page::first(); - $expectedPath = public_path('uploads/images/system/' . Date('Y-m-M') . '/my-system-test-upload'); + $expectedPath = public_path('uploads/images/system/' . Date('Y-m-M') . '/my-system-test-upload.png'); $upload = $this->call('POST', '/images/system/upload', ['uploaded_to' => $page->id], [], ['file' => $galleryFile], []); $upload->assertStatus(200); diff --git a/tests/test-data/bad.phtml b/tests/test-data/bad.phtml new file mode 100644 index 0000000000000000000000000000000000000000..3b7c0f36ca6181209d881a328dff36a33887dea8 GIT binary patch literal 560 zcmex=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