Skip to main content
Code Review

Return to Answer

added 468 characters in body
Source Link
Maarten Bodewes
Maarten Bodewes
  • 5.8k
  • 18
  • 52

Seems to me that this depends on the key size and algorithm, both of which are undefined within the method. Better provide a helper method to retrieve the maximum size instead (Cipher has a helper function - getOutputSize - for that already). I presume you are going to write more documentation than just repeating this statement? And is this actually true for the data delivered for decryption?

Why not use RSAPublicKey here? That way you don't get surprisedsurprise exceptions late on.

The private key is determined by the name using a publicly known, unkeyed algorithm. Wow. Just wow. Um, I'm flabbergasted.

RSA key pairs must be generated from random unless you can be 100% sure that the key pair generation (including prime number generation) never changes. For that, you'd need to include the entire RSA key pair generation and random number generation in your source code, or updates of the system may break your scheme.

And you should of course feed it a secret value containing enough (~128 bits of) entropy rather than a public value.

Seems to me that this depends on the key size and algorithm, both of which are undefined within the method. Better provide a helper method to retrieve the maximum size instead (Cipher has a helper function for that already). I presume you are going to write more documentation than just repeating this statement? And is this actually true for the data delivered for decryption?

Why not use RSAPublicKey here? That way you don't get surprised exceptions late on.

The private key is determined by the name using a publicly known, unkeyed algorithm. Wow. Just wow. Um, I'm flabbergasted.

Seems to me that this depends on the key size and algorithm, both of which are undefined within the method. Better provide a helper method to retrieve the maximum size instead (Cipher has a helper function - getOutputSize - for that already). I presume you are going to write more documentation than just repeating this statement? And is this actually true for the data delivered for decryption?

Why not use RSAPublicKey here? That way you don't get surprise exceptions late on.

The private key is determined by the name using a publicly known, unkeyed algorithm. Wow. Just wow. Um, I'm flabbergasted.

RSA key pairs must be generated from random unless you can be 100% sure that the key pair generation (including prime number generation) never changes. For that, you'd need to include the entire RSA key pair generation and random number generation in your source code, or updates of the system may break your scheme.

And you should of course feed it a secret value containing enough (~128 bits of) entropy rather than a public value.

added 424 characters in body
Source Link
Maarten Bodewes
Maarten Bodewes
  • 5.8k
  • 18
  • 52

Most generic wrapper classes for crypto are used to wrap the knowledge of the user that wrote them, and this is no exception. Almost every line of cryptography in here is unfortunately below par. There is no need for the wrapper class at all, directly using the primitives is much better and will safe you from a maintainability nightmare.

I spend ages getting rid of my badly written wrapper classes in Java when I started crypto, inspired by the other - C++ based - wrapper trash written by my colleagues. I have been at this exact cross road, and I chose wrong. Do not use this as generic class in your applications unless you want to perform weeks of refactoring in the future (and that's not counting stupid bug fixes before that).

If you do not listen to this advice, please at least include a version number with your resulting signatures and encryption methods so you are able to upgrade/replace with something secure later on.

That all said, I do admire that you posted here and I do hope you keep your code here, if just to warn others. I hope you learn a lot. Use the knowledge you learn here to create classes for specific use cases instead.

Here is a line-by-line review for your learning experience.

The signature and encryption classes are OK-ish, but remember: the idea of standardized signature and encryption algorithms is be able to communicate between runtimes. You'll have to think of some kind of serialization / protocol to do that.

Most generic wrapper classes for crypto are used to wrap the knowledge of the user that wrote them, and this is no exception. Almost every line of cryptography in here is unfortunately below par. There is no need for the wrapper class at all, directly using the primitives is much better and will safe you from a maintainability nightmare (I spend ages getting rid of my one in Java, inspired by the C++ based wrapper trash written by my colleagues - I have been there).

I do however admire that you posted here and I hope you keep your code here, if just to warn others. I hope you learned a lot. Now please put this code somewhere safe, out of reach. Use the knowledge you learned here to create classes for specific use cases instead.

The signature and encryption classes are OK-ish, but remember: the idea of standardized signature and encryption algorithms is be able to communicate between runtimes. You'll have to think of some kind of serialization / protocol to do that.

Most generic wrapper classes for crypto are used to wrap the knowledge of the user that wrote them, and this is no exception. Almost every line of cryptography in here is unfortunately below par. There is no need for the wrapper class at all, directly using the primitives is much better and will safe you from a maintainability nightmare (I spend ages getting rid of my one in Java, inspired by the C++ based wrapper trash written by my colleagues - I have been there).

I do however admire that you posted here and I hope you keep your code here, if just to warn others. I hope you learned a lot. Now please put this code somewhere safe, out of reach. Use the knowledge you learned here to create classes for specific use cases instead.

Most generic wrapper classes for crypto are used to wrap the knowledge of the user that wrote them, and this is no exception. Almost every line of cryptography in here is unfortunately below par. There is no need for the wrapper class at all, directly using the primitives is much better and will safe you from a maintainability nightmare.

I spend ages getting rid of my badly written wrapper classes in Java when I started crypto, inspired by the other - C++ based - wrapper trash written by my colleagues. I have been at this exact cross road, and I chose wrong. Do not use this as generic class in your applications unless you want to perform weeks of refactoring in the future (and that's not counting stupid bug fixes before that).

If you do not listen to this advice, please at least include a version number with your resulting signatures and encryption methods so you are able to upgrade/replace with something secure later on.

That all said, I do admire that you posted here and I do hope you keep your code here, if just to warn others. I hope you learn a lot. Use the knowledge you learn here to create classes for specific use cases instead.

Here is a line-by-line review for your learning experience.

The signature and encryption classes are OK-ish, but remember: the idea of standardized signature and encryption algorithms is be able to communicate between runtimes. You'll have to think of some kind of serialization / protocol to do that.

Source Link
Maarten Bodewes
Maarten Bodewes
  • 5.8k
  • 18
  • 52

class Person private constructor(val name :String, private val keyPair: KeyPair) {

What does a person have to do with the private key? Is it the owner maybe? Why is it not an entity? Can't a server have a key pair?

get() = keyPair.private ?: error("Current Person implementation doesn't support functions that use the private key")

How can you not have a private key in a key pair object? That doesn't make sense; just pass a public key in that case.

fun isValid() = fromPrivateKey(name,privateKey).publicKey == publicKey

I don't think it is all that funny if isValid is correct for some unexplained reason (or not).

return "Person(name=$name, finger=${fingerprint().substring(0,10)})"

Only later in the code it becomes clear that this is not a fingerprint of a person, but of the key. The 10 is a magic value, if you think that the fingerprint is smaller then you should let that be the result of the fingerprint calculation.

val aesCipher = Cipher.getInstance("AES/CBC/PKCS5Padding")

Why not a modern mode such as GCM?

val final = aesCipher.doFinal(data)

final is a terrible name for a variable, why not call it, say, ciphertext?

fun sign(data :ByteArray): Signature {
    sig.initSign(privateKey)

sig comes falling out of the sky, and it is unclear what kind of algorithm is used.

/** This needs to be below 245 bytes */

Seems to me that this depends on the key size and algorithm, both of which are undefined within the method. Better provide a helper method to retrieve the maximum size instead (Cipher has a helper function for that already). I presume you are going to write more documentation than just repeating this statement? And is this actually true for the data delivered for decryption?

fun encrypt(data :ByteArray, publicKey: PublicKey): ByteArray {

Why not use RSAPublicKey here? That way you don't get surprised exceptions late on.

fun fingerprint():String = DigestUtils.sha1Hex(publicKey.encoded)

Probably better to calculate the fingerprint over the modulus, that way the public and private key share the same fingerprint.

val cipher = Cipher.getInstance("RSA")

Never relies on defaults like that. This is "RSA/ECB/PKCS1Padding" for the default Sun provider, but it could be another algorithm for different providers. OAEP padding should be preferred.

fun encryptAES(data :ByteArray, publicKey : PublicKey): EncryptedData {

Encryption with AES that takes a public key. This is definitely not a good name for the function. Just encrypt or hybridEncrypt would be better. Why is the public key passed if it is part of the fields of the object?

/** This will encrypt over 245 bytes */

And we're going to know in advance which method is chosen by the user I suppose? Wouldn't it also encrypt plaintext below that size?

val iv = ByteArray(16) { -1 }

Minus one. Just because we ... can?

SecureRandom.getInstanceStrong().nextBytes(iv)

Ah we use a strong random for the IV, which just needs to be unpredictable. Why? SecureRandom is strong enough. Blocking RNG's are not fun in any application.

keyGen.init(128)

Not wrong, but note that you're using the default SecureRandom here, so the key is less strong than the IV. Not that you should change this line.

//need to return encrypted secret key and the encrypted message

"returns" not "need to return" and put the comment before the actual return statement.

On the companion object.

Why is there a companion object for signature generation, but not for encryption / decryption?

    private val sig = java.security.Signature.getInstance("SHA1WithRSA")

SHA1 is completely insecure for signature generation.

 private fun buildKeyPair(seed :Long): KeyPair {

A long is 64 bits. It is becoming easy to try 2^64 values and simply regenerate the key pair. Or one of multiple key pairs if more are available. Why is a normal random not used?

val random = SecureRandom.getInstance("SHA1PRNG")

SHA1PRNG is not well defined, so this may return any kind of RNG. It may not be the best one either.

random.setSeed(seed)

Oh, boy. This may either mix in the seed or make the SHA1PRNG act deterministically. The outcome depends on a lot of factors. Java platform and version just being two of them. Use this and you're already borked.

val keySize = 2048

A rather small undefined, unexplained default and magic number. See keylength.com.

return keyPairGenerator.genKeyPair()

I'm pretty sure that method has been deprecated eons ago.

 val spec = RSAPublicKeySpec(privateKey.modulus,privateKey.publicExponent)

... but I'm not sure if the publicExponent is always available even for CRT private keys. Let's assume it is because it seems that way in the API (?).

 fun deterministicFromName(name :String) :Person = Person(name,buildKeyPair(DigestUtils.sha1(name)!!.contentHashCode().toLong()))

The private key is determined by the name using a publicly known, unkeyed algorithm. Wow. Just wow. Um, I'm flabbergasted.

The signature and encryption classes are OK-ish, but remember: the idea of standardized signature and encryption algorithms is be able to communicate between runtimes. You'll have to think of some kind of serialization / protocol to do that.

Most generic wrapper classes for crypto are used to wrap the knowledge of the user that wrote them, and this is no exception. Almost every line of cryptography in here is unfortunately below par. There is no need for the wrapper class at all, directly using the primitives is much better and will safe you from a maintainability nightmare (I spend ages getting rid of my one in Java, inspired by the C++ based wrapper trash written by my colleagues - I have been there).

I do however admire that you posted here and I hope you keep your code here, if just to warn others. I hope you learned a lot. Now please put this code somewhere safe, out of reach. Use the knowledge you learned here to create classes for specific use cases instead.