Added extension whitelist for image uploads

- A continuation of the security issues addressed in v0.25.3

diff --git a/app/Http/Controllers/ImageController.php b/app/Http/Controllers/ImageController.php
index a6c92bccc720f6869de3b579211482da714c2f94..fdcb3aba49fbd0380cc878c9953b232e26cedf61 100644 (file)
--- 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 77154baac6b2b4bdf3683c08815d998d306fe0e8..1d9a8736e6f66a1c5e4305a6a5869af08468f86a 100644 (file)
--- 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 "<?php echo icon($expression); ?>";

diff --git a/resources/lang/en/validation.php b/resources/lang/en/validation.php
index 0b6a1c170a5fb31d7d470eedf61bb7a89bd96dbb..3de4c54b001c915a76dffc06225d6809045b0498 100644 (file)
--- 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 e264568244fe7cfba72822845929c320f8b232d9..4091bec570abd1d588599d619996d41129360ce3 100644 (file)
--- 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 (file)
index 0000000..3b7c0f3
Binary files /dev/null and b/tests/test-data/bad.phtml differ