From 349162ea139556b2d25e09e155cec84e21cc9227 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 31 Oct 2020 15:01:52 +0000 Subject: [PATCH] Prevented possible XSS via link attachments This filters out potentially malicious javascript: or data: uri's coming through to be attached to attachments. Added tests to cover. Thanks to Yassine ABOUKIR (@yassineaboukir on twitter) for reporting this vulnerability. --- app/Http/Controllers/AttachmentController.php | 6 +- app/Providers/AppServiceProvider.php | 7 ++ app/Uploads/AttachmentService.php | 13 +-- resources/lang/en/validation.php | 1 + tests/Uploads/AttachmentTest.php | 84 ++++++++++++++----- 5 files changed, 80 insertions(+), 31 deletions(-) diff --git a/app/Http/Controllers/AttachmentController.php b/app/Http/Controllers/AttachmentController.php index 0830693bc..f52143292 100644 --- a/app/Http/Controllers/AttachmentController.php +++ b/app/Http/Controllers/AttachmentController.php @@ -110,7 +110,7 @@ class AttachmentController extends Controller try { $this->validate($request, [ 'attachment_edit_name' => 'required|string|min:1|max:255', - 'attachment_edit_url' => 'string|min:1|max:255' + 'attachment_edit_url' => 'string|min:1|max:255|safe_url' ]); } catch (ValidationException $exception) { return response()->view('attachments.manager-edit-form', array_merge($request->only(['attachment_edit_name', 'attachment_edit_url']), [ @@ -145,7 +145,7 @@ class AttachmentController extends Controller $this->validate($request, [ 'attachment_link_uploaded_to' => 'required|integer|exists:pages,id', 'attachment_link_name' => 'required|string|min:1|max:255', - 'attachment_link_url' => 'required|string|min:1|max:255' + 'attachment_link_url' => 'required|string|min:1|max:255|safe_url' ]); } catch (ValidationException $exception) { return response()->view('attachments.manager-link-form', array_merge($request->only(['attachment_link_name', 'attachment_link_url']), [ @@ -161,7 +161,7 @@ class AttachmentController extends Controller $attachmentName = $request->get('attachment_link_name'); $link = $request->get('attachment_link_url'); - $attachment = $this->attachmentService->saveNewFromLink($attachmentName, $link, $pageId); + $attachment = $this->attachmentService->saveNewFromLink($attachmentName, $link, intval($pageId)); return view('attachments.manager-link-form', [ 'pageId' => $pageId, diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index 1cc3e09c2..f41815399 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -43,6 +43,13 @@ class AppServiceProvider extends ServiceProvider return substr_count($uploadName, '.') < 2; }); + Validator::extend('safe_url', function ($attribute, $value, $parameters, $validator) { + $cleanLinkName = strtolower(trim($value)); + $isJs = strpos($cleanLinkName, 'javascript:') === 0; + $isData = strpos($cleanLinkName, 'data:') === 0; + return !$isJs && !$isData; + }); + // Custom blade view directives Blade::directive('icon', function ($expression) { return ""; diff --git a/app/Uploads/AttachmentService.php b/app/Uploads/AttachmentService.php index 02220771a..e85901e17 100644 --- a/app/Uploads/AttachmentService.php +++ b/app/Uploads/AttachmentService.php @@ -88,12 +88,8 @@ class AttachmentService extends UploadService /** * Save a new File attachment from a given link and name. - * @param string $name - * @param string $link - * @param int $page_id - * @return Attachment */ - public function saveNewFromLink($name, $link, $page_id) + public function saveNewFromLink(string $name, string $link, int $page_id): Attachment { $largestExistingOrder = Attachment::where('uploaded_to', '=', $page_id)->max('order'); return Attachment::forceCreate([ @@ -123,13 +119,11 @@ class AttachmentService extends UploadService /** * Update the details of a file. - * @param Attachment $attachment - * @param $requestData - * @return Attachment */ - public function updateFile(Attachment $attachment, $requestData) + public function updateFile(Attachment $attachment, array $requestData): Attachment { $attachment->name = $requestData['name']; + if (isset($requestData['link']) && trim($requestData['link']) !== '') { $attachment->path = $requestData['link']; if (!$attachment->external) { @@ -137,6 +131,7 @@ class AttachmentService extends UploadService $attachment->external = true; } } + $attachment->save(); return $attachment; } diff --git a/resources/lang/en/validation.php b/resources/lang/en/validation.php index 76b57a2a3..578ea999f 100644 --- a/resources/lang/en/validation.php +++ b/resources/lang/en/validation.php @@ -90,6 +90,7 @@ return [ 'required_without' => 'The :attribute field is required when :values is not present.', 'required_without_all' => 'The :attribute field is required when none of :values are present.', 'same' => 'The :attribute and :other must match.', + 'safe_url' => 'The provided link may not be safe.', 'size' => [ 'numeric' => 'The :attribute must be :size.', 'file' => 'The :attribute must be :size kilobytes.', diff --git a/tests/Uploads/AttachmentTest.php b/tests/Uploads/AttachmentTest.php index a7efe08ab..5838b019e 100644 --- a/tests/Uploads/AttachmentTest.php +++ b/tests/Uploads/AttachmentTest.php @@ -3,39 +3,51 @@ use BookStack\Uploads\Attachment; use BookStack\Entities\Page; use BookStack\Auth\Permissions\PermissionService; +use BookStack\Uploads\AttachmentService; +use Illuminate\Http\UploadedFile; use Tests\TestCase; +use Tests\TestResponse; class AttachmentTest extends TestCase { /** * Get a test file that can be uploaded - * @param $fileName - * @return \Illuminate\Http\UploadedFile */ - protected function getTestFile($fileName) + protected function getTestFile(string $fileName): UploadedFile { - return new \Illuminate\Http\UploadedFile(base_path('tests/test-data/test-file.txt'), $fileName, 'text/plain', 55, null, true); + return new UploadedFile(base_path('tests/test-data/test-file.txt'), $fileName, 'text/plain', 55, null, true); } /** * Uploads a file with the given name. - * @param $name - * @param int $uploadedTo - * @return \Illuminate\Foundation\Testing\TestResponse */ - protected function uploadFile($name, $uploadedTo = 0) + protected function uploadFile(string $name, int $uploadedTo = 0): \Illuminate\Foundation\Testing\TestResponse { $file = $this->getTestFile($name); return $this->call('POST', '/attachments/upload', ['uploaded_to' => $uploadedTo], [], ['file' => $file], []); } + /** + * Create a new attachment + */ + protected function createAttachment(Page $page): Attachment + { + $this->post('attachments/link', [ + 'attachment_link_url' => 'https://example.com', + 'attachment_link_name' => 'Example Attachment Link', + 'attachment_link_uploaded_to' => $page->id, + ]); + + return Attachment::query()->latest()->first(); + } + /** * Delete all uploaded files. * To assist with cleanup. */ protected function deleteUploads() { - $fileService = $this->app->make(\BookStack\Uploads\AttachmentService::class); + $fileService = $this->app->make(AttachmentService::class); foreach (Attachment::all() as $file) { $fileService->deleteFile($file); } @@ -145,21 +157,14 @@ class AttachmentTest extends TestCase $page = Page::first(); $this->asAdmin(); - $this->call('POST', 'attachments/link', [ - 'attachment_link_url' => 'https://example.com', - 'attachment_link_name' => 'Example Attachment Link', - 'attachment_link_uploaded_to' => $page->id, - ]); - - $attachmentId = Attachment::first()->id; - - $update = $this->call('PUT', 'attachments/' . $attachmentId, [ + $attachment = $this->createAttachment($page); + $update = $this->call('PUT', 'attachments/' . $attachment->id, [ 'attachment_edit_name' => 'My new attachment name', 'attachment_edit_url' => 'https://test.example.com' ]); $expectedData = [ - 'id' => $attachmentId, + 'id' => $attachment->id, 'path' => 'https://test.example.com', 'name' => 'My new attachment name', 'uploaded_to' => $page->id @@ -242,4 +247,45 @@ class AttachmentTest extends TestCase $this->deleteUploads(); } + + public function test_data_and_js_links_cannot_be_attached_to_a_page() + { + $page = Page::first(); + $this->asAdmin(); + + $badLinks = [ + 'javascript:alert("bunny")', + ' javascript:alert("bunny")', + 'JavaScript:alert("bunny")', + "\t\n\t\nJavaScript:alert(\"bunny\")", + "data:text/html;", + "Data:text/html;", + "Data:text/html;", + ]; + + foreach ($badLinks as $badLink) { + $linkReq = $this->post('attachments/link', [ + 'attachment_link_url' => $badLink, + 'attachment_link_name' => 'Example Attachment Link', + 'attachment_link_uploaded_to' => $page->id, + ]); + $linkReq->assertStatus(422); + $this->assertDatabaseMissing('attachments', [ + 'path' => $badLink, + ]); + } + + $attachment = $this->createAttachment($page); + + foreach ($badLinks as $badLink) { + $linkReq = $this->put('attachments/' . $attachment->id, [ + 'attachment_edit_url' => $badLink, + 'attachment_edit_name' => 'Example Attachment Link', + ]); + $linkReq->assertStatus(422); + $this->assertDatabaseMissing('attachments', [ + 'path' => $badLink, + ]); + } + } }