1
\$\begingroup\$

I've written up a small wrapper for RSA and AES encryption in Kotlin, and I want to make sure I'm not making any glaring mistakes before use it. This is going on a small project with basically no needed security, but I wanted to try and write it correctly.

Here's my Person class:

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

    val publicKey : PublicKey
        get() = keyPair.public
    private val privateKey : PrivateKey
        get() = keyPair.private ?: error("Current Person implementation doesn't support functions that use the private key")

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

    override fun toString(): String {
        return "Person(name=$name, finger=${fingerprint().substring(0,10)})"
    }


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

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

    /** This needs to be below 245 bytes */
    fun encrypt(data :ByteArray, publicKey: PublicKey): ByteArray {
        val cipher = Cipher.getInstance("RSA")
        cipher.init(Cipher.ENCRYPT_MODE, publicKey)
        return cipher.doFinal(data)
    }

    /** This needs to be below 245 bytes */
    fun encrypt(data :ByteArray, person :Person):ByteArray = encrypt(data,person.publicKey)

    /** This needs to be below 245 bytes */
    fun decrypt(encrypted: ByteArray): ByteArray {
        val cipher = Cipher.getInstance("RSA")
        cipher.init(Cipher.DECRYPT_MODE, privateKey)
        return cipher.doFinal(encrypted)
    }


    /** This will encrypt over 245 bytes */
    fun encryptAES(data :ByteArray,person :Person):EncryptedData = encryptAES(data,person.publicKey)

    /** This will encrypt over 245 bytes */
    fun encryptAES(data :ByteArray, publicKey : PublicKey): EncryptedData {
        val iv = ByteArray(16) { -1 }
        SecureRandom.getInstanceStrong().nextBytes(iv)

        val keyGen = KeyGenerator.getInstance("AES")
        keyGen.init(128)
        val secretKey = keyGen.generateKey()


        val ivParameterSpec = IvParameterSpec(iv)
        val aesCipher = Cipher.getInstance("AES/CBC/PKCS5Padding")
        aesCipher.init(Cipher.ENCRYPT_MODE, secretKey, ivParameterSpec)

        val final = aesCipher.doFinal(data)
        return EncryptedData(iv, encrypt(secretKey.encoded,publicKey), final)
        //need to return encrypted secret key and the encrypted message
    }

    fun decryptAES(data : EncryptedData) :ByteArray{
        val iv = data.iv
        val ivParameterSpec = IvParameterSpec(iv)

        val decryptedSecretKey = decrypt(data.encryptedSecretKey)

        val secretKey = SecretKeySpec(decryptedSecretKey, 0, decryptedSecretKey.size, "AES")


        val aesCipher = Cipher.getInstance("AES/CBC/PKCS5Padding")
        aesCipher.init(Cipher.DECRYPT_MODE, secretKey, ivParameterSpec)

        return aesCipher.doFinal(data.encryptedData)
    }

And then I use the Companion object to make new instances of this class:

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

        fun verify(publicKey: PublicKey, signature: Signature, data: ByteArray): Boolean {
            sig.initVerify(publicKey)
            sig.update(data)
            return sig.verify(signature.byteArray)
        }

        //buildKeyPair(DigestUtils.sha1(name)!!.contentHashCode().toLong())
        private fun buildKeyPair(seed :Long): KeyPair {
            val random = SecureRandom.getInstance("SHA1PRNG")
            random.setSeed(seed)
            val keySize = 2048
            val keyPairGenerator = KeyPairGenerator.getInstance("RSA")
            keyPairGenerator.initialize(keySize,random)
            return keyPairGenerator.genKeyPair()
        }

        private fun buildKeyPair():KeyPair {
            val keySize = 2048
            val keyPairGenerator = KeyPairGenerator.getInstance("RSA")
            keyPairGenerator.initialize(keySize)
            return keyPairGenerator.genKeyPair()
        }


        //generators
        fun fromKeyPair(name :String, keyPair: KeyPair):Person = Person(name, keyPair)
        fun fromPublicKey(name :String, publicKey: PublicKey):Person = Person(name,KeyPair(publicKey,null))
        fun fromPrivateKey(name :String, privateKey: PrivateKey):Person {
            //attempt to find the correct public key
            if(privateKey !is RSAPrivateCrtKey)
                error("Private key is not a RSAPrivateCrtKey and does not contain enough data to compute the public key")
            val spec = RSAPublicKeySpec(privateKey.modulus,privateKey.publicExponent)
            val factory = KeyFactory.getInstance("RSA")
            val publicKey = factory.generatePublic(spec)
            return Person(name, KeyPair(publicKey,privateKey))
        }
        fun deterministicFromName(name :String) :Person = Person(name,buildKeyPair(DigestUtils.sha1(name)!!.contentHashCode().toLong()))
        fun generateNew(name :String) :Person = Person(name, buildKeyPair())
    }

This also uses these classes:

class Signature(val byteArray: ByteArray) {
    override fun equals(other: Any?): Boolean {
        if (this === other) return true
        if (javaClass != other?.javaClass) return false

        other as Signature

        if (!byteArray.contentEquals(other.byteArray)) return false

        return true
    }

    override fun hashCode(): Int {
        return byteArray.contentHashCode()
    }
}

and

class EncryptedData(val iv :ByteArray, val encryptedSecretKey :ByteArray,val encryptedData :ByteArray)

I'm using RSA keys to encrypt a randomly generated AES key, which can be used to encrypt data that is larger than 245 bytes.

The idea is that I can pass public keys around but treat everything as a Person, and then throw an error if the private key is invalid. Is this a good design?

\$\endgroup\$
  • \$\begingroup\$ The code formatting is inconsistent. You should let your IDE do the formatting for you, automatically whenevery you save the code. \$\endgroup\$ – Roland Illig Apr 14 at 5:59
2
\$\begingroup\$

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.

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 - 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?

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

Why not use RSAPublicKey here? That way you don't get surprise 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.

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.


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.

\$\endgroup\$
  • \$\begingroup\$ Thanks! I have some comments and small additional questions, but I was 300 characters over the comment limit :( Here is is: hastebin.com/ivawuzexos.txt \$\endgroup\$ – Carson Graham Apr 28 at 19:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.