Skip to main content
2 of 3
added 181 characters in body

You're creating a CBC wrapper, but your wrapper uses HMAC for authentication. This is great, but you would not expect HMAC for a CBC class. Even worse, you're making the mode of operation part of the BlockCipher interface. This is the wrong way around: a mode of operation uses a block cipher to be implemented. And a block cipher certainly doesn't include HMAC.

Furthermore, you're also adding HMAC for CTR mode, which has many uses for specialized encryption constructs, which would not be available for your class. I don't think you should do this, but if you do you should do this by creating a separate class that performs the calculation of the authentication tag.

Inexplicably, you're not adding HMAC for the old CFB mode that nobody uses anymore. That means that your code is not symmetric. Fortunately you do use encrypt-then-MAC.

So your complete design is off, to the point that it should not be used.


Your code contains code to perform transport security and - what I presume is - in place encryption using passwords. It's therefore try-to-do everything code. That kind of code should not be written in the first place. Cryptography related code should be written with a specific use case in mind. Generic crypto code should be a very well written API so it can be used for those kind of use cases, and this is not it.

Your transport mode security seems to contain key agreement but not authentication. There doesn't seem to be any warning about this in the comments.


The idea that you need to pad keys supplied by the users is very dangerous. It lets users believe that their key is secure, while it should be either 16, 24 or 32 bytes for AES. There is a reason why PHP / mcrypt is being deprecated, and this is one big reason why this is the case.


Good, you made it this far. If you're anything like me you should be almost translucently red with anger by now. Please understand that I was in the same place as you are now - probably worse, as I didn't know much about NaCl or authenticated ciphertext.

Unfortunately I don't think your code has any value. You should see it as a good learning experience and a reminder not to write wrapper classes. And you should not do this even if you're going to be an absolute expert in crypto.

Otherwise you are going to spend a significant amount of time in the future trying to remove your wrapper class from the code base. Been there, done that.