Added support for AES IGE #1095
Conversation
| @@ -498,10 +505,12 @@ public function setIV($iv) | |||
| throw new \InvalidArgumentException('This algorithm does not use an IV.'); | |||
| } | |||
|
|
|||
| if (strlen($iv) != $this->block_size) { | |||
| throw new \LengthException('Received initialization vector of size ' . strlen($iv) . ', but size ' . $this->block_size . ' is required'); | |||
| if (strlen($iv) != $this->block_size*($this->mode === self::MODE_IGE ? 2 : 1)) { | |||
There was a problem hiding this comment.
Let's introduce a new variable: expected_iv_length
| @@ -840,6 +849,21 @@ public function encrypt($plaintext) | |||
| return $result; | |||
| case self::MODE_CTR: | |||
| return $this->openssl_ctr_process($plaintext, $this->encryptIV, $this->enbuffer); | |||
| case self::MODE_IGE: | |||
| $ciphertext = ''; | |||
There was a problem hiding this comment.
This block seems to be (largely) duplicated a bunch of times below.
There was a problem hiding this comment.
Yes, but I wasn't sure whether it was a good idea to create a separate function just to avoid duplicating a few lines of code.
Should I do that?
There was a problem hiding this comment.
Also, what about the problems in the native module?
There was a problem hiding this comment.
I've been on the fence with this. My initial searches led me to conclude that this was a one off mode (and not even a very good one at that) used for only one app:
https://www.mgp25.com/AESIGE/
http://www.cryptofails.com/post/70546720222/telegrams-cryptanalysis-contest
Keeping that in mind, I don't really want to add every homegrown block cipher algorithm under the sun.
If Joe Schmoe made his own shitty symmetric key algorithm that no one other than he used should I include that in phpseclib as well if he did a PR? No. There has to be some sort of bar for inclusion.
That said, I did a Google search just now. I guess OpenSSL supports this mode:
https://lists.gnupg.org/pipermail/gcrypt-devel/2015-September/003572.html
https://stackoverflow.com/questions/18171973/aes-aes-ige-128-aes-ige-192-aes-ige-256-encryption-decryption-with-openssl-c
So I guess that fact merits reconsideration of this PR.
There was a problem hiding this comment.
I agree with bantu's comment on the code duplication. Seems like you could just call a new method - handleIGEEncrypt:
function handleIGEEncrypt($plaintext) {
$ciphertext = '';
$iv_part_1 = substr($this->encryptIV, 0, $this->block_size);
$iv_part_2 = substr($this->encryptIV, $this->block_size);
for ($i = 0; $i < strlen($plaintext); $i+= $this->block_size) {
$indata = substr($plaintext, $i, $this->block_size);
switch ($this->engine) {
self::ENGINE_OPENSSL:
$outdata = openssl_encrypt($indata ^ $iv_part_1, $this->cipher_name_openssl, $this->key, OPENSSL_RAW_DATA | OPENSSL_ZERO_PADDING) ^ $iv_part_2;
break;
self::ENGINE_MCRYPT:
$outdata = @mcrypt_generic($this->enmcrypt, $indata ^ $iv_part_1) ^ $iv_part_2;
break;
self::ENGINE_INTERNAL:
$outdata = $this->encryptBlock($indata ^ $iv_part_1) ^ $iv_part_2;
}
$iv_part_1 = $outdata;
$iv_part_2 = $indata;
$ciphertext.= $outdata;
}
if ($this->continuousBuffer) {
$this->encryptIV = $iv_part_1 . $iv_part_2;
}
return $ciphertext;
}I mean, I suppose it's somewhat hypocritical for me to criticize it here when phpseclib does it with CFB mode, but... I just pushed a commit, in a branch, aimed at reducing some of the duplication for CFB mode:
https://github.com/terrafrost/phpseclib/tree/cfb-optimization
Of course, in this case, I suppose the old addage applies: "if it ain't broke don't fix it". We'll see where that branch goes.
tests/Unit/Crypt/AES/TestCase.php
Outdated
| @@ -61,6 +62,7 @@ public function continuousBufferCombos() | |||
| foreach ($modes as $mode) { | |||
| foreach ($plaintexts as $plaintext) { | |||
| foreach ($ivs as $iv) { | |||
| if ($mode === BlockCipher::MODE_IGE) $iv .= strrev($iv); | |||
There was a problem hiding this comment.
I suppose it doesn't matter as much in a unit test but, technically, this is a violation of the PSR-2 coding standards. That should read more like this:
if ($mode === BlockCipher::MODE_IGE) {
$iv.= strrev($iv);
}
| * | ||
| * @var array | ||
| */ | ||
| protected static $primes; |
There was a problem hiding this comment.
$primes is defined in PHP32.php and PHP64.php. PHP.php is abstract so it can't be instantiated anyway.
The reason I defined $primes in PHP32.php and PHP64.php was so that you wouldn't run into problems if you tried to instantiate both of them.
There was a problem hiding this comment.
Never mind - I see you undid that in a newer commit - that you changed, later, self::$primes to static::$primes. I'd say that'd be a good PR on it's own. Maybe try adding a unit test too.
|
My thoughts:
|
|
Welp rewrote now as asked. |
|
Ping |
|
Ping, would you please consider merging this? It's literally just adding support for IGE, thousands of people are already relying on my fork for these features, I can't see why these features shouldn't be merged into the main branch. |
And keep in mind, too... when something is merged into the main branch I am pretty much assuming full responsibility of it. If you're able to provide ongoing support for that specific functionality that's great but I can't work on that assumption with any PR. I have to work on the assumption that once any PR is merged I will solely be responsible for any and all support related to that feature. So that's a con. A pro would be you would stop hassling me and another PR could be closed. But do the pros of merging any PR outweigh the cons? tbh that probably depends on my mood of the day but once a PR is merged a PR is merged. But the broader point is that I don't merge PR's lightly. I mean, a clear bug fix, sure. Adding PHP 7.4 to .travis.yml, sure. Neither of those are big PR's. But this is a whole new feature. Additionally, I'm always willing to re-arrange my priority list for In conclusion, if you want to make code reviewing this a higher priority for me, address points (1) and (3). There's not a whole lot you can do about point (2) but I think addressing points (1) and (3) might psychologically have an impact that would boost the priority of this PR. Money would impact (2) as well lol |
Evidence? |
Added support for AES IGE encryption/decryption mode: unfortunately, native and inline mode don't seem to work properly, and I can't find the problem, so a bit of help would be real nice :)