CVE-2021-3916: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') (CWE-22)
value: bookstack is vulnerable to Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')
CWE description: The product uses external input to construct a pathname that is intended to identify a file or directory that is located underneath a restricted parent directory, but the product does not properly neutralize special elements within the pathname that can cause the pathname to resolve to a location that is outside of the restricted directory.
CVSS v3 base score: 6.5
Published: 2021-11-05T15:15Z
Repository: https://github.com/bookstackapp/bookstack
Language: PHP
Files changed: app/Http/Controllers/Images/ImageController.php, app/Uploads/AttachmentService.php, app/Uploads/ImageService.php, tests/Uploads/ImageTest.php
Commit date: 2021-10-31 23:53:17 +0000
Commit message: Updated showImage file serving to not be traversable

For #3030

Diff: diff --git a/app/Http/Controllers/Images/ImageController.php b/app/Http/Controllers/Images/ImageController.php
index 4070a0e2f..66ccadc5e 100644
--- a/app/Http/Controllers/Images/ImageController.php
+++ b/app/Http/Controllers/Images/ImageController.php
@@ -5,46 +5,51 @@ namespace BookStack\Http\Controllers\Images;
 use BookStack\Exceptions\ImageUploadException;
 use BookStack\Exceptions\NotFoundException;
 use BookStack\Http\Controllers\Controller;
 use BookStack\Uploads\Image;
 use BookStack\Uploads\ImageRepo;
+use BookStack\Uploads\ImageService;
 use Exception;
 use Illuminate\Filesystem\Filesystem as File;
+use Illuminate\Filesystem\FilesystemAdapter;
 use Illuminate\Http\Request;
+use Illuminate\Support\Facades\Storage;
 use Illuminate\Validation\ValidationException;
+use League\Flysystem\Util;
 
 class ImageController extends Controller
 {
     protected $image;
     protected $file;
     protected $imageRepo;
+    protected $imageService;
 
     /**
      * ImageController constructor.
      */
-    public function __construct(Image $image, File $file, ImageRepo $imageRepo)
+    public function __construct(Image $image, File $file, ImageRepo $imageRepo, ImageService $imageService)
     {
         $this->image = $image;
         $this->file = $file;
         $this->imageRepo = $imageRepo;
+        $this->imageService = $imageService;
     }
 
     /**
      * Provide an image file from storage.
      *
      * @throws NotFoundException
      */
     public function showImage(string $path)
     {
-        $path = storage_path('uploads/images/' . $path);
-        if (!file_exists($path)) {
+        if (!$this->imageService->pathExistsInLocalSecure($path)) {
             throw (new NotFoundException(trans('errors.image_not_found')))
                 ->setSubtitle(trans('errors.image_not_found_subtitle'))
                 ->setDetails(trans('errors.image_not_found_details'));
         }
 
-        return response()->file($path);
+        return $this->imageService->streamImageFromStorageResponse('gallery', $path);
     }
 
     /**
      * Update image details.
      *
diff --git a/app/Uploads/AttachmentService.php b/app/Uploads/AttachmentService.php
index f7a0918c6..c9cd99b38 100644
--- a/app/Uploads/AttachmentService.php
+++ b/app/Uploads/AttachmentService.php
@@ -25,11 +25,11 @@ class AttachmentService
     }
 
     /**
      * Get the storage that will be used for storing files.
      */
-    protected function getStorage(): FileSystemInstance
+    protected function getStorageDisk(): FileSystemInstance
     {
         return $this->fileSystem->disk($this->getStorageDiskName());
     }
 
     /**
@@ -68,11 +68,11 @@ class AttachmentService
      *
      * @throws FileNotFoundException
      */
     public function getAttachmentFromStorage(Attachment $attachment): string
     {
-        return $this->getStorage()->get($this->adjustPathForStorageDisk($attachment->path));
+        return $this->getStorageDisk()->get($this->adjustPathForStorageDisk($attachment->path));
     }
 
     /**
      * Store a new attachment upon user upload.
      *
@@ -193,11 +193,11 @@ class AttachmentService
      * Delete a file from the filesystem it sits on.
      * Cleans any empty leftover folders.
      */
     protected function deleteFileInStorage(Attachment $attachment)
     {
-        $storage = $this->getStorage();
+        $storage = $this->getStorageDisk();
         $dirPath = $this->adjustPathForStorageDisk(dirname($attachment->path));
 
         $storage->delete($this->adjustPathForStorageDisk($attachment->path));
         if (count($storage->allFiles($dirPath)) === 0) {
             $storage->deleteDirectory($dirPath);
@@ -211,11 +211,11 @@ class AttachmentService
      */
     protected function putFileInStorage(UploadedFile $uploadedFile): string
     {
         $attachmentData = file_get_contents($uploadedFile->getRealPath());
 
-        $storage = $this->getStorage();
+        $storage = $this->getStorageDisk();
         $basePath = 'uploads/files/' . date('Y-m-M') . '/';
 
         $uploadFileName = Str::random(16) . '.' . $uploadedFile->getClientOriginalExtension();
         while ($storage->exists($this->adjustPathForStorageDisk($basePath . $uploadFileName))) {
             $uploadFileName = Str::random(3) . $uploadFileName;
diff --git a/app/Uploads/ImageService.php b/app/Uploads/ImageService.php
index d6c74c751..dc18783c5 100644
--- a/app/Uploads/ImageService.php
+++ b/app/Uploads/ImageService.php
@@ -14,10 +14,11 @@ use Illuminate\Support\Facades\DB;
 use Illuminate\Support\Str;
 use Intervention\Image\Exception\NotSupportedException;
 use Intervention\Image\ImageManager;
 use League\Flysystem\Util;
 use Symfony\Component\HttpFoundation\File\UploadedFile;
+use Symfony\Component\HttpFoundation\StreamedResponse;
 
 class ImageService
 {
     protected $imageTool;
     protected $cache;
@@ -37,15 +38,24 @@ class ImageService
     }
 
     /**
      * Get the storage that will be used for storing images.
      */
-    protected function getStorage(string $imageType = ''): FileSystemInstance
+    protected function getStorageDisk(string $imageType = ''): FileSystemInstance
     {
         return $this->fileSystem->disk($this->getStorageDiskName($imageType));
     }
 
+    /**
+     * Check if local secure image storage (Fetched behind authentication)
+     * is currently active in the instance.
+     */
+    protected function usingSecureImages(): bool
+    {
+        return $this->getStorageDiskName('gallery') === 'local_secure_images';
+    }
+
     /**
      * Change the originally provided path to fit any disk-specific requirements.
      * This also ensures the path is kept to the expected root folders.
      */
     protected function adjustPathForStorageDisk(string $path, string $imageType = ''): string
@@ -124,11 +134,11 @@ class ImageService
      *
      * @throws ImageUploadException
      */
     public function saveNew(string $imageName, string $imageData, string $type, int $uploadedTo = 0): Image
     {
-        $storage = $this->getStorage($type);
+        $storage = $this->getStorageDisk($type);
         $secureUploads = setting('app-secure-images');
         $fileName = $this->cleanImageFileName($imageName);
 
         $imagePath = '/uploads/images/' . $type . '/' . date('Y-m') . '/';
 
@@ -241,11 +251,11 @@ class ImageService
 
         if ($this->cache->has('images-' . $image->id . '-' . $thumbFilePath) && $this->cache->get('images-' . $thumbFilePath)) {
             return $this->getPublicUrl($thumbFilePath);
         }
 
-        $storage = $this->getStorage($image->type);
+        $storage = $this->getStorageDisk($image->type);
         if ($storage->exists($this->adjustPathForStorageDisk($thumbFilePath, $image->type))) {
             return $this->getPublicUrl($thumbFilePath);
         }
 
         $thumbData = $this->resizeImage($storage->get($this->adjustPathForStorageDisk($imagePath, $image->type)), $width, $height, $keepRatio);
@@ -305,11 +315,11 @@ class ImageService
      *
      * @throws FileNotFoundException
      */
     public function getImageData(Image $image): string
     {
-        $storage = $this->getStorage();
+        $storage = $this->getStorageDisk();
 
         return $storage->get($this->adjustPathForStorageDisk($image->path, $image->type));
     }
 
     /**
@@ -328,11 +338,11 @@ class ImageService
      * Searches for image thumbnails in addition to main provided path.
      */
     protected function destroyImagesFromPath(string $path, string $imageType): bool
     {
         $path = $this->adjustPathForStorageDisk($path, $imageType);
-        $storage = $this->getStorage($imageType);
+        $storage = $this->getStorageDisk($imageType);
 
         $imageFolder = dirname($path);
         $imageFileName = basename($path);
         $allImages = collect($storage->allFiles($imageFolder));
 
@@ -415,11 +425,11 @@ class ImageService
         if (empty($uri) || is_null($storagePath)) {
             return null;
         }
 
         $storagePath = $this->adjustPathForStorageDisk($storagePath);
-        $storage = $this->getStorage();
+        $storage = $this->getStorageDisk();
         $imageData = null;
         if ($storage->exists($storagePath)) {
             $imageData = $storage->get($storagePath);
         }
 
@@ -433,10 +443,35 @@ class ImageService
         }
 
         return 'data:image/' . $extension . ';base64,' . base64_encode($imageData);
     }
 
+    /**
+     * Check if the given path exists in the local secure image system.
+     * Returns false if local_secure is not in use.
+     */
+    public function pathExistsInLocalSecure(string $imagePath): bool
+    {
+        $disk = $this->getStorageDisk('gallery');
+
+        // Check local_secure is active
+        return $this->usingSecureImages()
+            // Check the image file exists
+            && $disk->exists($imagePath)
+            // Check the file is likely an image file
+            && strpos($disk->getMimetype($imagePath), 'image/') === 0;
+    }
+
+    /**
+     * For the given path, if existing, provide a response that will stream the image contents.
+     */
+    public function streamImageFromStorageResponse(string $imageType, string $path): StreamedResponse
+    {
+        $disk = $this->getStorageDisk($imageType);
+        return $disk->response($path);
+    }
+
     /**
      * Get a storage path for the given image URL.
      * Ensures the path will start with "uploads/images".
      * Returns null if the url cannot be resolved to a local URL.
      */
diff --git a/tests/Uploads/ImageTest.php b/tests/Uploads/ImageTest.php
index 69b6dc90e..296e4d187 100644
--- a/tests/Uploads/ImageTest.php
+++ b/tests/Uploads/ImageTest.php
@@ -239,10 +239,40 @@ class ImageTest extends TestCase
         if (file_exists($expectedPath)) {
             unlink($expectedPath);
         }
     }
 
+    public function test_secure_image_paths_traversal_causes_500()
+    {
+        config()->set('filesystems.images', 'local_secure');
+        $this->asEditor();
+
+        $resp = $this->get('/uploads/images/../../logs/laravel.log');
+        $resp->assertStatus(500);
+    }
+
+    public function test_secure_image_paths_traversal_on_non_secure_images_causes_404()
+    {
+        config()->set('filesystems.images', 'local');
+        $this->asEditor();
+
+        $resp = $this->get('/uploads/images/../../logs/laravel.log');
+        $resp->assertStatus(404);
+    }
+
+    public function test_secure_image_paths_dont_serve_non_images()
+    {
+        config()->set('filesystems.images', 'local_secure');
+        $this->asEditor();
+
+        $testFilePath = storage_path('/uploads/images/testing.txt');
+        file_put_contents($testFilePath, 'hello from test_secure_image_paths_dont_serve_non_images');
+
+        $resp = $this->get('/uploads/images/testing.txt');
+        $resp->assertStatus(404);
+    }
+
     public function test_secure_images_included_in_exports()
     {
         config()->set('filesystems.images', 'local_secure');
         $this->asEditor();
         $galleryFile = $this->getTestImage('my-secure-test-upload.png');

Vulnerable code:     public function __construct(Image $image, File $file, ImageRepo $imageRepo)
        $path = storage_path('uploads/images/' . $path);
        if (!file_exists($path)) {
        return response()->file($path);
    protected function getStorage(): FileSystemInstance
        return $this->getStorage()->get($this->adjustPathForStorageDisk($attachment->path));
        $storage = $this->getStorage();
        $storage = $this->getStorage();
    protected function getStorage(string $imageType = ''): FileSystemInstance
        $storage = $this->getStorage($type);
        $storage = $this->getStorage($image->type);
        $storage = $this->getStorage();
        $storage = $this->getStorage($imageType);
        $storage = $this->getStorage();
Fixed code: use BookStack\Uploads\ImageService;
use Illuminate\Filesystem\FilesystemAdapter;
use Illuminate\Support\Facades\Storage;
use League\Flysystem\Util;
    protected $imageService;
    public function __construct(Image $image, File $file, ImageRepo $imageRepo, ImageService $imageService)
        $this->imageService = $imageService;
        if (!$this->imageService->pathExistsInLocalSecure($path)) {
        return $this->imageService->streamImageFromStorageResponse('gallery', $path);
    protected function getStorageDisk(): FileSystemInstance
        return $this->getStorageDisk()->get($this->adjustPathForStorageDisk($attachment->path));
        $storage = $this->getStorageDisk();
        $storage = $this->getStorageDisk();
use Symfony\Component\HttpFoundation\StreamedResponse;
    protected function getStorageDisk(string $imageType = ''): FileSystemInstance
    /**
     * Check if local secure image storage (Fetched behind authentication)
     * is currently active in the instance.
     */
    protected function usingSecureImages(): bool
    {
        return $this->getStorageDiskName('gallery') === 'local_secure_images';
    }

        $storage = $this->getStorageDisk($type);
        $storage = $this->getStorageDisk($image->type);
        $storage = $this->getStorageDisk();
        $storage = $this->getStorageDisk($imageType);
        $storage = $this->getStorageDisk();
    /**
     * Check if the given path exists in the local secure image system.
     * Returns false if local_secure is not in use.
     */
    public function pathExistsInLocalSecure(string $imagePath): bool
    {
        $disk = $this->getStorageDisk('gallery');

        // Check local_secure is active
        return $this->usingSecureImages()
            // Check the image file exists
            && $disk->exists($imagePath)
            // Check the file is likely an image file
            && strpos($disk->getMimetype($imagePath), 'image/') === 0;
    }

    /**
     * For the given path, if existing, provide a response that will stream the image contents.
     */
    public function streamImageFromStorageResponse(string $imageType, string $path): StreamedResponse
    {
        $disk = $this->getStorageDisk($imageType);
        return $disk->response($path);
    }

    public function test_secure_image_paths_traversal_causes_500()
    {
        config()->set('filesystems.images', 'local_secure');
        $this->asEditor();

        $resp = $this->get('/uploads/images/../../logs/laravel.log');
        $resp->assertStatus(500);
    }

    public function test_secure_image_paths_traversal_on_non_secure_images_causes_404()
    {
        config()->set('filesystems.images', 'local');
        $this->asEditor();

        $resp = $this->get('/uploads/images/../../logs/laravel.log');
        $resp->assertStatus(404);
    }

    public function test_secure_image_paths_dont_serve_non_images()
    {
        config()->set('filesystems.images', 'local_secure');
        $this->asEditor();

        $testFilePath = storage_path('/uploads/images/testing.txt');
        file_put_contents($testFilePath, 'hello from test_secure_image_paths_dont_serve_non_images');

        $resp = $this->get('/uploads/images/testing.txt');
        $resp->assertStatus(404);
    }