PyCrypto AES-CFB with SCrypt and HMAC

Question

I am brushing up on my Python, and since I would like to learn a bit about encryption, I figured I would give PyCrypto a try. I obviously wouldn't implement encryption in any production environment, but I am curious to see if I made any obvious mistakes from a security perspective, and also if my code could be optimized or made more pythonic.

PyCryptodome 3.4.6
quantumrandom 1.9.0
pyscrypt 1.6.2

from Crypto.Cipher import AES
import quantumrandom
import hmac
import hashlib
import pyscrypt

HMAC_KEY = "738525cf1f8acd06beb4cf6cd816d1dc"


class AuthenticationError(Exception):
    pass


def gen_key(password):
    """Derives a key from input string using SCrypt and returns it hex encoded."""   
    hashed = pyscrypt.hash(
        password=password,
        salt="2df2e24c76d4d7b37e7ffcdf787e426b",
        N=16384,
        r=8,
        p=1,
        dkLen=16)

    return hashed.encode('hex')


def encrypt(plaintext, password):
    """Takes plaintext input and returns hex encoded ciphertext
    with hex encoded HMAC-SHA-512 hash appended."""

    key = gen_key(password)
    iv = quantumrandom.binary()[:16]
    cipher = AES.new(key, AES.MODE_CFB, iv)
    data = iv.encode("hex") + cipher.encrypt(plaintext).encode("hex")
    sig = hmac.new(HMAC_KEY, data, hashlib.sha512).digest().encode("hex")

    ciphertext = data + sig

    return ciphertext


def decrypt(ciphertext, password):
    """Takes ciphertext and password input, splits it into MAC hash and message
    data, verifies the hash and returns message plaintext if successful."""

    sig = ciphertext[-128:]
    data = ciphertext[:-128]

    if hmac.new(HMAC_KEY, data, hashlib.sha512).digest().encode("hex") != sig:
        raise AuthenticationError("Message Authentication failed!")

    iv = data[:32].decode("hex")
    message = data[32:].decode("hex")
    key = gen_key(password)
    cipher = AES.new(key, AES.MODE_CFB, iv)

    return cipher.decrypt(message)


SHARED_SECRET = "correct horse battery staple"
plaintext = "If You're Typing the Letters A-E-S Into Your Code You're Doing It Wrong"
ciphertext = encrypt(plaintext, SHARED_SECRET)

print "Ciphertext:", ciphertext

print "Decrypted:", decrypt(ciphertext, SHARED_SECRET)

Answer 1

import quantumrandom

The problem is that you should not trust third parties when importing random numbers. Furthermore, getting the randomness requires a secure connection in the first place. The secure connection relies on random numbers to remain secure. A secure random number generator that has been seeded by the OS is generally the best way to go.

HMAC_KEY = "738525cf1f8acd06beb4cf6cd816d1dc"

Generally keys should not be strings, not even hexadecimal strings. Strings are easy to find. I'd rather even use a Python \b string than hexadecimals, but preferably you'd use a keystore, or keys derived from a password supplied by the user. Which brings us to the next point:

hashed = pyscrypt.hash(
    password=password,
    salt="2df2e24c76d4d7b37e7ffcdf787e426b",
    N=16384,
    r=8,
    p=1,
    dkLen=16)

Don't use single-person unmaintained libraries. Never ever use a static salt. A salt, again, is a binary string, not a normal string. 16Ki iterations is on the short scale of things.

iv = quantumrandom.binary()[:16]

If you'd have used a random salt for each encryption then you can simply derive the keys (the encryption and MAC key) and IV from it. As the salt is unique, the key and IV combination will be unique as well.

data = iv.encode("hex") + cipher.encrypt(plaintext).encode("hex")
sig = hmac.new(HMAC_KEY, data, hashlib.sha512).digest().encode("hex")

I applaud you for having the IV authenticated as well. I also applaud you for using encrypt-then-MAC which is generally considered more secure than encrypt-and-MAC or MAC-then-encrypt.

I cannot applaud you to perform the HMAC over the hexadecimal representation of the bytes though; you've just doubled the required effort for calculating it. Never confuse binary with the representation of the binary in some textual format (hex, base64).

And while we're at it, hex requires an overhead of 100% when viewed as bytes (assuming an 8 bit encoding). Base 64 is a lot more efficient. Generally: first concatenate the binary, then encode; this will generate fewer characters as well. Or keep it as binary of course; a lot of ciphertext is encoded unnecessarily (leading to so called "stringified" code).

---

One improvement would be the use of an authenticated cipher such as AES-GCM instead of performing the encryption and HMAC separately.

For larger plaintext / ciphertext you could think about piecemeal updates / streaming so that the entire plaintext / ciphertext doesn't need to be in memory all at once.