Skip to content
This repository

Hotfix/random crypt test fail #3492

Merged
merged 2 commits into from about 1 year ago

3 participants

Maks Enrico Zimuel Marc Bennewitz
Maks
Collaborator

The problem is that base64_decode strict check is not enough for ensure base64 encoding detection.

See http://stackoverflow.com/questions/2556345/detect-base64-encoding-in-php

Enrico Zimuel
Owner

@Maks3w see my comment above.

Enrico Zimuel
Owner

The correct way to check for base64 is to apply my fix, the check with "strlen() % 4" is not enough.
I tried on my box (GNU/Linux 3.2.0-36, PHP 5.3.10, PHPUnit 3.7) with your fix and I got a random error after 70/90 executions.
Using my proposal the fix disappears.
Please, update your PR with my suggestion and I will merge the fix.

Maks
Collaborator

@ezimuel Then I think that we need a different aproach. Decode + Encode sounds a waste of resources.

I suggest to have two methods one for base64 encode and another one without base64 de encode or to have an argument which specify if the input is or not base64

Enrico Zimuel
Owner

I don't think that this approach is a waste of resource. I only added a base64_encode() in the code. Moreover, the usage of the decrypt() or encrypt() method in the Rsa class is only for small strings, see http://zf2.readthedocs.org/en/latest/modules/zend.crypt.public-key.html#encrypt-and-decrypt-a-string for more details.

Marc Bennewitz

From my point of view the method decrypt can't autodetect if $data is encoded in base64 because:

  • There are more than one variants of base64
  • base64 with or without the leading = can be valid
  • I don't see a definition that encrypted $data have to contain a defined set of bytes that can't be interpreted as base64 in all cases

That means it's unknown and should be up to the consumer to know how $data is encoded and having a defined API working with binary data so the consumer have to decode it or defining different methods handle well defined types of encoded data.

(Sorry for my poor English)

Enrico Zimuel
Owner

We have discussed with @Maks3w and we decided to propose an alternative signature for the decrypt method.
Something like:

public function decrypt ($data, $key = null, $mode = self::MODE_AUTO)

where $mode can be MODE_AUTO with the automatic recognition of base64 (with my suggested change), MODE_BASE64 with the decode of the $data in base64 and MODE_RAW without any decoding.
We think this API will cover all the possible use case taking care of performance. At the end, as @marc-mabe suggested the user should now the format for decrypt a string.

Maks Maks3w closed this January 22, 2013
Maks Maks3w reopened this January 23, 2013
Maks
Collaborator

@ezimuel done

Marc Bennewitz marc-mabe commented on the diff January 23, 2013
library/Zend/Crypt/PublicKey/Rsa.php
@@ -251,10 +290,20 @@ public function decrypt($data, Rsa\AbstractKey $key = null)
251 290
             throw new Exception\InvalidArgumentException('No key specified for the decryption');
252 291
         }
253 292
 
254  
-        // check if data is encoded in Base64
255  
-        $output = base64_decode($data, true);
256  
-        if (false !== $output) {
257  
-            $data = $output;
  293
+        switch ($mode) {
  294
+            case self::MODE_AUTO:
2

Should we deprecate this mode and trigger an E_USER_DEPRECATED error ?

Maks Collaborator
Maks3w added a note January 23, 2013

Is a feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Enrico Zimuel

Instead of this sentence I would prefer: "Check if the $signature is encoded in base64. Not recommended for performance."

Maks
Collaborator

done

Enrico Zimuel ezimuel merged commit 44265c3 into from January 23, 2013
Enrico Zimuel ezimuel closed this January 23, 2013
Maks Maks3w deleted the branch January 23, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 2 unique commits by 1 author.

Jan 23, 2013
Maks [crypt][rsa] Fix base64 detection algorithm and add specific modes fo…
…r avoid magic detection
fe14e5e
Maks [crypt][rsa] feedback 44265c3
This page is out of date. Refresh to see the latest.
73  library/Zend/Crypt/PublicKey/Rsa.php
@@ -24,6 +24,10 @@
24 24
  */
25 25
 class Rsa
26 26
 {
  27
+    const MODE_AUTO   = 1;
  28
+    const MODE_BASE64 = 2;
  29
+    const MODE_RAW    = 3;
  30
+
27 31
     /**
28 32
      * @var RsaOptions
29 33
      */
@@ -173,22 +177,45 @@ public function sign($data, Rsa\PrivateKey $privateKey = null)
173 177
     /**
174 178
      * Verify signature with public key
175 179
      *
  180
+     * $signature can be encoded in base64 or not. $mode sets how the input must be processed:
  181
+     *  - MODE_AUTO: Check if the $signature is encoded in base64. Not recommended for performance.
  182
+     *  - MODE_BASE64: Decode $signature using base64 algorithm.
  183
+     *  - MODE_RAW: $signature is not encoded.
  184
+     *
176 185
      * @param  string $data
177 186
      * @param  string $signature
178 187
      * @param  null|Rsa\PublicKey $publicKey
  188
+     * @param  int                $mode Input encoding
179 189
      * @return bool
180 190
      * @throws Rsa\Exception\RuntimeException
  191
+     * @see Rsa::MODE_AUTO
  192
+     * @see Rsa::MODE_BASE64
  193
+     * @see Rsa::MODE_RAW
181 194
      */
182  
-    public function verify($data, $signature, Rsa\PublicKey $publicKey = null)
183  
-    {
  195
+    public function verify(
  196
+        $data,
  197
+        $signature,
  198
+        Rsa\PublicKey $publicKey = null,
  199
+        $mode = self::MODE_AUTO
  200
+    ) {
184 201
         if (null === $publicKey) {
185 202
             $publicKey = $this->options->getPublicKey();
186 203
         }
187 204
 
188  
-        // check if signature is encoded in Base64
189  
-        $output = base64_decode($signature, true);
190  
-        if (false !== $output) {
191  
-            $signature = $output;
  205
+        switch ($mode) {
  206
+            case self::MODE_AUTO:
  207
+                // check if data is encoded in Base64
  208
+                $output = base64_decode($signature, true);
  209
+                if ((false !== $output) && ($signature === base64_encode($output))) {
  210
+                    $signature = $output;
  211
+                }
  212
+                break;
  213
+            case self::MODE_BASE64:
  214
+                $signature = base64_decode($signature);
  215
+                break;
  216
+            case self::MODE_RAW:
  217
+            default:
  218
+                break;
192 219
         }
193 220
 
194 221
         $result = openssl_verify(
@@ -236,13 +263,25 @@ public function encrypt($data, Rsa\AbstractKey $key = null)
236 263
     /**
237 264
      * Decrypt with private/public key
238 265
      *
  266
+     * $data can be encoded in base64 or not. $mode sets how the input must be processed:
  267
+     *  - MODE_AUTO: Check if the $signature is encoded in base64. Not recommended for performance.
  268
+     *  - MODE_BASE64: Decode $data using base64 algorithm.
  269
+     *  - MODE_RAW: $data is not encoded.
  270
+     *
239 271
      * @param  string          $data
240 272
      * @param  Rsa\AbstractKey $key
  273
+     * @param  int             $mode Input encoding
241 274
      * @return string
242 275
      * @throws Rsa\Exception\InvalidArgumentException
  276
+     * @see Rsa::MODE_AUTO
  277
+     * @see Rsa::MODE_BASE64
  278
+     * @see Rsa::MODE_RAW
243 279
      */
244  
-    public function decrypt($data, Rsa\AbstractKey $key = null)
245  
-    {
  280
+    public function decrypt(
  281
+        $data,
  282
+        Rsa\AbstractKey $key = null,
  283
+        $mode = self::MODE_AUTO
  284
+    ) {
246 285
         if (null === $key) {
247 286
             $key = $this->options->getPrivateKey();
248 287
         }
@@ -251,10 +290,20 @@ public function decrypt($data, Rsa\AbstractKey $key = null)
251 290
             throw new Exception\InvalidArgumentException('No key specified for the decryption');
252 291
         }
253 292
 
254  
-        // check if data is encoded in Base64
255  
-        $output = base64_decode($data, true);
256  
-        if (false !== $output) {
257  
-            $data = $output;
  293
+        switch ($mode) {
  294
+            case self::MODE_AUTO:
  295
+                // check if data is encoded in Base64
  296
+                $output = base64_decode($data, true);
  297
+                if ((false !== $output) && ($data === base64_encode($output))) {
  298
+                    $data = $output;
  299
+                }
  300
+                break;
  301
+            case self::MODE_BASE64:
  302
+                $data = base64_decode($data);
  303
+                break;
  304
+            case self::MODE_RAW:
  305
+            default:
  306
+                break;
258 307
         }
259 308
 
260 309
         return $key->decrypt($data);
62  tests/ZendTest/Crypt/PublicKey/RsaTest.php
@@ -413,4 +413,66 @@ public function testRsaLoadsPassphrasedKeys()
413 413
             'private_key' => $rsaOptions->getPrivateKey()->toString(),
414 414
         ));
415 415
     }
  416
+
  417
+    public function testZf3492Base64DetectDecrypt()
  418
+    {
  419
+        $data = 'vNKINbWV6qUKGsmawN8ii0mak7PPNoVQPC7fwXJOgMNfCgdT+9W4PUte4fic6U4A6fMra4gv7NCTESxap2qpBQ==';
  420
+        $this->assertEquals('1234567890', $this->rsa->decrypt($data));
  421
+    }
  422
+
  423
+    public function testZf3492Base64DetectVerify()
  424
+    {
  425
+        $data = 'sMHpp3u6DNecIm5RIkDD3xyKaH6qqP8roUWDs215iOGHehfK1ypqwoETKNP7NaksGS2C1Up813ixlGXkipPVbQ==';
  426
+        $this->assertTrue($this->rsa->verify('1234567890', $data));
  427
+    }
  428
+
  429
+    public function testDecryptBase64()
  430
+    {
  431
+        $data = 'vNKINbWV6qUKGsmawN8ii0mak7PPNoVQPC7fwXJOgMNfCgdT+9W4PUte4fic6U4A6fMra4gv7NCTESxap2qpBQ==';
  432
+        $this->assertEquals('1234567890', $this->rsa->decrypt($data, null, Rsa::MODE_BASE64));
  433
+    }
  434
+
  435
+    public function testDecryptCorruptBase64()
  436
+    {
  437
+        $data = 'vNKINbWV6qUKGsmawN8ii0mak7PPNoVQPC7fwXJOgMNfCgdT+9W4PUte4fic6U4A6fMra4gv7NCTESxap2qpBQ==';
  438
+        $this->setExpectedException('Zend\Crypt\PublicKey\Rsa\Exception\RuntimeException');
  439
+        $this->rsa->decrypt(base64_decode($data), null, Rsa::MODE_BASE64);
  440
+    }
  441
+
  442
+    public function testDecryptRaw()
  443
+    {
  444
+        $data = 'vNKINbWV6qUKGsmawN8ii0mak7PPNoVQPC7fwXJOgMNfCgdT+9W4PUte4fic6U4A6fMra4gv7NCTESxap2qpBQ==';
  445
+        $this->assertEquals('1234567890', $this->rsa->decrypt(base64_decode($data), null, Rsa::MODE_RAW));
  446
+    }
  447
+
  448
+    public function testDecryptCorruptRaw()
  449
+    {
  450
+        $data = 'vNKINbWV6qUKGsmawN8ii0mak7PPNoVQPC7fwXJOgMNfCgdT+9W4PUte4fic6U4A6fMra4gv7NCTESxap2qpBQ==';
  451
+        $this->setExpectedException('Zend\Crypt\PublicKey\Rsa\Exception\RuntimeException');
  452
+        $this->rsa->decrypt($data, null, Rsa::MODE_RAW);
  453
+    }
  454
+
  455
+    public function testVerifyBase64()
  456
+    {
  457
+        $data = 'sMHpp3u6DNecIm5RIkDD3xyKaH6qqP8roUWDs215iOGHehfK1ypqwoETKNP7NaksGS2C1Up813ixlGXkipPVbQ==';
  458
+        $this->assertTrue($this->rsa->verify('1234567890', $data, null, Rsa::MODE_BASE64));
  459
+    }
  460
+
  461
+    public function testVerifyCorruptBase64()
  462
+    {
  463
+        $data = 'sMHpp3u6DNecIm5RIkDD3xyKaH6qqP8roUWDs215iOGHehfK1ypqwoETKNP7NaksGS2C1Up813ixlGXkipPVbQ==';
  464
+        $this->assertFalse($this->rsa->verify('1234567890', base64_decode($data), null, Rsa::MODE_BASE64));
  465
+    }
  466
+
  467
+    public function testVerifyRaw()
  468
+    {
  469
+        $data = 'sMHpp3u6DNecIm5RIkDD3xyKaH6qqP8roUWDs215iOGHehfK1ypqwoETKNP7NaksGS2C1Up813ixlGXkipPVbQ==';
  470
+        $this->assertTrue($this->rsa->verify('1234567890', base64_decode($data), null, Rsa::MODE_RAW));
  471
+    }
  472
+
  473
+    public function testVerifyCorruptRaw()
  474
+    {
  475
+        $data = 'sMHpp3u6DNecIm5RIkDD3xyKaH6qqP8roUWDs215iOGHehfK1ypqwoETKNP7NaksGS2C1Up813ixlGXkipPVbQ==';
  476
+        $this->assertFalse($this->rsa->verify('1234567890', $data, null, Rsa::MODE_RAW));
  477
+    }
416 478
 }
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.