From 2fb873f7efeb0279aea0241c09ec51ddcdd2f119 Mon Sep 17 00:00:00 2001
From: Dan Brown <email@danb.me>
Date: Sun, 19 Nov 2023 15:57:19 +0000
Subject: [PATCH 1/3] Favicon: Moved resizing to specific resizer class
---
app/Uploads/FaviconHandler.php | 15 ++++++---------
app/Uploads/ImageResizer.php | 12 +++++++++---
2 files changed, 15 insertions(+), 12 deletions(-)
diff --git a/app/Uploads/FaviconHandler.php b/app/Uploads/FaviconHandler.php
index c637356e0..d5044943c 100644
--- a/app/Uploads/FaviconHandler.php
+++ b/app/Uploads/FaviconHandler.php
@@ -3,14 +3,13 @@
namespace BookStack\Uploads;
use Illuminate\Http\UploadedFile;
-use Intervention\Image\ImageManager;
class FaviconHandler
{
protected string $path;
public function __construct(
- protected ImageManager $imageTool
+ protected ImageResizer $imageResizer,
) {
$this->path = public_path('favicon.ico');
}
@@ -25,10 +24,8 @@ class FaviconHandler
}
$imageData = file_get_contents($file->getRealPath());
- $image = $this->imageTool->make($imageData);
- $image->resize(32, 32);
- $bmpData = $image->encode('png');
- $icoData = $this->pngToIco($bmpData, 32, 32);
+ $pngData = $this->imageResizer->resizeImageData($imageData, 32, 32, false, 'png');
+ $icoData = $this->pngToIco($pngData, 32, 32);
file_put_contents($this->path, $icoData);
}
@@ -81,7 +78,7 @@ class FaviconHandler
* Built following the file format info from Wikipedia:
* https://en.wikipedia.org/wiki/ICO_(file_format)
*/
- protected function pngToIco(string $bmpData, int $width, int $height): string
+ protected function pngToIco(string $pngData, int $width, int $height): string
{
// ICO header
$header = pack('v', 0x00); // Reserved. Must always be 0
@@ -100,11 +97,11 @@ class FaviconHandler
// via intervention from png typically provides this as 24.
$entry .= pack('v', 0x00);
// Size of the image data in bytes
- $entry .= pack('V', strlen($bmpData));
+ $entry .= pack('V', strlen($pngData));
// Offset of the bmp data from file start
$entry .= pack('V', strlen($header) + strlen($entry) + 4);
// Join & return the combined parts of the ICO image data
- return $header . $entry . $bmpData;
+ return $header . $entry . $pngData;
}
}
diff --git a/app/Uploads/ImageResizer.php b/app/Uploads/ImageResizer.php
index 0d090a94b..e229bb5a0 100644
--- a/app/Uploads/ImageResizer.php
+++ b/app/Uploads/ImageResizer.php
@@ -105,11 +105,17 @@ class ImageResizer
/**
* Resize the image of given data to the specified size, and return the new image data.
+ * Format will remain the same as the input format, unless specified.
*
* @throws ImageUploadException
*/
- public function resizeImageData(string $imageData, ?int $width, ?int $height, bool $keepRatio): string
- {
+ public function resizeImageData(
+ string $imageData,
+ ?int $width,
+ ?int $height,
+ bool $keepRatio,
+ ?string $format = null,
+ ): string {
try {
$thumb = $this->intervention->make($imageData);
} catch (Exception $e) {
@@ -127,7 +133,7 @@ class ImageResizer
$thumb->fit($width, $height);
}
- $thumbData = (string) $thumb->encode();
+ $thumbData = (string) $thumb->encode($format);
// Use original image data if we're keeping the ratio
// and the resizing does not save any space.
From 9b1f82059659d0af745fab239f6b306f436d1e99 Mon Sep 17 00:00:00 2001
From: Dan Brown <email@danb.me>
Date: Sun, 19 Nov 2023 16:34:29 +0000
Subject: [PATCH 2/3] Images: Forced intervention loading via specific method
Updated image loading for intervention library to be via a specific
'initFromBinary' method to avoid being overly accepting of input types
and mechansisms.
For CVE-2023-6199
---
app/Config/app.php | 4 ----
app/Uploads/ImageResizer.php | 16 +++++++++++++---
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/app/Config/app.php b/app/Config/app.php
index dcd3ffc31..fc913eb8f 100644
--- a/app/Config/app.php
+++ b/app/Config/app.php
@@ -141,7 +141,6 @@ return [
// Third party service providers
Barryvdh\DomPDF\ServiceProvider::class,
Barryvdh\Snappy\ServiceProvider::class,
- Intervention\Image\ImageServiceProvider::class,
SocialiteProviders\Manager\ServiceProvider::class,
// BookStack custom service providers
@@ -161,9 +160,6 @@ return [
// Laravel Packages
'Socialite' => Laravel\Socialite\Facades\Socialite::class,
- // Third Party
- 'ImageTool' => Intervention\Image\Facades\Image::class,
-
// Custom BookStack
'Activity' => BookStack\Facades\Activity::class,
'Theme' => BookStack\Facades\Theme::class,
diff --git a/app/Uploads/ImageResizer.php b/app/Uploads/ImageResizer.php
index e229bb5a0..4dc1b0b99 100644
--- a/app/Uploads/ImageResizer.php
+++ b/app/Uploads/ImageResizer.php
@@ -6,15 +6,14 @@ use BookStack\Exceptions\ImageUploadException;
use Exception;
use GuzzleHttp\Psr7\Utils;
use Illuminate\Support\Facades\Cache;
+use Intervention\Image\Gd\Driver;
use Intervention\Image\Image as InterventionImage;
-use Intervention\Image\ImageManager;
class ImageResizer
{
protected const THUMBNAIL_CACHE_TIME = 604_800; // 1 week
public function __construct(
- protected ImageManager $intervention,
protected ImageStorage $storage,
) {
}
@@ -117,7 +116,7 @@ class ImageResizer
?string $format = null,
): string {
try {
- $thumb = $this->intervention->make($imageData);
+ $thumb = $this->interventionFromImageData($imageData);
} catch (Exception $e) {
throw new ImageUploadException(trans('errors.cannot_create_thumbs'));
}
@@ -144,6 +143,17 @@ class ImageResizer
return $thumbData;
}
+ /**
+ * Create an intervention image instance from the given image data.
+ * Performs some manual library usage to ensure image is specifically loaded
+ * from given binary data instead of data being misinterpreted.
+ */
+ protected function interventionFromImageData(string $imageData): InterventionImage
+ {
+ $driver = new Driver();
+ return $driver->decoder->initFromBinary($imageData);
+ }
+
/**
* Orientate the given intervention image based upon the given original image data.
* Intervention does have an `orientate` method but the exif data it needs is lost before it
From 15d7161428832d2ebf12061f69fff780cdb10550 Mon Sep 17 00:00:00 2001
From: Dan Brown <email@danb.me>
Date: Mon, 20 Nov 2023 13:32:31 +0000
Subject: [PATCH 3/3] Images: Prevented base64 extraction without permission
Also added content sniffing as an extra check.
Added tests to cover.
---
app/Entities/Repos/PageRepo.php | 10 ++---
app/Entities/Tools/PageContent.php | 32 +++++++++++-----
app/Users/Models/User.php | 4 --
tests/Entity/PageContentTest.php | 61 +++++++++++++++++++++++++++++-
tests/ThemeTest.php | 2 +-
5 files changed, 88 insertions(+), 21 deletions(-)
diff --git a/app/Entities/Repos/PageRepo.php b/app/Entities/Repos/PageRepo.php
index 61a1db63e..dbd4a47d2 100644
--- a/app/Entities/Repos/PageRepo.php
+++ b/app/Entities/Repos/PageRepo.php
@@ -211,13 +211,13 @@ class PageRepo
$inputEmpty = empty($input['markdown']) && empty($input['html']);
if ($haveInput && $inputEmpty) {
- $pageContent->setNewHTML('');
+ $pageContent->setNewHTML('', user());
} elseif (!empty($input['markdown']) && is_string($input['markdown'])) {
$newEditor = 'markdown';
- $pageContent->setNewMarkdown($input['markdown']);
+ $pageContent->setNewMarkdown($input['markdown'], user());
} elseif (isset($input['html'])) {
$newEditor = 'wysiwyg';
- $pageContent->setNewHTML($input['html']);
+ $pageContent->setNewHTML($input['html'], user());
}
if ($newEditor !== $currentEditor && userCan('editor-change')) {
@@ -284,9 +284,9 @@ class PageRepo
$content = new PageContent($page);
if (!empty($revision->markdown)) {
- $content->setNewMarkdown($revision->markdown);
+ $content->setNewMarkdown($revision->markdown, user());
} else {
- $content->setNewHTML($revision->html);
+ $content->setNewHTML($revision->html, user());
}
$page->updated_by = user()->id;
diff --git a/app/Entities/Tools/PageContent.php b/app/Entities/Tools/PageContent.php
index 949816eff..1bde7b6ce 100644
--- a/app/Entities/Tools/PageContent.php
+++ b/app/Entities/Tools/PageContent.php
@@ -9,7 +9,9 @@ use BookStack\Facades\Theme;
use BookStack\Theming\ThemeEvents;
use BookStack\Uploads\ImageRepo;
use BookStack\Uploads\ImageService;
+use BookStack\Users\Models\User;
use BookStack\Util\HtmlContentFilter;
+use BookStack\Util\WebSafeMimeSniffer;
use DOMDocument;
use DOMElement;
use DOMNode;
@@ -27,9 +29,9 @@ class PageContent
/**
* Update the content of the page with new provided HTML.
*/
- public function setNewHTML(string $html): void
+ public function setNewHTML(string $html, User $updater): void
{
- $html = $this->extractBase64ImagesFromHtml($html);
+ $html = $this->extractBase64ImagesFromHtml($html, $updater);
$this->page->html = $this->formatHtml($html);
$this->page->text = $this->toPlainText();
$this->page->markdown = '';
@@ -38,9 +40,9 @@ class PageContent
/**
* Update the content of the page with new provided Markdown content.
*/
- public function setNewMarkdown(string $markdown): void
+ public function setNewMarkdown(string $markdown, User $updater): void
{
- $markdown = $this->extractBase64ImagesFromMarkdown($markdown);
+ $markdown = $this->extractBase64ImagesFromMarkdown($markdown, $updater);
$this->page->markdown = $markdown;
$html = (new MarkdownToHtml($markdown))->convert();
$this->page->html = $this->formatHtml($html);
@@ -50,7 +52,7 @@ class PageContent
/**
* Convert all base64 image data to saved images.
*/
- protected function extractBase64ImagesFromHtml(string $htmlText): string
+ protected function extractBase64ImagesFromHtml(string $htmlText, User $updater): string
{
if (empty($htmlText) || !str_contains($htmlText, 'data:image')) {
return $htmlText;
@@ -66,7 +68,7 @@ class PageContent
$imageNodes = $xPath->query('//img[contains(@src, \'data:image\')]');
foreach ($imageNodes as $imageNode) {
$imageSrc = $imageNode->getAttribute('src');
- $newUrl = $this->base64ImageUriToUploadedImageUrl($imageSrc);
+ $newUrl = $this->base64ImageUriToUploadedImageUrl($imageSrc, $updater);
$imageNode->setAttribute('src', $newUrl);
}
@@ -86,7 +88,7 @@ class PageContent
* Attempting to capture the whole data uri using regex can cause PHP
* PCRE limits to be hit with larger, multi-MB, files.
*/
- protected function extractBase64ImagesFromMarkdown(string $markdown): string
+ protected function extractBase64ImagesFromMarkdown(string $markdown, User $updater): string
{
$matches = [];
$contentLength = strlen($markdown);
@@ -104,7 +106,7 @@ class PageContent
$dataUri .= $char;
}
- $newUrl = $this->base64ImageUriToUploadedImageUrl($dataUri);
+ $newUrl = $this->base64ImageUriToUploadedImageUrl($dataUri, $updater);
$replacements[] = [$dataUri, $newUrl];
}
@@ -119,16 +121,28 @@ class PageContent
* Parse the given base64 image URI and return the URL to the created image instance.
* Returns an empty string if the parsed URI is invalid or causes an error upon upload.
*/
- protected function base64ImageUriToUploadedImageUrl(string $uri): string
+ protected function base64ImageUriToUploadedImageUrl(string $uri, User $updater): string
{
$imageRepo = app()->make(ImageRepo::class);
$imageInfo = $this->parseBase64ImageUri($uri);
+ // Validate user has permission to create images
+ if (!$updater->can('image-create-all')) {
+ return '';
+ }
+
// Validate extension and content
if (empty($imageInfo['data']) || !ImageService::isExtensionSupported($imageInfo['extension'])) {
return '';
}
+ // Validate content looks like an image via sniffing mime type
+ $mimeSniffer = new WebSafeMimeSniffer();
+ $mime = $mimeSniffer->sniff($imageInfo['data']);
+ if (!str_starts_with($mime, 'image/')) {
+ return '';
+ }
+
// Validate that the content is not over our upload limit
$uploadLimitBytes = (config('app.upload_limit') * 1000000);
if (strlen($imageInfo['data']) > $uploadLimitBytes) {
diff --git a/app/Users/Models/User.php b/app/Users/Models/User.php
index 5bd308ae8..3797e7cb0 100644
--- a/app/Users/Models/User.php
+++ b/app/Users/Models/User.php
@@ -160,10 +160,6 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon
*/
public function can(string $permissionName): bool
{
- if ($this->email === 'guest') {
- return false;
- }
-
return $this->permissions()->contains($permissionName);
}
diff --git a/tests/Entity/PageContentTest.php b/tests/Entity/PageContentTest.php
index d8845fe12..78b739512 100644
--- a/tests/Entity/PageContentTest.php
+++ b/tests/Entity/PageContentTest.php
@@ -8,7 +8,7 @@ use Tests\TestCase;
class PageContentTest extends TestCase
{
- protected $base64Jpeg = '/9j/2wBDAAMCAgICAgMCAgIDAwMDBAYEBAQEBAgGBgUGCQgKCgkICQkKDA8MCgsOCwkJDRENDg8QEBEQCgwSExIQEw8QEBD/yQALCAABAAEBAREA/8wABgAQEAX/2gAIAQEAAD8A0s8g/9k=';
+ protected string $base64Jpeg = '/9j/2wBDAAMCAgICAgMCAgIDAwMDBAYEBAQEBAgGBgUGCQgKCgkICQkKDA8MCgsOCwkJDRENDg8QEBEQCgwSExIQEw8QEBD/yQALCAABAAEBAREA/8wABgAQEAX/2gAIAQEAAD8A0s8g/9k=';
public function test_page_includes()
{
@@ -649,6 +649,35 @@ class PageContentTest extends TestCase
}
}
+ public function test_base64_images_within_html_blanked_if_no_image_create_permission()
+ {
+ $editor = $this->users->editor();
+ $page = $this->entities->page();
+ $this->permissions->removeUserRolePermissions($editor, ['image-create-all']);
+
+ $this->actingAs($editor)->put($page->getUrl(), [
+ 'name' => $page->name,
+ 'html' => '<p>test<img src="data:image/jpeg;base64,' . $this->base64Jpeg . '"/></p>',
+ ]);
+
+ $page->refresh();
+ $this->assertStringMatchesFormat('%A<p%A>test<img src="">%A</p>%A', $page->html);
+ }
+
+ public function test_base64_images_within_html_blanked_if_content_does_not_appear_like_an_image()
+ {
+ $page = $this->entities->page();
+
+ $imgContent = base64_encode('file://test/a/b/c');
+ $this->asEditor()->put($page->getUrl(), [
+ 'name' => $page->name,
+ 'html' => '<p>test<img src="data:image/jpeg;base64,' . $imgContent . '"/></p>',
+ ]);
+
+ $page->refresh();
+ $this->assertStringMatchesFormat('%A<p%A>test<img src="">%A</p>%A', $page->html);
+ }
+
public function test_base64_images_get_extracted_from_markdown_page_content()
{
$this->asEditor();
@@ -682,7 +711,7 @@ class PageContentTest extends TestCase
ini_set('pcre.backtrack_limit', '500');
ini_set('pcre.recursion_limit', '500');
- $content = str_repeat('a', 5000);
+ $content = str_repeat(base64_decode($this->base64Jpeg), 50);
$base64Content = base64_encode($content);
$this->put($page->getUrl(), [
@@ -716,6 +745,34 @@ class PageContentTest extends TestCase
$this->assertStringContainsString('<img src=""', $page->refresh()->html);
}
+ public function test_base64_images_within_markdown_blanked_if_no_image_create_permission()
+ {
+ $editor = $this->users->editor();
+ $page = $this->entities->page();
+ $this->permissions->removeUserRolePermissions($editor, ['image-create-all']);
+
+ $this->actingAs($editor)->put($page->getUrl(), [
+ 'name' => $page->name,
+ 'markdown' => 'test ',
+ ]);
+
+ $this->assertStringContainsString('<img src=""', $page->refresh()->html);
+ }
+
+ public function test_base64_images_within_markdown_blanked_if_content_does_not_appear_like_an_image()
+ {
+ $page = $this->entities->page();
+
+ $imgContent = base64_encode('file://test/a/b/c');
+ $this->asEditor()->put($page->getUrl(), [
+ 'name' => $page->name,
+ 'markdown' => 'test ',
+ ]);
+
+ $page->refresh();
+ $this->assertStringContainsString('<img src=""', $page->refresh()->html);
+ }
+
public function test_nested_headers_gets_assigned_an_id()
{
$page = $this->entities->page();
diff --git a/tests/ThemeTest.php b/tests/ThemeTest.php
index 53361e351..f6706595b 100644
--- a/tests/ThemeTest.php
+++ b/tests/ThemeTest.php
@@ -78,7 +78,7 @@ class ThemeTest extends TestCase
$page = $this->entities->page();
$content = new PageContent($page);
- $content->setNewMarkdown('# test');
+ $content->setNewMarkdown('# test', $this->users->editor());
$this->assertTrue($callbackCalled);
}