Browse Source

Check if password hasher is called exactly once in all cases.

* This prevents timing side-channels.
* Test DigestAuthenticate to ensure password hasher is never used on password-less authenticators.

Backport of changes in #15168 to 3.x
Edgaras Janušauskas 6 years ago
parent
commit
33d09b03e5

+ 10 - 0
src/Auth/BaseAuthenticate.php

@@ -131,6 +131,16 @@ abstract class BaseAuthenticate implements EventListenerInterface
         if ($password !== null) {
             $hasher = $this->passwordHasher();
             $hashedPassword = $result->get($passwordField);
+
+            if ($hashedPassword === null || $hashedPassword === '') {
+                // Waste time hashing the password, to prevent
+                // timing side-channels to distinguish whether
+                // user has password or not.
+                $hasher->hash($password);
+
+                return false;
+            }
+
             if (!$hasher->check($password, $hashedPassword)) {
                 return false;
             }

+ 8 - 3
tests/TestCase/Auth/DigestAuthenticateTest.php

@@ -25,6 +25,7 @@ use Cake\Http\ServerRequest;
 use Cake\I18n\Time;
 use Cake\ORM\Entity;
 use Cake\TestSuite\TestCase;
+use Cake\Utility\Security;
 
 /**
  * Entity for testing with hidden fields.
@@ -61,6 +62,8 @@ class DigestAuthenticateTest extends TestCase
             'realm' => 'localhost',
             'nonce' => 123,
             'opaque' => '123abc',
+            'secret' => Security::getSalt(),
+            'passwordHasher' => 'ShouldNeverTryToUsePasswordHasher',
         ]);
 
         $password = DigestAuthenticate::password('mariano', 'cake', 'localhost');
@@ -110,8 +113,6 @@ class DigestAuthenticateTest extends TestCase
      */
     public function testAuthenticateWrongUsername()
     {
-        $this->expectException(\Cake\Http\Exception\UnauthorizedException::class);
-        $this->expectExceptionCode(401);
         $request = new ServerRequest(['url' => 'posts/index']);
 
         $data = [
@@ -126,6 +127,10 @@ class DigestAuthenticateTest extends TestCase
         $data['response'] = $this->auth->generateResponseHash($data, '09faa9931501bf30f0d4253fa7763022', 'GET');
         $request = $request->withEnv('PHP_AUTH_DIGEST', $this->digestHeader($data));
 
+        $this->assertFalse($this->auth->authenticate($request, new Response()));
+
+        $this->expectException(UnauthorizedException::class);
+        $this->expectExceptionCode(401);
         $this->auth->unauthenticated($request, $this->response);
     }
 
@@ -525,7 +530,7 @@ DIGEST;
             'opaque' => '123abc',
         ];
         $digest = <<<DIGEST
-Digest username="mariano",
+Digest username="{$data['username']}",
 realm="{$data['realm']}",
 nonce="{$data['nonce']}",
 uri="{$data['uri']}",

+ 47 - 0
tests/TestCase/Auth/FormAuthenticateTest.php

@@ -23,6 +23,7 @@ use Cake\I18n\Time;
 use Cake\ORM\Entity;
 use Cake\TestSuite\TestCase;
 use Cake\Utility\Security;
+use TestApp\Auth\CallCounterPasswordHasher;
 
 /**
  * Test case for FormAuthentication
@@ -481,4 +482,50 @@ class FormAuthenticateTest extends TestCase
         $this->assertNotEmpty($result);
         $this->assertTrue($this->auth->needsPasswordRehash());
     }
+
+    /**
+     * Tests that password hasher function is called exactly once in all cases.
+     *
+     * @param string $username
+     * @param string|null $password
+     * @return void
+     * @dataProvider userList
+     */
+    public function testAuthenticateSingleHash(string $username, ?string $password): void
+    {
+        $this->auth = new FormAuthenticate($this->Collection, [
+            'userModel' => 'Users',
+            'passwordHasher' => CallCounterPasswordHasher::class,
+        ]);
+        $this->getTableLocator()->get('Users')->updateAll(
+            ['password' => $password],
+            ['username' => $username]
+        );
+
+        $request = new ServerRequest([
+            'url' => 'posts/index',
+            'post' => [
+                'username' => $username,
+                'password' => 'anything',
+            ],
+        ]);
+        $result = $this->auth->authenticate($request, new Response());
+        $this->assertFalse($result);
+
+        /** @var \TestApp\Auth\CallCounterPasswordHasher $passwordHasher */
+        $passwordHasher = $this->auth->passwordHasher();
+
+        $this->assertInstanceOf(CallCounterPasswordHasher::class, $passwordHasher);
+        $this->assertSame(1, $passwordHasher->callCount);
+    }
+
+    public function userList()
+    {
+        return [
+            ['notexist', ''],
+            ['mariano', null],
+            ['mariano', ''],
+            ['mariano', 'somehash'],
+        ];
+    }
 }

+ 36 - 0
tests/test_app/TestApp/Auth/CallCounterPasswordHasher.php

@@ -0,0 +1,36 @@
+<?php
+declare(strict_types=1);
+
+namespace TestApp\Auth;
+
+use Cake\Auth\AbstractPasswordHasher;
+use InvalidArgumentException;
+
+class CallCounterPasswordHasher extends AbstractPasswordHasher
+{
+    public $callCount = 0;
+
+    /**
+     * @inheritDoc
+     */
+    public function hash($password)
+    {
+        $this->callCount++;
+
+        return 'hash123';
+    }
+
+    /**
+     * @inheritDoc
+     */
+    public function check($password, $hashedPassword)
+    {
+        if ($hashedPassword == null || $hashedPassword === '') {
+            throw new InvalidArgumentException('Empty hash not expected');
+        }
+
+        $this->callCount++;
+
+        return false;
+    }
+}