Skip to main content
deleted 133 characters in body
Source Link

You're creatingThe code in the question shows a CBC wrapper class. However, but yourthat wrapper class uses HMAC for authentication, without this being apparent in the name of the class. ThisHaving authenticated ciphertext is great, but you would not expect HMAC for a CBC classif it is present then that should be made clear. Even worse, you'reAnother design mistake is making the mode of operation part of the BlockCipher interface. ThisThat is the wrong way around: a mode of operation uses a block cipher to be implementeduses a block cipher as configuration parameter. And aA block cipher certainly doesn'tdoes not include a HMAC.

Furthermore, you're also adding HMAC authentication is also implemented for CTR mode, which. CTR mode has many uses for specialized encryption constructs. For instance, which would not be available for your classCTR mode is used in GCM, CCM and EAX modes of operation. I don't think you should do thisHowever, but if you do you shouldthose do this by creating a separate class that performs the calculation of the authentication tagcourse not use HMAC.

  Inexplicably, you're not addingthe HMAC authentication is not implemented for the old CFB mode that nobody uses anymore, which, although secure, is generally not used in new protocols. ThatIt also means that your codethe API is not symmetric.symmetric; CBC and CTR do have HMAC authentication, why is CFB left behind?

Fortunately you do usethe code uses encrypt-then-MAC.

So your complete design is off, towhich is the point that it should not be usedcorrect order.


YourThe code contains code tofunctionality 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 inmeans it tries t do multiple things at the first placesame time. 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.

YourThe 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 usersuser needs padding is very dangerous. It lets users believe that their key is secure, while it should be either 16, 24 or 32 fully random bytes for AES. ThereGenerally passwords should not be treated as keys. It is possible to turn passwords into keys using a reason why PHP / mcrypt is being deprecatedPBKDF2, and this is one big reason why this is the casealthough for encryption password should be avoided where possible.


GoodIn general I'd warn against using "wrapper" classes, 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 goingcreated to be an absolute expert in cryptomake AES "easy".

Otherwise you are going to spend It takes a very significant amount of time in the future trying to remove youra wrapper class from thea code base. Been thereI've spend many hours trying to remove a self-spun wrapper class, donemimicking a C++ wrapper class in Java.

You have been warned; don't make the same mistake that I did.

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.

The code in the question shows a CBC wrapper class. However, that wrapper class uses HMAC for authentication, without this being apparent in the name of the class. Having authenticated ciphertext is great, but if it is present then that should be made clear. Another design mistake is making the mode of operation part of the BlockCipher interface. That is the wrong way around: a mode of operation uses a block cipher as configuration parameter. A block cipher does not include a HMAC.

HMAC authentication is also implemented for CTR mode. CTR mode has many uses for specialized encryption constructs. For instance, CTR mode is used in GCM, CCM and EAX modes of operation. However, those do of course not use HMAC. Inexplicably, the HMAC authentication is not implemented for the old CFB mode that, which, although secure, is generally not used in new protocols. It also means that the API is not symmetric; CBC and CTR do have HMAC authentication, why is CFB left behind?

Fortunately the code uses encrypt-then-MAC, which is the correct order.


The code contains functionality perform transport security and - what I presume is - in place encryption using passwords. That means it tries t do multiple things at the same time. 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.

The 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 keys supplied by the user needs padding is very dangerous. It lets users believe that their key is secure, while it should be either 16, 24 or 32 fully random bytes for AES. Generally passwords should not be treated as keys. It is possible to turn passwords into keys using a PBKDF2, although for encryption password should be avoided where possible.


In general I'd warn against using "wrapper" classes, that are created to make AES "easy". It takes a very significant amount of time to remove a wrapper class from a code base. I've spend many hours trying to remove a self-spun wrapper class, mimicking a C++ wrapper class in Java.

You have been warned; don't make the same mistake that I did.

added 181 characters in body
Source Link

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 ridiculousvery dangerous. It lets users believe that their key is OKsecure, 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 frustrationanger 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.

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 is ridiculous. It lets users believe that their key is OK, 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 frustration 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.

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.

Source Link

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 is ridiculous. It lets users believe that their key is OK, 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 frustration 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.