Skip to content

Commit 15d7161

Browse files
ssddanbrownssddanbrown
committed
Images: Prevented base64 extraction without permission
Also added content sniffing as an extra check. Added tests to cover.
1 parent 9b1f820 commit 15d7161

File tree

5 files changed

+88
-21
lines changed

5 files changed

+88
-21
lines changed

‎app/Entities/Repos/PageRepo.php‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -211,13 +211,13 @@ protected function updateTemplateStatusAndContentFromInput(Page $page, array $in
211211
$inputEmpty = empty($input['markdown']) && empty($input['html']);
212212

213213
if ($haveInput && $inputEmpty) {
214-
$pageContent->setNewHTML('');
214+
$pageContent->setNewHTML('', user());
215215
} elseif (!empty($input['markdown']) && is_string($input['markdown'])) {
216216
$newEditor = 'markdown';
217-
$pageContent->setNewMarkdown($input['markdown']);
217+
$pageContent->setNewMarkdown($input['markdown'], user());
218218
} elseif (isset($input['html'])) {
219219
$newEditor = 'wysiwyg';
220-
$pageContent->setNewHTML($input['html']);
220+
$pageContent->setNewHTML($input['html'], user());
221221
}
222222

223223
if ($newEditor !== $currentEditor && userCan('editor-change')) {
@@ -284,9 +284,9 @@ public function restoreRevision(Page $page, int $revisionId): Page
284284
$content = new PageContent($page);
285285

286286
if (!empty($revision->markdown)) {
287-
$content->setNewMarkdown($revision->markdown);
287+
$content->setNewMarkdown($revision->markdown, user());
288288
} else {
289-
$content->setNewHTML($revision->html);
289+
$content->setNewHTML($revision->html, user());
290290
}
291291

292292
$page->updated_by = user()->id;

‎app/Entities/Tools/PageContent.php‎

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,9 @@
99
use BookStack\Theming\ThemeEvents;
1010
use BookStack\Uploads\ImageRepo;
1111
use BookStack\Uploads\ImageService;
12+
use BookStack\Users\Models\User;
1213
use BookStack\Util\HtmlContentFilter;
14+
use BookStack\Util\WebSafeMimeSniffer;
1315
use DOMDocument;
1416
use DOMElement;
1517
use DOMNode;
@@ -27,9 +29,9 @@ public function __construct(
2729
/**
2830
* Update the content of the page with new provided HTML.
2931
*/
30-
public function setNewHTML(string $html): void
32+
public function setNewHTML(string $html, User $updater): void
3133
{
32-
$html = $this->extractBase64ImagesFromHtml($html);
34+
$html = $this->extractBase64ImagesFromHtml($html, $updater);
3335
$this->page->html = $this->formatHtml($html);
3436
$this->page->text = $this->toPlainText();
3537
$this->page->markdown = '';
@@ -38,9 +40,9 @@ public function setNewHTML(string $html): void
3840
/**
3941
* Update the content of the page with new provided Markdown content.
4042
*/
41-
public function setNewMarkdown(string $markdown): void
43+
public function setNewMarkdown(string $markdown, User $updater): void
4244
{
43-
$markdown = $this->extractBase64ImagesFromMarkdown($markdown);
45+
$markdown = $this->extractBase64ImagesFromMarkdown($markdown, $updater);
4446
$this->page->markdown = $markdown;
4547
$html = (new MarkdownToHtml($markdown))->convert();
4648
$this->page->html = $this->formatHtml($html);
@@ -50,7 +52,7 @@ public function setNewMarkdown(string $markdown): void
5052
/**
5153
* Convert all base64 image data to saved images.
5254
*/
53-
protected function extractBase64ImagesFromHtml(string $htmlText): string
55+
protected function extractBase64ImagesFromHtml(string $htmlText, User $updater): string
5456
{
5557
if (empty($htmlText) || !str_contains($htmlText, 'data:image')) {
5658
return $htmlText;
@@ -66,7 +68,7 @@ protected function extractBase64ImagesFromHtml(string $htmlText): string
6668
$imageNodes = $xPath->query('//img[contains(@src, \'data:image\')]');
6769
foreach ($imageNodes as $imageNode) {
6870
$imageSrc = $imageNode->getAttribute('src');
69-
$newUrl = $this->base64ImageUriToUploadedImageUrl($imageSrc);
71+
$newUrl = $this->base64ImageUriToUploadedImageUrl($imageSrc, $updater);
7072
$imageNode->setAttribute('src', $newUrl);
7173
}
7274

@@ -86,7 +88,7 @@ protected function extractBase64ImagesFromHtml(string $htmlText): string
8688
* Attempting to capture the whole data uri using regex can cause PHP
8789
* PCRE limits to be hit with larger, multi-MB, files.
8890
*/
89-
protected function extractBase64ImagesFromMarkdown(string $markdown): string
91+
protected function extractBase64ImagesFromMarkdown(string $markdown, User $updater): string
9092
{
9193
$matches = [];
9294
$contentLength = strlen($markdown);
@@ -104,7 +106,7 @@ protected function extractBase64ImagesFromMarkdown(string $markdown): string
104106
$dataUri .= $char;
105107
}
106108

107-
$newUrl = $this->base64ImageUriToUploadedImageUrl($dataUri);
109+
$newUrl = $this->base64ImageUriToUploadedImageUrl($dataUri, $updater);
108110
$replacements[] = [$dataUri, $newUrl];
109111
}
110112

@@ -119,16 +121,28 @@ protected function extractBase64ImagesFromMarkdown(string $markdown): string
119121
* Parse the given base64 image URI and return the URL to the created image instance.
120122
* Returns an empty string if the parsed URI is invalid or causes an error upon upload.
121123
*/
122-
protected function base64ImageUriToUploadedImageUrl(string $uri): string
124+
protected function base64ImageUriToUploadedImageUrl(string $uri, User $updater): string
123125
{
124126
$imageRepo = app()->make(ImageRepo::class);
125127
$imageInfo = $this->parseBase64ImageUri($uri);
126128

129+
// Validate user has permission to create images
130+
if (!$updater->can('image-create-all')) {
131+
return '';
132+
}
133+
127134
// Validate extension and content
128135
if (empty($imageInfo['data']) || !ImageService::isExtensionSupported($imageInfo['extension'])) {
129136
return '';
130137
}
131138

139+
// Validate content looks like an image via sniffing mime type
140+
$mimeSniffer = new WebSafeMimeSniffer();
141+
$mime = $mimeSniffer->sniff($imageInfo['data']);
142+
if (!str_starts_with($mime, 'image/')) {
143+
return '';
144+
}
145+
132146
// Validate that the content is not over our upload limit
133147
$uploadLimitBytes = (config('app.upload_limit') * 1000000);
134148
if (strlen($imageInfo['data']) > $uploadLimitBytes) {

‎app/Users/Models/User.php‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -160,10 +160,6 @@ public function attachDefaultRole(): void
160160
*/
161161
public function can(string $permissionName): bool
162162
{
163-
if ($this->email === 'guest') {
164-
return false;
165-
}
166-
167163
return $this->permissions()->contains($permissionName);
168164
}
169165

‎tests/Entity/PageContentTest.php‎

Lines changed: 59 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

99
class PageContentTest extends TestCase
1010
{
11-
protected $base64Jpeg = '/9j/2wBDAAMCAgICAgMCAgIDAwMDBAYEBAQEBAgGBgUGCQgKCgkICQkKDA8MCgsOCwkJDRENDg8QEBEQCgwSExIQEw8QEBD/yQALCAABAAEBAREA/8wABgAQEAX/2gAIAQEAAD8A0s8g/9k=';
11+
protected string $base64Jpeg = '/9j/2wBDAAMCAgICAgMCAgIDAwMDBAYEBAQEBAgGBgUGCQgKCgkICQkKDA8MCgsOCwkJDRENDg8QEBEQCgwSExIQEw8QEBD/yQALCAABAAEBAREA/8wABgAQEAX/2gAIAQEAAD8A0s8g/9k=';
1212

1313
public function test_page_includes()
1414
{
@@ -649,6 +649,35 @@ public function test_base64_images_within_html_blanked_if_not_supported_extensio
649649
}
650650
}
651651

652+
public function test_base64_images_within_html_blanked_if_no_image_create_permission()
653+
{
654+
$editor = $this->users->editor();
655+
$page = $this->entities->page();
656+
$this->permissions->removeUserRolePermissions($editor, ['image-create-all']);
657+
658+
$this->actingAs($editor)->put($page->getUrl(), [
659+
'name' => $page->name,
660+
'html' => '<p>test<img src="data:image/jpeg;base64,' . $this->base64Jpeg . '"/></p>',
661+
]);
662+
663+
$page->refresh();
664+
$this->assertStringMatchesFormat('%A<p%A>test<img src="">%A</p>%A', $page->html);
665+
}
666+
667+
public function test_base64_images_within_html_blanked_if_content_does_not_appear_like_an_image()
668+
{
669+
$page = $this->entities->page();
670+
671+
$imgContent = base64_encode('file://test/a/b/c');
672+
$this->asEditor()->put($page->getUrl(), [
673+
'name' => $page->name,
674+
'html' => '<p>test<img src="data:image/jpeg;base64,' . $imgContent . '"/></p>',
675+
]);
676+
677+
$page->refresh();
678+
$this->assertStringMatchesFormat('%A<p%A>test<img src="">%A</p>%A', $page->html);
679+
}
680+
652681
public function test_base64_images_get_extracted_from_markdown_page_content()
653682
{
654683
$this->asEditor();
@@ -682,7 +711,7 @@ public function test_markdown_base64_extract_not_limited_by_pcre_limits()
682711
ini_set('pcre.backtrack_limit', '500');
683712
ini_set('pcre.recursion_limit', '500');
684713

685-
$content = str_repeat('a', 5000);
714+
$content = str_repeat(base64_decode($this->base64Jpeg), 50);
686715
$base64Content = base64_encode($content);
687716

688717
$this->put($page->getUrl(), [
@@ -716,6 +745,34 @@ public function test_base64_images_within_markdown_blanked_if_not_supported_exte
716745
$this->assertStringContainsString('<img src=""', $page->refresh()->html);
717746
}
718747

748+
public function test_base64_images_within_markdown_blanked_if_no_image_create_permission()
749+
{
750+
$editor = $this->users->editor();
751+
$page = $this->entities->page();
752+
$this->permissions->removeUserRolePermissions($editor, ['image-create-all']);
753+
754+
$this->actingAs($editor)->put($page->getUrl(), [
755+
'name' => $page->name,
756+
'markdown' => 'test ![test](data:image/jpeg;base64,' . $this->base64Jpeg . ')',
757+
]);
758+
759+
$this->assertStringContainsString('<img src=""', $page->refresh()->html);
760+
}
761+
762+
public function test_base64_images_within_markdown_blanked_if_content_does_not_appear_like_an_image()
763+
{
764+
$page = $this->entities->page();
765+
766+
$imgContent = base64_encode('file://test/a/b/c');
767+
$this->asEditor()->put($page->getUrl(), [
768+
'name' => $page->name,
769+
'markdown' => 'test ![test](data:image/jpeg;base64,' . $imgContent . ')',
770+
]);
771+
772+
$page->refresh();
773+
$this->assertStringContainsString('<img src=""', $page->refresh()->html);
774+
}
775+
719776
public function test_nested_headers_gets_assigned_an_id()
720777
{
721778
$page = $this->entities->page();

‎tests/ThemeTest.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ public function test_event_commonmark_environment_configure()
7878

7979
$page = $this->entities->page();
8080
$content = new PageContent($page);
81-
$content->setNewMarkdown('# test');
81+
$content->setNewMarkdown('# test', $this->users->editor());
8282

8383
$this->assertTrue($callbackCalled);
8484
}

0 commit comments

Comments
 (0)