Merge pull request #98 from moonmoon/anti-csrf

Implement a mitigation against CSRF attacks

12 files changed, 203 insertions, 9 deletions

diff --git a/admin/administration.php b/admin/administration.php
index 34afe73..26f6710 100755
--- a/admin/administration.php
+++ b/admin/administration.php
@@ -24,6 +24,7 @@ $page_content = <<<"FRAGMENT"
             <div class="widget">
                 <h3>{$l10n->getString('Clear cache')}</h3>
                 <form action="purgecache.php" method="post" id="frmPurge">
+                    <input type="hidden" value="{$csrf->generate('frmPurge')}" name="_csrf">
                     <p><label>{$l10n->getString('Clear cache:')}</label><input type="submit" class="submit delete" name="purge" id="purge" value="{$l10n->getString('Clear')}" /></p>
                     <p class="help">{$l10n->getString('Clearing the cache will make moonmoon reload all feeds.')}</p>
                 </form>
@@ -32,6 +33,7 @@ $page_content = <<<"FRAGMENT"
             <div class="widget">
                 <h3>{$l10n->getString('Change administrator password')}</h3>
                 <form action="changepassword.php" method="post" id="frmPassword">
+                    <input type="hidden" value="{$csrf->generate('frmPassword')}" name="_csrf">
                     <p><label for="password">{$l10n->getString('New password:')}</label> <input type="password" class="text" value="" name="password" id="password" size="20" /> <input type="submit" class="submit delete" name="changepwd" id="changepwd" value="{$l10n->getString('Change password')}" /></p>
                 </form>
             </div>
diff --git a/admin/changepassword.php b/admin/changepassword.php
index 8c38769..3b4500e 100644
--- a/admin/changepassword.php
+++ b/admin/changepassword.php
@@ -1,7 +1,9 @@
 <?php
+
+require_once __DIR__.'/../app/app.php';
 require_once __DIR__.'/inc/auth.inc.php';
 
-if (isset($_POST['password']) && ('' != $_POST['password'])){
+if ($csrf->verify($_POST['_csrf'], 'frmPassword') && isset($_POST['password']) && ('' != $_POST['password'])) {
     $out = '<?php $login="admin"; $password="'.md5($_POST['password']).'"; ?>';
     file_put_contents(__DIR__.'/inc/pwd.inc.php', $out);
     die("Password changed. <a href='administration.php'>Login</a>");
diff --git a/admin/index.php b/admin/index.php
index a01b77b..0118923 100755
--- a/admin/index.php
+++ b/admin/index.php
@@ -79,6 +79,7 @@ ob_start();
                         <input type="submit" class="submit add" name="add" value="<?=_g('Add Feed')?>" />
                     </fieldset>
                     <p class="help"><?=_g('Accepted formats are RSS and ATOM. If the link is not a feed, moonmoon will try to autodiscover the feed.')?></p>
+                <input type="hidden" value="<?php echo $csrf->generate('feedmanage'); ?>" name="_csrf">
                 </form>
             </div>
 
@@ -87,6 +88,7 @@ ob_start();
                 <form action="subscriptions.php" method="post" id="feedmanage">
                 <p class="action">
                 <span class="count"><?php echo sprintf(_g('Number of feeds: %s'), $count_feeds)?></span>
+                <input type="hidden" value="<?php echo $csrf->generate('feedmanage'); ?>" name="_csrf">
                 <input type="submit" class="submit save" name="save" id="save" value="<?=_g('Save changes')?>" />
                 <input type="submit" class="submit delete" name="delete" id="delete" value="<?=_g('Delete selected Feeds')?>" />
                 </p>
diff --git a/admin/login.php b/admin/login.php
index 3ba4d2b..a95e59f 100755
--- a/admin/login.php
+++ b/admin/login.php
@@ -1,10 +1,13 @@
 <?php
+
+require_once __DIR__ . '/../app/app.php';
+
 if (isset($_POST['password'])) {
+    session_regenerate_id();
     setcookie('auth',md5($_POST['password']));
     header('Location: index.php');
 }
 
-require_once __DIR__ . '/../app/app.php';
 $page_content = <<<FRAGMENT
             <form action="" method="post" class="login">
                 <fieldset>
diff --git a/admin/logout.php b/admin/logout.php
index 6dd32aa..adb843f 100644
--- a/admin/logout.php
+++ b/admin/logout.php
@@ -1,5 +1,10 @@
 <?php
+
+require_once __DIR__ . '/../app/app.php';
+
 setcookie('auth','', time()-3600);
+session_destroy();
+session_regenerate_id();
+
 header('Location: login.php');
-die;
-?>
\ No newline at end of file
+die();
diff --git a/admin/subscriptions.php b/admin/subscriptions.php
index f63af8f..7b2fb6f 100755
--- a/admin/subscriptions.php
+++ b/admin/subscriptions.php
@@ -7,6 +7,10 @@ function removeSlashes(&$item, $key){
     $item = stripslashes($item);
 }
 
+if (!$csrf->verify($_POST['_csrf'], 'feedmanage')) {
+    die('Invalid CSRF token!');
+}
+
 if (isset($_POST['opml']) || isset($_POST['add'])) {
 
     // Load old OPML
diff --git a/app/app.php b/app/app.php
index a6232cf..0797cc7 100755
--- a/app/app.php
+++ b/app/app.php
@@ -7,6 +7,8 @@ require_once __DIR__.'/../vendor/autoload.php';
 $savedConfig  = __DIR__.'/../custom/config.yml';
 $moon_version = file_get_contents(__DIR__.'/../VERSION');
 
+session_start();
+
 if (is_installed()) {
     $conf = Spyc::YAMLLoad($savedConfig);
 
@@ -27,4 +29,4 @@ if (is_installed()) {
 }
 
 $l10n = new Simplel10n($conf['locale']);
-
+$csrf = new CSRF();
diff --git a/app/classes/CSRF.php b/app/classes/CSRF.php
new file mode 100644
index 0000000..639f573
--- /dev/null
+++ b/app/classes/CSRF.php
@@ -0,0 +1,55 @@
+<?php
+
+class CSRF
+{
+    /** @var string */
+    const HMAC_ALGORITHM = 'sha1';
+
+    /** @var string */
+    const SESSION_KEY_NAME = '_csrf_key';
+
+    /**
+     * Ensure that a CSRF token is valid for a given action.
+     *
+     * @param  string $token
+     * @param  string $action
+     * @return bool
+     */
+    public static function verify($token = '', $action = null)
+    {
+        if (!is_string($token) || !is_string($action)) {
+            return false;
+        }
+
+        $known = self::generate($action);
+        return hash_equals($known, $token);
+    }
+
+    /**
+     * Generate a CSRF token for a given action.
+     *
+     * @param  string $action
+     * @throws InvalidArgumentException
+     * @return string
+     */
+    public static function generate($action = null)
+    {
+        if (!is_string($action)) {
+            throw InvalidArgumentException('A valid action must be defined.');
+        }
+        return hash_hmac(self::HMAC_ALGORITHM, $action, self::getKey());
+    }
+
+    /**
+     * Get HMAC key.
+     *
+     * @return string
+     */
+    public static function getKey()
+    {
+        if (empty($_SESSION[self::SESSION_KEY_NAME])) {
+            $_SESSION[self::SESSION_KEY_NAME] = random_bytes(16);
+        }
+        return $_SESSION[self::SESSION_KEY_NAME];
+    }
+}
diff --git a/app/helpers.php b/app/helpers.php
index 3bce65f..01d0086 100644
--- a/app/helpers.php
+++ b/app/helpers.php
@@ -1,6 +1,29 @@
 <?php
 
 /**
+ * Register polyfills for old PHP versions.
+ * 
+ * This way, the real function will only be called if it
+ * is available, and we won't force the use of our own 
+ * implementation.
+ */
+function register_polyfills()
+{
+    if (!function_exists('hash_equals')) {
+        function hash_equals($known_string, $user_string) {
+            call_user_func_array('_hash_equals', func_get_args());
+        }
+    }
+
+    if (!function_exists('random_bytes')) {
+        // If this function does not exist, it will be exposed
+        // automatically by paragonie/random_compat.
+    }
+}
+
+register_polyfills();
+
+/**
  * Path to the _custom_ directory.
  *
  * @param  string $file Append this filename to the returned path.
@@ -72,4 +95,36 @@ function removeCustomFiles()
             unlink($path);
         }
     }
-}
\ No newline at end of file
+}
+
+/**
+ * Compare two strings in a constant-time manner.
+ *
+ * It returns `true` if both strings are exactly the same
+ * (same size and same value).
+ *
+ * @param  string $known_string
+ * @param  string $user_string
+ * @return bool
+ */
+function _hash_equals($known_string = '', $user_string = '')
+{
+    // In our case, it's not problematic if `$known_string`'s 
+    // size leaks, we will only compare password hashes and 
+    // CSRF tokens—their size is already somehow public.
+    if (!is_string($known_string) || !is_string($user_string) 
+         || strlen($known_string) !== strlen($user_string)) {
+        return false;
+    }
+
+    $ret = 0;    
+
+    // Do not stop the comparison when a difference is found,
+    // always completely compare them.
+    for ($i = 0; $i < strlen($known_string); $i++) {
+        $ret |=  (ord($known_string[$i]) ^ ord($user_string[$i]));
+    }   
+
+    return !$ret;
+}
+
diff --git a/composer.json b/composer.json
index c43b48e..ea5032d 100644
--- a/composer.json
+++ b/composer.json
@@ -14,7 +14,8 @@
     "require": {
         "php": "^5.6 || ^7.0",
         "mustangostang/spyc": "0.5.1",
-        "simplepie/simplepie": "^1.5"
+        "simplepie/simplepie": "^1.5",
+        "paragonie/random_compat": "^2.0"
     },
     "require-dev": {
         "guzzlehttp/guzzle": "^6.3",
diff --git a/composer.lock b/composer.lock
index 1d3cf2e..7959c9c 100644
--- a/composer.lock
+++ b/composer.lock
@@ -4,7 +4,7 @@
         "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#composer-lock-the-lock-file",
         "This file is @generated automatically"
     ],
-    "content-hash": "381ab1da48dd363669e218a50e19f0b7",
+    "content-hash": "a4fc919a4e8ef2463ff4a336940bc993",
     "packages": [
         {
             "name": "mustangostang/spyc",
@@ -54,6 +54,54 @@
             "time": "2013-02-21T10:52:01+00:00"
         },
         {
+            "name": "paragonie/random_compat",
+            "version": "v2.0.11",
+            "source": {
+                "type": "git",
+                "url": "https://github.com/paragonie/random_compat.git",
+                "reference": "5da4d3c796c275c55f057af5a643ae297d96b4d8"
+            },
+            "dist": {
+                "type": "zip",
+                "url": "https://api.github.com/repos/paragonie/random_compat/zipball/5da4d3c796c275c55f057af5a643ae297d96b4d8",
+                "reference": "5da4d3c796c275c55f057af5a643ae297d96b4d8",
+                "shasum": ""
+            },
+            "require": {
+                "php": ">=5.2.0"
+            },
+            "require-dev": {
+                "phpunit/phpunit": "4.*|5.*"
+            },
+            "suggest": {
+                "ext-libsodium": "Provides a modern crypto API that can be used to generate random bytes."
+            },
+            "type": "library",
+            "autoload": {
+                "files": [
+                    "lib/random.php"
+                ]
+            },
+            "notification-url": "https://packagist.org/downloads/",
+            "license": [
+                "MIT"
+            ],
+            "authors": [
+                {
+                    "name": "Paragon Initiative Enterprises",
+                    "email": "security@paragonie.com",
+                    "homepage": "https://paragonie.com"
+                }
+            ],
+            "description": "PHP 5.x polyfill for random_bytes() and random_int() from PHP 7",
+            "keywords": [
+                "csprng",
+                "pseudorandom",
+                "random"
+            ],
+            "time": "2017-09-27T21:40:39+00:00"
+        },
+        {
             "name": "simplepie/simplepie",
             "version": "1.5",
             "source": {
@@ -1543,7 +1591,7 @@
     "minimum-stability": "stable",
     "stability-flags": [],
     "prefer-stable": false,
-    "prefer-lowest": true,
+    "prefer-lowest": false,
     "platform": {
         "php": "^5.6 || ^7.0"
     },
diff --git a/tests/HelpersTest.php b/tests/HelpersTest.php
new file mode 100644
index 0000000..141e604
--- /dev/null
+++ b/tests/HelpersTest.php
@@ -0,0 +1,15 @@
+<?php
+
+use PHPUnit\Framework\TestCase;
+
+class HelpersTest extends TestCase
+{
+    function test_constant_time_compare()
+    {
+        $this->assertTrue(_hash_equals('abc', 'abc'));
+        $this->assertFalse(_hash_equals('abc', 'ab'));
+        $this->assertFalse(_hash_equals('ab', 'abc'));
+        $this->assertFalse(_hash_equals('abcd', 'adbc'));
+        $this->assertFalse(_hash_equals(0, 0));
+    }
+}