From 86ceea054f5f85157b04473bac5bfb6ff86ca31f Mon Sep 17 00:00:00 2001 From: ArthurHoaro Date: Thu, 25 May 2017 14:52:42 +0200 Subject: [PATCH] Add a whitelist of protocols for URLs - for Shaare - for markdown description links and images Not whitelisted protocols will be replaced by `http://` --- application/Url.php | 24 ++++++++++ application/config/ConfigManager.php | 1 + index.php | 8 +--- plugins/markdown/markdown.php | 37 ++++++++++++++-- tests/Url/WhitelistProtocolsTest.php | 63 +++++++++++++++++++++++++++ tests/plugins/PluginMarkdownTest.php | 11 +++-- tests/plugins/resources/markdown.html | 11 ++++- tests/plugins/resources/markdown.md | 12 ++++- 8 files changed, 151 insertions(+), 16 deletions(-) create mode 100644 tests/Url/WhitelistProtocolsTest.php diff --git a/application/Url.php b/application/Url.php index 25a62a8a..b3759377 100644 --- a/application/Url.php +++ b/application/Url.php @@ -63,6 +63,30 @@ function add_trailing_slash($url) return $url . (!endsWith($url, '/') ? '/' : ''); } +/** + * Replace not whitelisted protocols by 'http://' from given URL. + * + * @param string $url URL to clean + * @param array $protocols List of allowed protocols (aside from http(s)). + * + * @return string URL with allowed protocol + */ +function whitelist_protocols($url, $protocols) +{ + if (startsWith($url, '?') || startsWith($url, '/')) { + return $url; + } + $protocols = array_merge(['http', 'https'], $protocols); + $protocol = preg_match('#^(\w+):/?/?#', $url, $match); + // Protocol not allowed: we remove it and replace it with http + if ($protocol === 1 && ! in_array($match[1], $protocols)) { + $url = str_replace($match[0], 'http://', $url); + } else if ($protocol !== 1) { + $url = 'http://' . $url; + } + return $url; +} + /** * URL representation and cleanup utilities * diff --git a/application/config/ConfigManager.php b/application/config/ConfigManager.php index 86a917fb..8eab26f1 100644 --- a/application/config/ConfigManager.php +++ b/application/config/ConfigManager.php @@ -312,6 +312,7 @@ protected function setDefaultValues() $this->setEmpty('security.ban_duration', 1800); $this->setEmpty('security.session_protection_disabled', false); $this->setEmpty('security.open_shaarli', false); + $this->setEmpty('security.allowed_protocols', ['ftp', 'ftps', 'magnet']); $this->setEmpty('general.header_link', '?'); $this->setEmpty('general.links_per_page', 20); diff --git a/index.php b/index.php index 468dd091..944af674 100644 --- a/index.php +++ b/index.php @@ -1237,13 +1237,7 @@ function renderPage($conf, $pluginManager, $LINKSDB, $history) // Remove duplicates. $tags = implode(' ', array_unique(explode(' ', $tags))); - $url = trim($_POST['lf_url']); - if (! startsWith($url, 'http:') && ! startsWith($url, 'https:') - && ! startsWith($url, 'ftp:') && ! startsWith($url, 'magnet:') - && ! startsWith($url, '?') && ! startsWith($url, 'javascript:') - ) { - $url = 'http://' . $url; - } + $url = whitelist_protocols(trim($_POST['lf_url']), $conf->get('security.allowed_protocols')); $link = array( 'id' => $id, diff --git a/plugins/markdown/markdown.php b/plugins/markdown/markdown.php index de7c823d..772c56e8 100644 --- a/plugins/markdown/markdown.php +++ b/plugins/markdown/markdown.php @@ -26,7 +26,11 @@ function hook_markdown_render_linklist($data, $conf) $value = stripNoMarkdownTag($value); continue; } - $value['description'] = process_markdown($value['description'], $conf->get('security.markdown_escape', true)); + $value['description'] = process_markdown( + $value['description'], + $conf->get('security.markdown_escape', true), + $conf->get('security.allowed_protocols') + ); } return $data; } @@ -46,7 +50,11 @@ function hook_markdown_render_feed($data, $conf) $value = stripNoMarkdownTag($value); continue; } - $value['description'] = process_markdown($value['description'], $conf->get('security.markdown_escape', true)); + $value['description'] = process_markdown( + $value['description'], + $conf->get('security.markdown_escape', true), + $conf->get('security.allowed_protocols') + ); } return $data; @@ -71,7 +79,8 @@ function hook_markdown_render_daily($data, $conf) } $value2['formatedDescription'] = process_markdown( $value2['formatedDescription'], - $conf->get('security.markdown_escape', true) + $conf->get('security.markdown_escape', true), + $conf->get('security.allowed_protocols') ); } } @@ -231,6 +240,25 @@ function reverse_space2nbsp($description) return preg_replace('/(^| ) /m', '$1 ', $description); } +/** + * Replace not whitelisted protocols with http:// in given description. + * + * @param string $description input description text. + * @param array $allowedProtocols list of allowed protocols. + * + * @return string $description without malicious link. + */ +function filter_protocols($description, $allowedProtocols) +{ + return preg_replace_callback( + '#]\((.*?)\)#is', + function ($match) use ($allowedProtocols) { + return ']('. whitelist_protocols($match[1], $allowedProtocols) .')'; + }, + $description + ); +} + /** * Remove dangerous HTML tags (tags, iframe, etc.). * Doesn't affect content (already escaped by Parsedown). @@ -275,7 +303,7 @@ function ($match) { return escape($match[0]); }, * * @return string HTML processed $description. */ -function process_markdown($description, $escape = true) +function process_markdown($description, $escape = true, $allowedProtocols = []) { $parsedown = new Parsedown(); @@ -283,6 +311,7 @@ function process_markdown($description, $escape = true) $processedDescription = reverse_nl2br($processedDescription); $processedDescription = reverse_space2nbsp($processedDescription); $processedDescription = reverse_text2clickable($processedDescription); + $processedDescription = filter_protocols($processedDescription, $allowedProtocols); $processedDescription = unescape($processedDescription); $processedDescription = $parsedown ->setMarkupEscaped($escape) diff --git a/tests/Url/WhitelistProtocolsTest.php b/tests/Url/WhitelistProtocolsTest.php new file mode 100644 index 00000000..a3156804 --- /dev/null +++ b/tests/Url/WhitelistProtocolsTest.php @@ -0,0 +1,63 @@ +assertEquals($url, whitelist_protocols($url, $whitelist)); + $url = '/path.jpg'; + $this->assertEquals($url, whitelist_protocols($url, $whitelist)); + } + + /** + * Test whitelist_protocols() on a note (relative URL). + */ + public function testWhitelistProtocolMissing() + { + $whitelist = ['ftp', 'magnet']; + $url = 'test.tld/path/?query=value#hash'; + $this->assertEquals('http://'. $url, whitelist_protocols($url, $whitelist)); + } + + /** + * Test whitelist_protocols() with allowed protocols. + */ + public function testWhitelistAllowedProtocol() + { + $whitelist = ['ftp', 'magnet']; + $url = 'http://test.tld/path/?query=value#hash'; + $this->assertEquals($url, whitelist_protocols($url, $whitelist)); + $url = 'https://test.tld/path/?query=value#hash'; + $this->assertEquals($url, whitelist_protocols($url, $whitelist)); + $url = 'ftp://test.tld/path/?query=value#hash'; + $this->assertEquals($url, whitelist_protocols($url, $whitelist)); + $url = 'magnet:test.tld/path/?query=value#hash'; + $this->assertEquals($url, whitelist_protocols($url, $whitelist)); + } + + /** + * Test whitelist_protocols() with allowed protocols. + */ + public function testWhitelistDisallowedProtocol() + { + $whitelist = ['ftp', 'magnet']; + $url = 'javascript:alert("xss");'; + $this->assertEquals('http://alert("xss");', whitelist_protocols($url, $whitelist)); + $url = 'other://test.tld/path/?query=value#hash'; + $this->assertEquals('http://test.tld/path/?query=value#hash', whitelist_protocols($url, $whitelist)); + } +} diff --git a/tests/plugins/PluginMarkdownTest.php b/tests/plugins/PluginMarkdownTest.php index d8180ad6..96891f1f 100644 --- a/tests/plugins/PluginMarkdownTest.php +++ b/tests/plugins/PluginMarkdownTest.php @@ -26,6 +26,7 @@ public function setUp() { PluginManager::$PLUGINS_PATH = 'plugins'; $this->conf = new ConfigManager('tests/utils/config/configJson'); + $this->conf->set('security.allowed_protocols', ['ftp', 'magnet']); } /** @@ -183,15 +184,19 @@ public function testNoMarkdownNotExcactlyMatching() } /** - * Test hashtag links processed with markdown. + * Make sure that the generated HTML match the reference HTML file. */ - public function testMarkdownHashtagLinks() + public function testMarkdownGlobalProcessDescription() { $md = file_get_contents('tests/plugins/resources/markdown.md'); $md = format_description($md); $html = file_get_contents('tests/plugins/resources/markdown.html'); - $data = process_markdown($md); + $data = process_markdown( + $md, + $this->conf->get('security.markdown_escape', true), + $this->conf->get('security.allowed_protocols') + ); $this->assertEquals($html, $data); } diff --git a/tests/plugins/resources/markdown.html b/tests/plugins/resources/markdown.html index 07a5a32e..844a6f31 100644 --- a/tests/plugins/resources/markdown.html +++ b/tests/plugins/resources/markdown.html @@ -21,4 +21,13 @@ next #foo

Block:

lorem ipsum #foobar http://link.tld
-#foobar http://link.tld
\ No newline at end of file +#foobar http://link.tld +

link
+link
+link
+link
+link
+link
+link
+link
+link

\ No newline at end of file diff --git a/tests/plugins/resources/markdown.md b/tests/plugins/resources/markdown.md index 0b8be7c5..b8ebd934 100644 --- a/tests/plugins/resources/markdown.md +++ b/tests/plugins/resources/markdown.md @@ -21,4 +21,14 @@ Block: ``` lorem ipsum #foobar http://link.tld #foobar http://link.tld -``` \ No newline at end of file +``` + +[link](?123456) +![link](/img/train.png) +[link](test.tld/path/?query=value#hash) +[link](http://test.tld/path/?query=value#hash) +[link](https://test.tld/path/?query=value#hash) +[link](ftp://test.tld/path/?query=value#hash) +[link](magnet:test.tld/path/?query=value#hash) +[link](javascript:alert('xss')) +[link](other://test.tld/path/?query=value#hash) \ No newline at end of file