Fix HMAC CSRF token payload

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## 2.2.4 under development
 
+- Bug #32: Stop exposing CSRF HMAC token identity in token payload (@samdark)
 - Enh #82: Explicitly import classes and functions in "use" section (@mspirkov)
 - Enh #83: Remove unnecessary files from Composer package (@mspirkov)
 

README.md

@@ -144,8 +144,8 @@ return [
 In case Yii framework is used along with config plugin, the package is [configured](./config/di-web.php)
 automatically to use synchronizer token and masked decorator. You can change that depending on your needs.
 
-Use synchronizer token for sensitive anonymous forms; use HMAC token for authenticated-only forms when a submitted
-token may stay valid for a few minutes.
+Use synchronizer token for sensitive anonymous forms; use HMAC token for authenticated-only forms when it is acceptable
+for a submitted token to stay valid until it expires.
 
 ```mermaid
 flowchart TD
@@ -168,7 +168,7 @@ Detailed comparison:
 | File based session GC | May scan session files | Not triggered by CSRF token storage |
 | Token storage growth | Depends on session storage | Nothing to store |
 | Token revocation | Possible by removing stored token | Not possible before token expiration |
-| Replay within lifetime | Prevented by storage policy | Possible until the token expires |
+| Replay within lifetime | Prevented by storage policy | Possible until expiration |
 
 To switch token to HMAC:
 
@@ -205,12 +205,12 @@ Package provides `RandomCsrfTokenGenerator` that generates a random token and
 To learn more about the synchronizer token pattern,
 [check OWASP CSRF cheat sheet](https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#synchronizer-token-pattern).
 
-### HMAC based token
+### HMAC signed token
 
-HMAC based token is a stateless CSRF token that does not require any storage. The token is a hash from session ID and
-a timestamp used to prevent replay attacks. The token is added to a form. When the form is submitted, we re-generate
-the token from the current session ID and a timestamp from the original token. If two hashes match, we check that the
-timestamp is less than the token lifetime.
+HMAC signed token is a stateless CSRF token that does not require any storage. The token contains expiration timestamp
+and random value, and its signature is bound to the current session ID. The token is added to a form. When the form is
+submitted, we verify the token signature, check that it belongs to the current session ID, and check that it has not
+expired.
 
 `HmacCsrfToken` requires implementation of `CsrfTokenIdentityGeneratorInterface` for generating an identity.
 The package provides `SessionCsrfTokenIdentityGenerator` that is using session ID thus making the session a token scope.
@@ -235,8 +235,8 @@ return [
 ];
 ```
 
-To learn more about HMAC based token pattern
-[check OWASP CSRF cheat sheet](https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#hmac-based-token-pattern).
+To learn more about employing HMAC CSRF tokens, check the
+[OWASP CSRF cheat sheet](https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#employing-hmac-csrf-tokens).
 
 ### Stub CSRF token
 

src/Hmac/HmacCsrfToken.php

@@ -4,37 +4,46 @@
 
 namespace Yiisoft\Csrf\Hmac;
 
+use InvalidArgumentException;
+use RuntimeException;
 use Yiisoft\Csrf\CsrfTokenInterface;
 use Yiisoft\Csrf\Hmac\IdentityGenerator\CsrfTokenIdentityGeneratorInterface;
-use Yiisoft\Security\DataIsTamperedException;
-use Yiisoft\Security\Mac;
-use Yiisoft\Strings\StringHelper;
 use Yiisoft\Csrf\MaskedCsrfToken;
+use Yiisoft\Security\Random;
+use Yiisoft\Strings\StringHelper;
 
 use function count;
+use function hash_equals;
+use function hash_hmac;
 
 /**
- * Stateless CSRF token that does not require any storage. The token is a hash from session ID and a timestamp
- * (to prevent replay attacks). It is added to forms. When the form is submitted, we re-generate the token from
- * the current session ID and a timestamp from the original token. If two hashes match, we check that timestamp is
- * less than {@see HmacCsrfToken::$lifetime}.
- *
- * The algorithm is also known as "HMAC Based Token".
+ * Stateless CSRF token that does not require any storage. The token contains expiration timestamp and random value,
+ * and is signed with a session-bound identity. It is added to forms. When the form is submitted, we verify the token
+ * signature, check that it belongs to the current session identity, and check that it has not expired.
  *
  * Do not forget to decorate the token with {@see MaskedCsrfToken} to prevent BREACH attack.
  *
- * @link https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#hmac-based-token-pattern
+ * @link https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#employing-hmac-csrf-tokens
  */
 final class HmacCsrfToken implements CsrfTokenInterface
 {
     private CsrfTokenIdentityGeneratorInterface $identityGenerator;
-    private Mac $mac;
 
     /**
      * @var string Shared secret key used to generate the hash.
      */
     private string $secretKey;
 
+    /**
+     * @var string Hash algorithm for message authentication.
+     */
+    private string $algorithm;
+
+    /**
+     * @var int Hash length in bytes.
+     */
+    private int $hashLength;
+
     /**
      * @var int|null Number of seconds that the token is valid for.
      */
@@ -47,8 +56,9 @@
         ?int $lifetime = null
     ) {
         $this->identityGenerator = $identityGenerator;
-        $this->mac = new Mac($algorithm);
         $this->secretKey = $secretKey;
+        $this->algorithm = $algorithm;
+        $this->hashLength = $this->generateHashLength();
         $this->lifetime = $lifetime;
     }
 
@@ -66,41 +76,47 @@
             return false;
         }
 
-        [$expiration, $identity] = $data;
+        [$expiration, $payload] = $data;
 
-        if ($expiration !== null && time() > $expiration) {
+        $hash = StringHelper::byteSubstring($payload, 0, $this->hashLength);
+        $message = StringHelper::byteSubstring($payload, $this->hashLength, null);
+
+        if (!hash_equals($hash, $this->generateHash($message))) {
             return false;
         }
 
-        return $identity === $this->identityGenerator->generate();
+        if ($expiration !== null && time() > $expiration) {
+            return false;
+        }
+        return true;
     }
 
     private function generateToken(?int $expiration): string
     {
-        return StringHelper::base64UrlEncode(
-            $this->mac->sign(
-                (string) $expiration . '~' . $this->identityGenerator->generate(),
-                $this->secretKey,
-                true,
-            ),
-        );
+        $message = (string) $expiration . '~' . Random::string(32);
+
+        return StringHelper::base64UrlEncode($this->generateHash($message) . $message);
     }
 
+    /**
+     * @return array{0: int|null, 1: string}|null
+     */
     private function extractData(string $token): ?array
     {
         try {
-            $raw = $this->mac->getMessage(
-                StringHelper::base64UrlDecode($token),
-                $this->secretKey,
-                true,
-            );
-        } catch (DataIsTamperedException $e) {
+            $payload = StringHelper::base64UrlDecode($token);
+        } catch (InvalidArgumentException $e) {
+            return null;
+        }
+
+        if (StringHelper::byteLength($payload) <= $this->hashLength) {
             return null;
         }
 
-        $chunks = explode('~', $raw, 2);
+        $message = StringHelper::byteSubstring($payload, $this->hashLength, null);
+        $chunks = explode('~', $message, 2);
         if (count($chunks) !== 2) {
             return null;
         }
 
         if ($chunks[0] === '') {
@@ -108,12 +124,30 @@
         } else {
             $expiration = (int) $chunks[0];
             if ((string) $expiration !== $chunks[0]) {
                 return null;
             }
         }
 
-        $identity = $chunks[1];
+        return [$expiration, $payload];
+    }
 
-        return [$expiration, $identity];
+    private function generateHash(string $message): string
+    {
+        $identity = $this->identityGenerator->generate();
+        $message = StringHelper::byteLength($identity) . '~' . $identity . '~' . $message;
+        $hash = hash_hmac($this->algorithm, $message, $this->secretKey, true);
+        if (!$hash) {
+            throw new RuntimeException("Failed to generate HMAC with hash algorithm: {$this->algorithm}.");
+        }
+        return $hash;
+    }
+
+    private function generateHashLength(): int
+    {
+        $hash = hash_hmac($this->algorithm, '', '', true);
+        if (!$hash) {
+            throw new RuntimeException("Failed to generate HMAC with hash algorithm: {$this->algorithm}.");
+        }
+        return StringHelper::byteLength($hash);
     }
 }

tests/Hmac/HmacCsrfTokenTest.php

@@ -6,6 +6,7 @@
 
 use PHPUnit\Framework\TestCase;
 use Yiisoft\Csrf\Hmac\HmacCsrfToken;
+use Yiisoft\Csrf\Hmac\IdentityGenerator\CsrfTokenIdentityGeneratorInterface;
 use Yiisoft\Csrf\Tests\Hmac\IdentityGenerator\MockCsrfTokenIdentityGenerator;
 use Yiisoft\Security\Mac;
 use Yiisoft\Security\Random;
@@ -38,6 +39,30 @@ public function testBase(): void
         $this->assertTrue($csrfToken->validate($token));
     }
 
+    public function testTokenValueChanges(): void
+    {
+        $csrfToken = new HmacCsrfToken(
+            new MockCsrfTokenIdentityGenerator('user7'),
+            'mySecretKey',
+        );
+
+        $this->assertNotSame($csrfToken->getValue(), $csrfToken->getValue());
+    }
+
+    public function testTokenDoesNotExposeIdentity(): void
+    {
+        $identity = 'session-id-that-must-not-be-in-token';
+        $csrfToken = new HmacCsrfToken(
+            new MockCsrfTokenIdentityGenerator($identity),
+            'mySecretKey',
+        );
+
+        $token = $csrfToken->getValue();
+
+        $this->assertStringNotContainsString($identity, StringHelper::base64UrlDecode($token));
+        $this->assertTrue($csrfToken->validate($token));
+    }
+
     public function testExpiration(): void
     {
         self::$timeResult = 300;
@@ -68,6 +93,7 @@ public function testIncorrectToken(): void
         );
 
         $this->assertFalse($csrfToken->validate(Random::string()));
+        $this->assertFalse($csrfToken->validate('*'));
 
         $token = StringHelper::base64UrlEncode(
             (new Mac('sha256'))->sign('a2~user1', 'mySecretKey', true),
@@ -80,6 +106,23 @@ public function testIncorrectToken(): void
         $this->assertFalse($csrfToken->validate($token));
     }
 
+    public function testInvalidTokenParsingDoesNotGenerateIdentity(): void
+    {
+        $identityGenerator = new class implements CsrfTokenIdentityGeneratorInterface {
+            public int $calls = 0;
+
+            public function generate(): string
+            {
+                $this->calls++;
+                return 'user7';
+            }
+        };
+        $csrfToken = new HmacCsrfToken($identityGenerator, 'mySecretKey');
+
+        $this->assertFalse($csrfToken->validate(StringHelper::base64UrlEncode('short')));
+        $this->assertSame(0, $identityGenerator->calls);
+    }
+
     public function testIdentityWithTilda(): void
     {
         $csrfToken = new HmacCsrfToken(