From 43830a372fc51a8793199d04a34c3f4ebdfccc7b Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 31 Oct 2021 23:53:17 +0000 Subject: [PATCH] Updated showImage file serving to not be traversable For #3030 --- .../Controllers/Images/ImageController.php | 13 +++-- app/Uploads/AttachmentService.php | 8 ++-- app/Uploads/ImageService.php | 47 ++++++++++++++++--- tests/Uploads/ImageTest.php | 30 ++++++++++++ 4 files changed, 84 insertions(+), 14 deletions(-) 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 @@ -7,25 +7,31 @@ 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; } /** @@ -35,14 +41,13 @@ class ImageController extends Controller */ 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); } /** 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 @@ -27,7 +27,7 @@ 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()); } @@ -70,7 +70,7 @@ class AttachmentService */ public function getAttachmentFromStorage(Attachment $attachment): string { - return $this->getStorage()->get($this->adjustPathForStorageDisk($attachment->path)); + return $this->getStorageDisk()->get($this->adjustPathForStorageDisk($attachment->path)); } /** @@ -195,7 +195,7 @@ class AttachmentService */ protected function deleteFileInStorage(Attachment $attachment) { - $storage = $this->getStorage(); + $storage = $this->getStorageDisk(); $dirPath = $this->adjustPathForStorageDisk(dirname($attachment->path)); $storage->delete($this->adjustPathForStorageDisk($attachment->path)); @@ -213,7 +213,7 @@ class AttachmentService { $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(); 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 @@ -16,6 +16,7 @@ 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 { @@ -39,11 +40,20 @@ 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. @@ -126,7 +136,7 @@ class ImageService */ 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); @@ -243,7 +253,7 @@ class ImageService 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); } @@ -307,7 +317,7 @@ class ImageService */ public function getImageData(Image $image): string { - $storage = $this->getStorage(); + $storage = $this->getStorageDisk(); return $storage->get($this->adjustPathForStorageDisk($image->path, $image->type)); } @@ -330,7 +340,7 @@ class ImageService 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); @@ -417,7 +427,7 @@ class ImageService } $storagePath = $this->adjustPathForStorageDisk($storagePath); - $storage = $this->getStorage(); + $storage = $this->getStorageDisk(); $imageData = null; if ($storage->exists($storagePath)) { $imageData = $storage->get($storagePath); @@ -435,6 +445,31 @@ 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". 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 @@ -241,6 +241,36 @@ class ImageTest extends TestCase } } + 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');