Merge pull request #1639 from PrivateBin/urlshortener/url-vadility

Strengthen validation of URL in proxy services

.devcontainer/postCreateCommand.sh

@@ -1,9 +1,16 @@
 #!/bin/sh
 
 export PATH="$PATH:$HOME/.composer/vendor/bin"
+export PATH="$PATH:$PWD/vendor/bin"
+echo 'export PATH="$PATH:$HOME/.composer/vendor/bin"' >> ~/.bashrc
+echo 'export PATH="$PATH:$PWD/vendor/bin"' >> ~/.bashrc
 ln -s ./conf.sample.php cfg/conf.php
 composer install --no-dev --optimize-autoloader
 
+# for PHP unit testing
+# composer require google/cloud-storage
+# composer install --optimize-autoloader
+
 sudo chmod a+x "$(pwd)" && sudo rm -rf /var/www/html && sudo ln -s "$(pwd)" /var/www/html
 
 npm install --global nyc

.gitignore

@@ -25,6 +25,7 @@ vendor/**/tst
 vendor/**/tests
 vendor/**/build_phar.php
 !vendor/**/*.php
+vendor/bin/**
 
 # Ignore local node modules, unit testing logs, api docs and IDE project files
 js/node_modules/

lib/Proxy/AbstractProxy.php

@@ -49,7 +49,14 @@ abstract class AbstractProxy
      */
     public function __construct(Configuration $conf, string $link)
     {
-        if (!str_starts_with($link, $conf->getKey('basepath') . '?')) {
+        if (!filter_var($link, FILTER_VALIDATE_URL, FILTER_FLAG_PATH_REQUIRED | FILTER_FLAG_QUERY_REQUIRED)) {
+            $this->_error = 'Invalid URL given.';
+            return;
+        }
+
+        if (!str_starts_with($link, $conf->getKey('basepath') . '?') ||
+            parse_url($link, PHP_URL_HOST) != parse_url($conf->getKey('basepath'), PHP_URL_HOST)
+        ) {
             $this->_error = 'Trying to shorten a URL that isn\'t pointing at our instance.';
             return;
         }

tst/YourlsProxyTest.php

@@ -45,22 +45,76 @@ class YourlsProxyTest extends TestCase
         $yourls = new YourlsProxy($this->_conf, 'https://example.com/?foo#bar');
         $this->assertFalse($yourls->isError());
         $this->assertEquals($yourls->getUrl(), 'https://example.com/1');
+
+        $yourls = new YourlsProxy($this->_conf, 'https://example.com/?@foreign.malicious.example?foo#bar');
+        $this->assertFalse($yourls->isError());
+        $this->assertEquals($yourls->getUrl(), 'https://example.com/1');
+    }
+
+    /**
+     * @dataProvider providerInvalidUrl
+     */
+    public function testImvalidUrl($url): void
+    {
+        $yourls = new YourlsProxy($this->_conf, $url);
+        $this->assertTrue($yourls->isError());
+        $this->assertEquals($yourls->getError(), 'Invalid URL given.');
+    }
+
+    public function providerInvalidUrl(): array
+    {
+        return array(
+            array(''),
+            array(' '),
+            array('foo'),
+            array('https://'),
+            array('https://example.com'), // missing path and query parameter,
+            array('https://example.com/'), // missing query parameter
+            array('https://example.com?paste=something'), // missing path parameter
+            array('https://example.com@foreign.malicious.example?foo#bar'), // missing path parameter
+        );
     }
 
-    public function testForeignUrl()
+    /**
+     * This tests for a trick using username of an URI, see:
+     * {@see https://cloud.google.com/blog/topics/threat-intelligence/url-obfuscation-schema-abuse/?hl=en}
+     *
+     * @dataProvider providerForeignUrlUsernameTrick
+     */
+    public function testForeignUrlUsingUsernameTrick($url): void
     {
-        $yourls = new YourlsProxy($this->_conf, 'https://other.example.com/?foo#bar');
+        $yourls = new YourlsProxy($this->_conf, $url);
         $this->assertTrue($yourls->isError());
         $this->assertEquals($yourls->getError(), 'Trying to shorten a URL that isn\'t pointing at our instance.');
     }
 
-    public function testSneakyForeignUrl()
+    public function providerForeignUrlUsernameTrick(): array
+    {
+        return array(
+            array('https://example.com@foreign.malicious.example/?foo#bar'),
+            array('https://example.com/@foreign.malicious.example?foo#bar'),
+        );
+    }
+
+    /**
+     * @dataProvider providerForeignUrl
+     */
+    public function testForeignUrl($url): void
     {
-        $yourls = new YourlsProxy($this->_conf, 'https://other.example.com/?q=https://example.com/?foo#bar');
+        $yourls = new YourlsProxy($this->_conf, $url);
         $this->assertTrue($yourls->isError());
         $this->assertEquals($yourls->getError(), 'Trying to shorten a URL that isn\'t pointing at our instance.');
     }
 
+    public function providerForeignUrl(): array
+    {
+        return array(
+            array('ftp://example.com/?n=np'), // wrong protocol
+            array('https://other.example.com/?foo#bar'), // wrong domain
+            array('https://other.example.com/?q=https://example.com/?foo#bar'), // domain included inside string
+        );
+    }
+
     public function testYourlsError()
     {
         // when statusCode is not 200, shorturl may not have been set