Encryption algorithm for chat app

Question

I'm working on a CLI chat app using Python and I've coded this encryption algorithm.

I used hybrid encryption.

This uses an AES key as the shared key for encryption and decryption and RSA to encrypt this AES key using the client's RSA public keys; sends it to them; and they decrypt it using their private key so all will have the same shared key for encryption and decryption.

Is this encryption good for a chat app?

This is the code:

import base64, hashlib, hmac
from Crypto import Random
from Crypto.Cipher import AES, PKCS1_OAEP
from Crypto.PublicKey import RSA 

class Encryptor(object):
    def __init__(self):
        self.bs = AES.block_size
        self.iv = self.random_kegen()
        self.key = hashlib.sha256(str(RSA.generate(2048, Random.new().read)).encode('UTF-8')).digest()
        self.mac_key = self.random_kegen()
        self.private_key = RSA.generate(1024, Random.new().read)
        self.public_key = self.private_key.publickey()


    random_kegen = lambda self: Random.new().read(self.bs)
    rsa_encrypt = lambda self, data, key=False: base64.b64encode(PKCS1_OAEP.new(self.public_key if not key else key).encrypt(data))
    rsa_decrypt = lambda self, enc_data, key=False: PKCS1_OAEP.new(self.private_key if not key else key).decrypt(base64.b64decode(enc_data))
    pad = lambda self, data: data + (self.bs - len(data) % self.bs) * chr(self.bs - len(data) % self.bs)
    unpad = staticmethod(lambda data: data[:-ord(data[len(data)-1:])])
    mac_kegen = lambda self, ciphertext: hmac.new(self.mac_key, msg=ciphertext, digestmod=hashlib.sha256).digest()

    def encrypt(self, raw):
        raw = self.pad(raw)
        cipher = AES.new(self.key, AES.MODE_CBC, self.iv)
        ciphertext = cipher.encrypt(raw.encode('UTF-8'))
        tag = self.mac_kegen(ciphertext)
        encrypted = self.iv + ciphertext + tag
        self.iv = self.random_kegen()
        return base64.b64encode(base64.b64encode(encrypted))

    def decrypt(self, ciphertext):
        encrypted = base64.b64decode(base64.b64decode(ciphertext))
        iv,ciphertext,tag = encrypted[0:16], encrypted[16:-32], encrypted[-32:]
        if self.mac_kegen(ciphertext) == tag:
            cipher = AES.new(self.key, AES.MODE_CBC, iv)
            return self.unpad(cipher.decrypt(ciphertext)).decode('UTF-8')

Answer 1

Using a public key cryptography system to share private keys is a fairly reasonable approach, yes. As always it's the details you have to watch out for though. Classic questions include "How do you make sure that you're actually using the right recipient's private key, and not talking to a man in the middle?" and "How do you ensure that an attacker can't read secret data right out of your memory, even after it's been garbage collected?" If doing cryptography in the real world, best practice is to use an existing library (such as the well-regarded libsodium) which does as much for you as possible.

Looking at your code though, I'd highlight the following as particular red flags:

• The long sequence of lambdas are hard to read, and consequently hard to reason about. Python best practice is to use full def functions for everything except anonymous single-use functions.
• I'm not fond of bs as a name. iv is probably fine because that's such a well established label for that concept, but in general acronyms don't make for readable code.
• Talking of the iv, it's a single use value and you are doing well to regenerate it after use. However, since it's single use and you want to remove even the risk of getting it wrong, why not remove it from self and just generate it as a local variable on demand?
• You are checking that tag matches in decryption, which is good. However, you don't actually handle the case where it doesn't match. Having random None objects floating through your chat app because of a random spot of corruption is an accident waiting to happen.
• You are using flexible parameters (such as for block size) which is good. However, you don't use them consistently. For example the line iv,ciphertext,tag = ... would break if the size of your iv or tag were to differ.
• Generating a key by hashing the string representation of some random RSA data feels unnecessarily convoluted and error prone.
• Consider whether 1024 bits is really enough for your key. The standard recommendation is to use 2048 bits or more, or to use a different public key encryption system such as elliptic curve cryptography which is safe with smaller keys.
• Mixed types for one variable is a bit of a red flag. For example, your lambdas have key = False by default but the alternative isn't True. It would surprise me for such a lambda to expect a Key object. You may get further surprises if the provided object happened to be Falsey.
• I would separate the encrypting bit from the base-64 bit. The latter may sometimes be needed for storage or transmission, but there are many settings where just working with the cyphertext Bytes makes most sense. In a similar way, requiring that the plaintext be actual text could cause problems. (e.g. is it impossible that your chat app may want to send a photo?) In any case, I am not aware of any situation where double encoding something as base64 is valuable.
• In modern Python you don't need to explicitly inherit from object

If I were reviewing this for a colleague in a professional context, my immediate response (other than "could you just use Libsodium?") would be to talk through testing against someone breaking protocol. That's perhaps the overall sense that I have of the code: it's clearly been developed to the point that you can encrypt and decrypt a valid message, but perhaps not to the point that you can be confident it won't break or leak data with adversarial input.

Answer 2

Cryptography

self.key = hashlib.sha256(str(RSA.generate(2048, Random.new().read)).encode('UTF-8')).digest()

So this generates two 1024 bit primes, creates an RSA key pair out of it, then encodes it, stringifies it, and then performs a digest over it. That's secure, but just generating 16, 24 or 32 bytes would be that much quicker. To me this makes zero sense.

---

self.private_key = RSA.generate(1024, Random.new().read)

There is no asymmetric encryption without trust. The RSA key used for key encapsulation should be static and the public key should be trusted, otherwise the whole exercise makes little sense.

As already indicated, RSA-1024 is using a way too small key size, see keylength.com for a comparison of key sizes.

---

random_kegen = lambda self: Random.new().read(self.bs)

Block size doesn't have much to do with key size, and using the block size of AES to indicate the key size of the MAC algorithm is even stranger. HMAC generally uses a key size that is identical to the output size of the used HMAC function, in this case 256 bits - not 128 bits. Normally the key sizes used for MAC and encryption should be identical anyway, otherwise you're using mismatched security definitions.

What also worries me is that you show here that you know how to create a secret key correctly, instead of using an RSA private key for it.

---

cipher = AES.new(self.key, AES.MODE_CBC, self.iv)

CBC is a pretty old mode and should be avoided. Things like CTR or even better GCM mode avoid the padding issue. Obviously the IV should be regenerated and included with the ciphertext for CBC mode. You cannot generate it as instance field and then reuse it for the encrypt / decrypt methods.

---

tag = self.mac_kegen(ciphertext)

Ooops, no, that doesn't work if you include the IV with the ciphertext. The attacker can now change the IV and thus have full capability to flip any bit in the first block by altering the IV at the same bit location.

Design

There is an Encryptor class that also performs decryption. That doesn't make any sense to me. It will also trick you into issues with the trust relationship of course, and you'd need a different class at the other side, as you would use different keys for either party in the chat.

---

You are encrypting messages, so the RSA encryption and decryption of the AES key should be part of the encrypt / decrypt methods that have been implemented.

Code

pad = lambda self, data: data + (self.bs - len(data) % self.bs) * chr(self.bs - len(data) % self.bs)

All that lambda stuff, but the self.bs - len(data) % self.bs is allowed to repeat, making for very hard to read code. It's not very byte oriented either. Note that adding padding to the data is relatively dumb, as it would require an entire new string in most environments. Generally the padding should be part of the cipher, or otherwise applied using buffering using an update / final method. This would come back to haunt you if the messages get larger in size (e.g. when images are embedded).

---

unpad = staticmethod(lambda data: data[:-ord(data[len(data)-1:])])

Well, fortunately Python doesn't allow buffer underruns, but just taking the last byte, ignoring all the other padding bytes and then removing the data is a disaster waiting to happen.

---

self.public_key if not key else key

I've got no idea what this does, but it shouldn't.

---

ciphertext = cipher.encrypt(raw.encode('UTF-8'))

In cryptography the term "raw" usually means raw bytes. You've just stringified your encryption / decryption operations for no apparent reason. First encode the message to binary using your "chat" protocol and then create the encryption function around that.

---

tag = self.mac_kegen(ciphertext)

Yes, a MAC generates a tag, but what the heck does kegen mean? If it means keygen then this method is named utterly incorrectly.

---

return base64.b64encode(base64.b64encode(encrypted))

The one thing that I hate more than stringified code is code that is stringified twice for no apparent reason. Why would the transport layer of your chat app not be able to handle binary? And why would the encryption class be burdened with the encoding?

---

if self.mac_kegen(ciphertext) == tag:

As already indicated, you should use a clear way of handling tag mismatch. I'd share this under a security issue if I was reviewing this code, and I suppose I am.

Some good decisions

To relieve the pain a bit, here are some good decisions:

• using OAEP instead of PKCS#1 v1.5 padding for the RSA encryption;
• SHA-256 is a solid choice as hash primitive for the HMAC function;
• at least a MAC is attempted to be present;
• the IV seems static for the class, but at least it is randomized once.

Conclusion

Your encoding / decoding decisions, class design, key size choice all leave quite a lot to be desired. The AES key generation clearly shows lack of knowledge and is trying to outsmart itself.

Worse: there is a direct design error when the IV is not included in the calculation of the tag, which makes the entire messaging system vulnerable against man-in-the-middle attacks.