Time attack protection on hmac comparison

This fixes issue 2.7 of https://defuse.ca/audits/zerobin.htm, and thus
(with commit a24212afda90ca3e4b4ff5ce30d2012709b58a28) also issue 2.8.

(cherry picked from commit 0b4db7ece313dd268e51fc47a0293a649927558a)

Conflicts:
	index.php

lib/filter.php

@@ -50,4 +50,35 @@ class filter
         }
         return number_format($size, ($i ? 2 : 0), '.', ' ') . ' ' . $iec[$i];
     }
+
+    /**
+     * validate paste ID
+     *
+     * @access public
+     * @param  string $dataid
+     * @return bool
+     */
+    public static function is_valid_paste_id($dataid)
+    {
+        return (bool) preg_match('#\A[a-f\d]{16}\z#', $dataid);
+    }
+
+    /**
+     * fixed time string comparison operation to prevent timing attacks
+     * https://crackstation.net/hashing-security.htm?=rd#slowequals
+     *
+     * @access public
+     * @param  string $a
+     * @param  string $b
+     * @return bool
+     */
+    public static function slow_equals($a, $b)
+    {
+        $diff = strlen($a) ^ strlen($b);
+        for($i = 0; $i < strlen($a) && $i < strlen($b); $i++)
+        {
+            $diff |= ord($a[$i]) ^ ord($b[$i]);
+        }
+        return $diff === 0;
+     }
 }

lib/zerobin.php

@@ -272,8 +272,8 @@ class zerobin
             $pasteid  = $_POST['pasteid'];
             $parentid = $_POST['parentid'];
             if (
-                !preg_match('#\A[a-f\d]{16}\z#', $pasteid) ||
+                !filter::is_valid_paste_id($pasteid) ||
-                !preg_match('#\A[a-f\d]{16}\z#', $parentid)
+                !filter::is_valid_paste_id($parentid)
             ) $this->_return_message(1, 'Invalid data.');
 
             // Comments do not expire (it's the paste that expires)
@@ -340,18 +340,21 @@ class zerobin
     private function _delete($dataid, $deletetoken)
     {
         // Is this a valid paste identifier?
-        if (preg_match('#\A[a-f\d]{16}\z#', $dataid))
+        if (!filter::is_valid_paste_id($dataid))
         {
-            // Check that paste exists.
+            $this->_error = 'Invalid paste ID.';
-            if (!$this->_model()->exists($dataid))
+            return;
-            {
+        }
-                $this->_error = 'Paste does not exist, has expired or has been deleted.';
+
-                return;
+        // Check that paste exists.
-            }
+        if (!$this->_model()->exists($dataid))
+        {
+            $this->_error = 'Paste does not exist, has expired or has been deleted.';
+            return;
         }
 
         // Make sure token is valid.
-        if ($deletetoken != hash_hmac('sha1', $dataid , serversalt::get()))
+        if (filter::slow_equals($deletetoken, hash_hmac('sha1', $dataid , serversalt::get())))
         {
             $this->_error = 'Wrong deletion token. Paste was not deleted.';
             return;
@@ -372,63 +375,62 @@ class zerobin
     private function _read($dataid)
     {
         // Is this a valid paste identifier?
-        if (preg_match('#\A[a-f\d]{16}\z#', $dataid))
+        if (!filter::is_valid_paste_id($dataid))
         {
-            // Check that paste exists.
+            $this->_error = 'Invalid paste ID.';
-            if ($this->_model()->exists($dataid))
+            return;
+        }
+
+        // Check that paste exists.
+        if ($this->_model()->exists($dataid))
+        {
+            // Get the paste itself.
+            $paste = $this->_model()->read($dataid);
+
+            // See if paste has expired.
+            if (
+                isset($paste->meta->expire_date) &&
+                $paste->meta->expire_date < time()
+            )
             {
-                // Get the paste itself.
+                // Delete the paste
-                $paste = $this->_model()->read($dataid);
+                $this->_model()->delete($dataid);
-
+                $this->_error = 'Paste does not exist, has expired or has been deleted.';
-                // See if paste has expired.
-                if (
-                    isset($paste->meta->expire_date) &&
-                    $paste->meta->expire_date < time()
-                )
-                {
-                    // Delete the paste
-                    $this->_model()->delete($dataid);
-                    $this->_error = 'Paste does not exist, has expired or has been deleted.';
-                }
-                // If no error, return the paste.
-                else
-                {
-                    // We kindly provide the remaining time before expiration (in seconds)
-                    if (
-                        property_exists($paste->meta, 'expire_date')
-                    ) $paste->meta->remaining_time = $paste->meta->expire_date - time();
-
-                    // The paste itself is the first in the list of encrypted messages.
-                    $messages = array($paste);
-
-                    // If it's a discussion, get all comments.
-                    if (
-                        property_exists($paste->meta, 'opendiscussion') &&
-                        $paste->meta->opendiscussion
-                    )
-                    {
-                        $messages = array_merge(
-                            $messages,
-                            $this->_model()->readComments($dataid)
-                        );
-                    }
-                    $this->_data = json_encode($messages);
-
-                    // If the paste was meant to be read only once, delete it.
-                    if (
-                        property_exists($paste->meta, 'burnafterreading') &&
-                        $paste->meta->burnafterreading
-                    ) $this->_model()->delete($dataid);
-                }
             }
+            // If no error, return the paste.
             else
             {
-                $this->_error = 'Paste does not exist or has expired.';
+                // We kindly provide the remaining time before expiration (in seconds)
+                if (
+                    property_exists($paste->meta, 'expire_date')
+                ) $paste->meta->remaining_time = $paste->meta->expire_date - time();
+
+                // The paste itself is the first in the list of encrypted messages.
+                $messages = array($paste);
+
+                // If it's a discussion, get all comments.
+                if (
+                    property_exists($paste->meta, 'opendiscussion') &&
+                    $paste->meta->opendiscussion
+                )
+                {
+                    $messages = array_merge(
+                        $messages,
+                        $this->_model()->readComments($dataid)
+                    );
+                }
+                $this->_data = json_encode($messages);
+
+                // If the paste was meant to be read only once, delete it.
+                if (
+                    property_exists($paste->meta, 'burnafterreading') &&
+                    $paste->meta->burnafterreading
+                ) $this->_model()->delete($dataid);
             }
         }
         else
         {
-            $this->_error = 'Invalid paste ID.';
+            $this->_error = 'Paste does not exist or has expired.';
         }
     }
 

tst/filter.php

@@ -44,4 +44,25 @@ class filterTest extends PHPUnit_Framework_TestCase
         $this->assertEquals('1.00 YiB', filter::size_humanreadable(1024 * $exponent));
         $this->assertEquals('1.21 YiB', filter::size_humanreadable(1234 * $exponent));
     }
+
+    public function testPasteIdValidation()
+    {
+        $this->assertTrue(filter::is_valid_paste_id('a242ab7bdfb2581a'), 'valid paste id');
+        $this->assertFalse(filter::is_valid_paste_id('foo'), 'invalid hex values');
+        $this->assertFalse(filter::is_valid_paste_id('../bar/baz'), 'path attack');
+    }
+
+    public function testSlowEquals()
+    {
+        $this->assertTrue(filter::slow_equals('foo', 'foo'), 'same string');
+        $this->assertFalse(filter::slow_equals('foo', true), 'string and boolean');
+        $this->assertFalse(filter::slow_equals('foo', 0), 'string and integer');
+        $this->assertFalse(filter::slow_equals('123foo', 123), 'string and integer');
+        $this->assertFalse(filter::slow_equals('123foo', '123'), 'different strings');
+        $this->assertFalse(filter::slow_equals('6', ' 6'), 'strings with space');
+        $this->assertFalse(filter::slow_equals('4.2', '4.20'), 'floats as strings');
+        $this->assertFalse(filter::slow_equals('1e3', '1000'), 'integers as strings');
+        $this->assertFalse(filter::slow_equals('9223372036854775807', '9223372036854775808'), 'large integers as strings');
+        $this->assertFalse(filter::slow_equals('61529519452809720693702583126814', '61529519452809720000000000000000'), 'larger integers as strings');
+    }
 }

tst/mcrypt_mock.php

@@ -0,0 +1,17 @@
+<?php
+
+define('MCRYPT_DEV_URANDOM', 1);
+
+function mcrypt_create_iv($int, $flag)
+{
+    $randomSalt = '';
+    for($i = 0; $i < 16; ++$i) {
+        $randomSalt .= base_convert(mt_rand(), 10, 16);
+    }
+    // hex2bin requires an even length, pad if necessary
+    if (strlen($randomSalt) % 2)
+    {
+        $randomSalt = '0' . $randomSalt;
+    }
+    return hex2bin($randomSalt);
+}

tst/phpunit.xml

@@ -1,6 +1,7 @@
 <phpunit bootstrap="bootstrap.php" colors="true">
 	<testsuite name="ZeroBin Test Suite">
 		<directory suffix=".php">./</directory>
+		<exclude>mcrypt_mock.php</exclude>
 	</testsuite>
 	<filter>
 		<whitelist>

tst/vizhash16x16.php

@@ -37,5 +37,10 @@ class vizhash16x16Test extends PHPUnit_Framework_TestCase
         $this->assertEquals('image/png', $finfo->file($this->_file));
         $this->assertNotEquals($pngdata, $vz->generate('2001:1620:2057:dead:beef::cafe:babe'));
         $this->assertEquals($pngdata, $vz->generate('127.0.0.1'));
+
+        // generating new salt
+        $salt = serversalt::get();
+        require 'mcrypt_mock.php';
+        $this->assertNotEquals($salt, serversalt::generate());
     }
 }