Repository: Spomky-Labs/otphp (php)

Issue:
Consider switching to paragonie/constant_time_encoding
| Q                    | A
| -------------------- | -----
| Bug report?          | no
| Feature request?     | yes
| BC Break report?     | no
| RFC? / Specification | no
| Library version      | 8.3.x, 9.x

The base 32 encoding library used by this project doesn't claim to protect against timing attacks.

It uses substr and strlen, which as per http://blog.ircmaxell.com/2014/11/its-all-about-time.html are not timing safe.

https://github.com/paragonie/constant_time_encoding is designed to prevent timing attacks

If you're interested in this change, could poke at it some.



Discussion:
Hi @larowlan,

This is a very interesting report. I will change both v8 and v9 branches in favor of this dependency.
As it is easier to bruteforce a n digits code, I am not sure there is a potential security issue at the moment, but that is a risk that I do not want to take.

Thanks.
Thanks, let me know if you want help, happy to work on it if you're in support of the change

Patch:
diff --git a/composer.json b/composer.json
--- a/composer.json
+++ b/composer.json
@@ -17,7 +17,7 @@
     ],
     "require": {
         "php": "^5.5|^7.0",
-        "christian-riesen/base32": "^1.1",
+        "paragonie/constant_time_encoding": "^1.0|^2.0",
         "beberlei/assert": "^2.4",
         "symfony/polyfill-mbstring": "^1.1",
         "symfony/polyfill-php56": "^1.1",
diff --git a/src/OTP.php b/src/OTP.php
--- a/src/OTP.php
+++ b/src/OTP.php
@@ -12,7 +12,7 @@
 namespace OTPHP;
 
 use Assert\Assertion;
-use Base32\Base32;
+use ParagonIE\ConstantTime\Base32;
 
 abstract class OTP implements OTPInterface
 {
@@ -108,7 +108,7 @@ protected function generateURI($type, array $options)
      */
     private function getDecodedSecret()
     {
-        $secret = Base32::decode($this->getSecret());
+        $secret = Base32::decodeUpper($this->getSecret());
 
         return $secret;
     }
diff --git a/src/ParameterTrait.php b/src/ParameterTrait.php
--- a/src/ParameterTrait.php
+++ b/src/ParameterTrait.php
@@ -12,7 +12,7 @@
 namespace OTPHP;
 
 use Assert\Assertion;
-use Base32\Base32;
+use ParagonIE\ConstantTime\Base32;
 
 trait ParameterTrait
 {
@@ -65,7 +65,7 @@ private function setSecret($secret)
     {
         Assertion::nullOrString($secret, 'The secret must be a string or null.');
         if (null === $secret) {
-            $secret = trim(Base32::encode(random_bytes(32)), '=');
+            $secret = trim(Base32::encodeUpper(random_bytes(32)), '=');
         }
 
         $this->parameters['secret'] = $secret;


Test patch:
diff --git a/tests/TOTPTest.php b/tests/TOTPTest.php
--- a/tests/TOTPTest.php
+++ b/tests/TOTPTest.php
@@ -9,8 +9,8 @@
  * of the MIT license.  See the LICENSE file for details.
  */
 
-use Base32\Base32;
 use OTPHP\TOTP;
+use ParagonIE\ConstantTime\Base32;
 
 class TOTPTest extends \PHPUnit_Framework_TestCase
 {
@@ -150,9 +150,9 @@ public function testVectors($totp, $timestamp, $expected_value)
      */
     public function testVectorsData()
     {
-        $totp_sha1 = $this->createTOTP(8, 'sha1', 30, Base32::encode('12345678901234567890'));
-        $totp_sha256 = $this->createTOTP(8, 'sha256', 30, Base32::encode('12345678901234567890123456789012'));
-        $totp_sha512 = $this->createTOTP(8, 'sha512', 30, Base32::encode('1234567890123456789012345678901234567890123456789012345678901234'));
+        $totp_sha1 = $this->createTOTP(8, 'sha1', 30, Base32::encodeUpper('12345678901234567890'));
+        $totp_sha256 = $this->createTOTP(8, 'sha256', 30, Base32::encodeUpper('12345678901234567890123456789012'));
+        $totp_sha512 = $this->createTOTP(8, 'sha512', 30, Base32::encodeUpper('1234567890123456789012345678901234567890123456789012345678901234'));
 
         return [
             [$totp_sha1,   59, '94287082'],