CVE-2021-3874: 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-10-15T14:15Z
Repository: https://github.com/bookstackapp/bookstack
Language: PHP
Files changed: app/Config/filesystems.php, app/Uploads/AttachmentService.php, app/Uploads/ImageService.php
Commit date: 2021-10-08 17:47:14 +0100
Commit message: Added protections against path traversal in file system operations

- Files within the storage/ path could be accessed via path traversal
  references in content, accessed upon HTML export.
- This addresses this via two layers:
  - Scoped local flysystem filesystems down to the specific image &
    file folders since flysystem has built-in checking against the
    escaping of the root folder.
  - Added path normalization before enforcement of uploads/{images,file}
    prefix to prevent traversal at a path level.

Thanks to @Haxatron via huntr.dev for discovery and reporting.
Ref: https://huntr.dev/bounties/ac268a17-72b5-446f-a09a-9945ef58607a/

Diff: diff --git a/app/Config/filesystems.php b/app/Config/filesystems.php
index 95fc35c2a..a6b05c8c4 100644
--- a/app/Config/filesystems.php
+++ b/app/Config/filesystems.php
@@ -35,13 +35,18 @@ return [
         'local' => [
             'driver' => 'local',
             'root'   => public_path(),
         ],
 
-        'local_secure' => [
+        'local_secure_attachments' => [
             'driver' => 'local',
-            'root'   => storage_path(),
+            'root'   => storage_path('uploads/files/'),
+        ],
+
+        'local_secure_images' => [
+            'driver' => 'local',
+            'root'   => storage_path('uploads/images/'),
         ],
 
         's3' => [
             'driver'                  => 's3',
             'key'                     => env('STORAGE_S3_KEY', 'your-key'),
diff --git a/app/Uploads/AttachmentService.php b/app/Uploads/AttachmentService.php
index b4cb1b88b..c957e8bea 100644
--- a/app/Uploads/AttachmentService.php
+++ b/app/Uploads/AttachmentService.php
@@ -7,10 +7,11 @@ use Exception;
 use Illuminate\Contracts\Filesystem\Factory as FileSystem;
 use Illuminate\Contracts\Filesystem\FileNotFoundException;
 use Illuminate\Contracts\Filesystem\Filesystem as FileSystemInstance;
 use Illuminate\Support\Facades\Log;
 use Illuminate\Support\Str;
+use League\Flysystem\Util;
 use Symfony\Component\HttpFoundation\File\UploadedFile;
 
 class AttachmentService
 {
     protected $fileSystem;
@@ -25,48 +26,67 @@ class AttachmentService
 
     /**
      * Get the storage that will be used for storing files.
      */
     protected function getStorage(): FileSystemInstance
+    {
+        return $this->fileSystem->disk($this->getStorageDiskName());
+    }
+
+    /**
+     * Get the name of the storage disk to use.
+     */
+    protected function getStorageDiskName(): string
     {
         $storageType = config('filesystems.attachments');
 
-        // Override default location if set to local public to ensure not visible.
-        if ($storageType === 'local') {
-            $storageType = 'local_secure';
+        // Change to our secure-attachment disk if any of the local options
+        // are used to prevent escaping that location.
+        if ($storageType === 'local' || $storageType === 'local_secure') {
+            $storageType = 'local_secure_attachments';
+        }
+
+        return $storageType;
+    }
+
+    /**
+     * 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
+    {
+        $path = Util::normalizePath(str_replace('uploads/files/', '', $path));
+
+        if ($this->getStorageDiskName() === 'local_secure_attachments') {
+            return $path;
         }
 
-        return $this->fileSystem->disk($storageType);
+        return 'uploads/files/' . $path;
     }
 
     /**
      * Get an attachment from storage.
      *
      * @throws FileNotFoundException
      */
     public function getAttachmentFromStorage(Attachment $attachment): string
     {
-        return $this->getStorage()->get($attachment->path);
+        return $this->getStorage()->get($this->adjustPathForStorageDisk($attachment->path));
     }
 
     /**
      * Store a new attachment upon user upload.
-     *
-     * @param UploadedFile $uploadedFile
-     * @param int          $page_id
-     *
      * @throws FileUploadException
-     *
-     * @return Attachment
      */
-    public function saveNewUpload(UploadedFile $uploadedFile, $page_id)
+    public function saveNewUpload(UploadedFile $uploadedFile, int $page_id): Attachment
     {
         $attachmentName = $uploadedFile->getClientOriginalName();
         $attachmentPath = $this->putFileInStorage($uploadedFile);
-        $largestExistingOrder = Attachment::where('uploaded_to', '=', $page_id)->max('order');
+        $largestExistingOrder = Attachment::query()->where('uploaded_to', '=', $page_id)->max('order');
 
-        $attachment = Attachment::forceCreate([
+        /** @var Attachment $attachment */
+        $attachment = Attachment::query()->forceCreate([
             'name'        => $attachmentName,
             'path'        => $attachmentPath,
             'extension'   => $uploadedFile->getClientOriginalExtension(),
             'uploaded_to' => $page_id,
             'created_by'  => user()->id,
@@ -76,21 +96,16 @@ class AttachmentService
 
         return $attachment;
     }
 
     /**
-     * Store a upload, saving to a file and deleting any existing uploads
+     * Store an upload, saving to a file and deleting any existing uploads
      * attached to that file.
      *
-     * @param UploadedFile $uploadedFile
-     * @param Attachment   $attachment
-     *
      * @throws FileUploadException
-     *
-     * @return Attachment
      */
-    public function saveUpdatedUpload(UploadedFile $uploadedFile, Attachment $attachment)
+    public function saveUpdatedUpload(UploadedFile $uploadedFile, Attachment $attachment): Attachment
     {
         if (!$attachment->external) {
             $this->deleteFileInStorage($attachment);
         }
 
@@ -157,13 +172,10 @@ class AttachmentService
         return $attachment;
     }
 
     /**
      * Delete a File from the database and storage.
-     *
-     * @param Attachment $attachment
-     *
      * @throws Exception
      */
     public function deleteFile(Attachment $attachment)
     {
         if ($attachment->external) {
@@ -177,49 +189,42 @@ class AttachmentService
     }
 
     /**
      * Delete a file from the filesystem it sits on.
      * Cleans any empty leftover folders.
-     *
-     * @param Attachment $attachment
      */
     protected function deleteFileInStorage(Attachment $attachment)
     {
         $storage = $this->getStorage();
-        $dirPath = dirname($attachment->path);
+        $dirPath = $this->adjustPathForStorageDisk(dirname($attachment->path));
 
-        $storage->delete($attachment->path);
+        $storage->delete($this->adjustPathForStorageDisk($attachment->path));
         if (count($storage->allFiles($dirPath)) === 0) {
             $storage->deleteDirectory($dirPath);
         }
     }
 
     /**
      * Store a file in storage with the given filename.
-     *
-     * @param UploadedFile $uploadedFile
-     *
      * @throws FileUploadException
-     *
-     * @return string
      */
-    protected function putFileInStorage(UploadedFile $uploadedFile)
+    protected function putFileInStorage(UploadedFile $uploadedFile): string
     {
         $attachmentData = file_get_contents($uploadedFile->getRealPath());
 
         $storage = $this->getStorage();
         $basePath = 'uploads/files/' . date('Y-m-M') . '/';
 
         $uploadFileName = Str::random(16) . '.' . $uploadedFile->getClientOriginalExtension();
-        while ($storage->exists($basePath . $uploadFileName)) {
+        while ($storage->exists($this->adjustPathForStorageDisk($basePath . $uploadFileName))) {
             $uploadFileName = Str::random(3) . $uploadFileName;
         }
 
         $attachmentPath = $basePath . $uploadFileName;
 
         try {
-            $storage->put($attachmentPath, $attachmentData);
+            $storage->put($this->adjustPathForStorageDisk($attachmentPath), $attachmentData);
         } catch (Exception $e) {
             Log::error('Error when attempting file upload:' . $e->getMessage());
 
             throw new FileUploadException(trans('errors.path_not_writable', ['filePath' => $attachmentPath]));
         }
diff --git a/app/Uploads/ImageService.php b/app/Uploads/ImageService.php
index 2c38c24f4..eff8005d7 100644
--- a/app/Uploads/ImageService.php
+++ b/app/Uploads/ImageService.php
@@ -12,10 +12,11 @@ use Illuminate\Contracts\Filesystem\Filesystem as FileSystemInstance;
 use Illuminate\Contracts\Filesystem\Filesystem as Storage;
 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;
 
 class ImageService
 {
     protected $imageTool;
@@ -36,20 +37,47 @@ class ImageService
     }
 
     /**
      * Get the storage that will be used for storing images.
      */
-    protected function getStorage(string $type = ''): FileSystemInstance
+    protected function getStorage(string $imageType = ''): FileSystemInstance
+    {
+        return $this->fileSystem->disk($this->getStorageDiskName($imageType));
+    }
+
+    /**
+     * 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
+    {
+        $path = Util::normalizePath(str_replace('uploads/images/', '', $path));
+
+        if ($this->getStorageDiskName($imageType) === 'local_secure_images') {
+            return $path;
+        }
+
+        return 'uploads/images/' . $path;
+    }
+
+    /**
+     * Get the name of the storage disk to use.
+     */
+    protected function getStorageDiskName(string $imageType): string
     {
         $storageType = config('filesystems.images');
 
         // Ensure system images (App logo) are uploaded to a public space
-        if ($type === 'system' && $storageType === 'local_secure') {
+        if ($imageType === 'system' && $storageType === 'local_secure') {
             $storageType = 'local';
         }
 
-        return $this->fileSystem->disk($storageType);
+        if ($storageType === 'local_secure') {
+            $storageType = 'local_secure_images';
+        }
+
+        return $storageType;
     }
 
     /**
      * Saves a new image from an upload.
      *
@@ -102,21 +130,21 @@ class ImageService
         $secureUploads = setting('app-secure-images');
         $fileName = $this->cleanImageFileName($imageName);
 
         $imagePath = '/uploads/images/' . $type . '/' . date('Y-m') . '/';
 
-        while ($storage->exists($imagePath . $fileName)) {
+        while ($storage->exists($this->adjustPathForStorageDisk($imagePath . $fileName, $type))) {
             $fileName = Str::random(3) . $fileName;
         }
 
         $fullPath = $imagePath . $fileName;
         if ($secureUploads) {
             $fullPath = $imagePath . Str::random(16) . '-' . $fileName;
         }
 
         try {
-            $this->saveImageDataInPublicSpace($storage, $fullPath, $imageData);
+            $this->saveImageDataInPublicSpace($storage, $this->adjustPathForStorageDisk($fullPath, $type), $imageData);
         } catch (Exception $e) {
             \Log::error('Error when attempting image upload:' . $e->getMessage());
 
             throw new ImageUploadException(trans('errors.path_not_writable', ['filePath' => $fullPath]));
         }
@@ -214,17 +242,17 @@ class ImageService
         if ($this->cache->has('images-' . $image->id . '-' . $thumbFilePath) && $this->cache->get('images-' . $thumbFilePath)) {
             return $this->getPublicUrl($thumbFilePath);
         }
 
         $storage = $this->getStorage($image->type);
-        if ($storage->exists($thumbFilePath)) {
+        if ($storage->exists($this->adjustPathForStorageDisk($thumbFilePath, $image->type))) {
             return $this->getPublicUrl($thumbFilePath);
         }
 
-        $thumbData = $this->resizeImage($storage->get($imagePath), $width, $height, $keepRatio);
+        $thumbData = $this->resizeImage($storage->get($this->adjustPathForStorageDisk($imagePath, $image->type)), $width, $height, $keepRatio);
 
-        $this->saveImageDataInPublicSpace($storage, $thumbFilePath, $thumbData);
+        $this->saveImageDataInPublicSpace($storage, $this->adjustPathForStorageDisk($thumbFilePath, $image->type), $thumbData);
         $this->cache->put('images-' . $image->id . '-' . $thumbFilePath, $thumbFilePath, 60 * 60 * 72);
 
         return $this->getPublicUrl($thumbFilePath);
     }
 
@@ -277,34 +305,33 @@ class ImageService
      *
      * @throws FileNotFoundException
      */
     public function getImageData(Image $image): string
     {
-        $imagePath = $image->path;
         $storage = $this->getStorage();
-
-        return $storage->get($imagePath);
+        return $storage->get($this->adjustPathForStorageDisk($image->path, $image->type));
     }
 
     /**
      * Destroy an image along with its revisions, thumbnails and remaining folders.
      *
      * @throws Exception
      */
     public function destroy(Image $image)
     {
-        $this->destroyImagesFromPath($image->path);
+        $this->destroyImagesFromPath($image->path, $image->type);
         $image->delete();
     }
 
     /**
      * Destroys an image at the given path.
      * Searches for image thumbnails in addition to main provided path.
      */
-    protected function destroyImagesFromPath(string $path): bool
+    protected function destroyImagesFromPath(string $path, string $imageType): bool
     {
-        $storage = $this->getStorage();
+        $path = $this->adjustPathForStorageDisk($path, $imageType);
+        $storage = $this->getStorage($imageType);
 
         $imageFolder = dirname($path);
         $imageFileName = basename($path);
         $allImages = collect($storage->allFiles($imageFolder));
 
@@ -324,11 +351,11 @@ class ImageService
 
         return true;
     }
 
     /**
-     * Check whether or not a folder is empty.
+     * Check whether a folder is empty.
      */
     protected function isFolderEmpty(FileSystemInstance $storage, string $path): bool
     {
         $files = $storage->files($path);
         $folders = $storage->directories($path);
@@ -372,11 +399,11 @@ class ImageService
 
         return $deletedPaths;
     }
 
     /**
-     * Convert a image URI to a Base64 encoded string.
+     * Convert an image URI to a Base64 encoded string.
      * Attempts to convert the URL to a system storage url then
      * fetch the data from the disk or storage location.
      * Returns null if the image data cannot be fetched from storage.
      *
      * @throws FileNotFoundException
@@ -386,10 +413,11 @@ class ImageService
         $storagePath = $this->imageUrlToStoragePath($uri);
         if (empty($uri) || is_null($storagePath)) {
             return null;
         }
 
+        $storagePath = $this->adjustPathForStorageDisk($storagePath);
         $storage = $this->getStorage();
         $imageData = null;
         if ($storage->exists($storagePath)) {
             $imageData = $storage->get($storagePath);
         }

Vulnerable code:         'local_secure' => [
            'root'   => storage_path(),
        // Override default location if set to local public to ensure not visible.
        if ($storageType === 'local') {
            $storageType = 'local_secure';
        return $this->fileSystem->disk($storageType);
        return $this->getStorage()->get($attachment->path);
     *
     * @param UploadedFile $uploadedFile
     * @param int          $page_id
     *
     *
     * @return Attachment
    public function saveNewUpload(UploadedFile $uploadedFile, $page_id)
        $largestExistingOrder = Attachment::where('uploaded_to', '=', $page_id)->max('order');
        $attachment = Attachment::forceCreate([
     * Store a upload, saving to a file and deleting any existing uploads
     * @param UploadedFile $uploadedFile
     * @param Attachment   $attachment
     *
     *
     * @return Attachment
    public function saveUpdatedUpload(UploadedFile $uploadedFile, Attachment $attachment)
     *
     * @param Attachment $attachment
     *
     *
     * @param Attachment $attachment
        $dirPath = dirname($attachment->path);
        $storage->delete($attachment->path);
     *
     * @param UploadedFile $uploadedFile
     *
     *
     * @return string
    protected function putFileInStorage(UploadedFile $uploadedFile)
        while ($storage->exists($basePath . $uploadFileName)) {
            $storage->put($attachmentPath, $attachmentData);
    protected function getStorage(string $type = ''): FileSystemInstance
        if ($type === 'system' && $storageType === 'local_secure') {
        return $this->fileSystem->disk($storageType);
        while ($storage->exists($imagePath . $fileName)) {
            $this->saveImageDataInPublicSpace($storage, $fullPath, $imageData);
        if ($storage->exists($thumbFilePath)) {
        $thumbData = $this->resizeImage($storage->get($imagePath), $width, $height, $keepRatio);
        $this->saveImageDataInPublicSpace($storage, $thumbFilePath, $thumbData);
        $imagePath = $image->path;

        return $storage->get($imagePath);
        $this->destroyImagesFromPath($image->path);
    protected function destroyImagesFromPath(string $path): bool
        $storage = $this->getStorage();
     * Check whether or not a folder is empty.
     * Convert a image URI to a Base64 encoded string.
Fixed code:         'local_secure_attachments' => [
            'root'   => storage_path('uploads/files/'),
        ],

        'local_secure_images' => [
            'driver' => 'local',
            'root'   => storage_path('uploads/images/'),
use League\Flysystem\Util;
    {
        return $this->fileSystem->disk($this->getStorageDiskName());
    }

    /**
     * Get the name of the storage disk to use.
     */
    protected function getStorageDiskName(): string
        // Change to our secure-attachment disk if any of the local options
        // are used to prevent escaping that location.
        if ($storageType === 'local' || $storageType === 'local_secure') {
            $storageType = 'local_secure_attachments';
        }

        return $storageType;
    }

    /**
     * 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
    {
        $path = Util::normalizePath(str_replace('uploads/files/', '', $path));

        if ($this->getStorageDiskName() === 'local_secure_attachments') {
            return $path;
        return 'uploads/files/' . $path;
        return $this->getStorage()->get($this->adjustPathForStorageDisk($attachment->path));
    public function saveNewUpload(UploadedFile $uploadedFile, int $page_id): Attachment
        $largestExistingOrder = Attachment::query()->where('uploaded_to', '=', $page_id)->max('order');
        /** @var Attachment $attachment */
        $attachment = Attachment::query()->forceCreate([
     * Store an upload, saving to a file and deleting any existing uploads
    public function saveUpdatedUpload(UploadedFile $uploadedFile, Attachment $attachment): Attachment
        $dirPath = $this->adjustPathForStorageDisk(dirname($attachment->path));
        $storage->delete($this->adjustPathForStorageDisk($attachment->path));
    protected function putFileInStorage(UploadedFile $uploadedFile): string
        while ($storage->exists($this->adjustPathForStorageDisk($basePath . $uploadFileName))) {
            $storage->put($this->adjustPathForStorageDisk($attachmentPath), $attachmentData);
use League\Flysystem\Util;
    protected function getStorage(string $imageType = ''): FileSystemInstance
    {
        return $this->fileSystem->disk($this->getStorageDiskName($imageType));
    }

    /**
     * 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
    {
        $path = Util::normalizePath(str_replace('uploads/images/', '', $path));

        if ($this->getStorageDiskName($imageType) === 'local_secure_images') {
            return $path;
        }

        return 'uploads/images/' . $path;
    }

    /**
     * Get the name of the storage disk to use.
     */
    protected function getStorageDiskName(string $imageType): string
        if ($imageType === 'system' && $storageType === 'local_secure') {
        if ($storageType === 'local_secure') {
            $storageType = 'local_secure_images';
        }

        return $storageType;
        while ($storage->exists($this->adjustPathForStorageDisk($imagePath . $fileName, $type))) {
            $this->saveImageDataInPublicSpace($storage, $this->adjustPathForStorageDisk($fullPath, $type), $imageData);
        if ($storage->exists($this->adjustPathForStorageDisk($thumbFilePath, $image->type))) {
        $thumbData = $this->resizeImage($storage->get($this->adjustPathForStorageDisk($imagePath, $image->type)), $width, $height, $keepRatio);
        $this->saveImageDataInPublicSpace($storage, $this->adjustPathForStorageDisk($thumbFilePath, $image->type), $thumbData);
        return $storage->get($this->adjustPathForStorageDisk($image->path, $image->type));
        $this->destroyImagesFromPath($image->path, $image->type);
    protected function destroyImagesFromPath(string $path, string $imageType): bool
        $path = $this->adjustPathForStorageDisk($path, $imageType);
        $storage = $this->getStorage($imageType);
     * Check whether a folder is empty.
     * Convert an image URI to a Base64 encoded string.
        $storagePath = $this->adjustPathForStorageDisk($storagePath);