CVE-2020-26210: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') (CWE-79)
value: In BookStack before version 0.30.4, a user with permissions to edit a page could add an attached link which would execute untrusted JavaScript code when clicked by a viewer of the page. Dangerous content may remain in the database after this update. If you think this could have been exploited the linked advisory provides a SQL query to test. As a workaround, page edit permissions could be limited to only those that are trusted until you can upgrade although this will not address existing exploitation of this vulnerability. The issue is fixed in version 0.30.4.
CWE description: The product does not neutralize or incorrectly neutralizes user-controllable input before it is placed in output that is used as a web page that is served to other users.
CVSS v3 base score: 8.7
Published: 2020-11-03T19:15Z
Repository: https://github.com/BookStackApp/BookStack
Language: PHP
Files changed: app/Http/Controllers/AttachmentController.php, app/Providers/AppServiceProvider.php, app/Uploads/AttachmentService.php, resources/lang/en/validation.php, tests/Uploads/AttachmentTest.php
Commit date: 2020-10-31 15:01:52 +0000
Commit message: 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.

Diff: diff --git a/app/Http/Controllers/AttachmentController.php b/app/Http/Controllers/AttachmentController.php
index 0830693b..f5214329 100644
--- a/app/Http/Controllers/AttachmentController.php
+++ b/app/Http/Controllers/AttachmentController.php
@@ -108,11 +108,11 @@ class AttachmentController extends Controller
         $attachment = $this->attachment->newQuery()->findOrFail($attachmentId);
 
         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']), [
                 'attachment' => $attachment,
                 'errors' => new MessageBag($exception->errors()),
@@ -143,11 +143,11 @@ class AttachmentController extends Controller
 
         try {
             $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']), [
                 'pageId' => $pageId,
                 'errors' => new MessageBag($exception->errors()),
@@ -159,11 +159,11 @@ class AttachmentController extends Controller
         $this->checkPermission('attachment-create-all');
         $this->checkOwnablePermission('page-update', $page);
 
         $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 1cc3e09c..f4181539 100644
--- a/app/Providers/AppServiceProvider.php
+++ b/app/Providers/AppServiceProvider.php
@@ -41,10 +41,17 @@ class AppServiceProvider extends ServiceProvider
         Validator::extend('no_double_extension', function ($attribute, $value, $parameters, $validator) {
             $uploadName = $value->getClientOriginalName();
             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 "<?php echo icon($expression); ?>";
         });
 
diff --git a/app/Uploads/AttachmentService.php b/app/Uploads/AttachmentService.php
index 02220771..e85901e1 100644
--- a/app/Uploads/AttachmentService.php
+++ b/app/Uploads/AttachmentService.php
@@ -86,16 +86,12 @@ class AttachmentService extends UploadService
         return $attachment;
     }
 
     /**
      * 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([
             'name' => $name,
             'path' => $link,
@@ -121,24 +117,23 @@ 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) {
                 $this->deleteFileInStorage($attachment);
                 $attachment->external = true;
             }
         }
+
         $attachment->save();
         return $attachment;
     }
 
     /**
diff --git a/resources/lang/en/validation.php b/resources/lang/en/validation.php
index 76b57a2a..578ea999 100644
--- a/resources/lang/en/validation.php
+++ b/resources/lang/en/validation.php
@@ -88,10 +88,11 @@ return [
     'required_with'        => 'The :attribute field is required when :values is present.',
     'required_with_all'    => 'The :attribute field is required when :values is present.',
     '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.',
         'string'  => 'The :attribute must be :size characters.',
         'array'   => 'The :attribute must contain :size items.',
diff --git a/tests/Uploads/AttachmentTest.php b/tests/Uploads/AttachmentTest.php
index a7efe08a..5838b019 100644
--- a/tests/Uploads/AttachmentTest.php
+++ b/tests/Uploads/AttachmentTest.php
@@ -1,43 +1,55 @@
 <?php namespace Tests\Uploads;
 
 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);
         }
     }
 
@@ -143,25 +155,18 @@ class AttachmentTest extends TestCase
     public function test_attachment_updating()
     {
         $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
         ];
 
@@ -240,6 +245,47 @@ class AttachmentTest extends TestCase
         $attachmentGet->assertStatus(404);
         $attachmentGet->assertSee("Attachment not found");
 
         $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;<a></a>",
+            "Data:text/html;<a></a>",
+            "Data:text/html;<a></a>",
+        ];
+
+        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,
+            ]);
+        }
+    }
 }

Vulnerable code:                 'attachment_edit_url' =>  'string|min:1|max:255'
                'attachment_link_url' =>  'required|string|min:1|max:255'
        $attachment = $this->attachmentService->saveNewFromLink($attachmentName, $link, $pageId);
     * @param string $name
     * @param string $link
     * @param int $page_id
     * @return Attachment
    public function saveNewFromLink($name, $link, $page_id)
     * @param Attachment $attachment
     * @param $requestData
     * @return Attachment
    public function updateFile(Attachment $attachment, $requestData)
     * @param $fileName
     * @return \Illuminate\Http\UploadedFile
    protected function getTestFile($fileName)
        return new \Illuminate\Http\UploadedFile(base_path('tests/test-data/test-file.txt'), $fileName, 'text/plain', 55, null, true);
     * @param $name
     * @param int $uploadedTo
     * @return \Illuminate\Foundation\Testing\TestResponse
    protected function uploadFile($name, $uploadedTo = 0)
        $fileService = $this->app->make(\BookStack\Uploads\AttachmentService::class);
        $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, [
            'id' => $attachmentId,
Fixed code:                 'attachment_edit_url' =>  'string|min:1|max:255|safe_url'
                'attachment_link_url' =>  'required|string|min:1|max:255|safe_url'
        $attachment = $this->attachmentService->saveNewFromLink($attachmentName, $link, intval($pageId));
        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;
        });

    public function saveNewFromLink(string $name, string $link, int $page_id): Attachment
    public function updateFile(Attachment $attachment, array $requestData): Attachment


    'safe_url'             => 'The provided link may not be safe.',
use BookStack\Uploads\AttachmentService;
use Illuminate\Http\UploadedFile;
use Tests\TestResponse;
    protected function getTestFile(string $fileName): UploadedFile
        return new UploadedFile(base_path('tests/test-data/test-file.txt'), $fileName, 'text/plain', 55, null, true);
    protected function uploadFile(string $name, int $uploadedTo = 0): \Illuminate\Foundation\Testing\TestResponse
    /**
     * 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();
    }

        $fileService = $this->app->make(AttachmentService::class);
        $attachment = $this->createAttachment($page);
        $update = $this->call('PUT', 'attachments/' . $attachment->id, [
            'id' => $attachment->id,

    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;<a></a>",
            "Data:text/html;<a></a>",
            "Data:text/html;<a></a>",
        ];

        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,
            ]);
        }
    }