-
-
Notifications
You must be signed in to change notification settings - Fork 877
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added support for AES IGE #1095
base: master
Are you sure you want to change the base?
Changes from 6 commits
12dc23d
e2f91cc
200c2a9
6d714a6
52c068a
1a730d1
273ad17
4e367c9
0d9dc46
a74a5f7
27370df
3097d29
0aff25e
c85ddd3
2dc68c8
c62fc0c
2dc143a
5e7d391
f715b29
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,6 +70,13 @@ abstract class PHP extends Engine | |
* @access protected | ||
*/ | ||
const ENGINE_DIR = 'PHP'; | ||
|
||
/** | ||
* Primes > 2 and < 1000 | ||
* | ||
* @var array | ||
*/ | ||
protected static $primes; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The reason I defined There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Never mind - I see you undid that in a newer commit - that you changed, later, |
||
|
||
/** | ||
* Default constructor | ||
|
@@ -1326,4 +1333,4 @@ protected function powHelper(PHP $n) | |
|
||
return $temp; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block seems to be (largely) duplicated a bunch of times below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what about the problems in the native module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with bantu's comment on the code duplication. Seems like you could just call a new method -
handleIGEEncrypt
: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.