From 72fbbcd6794facea2cf06d9742359d190257b00f Mon Sep 17 00:00:00 2001 From: ArthurHoaro Date: Tue, 6 Oct 2020 17:30:18 +0200 Subject: 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. --- application/Utils.php | 4 ++-- application/formatter/BookmarkFormatter.php | 26 ++++++++++++++++++++++ .../controller/admin/ManageShaareController.php | 10 ++++----- .../front/controller/admin/ManageTagController.php | 4 ++-- .../controller/visitor/BookmarkListController.php | 7 +++--- .../controller/visitor/TagCloudController.php | 12 ++++++++-- application/render/PageBuilder.php | 2 +- assets/default/js/base.js | 10 +++++++-- tpl/default/linklist.html | 4 ++-- tpl/default/tag.cloud.html | 6 ++--- tpl/default/tag.list.html | 10 ++++----- 11 files changed, 68 insertions(+), 27 deletions(-) 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"} - + {$value} @@ -183,7 +183,7 @@ {$tag_counter=count($value.taglist)} {loop="value.taglist"} - {$value} + {$value} {if="$tag_counter - 1 != $counter"}·{/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 @@

{'Tag cloud'|t} - {$countTags} {'tags'|t}

{if="!empty($search_tags)"}

- + {'List all links with those tags'|t}

@@ -48,8 +48,8 @@
{loop="tags"} - {$key}{$value.count} + {$key}{$value.count} {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 @@

{'Tag list'|t} - {$countTags} {'tags'|t}

{if="!empty($search_tags)"}

- + {'List all links with those tags'|t}

@@ -47,17 +47,17 @@
{loop="tags"} -
+
{if="$is_logged_in===true"}    - + {/if} - {$value} - {$key} + {$value} + {$key} {loop="$value.tag_plugin"} {$value} -- cgit v1.2.3