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
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.
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
decrypt methods that have been implemented.
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
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.
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.
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.