Merge pull request #33 from fkooman/fix-hash-algos

fix sha256 and sha512
ChristianRiesen · Jan 25, 2019 · 67d3351 · 67d3351
2 parents 7304237 + b23bfe5
commit 67d3351
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 52 deletions.
diff --git a/README.md b/README.md
@@ -12,11 +12,11 @@ Installation
 
 Use [composer](http://getcomposer.org/) and require the library in your `composer.json`
 
-	{
-    	"require": {
-        	"christian-riesen/otp": "^2.0",
-    	}
-	}
+    {
+        "require": {
+            "christian-riesen/otp": "^2.0",
+        }
+    }
 
 Usage
 -----
@@ -66,7 +66,7 @@ Sample script in `example` folder. Requires sessions to work (for secret storage
 Class Otp
 ---------
 
-Implements hotp according to [RFC4226](https://tools.ietf.org/html/rfc4226) and totp according to [RFC6238](https://tools.ietf.org/html/rfc6238) (only sha1 algorithm). Once you have a secret, you can use it directly in this class to create the passwords themselves (mainly for debugging use) or use the check functions to safely check the validity of the keys. The `checkTotp` function also includes a helper to battle timedrift.
+Implements hotp according to [RFC4226](https://tools.ietf.org/html/rfc4226) and totp according to [RFC6238](https://tools.ietf.org/html/rfc6238) (only sha1, sha256 and sha512 algorithms). Once you have a secret, you can use it directly in this class to create the passwords themselves (mainly for debugging use) or use the check functions to safely check the validity of the keys. The `checkTotp` function also includes a helper to battle timedrift.
 
 Class GoogleAuthenticator
 -------------------------
@@ -81,11 +81,11 @@ About
 Requirements
 ------------
 
-PHP 5.4.x+
+PHP >= 5.4.0
 
 Uses [paragonie/random_compat](https://github.com/paragonie/random_compat) and [paragonie/constant_time_encoding](https://github.com/paragonie/constant_time_encoding).
 
-If you want to run the tests, PHPUnit 3.6 or up is required.
+If you want to run the tests, PHPUnit >= 4.8.35 is required.
 
 Author
 ------

diff --git a/src/Otp.php b/src/Otp.php
@@ -174,14 +174,6 @@ public function checkTotp($secret, $key, $timedrift = 1)
      * @throws \InvalidArgumentException
      * @return \Otp\Otp
      */
-
-    /*
-     * This has been disabled since it does not bring the expected results
-     * according to the RFC test vectors for sha256 or sha512.
-     * Until that is fixed, the algorithm simply stays at sha1.
-     * Google Authenticator does not support sha256 and sha512 at the moment.
-     *
-    
     public function setAlgorithm($algorithm)
     {
         if (!in_array($algorithm, $this->allowedAlgorithms)) {
@@ -192,7 +184,6 @@ public function setAlgorithm($algorithm)
 
         return $this;
     }
-    // */
 
     /**
      * Get the algorithms name (lowercase)
@@ -330,7 +321,7 @@ private function getTimecounter()
      */
     private function truncate($hash)
     {
-        $offset = ord($hash[19]) & 0xf;
+        $offset = ord($hash[strlen($hash)-1]) & 0xf;
 
         return (
             ((ord($hash[$offset+0]) & 0x7f) << 24 ) |

diff --git a/tests/OtpTest.php b/tests/OtpTest.php
@@ -17,7 +17,7 @@ class OtpTest extends TestCase
 	private $Otp;
 
 	private $secret = "12345678901234567890";
-	
+
 	/**
 	 * Prepares the environment before running a test.
 	 */
@@ -57,10 +57,31 @@ public function hotpTestValues()
 	 */
 	public function totpTestValues()
 	{
+		// https://www.rfc-editor.org/errata_search.php?rfc=6238
+		$secretSha1   = '12345678901234567890';
+		$secretSha256 = '12345678901234567890123456789012';
+		$secretSha512 = '1234567890123456789012345678901234567890123456789012345678901234';
+
 		return [
-				 ['94287082', 59], ['07081804', 1111111109], ['14050471', 1111111111],
-				 ['89005924', 1234567890], ['69279037', 2000000000], ['65353130', 20000000000],
-			   ];
+			['sha1',   $secretSha1,   '94287082', 59],
+			['sha1',   $secretSha1,   '07081804', 1111111109],
+			['sha1',   $secretSha1,   '14050471', 1111111111],
+			['sha1',   $secretSha1,   '89005924', 1234567890],
+			['sha1',   $secretSha1,   '69279037', 2000000000],
+			['sha1',   $secretSha1,   '65353130', 20000000000],
+			['sha256', $secretSha256, '46119246', 59],
+			['sha256', $secretSha256, '68084774', 1111111109],
+			['sha256', $secretSha256, '67062674', 1111111111],
+			['sha256', $secretSha256, '91819424', 1234567890],
+			['sha256', $secretSha256, '90698825', 2000000000],
+			['sha256', $secretSha256, '77737706', 20000000000],
+			['sha512', $secretSha512, '90693936', 59],
+			['sha512', $secretSha512, '25091201', 1111111109],
+			['sha512', $secretSha512, '99943326', 1111111111],
+			['sha512', $secretSha512, '93441116', 1234567890],
+			['sha512', $secretSha512, '38618901', 2000000000],
+			['sha512', $secretSha512, '47863826', 20000000000],
+		];
 	}
 
 	/**
@@ -129,43 +150,16 @@ public function testHotpRfc($key, $counter)
 	 *
 	 * @dataProvider totpTestValues
 	 */
-	public function testTotpRfc($key, $time)
+	public function testTotpRfc($algo, $secret, $key, $time)
 	{
-		$secret = $this->secret;
-
 		// Test vectors are in 8 digits
 		$this->Otp->setDigits(8);
 
 		// The time presented in the test vector has to be first divided through 30
 		// to count as the key
 
-		// SHA 1 grouping
-		$this->assertEquals($key, $this->Otp->hotp($secret, floor($time/30)), "sha 1 with $time");
-
-
-		/*
-		The following tests do NOT pass.
-		Once the otp class can deal with these correctly, they can be used again.
-		They are here for completeness test vectors from the RFC.
-		
-		// SHA 256 grouping
-		$this->Otp->setAlgorithm('sha256');
-		$this->assertEquals('46119246', $this->Otp->hotp($secret,          floor(59/30)), 'sha256 with time 59');
-		$this->assertEquals('07081804', $this->Otp->hotp($secret,  floor(1111111109/30)), 'sha256 with time 1111111109');
-		$this->assertEquals('14050471', $this->Otp->hotp($secret,  floor(1111111111/30)), 'sha256 with time 1111111111');
-		$this->assertEquals('89005924', $this->Otp->hotp($secret,  floor(1234567890/30)), 'sha256 with time 1234567890');
-		$this->assertEquals('69279037', $this->Otp->hotp($secret,  floor(2000000000/30)), 'sha256 with time 2000000000');
-		$this->assertEquals('65353130', $this->Otp->hotp($secret, floor(20000000000/30)), 'sha256 with time 20000000000');
-		
-		// SHA 512 grouping
-		$this->Otp->setAlgorithm('sha512');
-		$this->assertEquals('90693936', $this->Otp->hotp($secret,          floor(59/30)), 'sha512 with time 59');
-		$this->assertEquals('25091201', $this->Otp->hotp($secret,  floor(1111111109/30)), 'sha512 with time 1111111109');
-		$this->assertEquals('99943326', $this->Otp->hotp($secret,  floor(1111111111/30)), 'sha512 with time 1111111111');
-		$this->assertEquals('93441116', $this->Otp->hotp($secret,  floor(1234567890/30)), 'sha512 with time 1234567890');
-		$this->assertEquals('38618901', $this->Otp->hotp($secret,  floor(2000000000/30)), 'sha512 with time 2000000000');
-		$this->assertEquals('47863826', $this->Otp->hotp($secret, floor(20000000000/30)), 'sha512 with time 20000000000');
-		*/
+		$this->Otp->setAlgorithm($algo);
+		$this->assertEquals($key, $this->Otp->hotp($secret, floor($time/30)), "$algo with $time");
 	}
 
 	/**