Ggthjy

I am trying to implement AES CTR encryption mode with HMAC authentication for messages.

It's encrypting and decrypting fine as long as the key length is 64 bytes, since AES key and HMAC key are being derived from this key.

Questions

• Is it safe to append IV or nonce to the encrypted messages?


• Is it safe to append HMAC digest to append to the messages?


• Can you review it for best security coding practices?
  I am using Pycryptodome

Code

import os 

import zlib

from Crypto.Hash import HMAC

from Crypto.Hash import SHA256

from Crypto.Cipher import AES

from Crypto.Util import Counter

import zlib 

def encrypt(full_key, plaintext):



    if len(full_key) != 64:

        raise Exception("FULL key length shall be equal to 64")

    key = full_key[:len(full_key) //2]



    # Use the last half as the HMAC key

    hmac_key = full_key[len(full_key) // 2:]





    if isinstance(plaintext, str):

        plaintext = plaintext.encode()



    compressed = zlib.compress(plaintext, 5)

    print (f"compressed plaintext {compressed}")







    # Choose a random, 16-byte IV.

    iv = os.urandom(16)

    # Convert the IV to a Python integer.

    iv_int = int(binascii.hexlify(iv), 16) 

    # Create a new Counter object with IV = iv_int.

    ctr = Counter.new(128, initial_value=iv_int)

    # Create AES-CTR cipher.

    aes = AES.new(key, AES.MODE_CTR, counter=ctr)

    # Encrypt and return IV and ciphertext.

    ciphertext = aes.encrypt(compressed)





    hmac_obj = HMAC.new(hmac_key, compressed, SHA256)

    mac = hmac_obj.digest()



    return iv+ciphertext+mac





def decrypt(key, ciphertext):

    # Initialize counter for decryption. iv should be the same as the output of

    # encrypt().





    if len(full_key) != 64:

        raise Exception("FULL key length shall be equal to 64")



    key = full_key[:len(full_key) //2]



    # Use the last half as the HMAC key

    hmac_key = full_key[len(full_key) // 2:]



    mac_length = 32

    iv_length = 16

    iv = ciphertext[:16]

    mac = ciphertext[-mac_length:] 



    _ciphertext = ciphertext[iv_length:-mac_length]





    iv_int = int(iv.hex(), 16) 

    ctr = Counter.new(128, initial_value=iv_int)

    # Create AES-CTR cipher.

    aes = AES.new(key, AES.MODE_CTR, counter=ctr)



    ciphertext = aes.decrypt(_ciphertext)



    # Extract the MAC from the end of the file

    hmac_obj = HMAC.new(hmac_key, ciphertext, SHA256)

    computed_mac = hmac_obj.digest()



    if computed_mac != mac:

        raise Exception("Messege integrity violated")











    plaintext= zlib.decompress(ciphertext)



    # Decrypt and return the plaintext.





    return plaintext

I'm not a security expert, so my review will focus on general best practices first.

Code structure and style

1. Imports

You can group import that belong together, e.g

from Crypto.Hash import HMAC

from Crypto.Hash import SHA256

can be written as

from Crypto.Hash import HMAC, SHA256

The Official Style Guide for Python also recommends to place import from the standard library before any third party libraries. Apart from the double import of zlib you're already following this.

2. Documentation

You started to add some kind of documentation on decrypt, so encrypt should be treated the same. The officially recommended way is to provide function documentation surrounded by """...""" like so

def decrypt(key, ciphertext):

    """Initialize counter for decryption.



    iv should be the same as the output of encrypt().

    """

   #^--- you probably also mean ciphertext here

This will also allow Python's built-in help(...) function to pick up your documentation.

3. Whitespace

You should tidy up the newlines in your code. There are often two (sometimes more) newlines within a function's body, while there are none between the imports and functions. This might be an issue from posting the code here on Code Review, but you should definitely check that. A reasonable convention many people agree on is to use two newlines to seperate functions (or imports from the following functions) and only use a single newline in function bodys to visually create logical groups of code.

The code itself

1. Exceptions

"You're not doing the right thing!" What thing you might ask? Well, ... "Read it! Find it out yourself!"

Sounds frustrating, doesn't it? Python offers a wide variety of built-in exceptions so you can easily convey the type of error that has happened, without the need to parse/read the actual error message. In your case ValueError seems to be appropriate. From the documentation:

exception ValueError
Raised when an operation or function receives an argument that has the right type but an inappropriate value, [...]

In case of an error on checking the HMAC, you might add your own exception derived from Exception (maybe HMACValidationError?). I will leave this as an exercise to you.

2. encrypt/decrypt

These functions look quite reasonable from what I can jugde and work the way I would expect them to work. A minor "issue" I found, is that they are not handling string encoding/decoding symmetrically. While encrypt does handle unicode strings by converting them to bytes representation before encryption, decrypt has no way of knowing that this step has taken place and will always return the raw bytes.

At the moment, I would favor an approach where the application using those functions has to take care that the input is already encoded into bytes and also decode the bytes back into a unicode string if desired. This would also be in line with not handling string values for the key, which strongly hints that there is an earlier step where those are created and possibly encoded.

3. Security

From what I've seen on my journey through cryptoland, it is a quite common practice to simply prepend the ciphertext with the initialiation vector (IV). Password hashing functions do something similar and tend to store the salt prepended to the actual hash to have it available for validation later. Remember: the IV does not protect your ciphertext in the same way the key does, and therefore must not be handled with the same care. It instead prevents an attacker to see the same plaintext mapped the same ciphertext repeatedly, which is maybe more than you can afford to reveal.

Something similar applys to the HMAC. The security of the HMAC authentication should only rely on the secrecy of the key, and nothing else. So as long as you use a suitable HMAC implementation and a reasonably secure key, you should be good to go. Just bear in mind that an HMAC alone does not and cannot prevent all kind of attacks, e.g. HMAC alone cannot prevent replay.