Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Hotfix/random crypt test fail #3492

Merged
merged 2 commits into from Jan 23, 2013

Conversation

Projects
None yet
3 participants
Member

Maks3w commented Jan 19, 2013

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

@ghost ghost assigned ezimuel Jan 21, 2013

Owner

ezimuel commented Jan 21, 2013

@Maks3w see my comment above.

Owner

ezimuel commented Jan 22, 2013

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.

Member

Maks3w commented Jan 22, 2013

@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

Owner

ezimuel commented Jan 22, 2013

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.

Member

marc-mabe commented Jan 22, 2013

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)

Owner

ezimuel commented Jan 22, 2013

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.

@Maks3w Maks3w closed this Jan 22, 2013

@Maks3w Maks3w reopened this Jan 23, 2013

Member

Maks3w commented Jan 23, 2013

@ezimuel done

@marc-mabe marc-mabe commented on the diff Jan 23, 2013

library/Zend/Crypt/PublicKey/Rsa.php
@@ -251,10 +290,20 @@ public function decrypt($data, Rsa\AbstractKey $key = null)
throw new Exception\InvalidArgumentException('No key specified for the decryption');
}
- // check if data is encoded in Base64
- $output = base64_decode($data, true);
- if (false !== $output) {
- $data = $output;
+ switch ($mode) {
+ case self::MODE_AUTO:
@marc-mabe

marc-mabe Jan 23, 2013

Member

Should we deprecate this mode and trigger an E_USER_DEPRECATED error ?

@Maks3w

Maks3w Jan 23, 2013

Member

Is a feature.

Member

Maks3w commented Jan 23, 2013

done

@ezimuel ezimuel merged commit 44265c3 into zendframework:master Jan 23, 2013

1 check failed

default The Travis build failed
Details

@Maks3w Maks3w deleted the Maks3w:hotfix/random-crypt-test-fail branch Jan 23, 2013

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