From: ArthurHoaro <arthur@hoa.ro>
Date: Tue, 6 Oct 2020 15:30:18 +0000 (+0200)
Subject: Security: fix multiple XSS vulnerabilities + fix search tags with special chars
X-Git-Tag: v0.12.0-beta-2~4^2
X-Git-Url: https://git.immae.eu/?p=github%2Fshaarli%2FShaarli.git;a=commitdiff_plain;h=72fbbcd6794facea2cf06d9742359d190257b00f

Security: fix multiple XSS vulnerabilities + fix search tags with special chars

XSS vulnerabilities fixed in editlink, linklist, tag.cloud and tag.list.

Also fixed tag search with special characters: urlencode function needs to be applied on raw data, before espaping, otherwise the rendered URL is wrong.
---

diff --git a/application/Utils.php b/application/Utils.php
index 9c9eaaa2..bcfda65c 100644
--- a/application/Utils.php
+++ b/application/Utils.php
@@ -95,14 +95,14 @@ function escape($input)
         return null;
     }
 
-    if (is_bool($input)) {
+    if (is_bool($input) || is_int($input) || is_float($input) || $input instanceof DateTimeInterface) {
         return $input;
     }
 
     if (is_array($input)) {
         $out = array();
         foreach ($input as $key => $value) {
-            $out[$key] = escape($value);
+            $out[escape($key)] = escape($value);
         }
         return $out;
     }
diff --git a/application/formatter/BookmarkFormatter.php b/application/formatter/BookmarkFormatter.php
index 22ba7aae..0042dafe 100644
--- a/application/formatter/BookmarkFormatter.php
+++ b/application/formatter/BookmarkFormatter.php
@@ -58,7 +58,9 @@ abstract class BookmarkFormatter
         $out['title'] = $this->formatTitle($bookmark);
         $out['description'] = $this->formatDescription($bookmark);
         $out['thumbnail'] = $this->formatThumbnail($bookmark);
+        $out['urlencoded_taglist'] = $this->formatUrlEncodedTagList($bookmark);
         $out['taglist'] = $this->formatTagList($bookmark);
+        $out['urlencoded_tags'] = $this->formatUrlEncodedTagString($bookmark);
         $out['tags'] = $this->formatTagString($bookmark);
         $out['sticky'] = $bookmark->isSticky();
         $out['private'] = $bookmark->isPrivate();
@@ -181,6 +183,18 @@ abstract class BookmarkFormatter
         return $this->filterTagList($bookmark->getTags());
     }
 
+    /**
+     * Format Url Encoded Tags
+     *
+     * @param Bookmark $bookmark instance
+     *
+     * @return array formatted Tags
+     */
+    protected function formatUrlEncodedTagList($bookmark)
+    {
+        return array_map('urlencode', $this->filterTagList($bookmark->getTags()));
+    }
+
     /**
      * Format TagString
      *
@@ -193,6 +207,18 @@ abstract class BookmarkFormatter
         return implode(' ', $this->formatTagList($bookmark));
     }
 
+    /**
+     * Format TagString
+     *
+     * @param Bookmark $bookmark instance
+     *
+     * @return string formatted TagString
+     */
+    protected function formatUrlEncodedTagString($bookmark)
+    {
+        return implode(' ', $this->formatUrlEncodedTagList($bookmark));
+    }
+
     /**
      * Format Class
      * Used to add specific CSS class for a link
diff --git a/application/front/controller/admin/ManageShaareController.php b/application/front/controller/admin/ManageShaareController.php
index 59ba2de9..bb083486 100644
--- a/application/front/controller/admin/ManageShaareController.php
+++ b/application/front/controller/admin/ManageShaareController.php
@@ -78,13 +78,13 @@ class ManageShaareController extends ShaarliAdminController
                 $title = $this->container->conf->get('general.default_note_title', t('Note: '));
             }
 
-            $link = escape([
+            $link = [
                 'title' => $title,
                 'url' => $url ?? '',
                 'description' => $description ?? '',
                 'tags' => $tags ?? '',
                 'private' => $private,
-            ]);
+            ];
         } else {
             $formatter = $this->container->formatterFactory->getFormatter('raw');
             $link = $formatter->format($bookmark);
@@ -345,14 +345,14 @@ class ManageShaareController extends ShaarliAdminController
             $tags[BookmarkMarkdownFormatter::NO_MD_TAG] = 1;
         }
 
-        $data = [
+        $data = escape([
             'link' => $link,
             'link_is_new' => $isNew,
-            'http_referer' => escape($this->container->environment['HTTP_REFERER'] ?? ''),
+            'http_referer' => $this->container->environment['HTTP_REFERER'] ?? '',
             'source' => $request->getParam('source') ?? '',
             'tags' => $tags,
             'default_private_links' => $this->container->conf->get('privacy.default_private_links', false),
-        ];
+        ]);
 
         $this->executePageHooks('render_editlink', $data, TemplatePage::EDIT_LINK);
 
diff --git a/application/front/controller/admin/ManageTagController.php b/application/front/controller/admin/ManageTagController.php
index 0380ef1f..2065c3e2 100644
--- a/application/front/controller/admin/ManageTagController.php
+++ b/application/front/controller/admin/ManageTagController.php
@@ -41,8 +41,8 @@ class ManageTagController extends ShaarliAdminController
 
         $isDelete = null !== $request->getParam('deletetag') && null === $request->getParam('renametag');
 
-        $fromTag = escape(trim($request->getParam('fromtag') ?? ''));
-        $toTag = escape(trim($request->getParam('totag') ?? ''));
+        $fromTag = trim($request->getParam('fromtag') ?? '');
+        $toTag = trim($request->getParam('totag') ?? '');
 
         if (0 === strlen($fromTag) || false === $isDelete && 0 === strlen($toTag)) {
             $this->saveWarningMessage(t('Invalid tags provided.'));
diff --git a/application/front/controller/visitor/BookmarkListController.php b/application/front/controller/visitor/BookmarkListController.php
index 2988bee6..18368751 100644
--- a/application/front/controller/visitor/BookmarkListController.php
+++ b/application/front/controller/visitor/BookmarkListController.php
@@ -34,7 +34,7 @@ class BookmarkListController extends ShaarliVisitorController
         $formatter = $this->container->formatterFactory->getFormatter();
         $formatter->addContextData('base_path', $this->container->basePath);
 
-        $searchTags = escape(normalize_spaces($request->getParam('searchtags') ?? ''));
+        $searchTags = normalize_spaces($request->getParam('searchtags') ?? '');
         $searchTerm = escape(normalize_spaces($request->getParam('searchterm') ?? ''));;
 
         // Filter bookmarks according search parameters.
@@ -104,8 +104,9 @@ class BookmarkListController extends ShaarliVisitorController
                 'page_current' => $page,
                 'page_max' => $pageCount,
                 'result_count' => count($linksToDisplay),
-                'search_term' => $searchTerm,
-                'search_tags' => $searchTags,
+                'search_term' => escape($searchTerm),
+                'search_tags' => escape($searchTags),
+                'search_tags_url' => array_map('urlencode', explode(' ', $searchTags)),
                 'visibility' => $visibility,
                 'links' => $linkDisp,
             ]
diff --git a/application/front/controller/visitor/TagCloudController.php b/application/front/controller/visitor/TagCloudController.php
index f9c529bc..76ed7690 100644
--- a/application/front/controller/visitor/TagCloudController.php
+++ b/application/front/controller/visitor/TagCloudController.php
@@ -66,10 +66,18 @@ class TagCloudController extends ShaarliVisitorController
             $tags = $this->formatTagsForCloud($tags);
         }
 
+        $tagsUrl = [];
+        foreach ($tags as $tag => $value) {
+            $tagsUrl[escape($tag)] = urlencode((string) $tag);
+        }
+
         $searchTags = implode(' ', escape($filteringTags));
+        $searchTagsUrl = urlencode(implode(' ', $filteringTags));
         $data = [
-            'search_tags' => $searchTags,
-            'tags' => $tags,
+            'search_tags' => escape($searchTags),
+            'search_tags_url' => $searchTagsUrl,
+            'tags' => escape($tags),
+            'tags_url' => $tagsUrl,
         ];
         $this->executePageHooks('render_tag' . $type, $data, 'tag.' . $type);
         $this->assignAllView($data);
diff --git a/application/render/PageBuilder.php b/application/render/PageBuilder.php
index c52e3b76..41b357dd 100644
--- a/application/render/PageBuilder.php
+++ b/application/render/PageBuilder.php
@@ -137,7 +137,7 @@ class PageBuilder
         $this->tpl->assign('language', $this->conf->get('translation.language'));
 
         if ($this->bookmarkService !== null) {
-            $this->tpl->assign('tags', $this->bookmarkService->bookmarksCountPerTag());
+            $this->tpl->assign('tags', escape($this->bookmarkService->bookmarksCountPerTag()));
         }
 
         $this->tpl->assign(
diff --git a/assets/default/js/base.js b/assets/default/js/base.js
index d9933152..be986ae0 100644
--- a/assets/default/js/base.js
+++ b/assets/default/js/base.js
@@ -555,6 +555,7 @@ function init(description) {
       }
       const refreshedToken = document.getElementById('token').value;
       const fromtag = block.getAttribute('data-tag');
+      const fromtagUrl = block.getAttribute('data-tag-url');
       const xhr = new XMLHttpRequest();
       xhr.open('POST', `${basePath}/admin/tags`);
       xhr.setRequestHeader('Content-Type', 'application/x-www-form-urlencoded');
@@ -564,6 +565,7 @@ function init(description) {
           location.reload();
         } else {
           block.setAttribute('data-tag', totag);
+          block.setAttribute('data-tag-url', encodeURIComponent(totag));
           input.setAttribute('name', totag);
           input.setAttribute('value', totag);
           findParent(input, 'div', { class: 'rename-tag-form' }).style.display = 'none';
@@ -571,6 +573,9 @@ function init(description) {
           block
             .querySelector('a.tag-link')
             .setAttribute('href', `${basePath}/?searchtags=${encodeURIComponent(totag)}`);
+          block
+            .querySelector('a.count')
+            .setAttribute('href', `${basePath}/add-tag/${encodeURIComponent(totag)}`);
           block
             .querySelector('a.rename-tag')
             .setAttribute('href', `${basePath}/admin/tags?fromtag=${encodeURIComponent(totag)}`);
@@ -580,7 +585,7 @@ function init(description) {
           awesomepletes = updateAwesompleteList('.rename-tag-input', existingTags, awesomepletes);
         }
       };
-      xhr.send(`renametag=1&fromtag=${encodeURIComponent(fromtag)}&totag=${encodeURIComponent(totag)}&token=${refreshedToken}`);
+      xhr.send(`renametag=1&fromtag=${fromtagUrl}&totag=${encodeURIComponent(totag)}&token=${refreshedToken}`);
       refreshToken(basePath);
     });
   });
@@ -603,6 +608,7 @@ function init(description) {
       event.preventDefault();
       const block = findParent(event.target, 'div', { class: 'tag-list-item' });
       const tag = block.getAttribute('data-tag');
+      const tagUrl = block.getAttribute('data-tag-url');
       const refreshedToken = document.getElementById('token').value;
 
       if (confirm(`Are you sure you want to delete the tag "${tag}"?`)) {
@@ -612,7 +618,7 @@ function init(description) {
         xhr.onload = () => {
           block.remove();
         };
-        xhr.send(encodeURI(`deletetag=1&fromtag=${tag}&token=${refreshedToken}`));
+        xhr.send(`deletetag=1&fromtag=${tagUrl}&token=${refreshedToken}`);
         refreshToken(basePath);
 
         existingTags = existingTags.filter((tagItem) => tagItem !== tag);
diff --git a/tpl/default/linklist.html b/tpl/default/linklist.html
index 2475f5fd..b08773d8 100644
--- a/tpl/default/linklist.html
+++ b/tpl/default/linklist.html
@@ -94,7 +94,7 @@
           {'tagged'|t}
           {loop="$exploded_tags"}
               <span class="label label-tag" title="{'Remove tag'|t}">
-                <a href="{$base_path}/remove-tag/{function="urlencode($value)"}" aria-label="{'Remove tag'|t}">
+                <a href="{$base_path}/remove-tag/{function="$search_tags_url.$key1"}" aria-label="{'Remove tag'|t}">
                   {$value}<span class="remove"><i class="fa fa-times" aria-hidden="true"></i></span>
                 </a>
               </span>
@@ -183,7 +183,7 @@
                 {$tag_counter=count($value.taglist)}
                 {loop="value.taglist"}
                   <span class="label label-tag" title="{$strAddTag}">
-                    <a href="{$base_path}/add-tag/{$value|urlencode}">{$value}</a>
+                    <a href="{$base_path}/add-tag/{$value1.urlencoded_taglist.$key2}">{$value}</a>
                   </span>
                   {if="$tag_counter - 1 != $counter"}&middot;{/if}
                 {/loop}
diff --git a/tpl/default/tag.cloud.html b/tpl/default/tag.cloud.html
index 024882ec..c067e1d4 100644
--- a/tpl/default/tag.cloud.html
+++ b/tpl/default/tag.cloud.html
@@ -15,7 +15,7 @@
     <h2 class="window-title">{'Tag cloud'|t} - {$countTags} {'tags'|t}</h2>
     {if="!empty($search_tags)"}
     <p class="center">
-      <a href="{$base_path}/?searchtags={$search_tags|urlencode}" class="pure-button pure-button-shaarli">
+      <a href="{$base_path}/?searchtags={$search_tags_url}" class="pure-button pure-button-shaarli">
         {'List all links with those tags'|t}
       </a>
     </p>
@@ -48,8 +48,8 @@
 
     <div id="cloudtag" class="cloudtag-container">
       {loop="tags"}
-        <a href="{$base_path}/?searchtags={$key|urlencode} {$search_tags|urlencode}" style="font-size:{$value.size}em;">{$key}</a
-        ><a href="{$base_path}/add-tag/{$key|urlencode}" title="{'Filter by tag'|t}" class="count">{$value.count}</a>
+        <a href="{$base_path}/?searchtags={$tags_url.$key1} {$search_tags_url}" style="font-size:{$value.size}em;">{$key}</a
+        ><a href="{$base_path}/add-tag/{$tags_url.$key1}" title="{'Filter by tag'|t}" class="count">{$value.count}</a>
         {loop="$value.tag_plugin"}
           {$value}
         {/loop}
diff --git a/tpl/default/tag.list.html b/tpl/default/tag.list.html
index 99ae44d2..96e7fbe0 100644
--- a/tpl/default/tag.list.html
+++ b/tpl/default/tag.list.html
@@ -15,7 +15,7 @@
     <h2 class="window-title">{'Tag list'|t} - {$countTags} {'tags'|t}</h2>
     {if="!empty($search_tags)"}
       <p class="center">
-        <a href="{$base_path}/?searchtags={$search_tags|urlencode}" class="pure-button pure-button-shaarli">
+        <a href="{$base_path}/?searchtags={$search_tags_url}" class="pure-button pure-button-shaarli">
           {'List all links with those tags'|t}
         </a>
       </p>
@@ -47,17 +47,17 @@
 
     <div id="taglist" class="taglist-container">
       {loop="tags"}
-        <div class="tag-list-item pure-g" data-tag="{$key}">
+        <div class="tag-list-item pure-g" data-tag="{$key}" data-tag-url="{$tags_url.$key1}">
           <div class="pure-u-1">
             {if="$is_logged_in===true"}
               <a href="#" class="delete-tag" aria-label="{'Delete'|t}"><i class="fa fa-trash" aria-hidden="true"></i></a>&nbsp;&nbsp;
-              <a href="{$base_path}/admin/tags?fromtag={$key|urlencode}" class="rename-tag" aria-label="{'Rename tag'|t}">
+              <a href="{$base_path}/admin/tags?fromtag={$tags_url.$key1}" class="rename-tag" aria-label="{'Rename tag'|t}">
                 <i class="fa fa-pencil-square-o {$key}" aria-hidden="true"></i>
               </a>
             {/if}
 
-            <a href="{$base_path}/add-tag/{$key|urlencode}" title="{'Filter by tag'|t}" class="count">{$value}</a>
-            <a href="{$base_path}/?searchtags={$key|urlencode} {$search_tags|urlencode}" class="tag-link">{$key}</a>
+            <a href="{$base_path}/add-tag/{$tags_url.$key1}" title="{'Filter by tag'|t}" class="count">{$value}</a>
+            <a href="{$base_path}/?searchtags={$tags_url.$key1} {$search_tags_url}" class="tag-link">{$key}</a>
 
             {loop="$value.tag_plugin"}
               {$value}