Merge pull request #880 from ArthurHoaro/hotfix/allowed-protocols

Add a whitelist of protocols for URLs

8 files changed, 151 insertions, 16 deletions

diff --git a/application/Url.php b/application/Url.php
index 25a62a8a..b3759377 100644
--- a/application/Url.php
+++ b/application/Url.php
@@ -64,6 +64,30 @@ function add_trailing_slash($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
  *
  * Form
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 @@ class ConfigManager
         $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 92eb443b..823eb8de 100644
--- a/index.php
+++ b/index.php
@@ -1256,13 +1256,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')
             );
         }
     }
@@ -232,6 +241,25 @@ function reverse_space2nbsp($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 <code> content (already escaped by Parsedown).
  *
@@ -275,7 +303,7 @@ function sanitize_html($description)
  *
  * @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 @@
+<?php
+
+require_once 'application/Url.php';
+
+use Shaarli\Config\ConfigManager;
+
+/**
+ * Class WhitelistProtocolsTest
+ *
+ * Test whitelist_protocols() function of Url.
+ */
+class WhitelistProtocolsTest extends PHPUnit_Framework_TestCase
+{
+    /**
+     * Test whitelist_protocols() on a note (relative URL).
+     */
+    public function testWhitelistProtocolsRelative()
+    {
+        $whitelist = ['ftp', 'magnet'];
+        $url = '?12443564';
+        $this->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 @@ class PluginMarkdownTest extends PHPUnit_Framework_TestCase
     {
         PluginManager::$PLUGINS_PATH = 'plugins';
         $this->conf = new ConfigManager('tests/utils/config/configJson');
+        $this->conf->set('security.allowed_protocols', ['ftp', 'magnet']);
     }
 
     /**
@@ -183,15 +184,19 @@ class PluginMarkdownTest extends PHPUnit_Framework_TestCase
     }
 
     /**
-     * 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</code></pre>
 <p>Block:</p>
 <pre><code>lorem ipsum #foobar http://link.tld
-#foobar http://link.tld</code></pre></div>
\ No newline at end of file
+#foobar http://link.tld</code></pre>
+<p><a href="?123456">link</a><br />
+<img src="/img/train.png" alt="link" /><br />
+<a href="http://test.tld/path/?query=value#hash">link</a><br />
+<a href="http://test.tld/path/?query=value#hash">link</a><br />
+<a href="https://test.tld/path/?query=value#hash">link</a><br />
+<a href="ftp://test.tld/path/?query=value#hash">link</a><br />
+<a href="magnet:test.tld/path/?query=value#hash">link</a><br />
+<a href="http://alert('xss')">link</a><br />
+<a href="http://test.tld/path/?query=value#hash">link</a></p></div>
\ 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