Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Apologies if this is too broad.

Is the code below (also available at github with tests, example, and description of algorithms) correct and secure?

It follows the recommendations at http://www.daemonology.net/blog/2009-06-11-cryptographic-right-answers.html as far as I can tell.

from Crypto.Cipher import AES
from Crypto.Hash import SHA256, HMAC
from Crypto.Protocol.KDF import PBKDF2
from Crypto.Random.random import getrandbits
from Crypto.Util import Counter


EXPANSION_COUNT = 10000
AES_KEY_LEN = 256
SALT_LEN = 128
HASH = SHA256
HEADER = b'sc\x00\x00'

# lengths here are in bits, but pcrypto uses block size in bytes
HALF_BLOCK = AES.block_size*8//2
assert HALF_BLOCK <= SALT_LEN  # we use a subset of the salt as nonce



def encrypt(password, data):
    '''
    Encrypt some data.  Input can be bytes or a string (which will be encoded
    using UTF-8).

    @param password: The secret value used as the basis for a key.
     This should be as long as varied as possible.  Try to avoid common words.

    @param data: The data to be encrypted.

    @return: The encrypted data, as bytes.
    '''
    data = _str_to_bytes(data)
    _assert_encrypt_length(data)
    salt = _random_bytes(SALT_LEN//8)
    hmac_key, cipher_key = _expand_keys(password, salt)
    counter = Counter.new(HALF_BLOCK, prefix=salt[:HALF_BLOCK//8])
    cipher = AES.new(cipher_key, AES.MODE_CTR, counter=counter)
    encrypted = cipher.encrypt(data)
    hmac = _hmac(hmac_key, HEADER + salt + encrypted)
    return HEADER + salt + encrypted + hmac


def decrypt(password, data):
    '''
    Decrypt some data.  Input must be bytes.

    @param password: The secret value used as the basis for a key.
     This should be as long as varied as possible.  Try to avoid common words.

    @param data: The data to be decrypted, typically as bytes.

    @return: The decrypted data, as bytes.  If the original message was a
    string you can re-create that using `result.decode('utf8')`.
    '''
    _assert_not_string(data)
    _assert_header_sc(data)
    _assert_header_version(data)
    _assert_decrypt_length(data)
    raw = data[len(HEADER):]
    salt = raw[:SALT_LEN//8]
    hmac_key, cipher_key = _expand_keys(password, salt)
    hmac = raw[-HASH.digest_size:]
    hmac2 = _hmac(hmac_key, data[:-HASH.digest_size])
    _assert_hmac(hmac_key, hmac, hmac2)
    counter = Counter.new(HALF_BLOCK, prefix=salt[:HALF_BLOCK//8])
    cipher = AES.new(cipher_key, AES.MODE_CTR, counter=counter)
    return cipher.decrypt(raw[SALT_LEN//8:-HASH.digest_size])



class DecryptionException(Exception): pass
class EncryptionException(Exception): pass

def _assert_not_string(data):
    # warn confused users
    if isinstance(data, str):
        raise DecryptionException('Data to decrypt must be bytes; ' +
        'you cannot use a string because no string encoding will accept all possible characters.')

def _assert_encrypt_length(data):
    # for AES this is never going to fail
    if len(data) > 2**HALF_BLOCK:
        raise EncryptionException('Message too long.')

def _assert_decrypt_length(data):
    if len(data) &lt; len(HEADER) + SALT_LEN//8 + HASH.digest_size:
        raise DecryptionException('Missing data.')

def _assert_header_sc(data):
    if len(data) &lt; 2 or data[:2] != HEADER[:2]:
        raise DecryptionException('Data passed to decrypt were not generated by simple-crypt (bad header).')

def _assert_header_version(data):
    if len(data) &lt; len(HEADER) or data[:len(HEADER)] != HEADER:
        raise DecryptionException('The data appear to be encrypted with a more recent version of simple-crypt (bad header). ' +
        'Please update the library and try again.')

def _assert_hmac(key ,hmac, hmac2):
    # https://www.isecpartners.com/news-events/news/2011/february/double-hmac-verification.aspx
    if _hmac(key, hmac) != _hmac(key, hmac2):
        raise DecryptionException('Bad password or corrupt / modified data.')

def _expand_keys(password, salt):
    if not salt: raise ValueError('Missing salt.')
    if not password: raise ValueError('Missing password.')
    key_len = AES_KEY_LEN // 8
    # the form of the prf below is taken from the code for PBKDF2
    keys = PBKDF2(_str_to_bytes(password), salt, dkLen=2*key_len,
        count=EXPANSION_COUNT, prf=lambda p,s: HMAC.new(p,s,HASH).digest())
    return keys[:key_len], keys[key_len:]

def _random_bytes(n):
    return bytes(getrandbits(8) for _ in range(n))

def _hmac(key, data):
    return HMAC.new(key, data, HASH).digest()

def _str_to_bytes(data):
    try: return data.encode('utf8')
    except AttributeError: return data

Finally, see also this HN thread which identified many issues in an earlier version of the code.

share|improve this question
add comment

1 Answer

up vote 5 down vote accepted

I'm not a crypto expert, but I see that at least your decryption code is vulnerable to verification timing attacks (see e.g. here: http://rdist.root.org/2009/05/28/timing-attack-in-google-keyczar-library/)

share|improve this answer
    
you're right, thanks, although it has been fixed since i posted here (i now compare the hash of the hmacs). i will update the code in the question (for the record i had if hmac != hmac2) –  andrew cooke Dec 28 '12 at 19:03
    
@andrewcooke, comparing hashes of hmacs is still a verification timing vulnarability, only of a different kind. You have to switch to a constant-time comparison method which executes in exactly the same time regardless of the string possible inequality location, the referenced page contains one such algorithm. –  abbot Dec 28 '12 at 21:01
    
it's the solution described here isecpartners.com/news-events/news/2011/february/… - can you explain why that's wrong? is it a different problem? they argue (afaict) that it's equivalent to a constant time comparison (that link is in the code next to the comparison) –  andrew cooke Dec 28 '12 at 22:13
    
@andrewcooke, double hashing makes this vulnerability much harder to exploit, but if you approach this from the cryptographer's point of view (in terms of a game played by an adversary against your crypto implementation), then this "new" approach still does disclose some information about the ciphertext, while a proper algorithm must disclose no information. It is not easily exploitable under current knowledge, but as usual this is just a matter of time and theory. I don't understand why not simply use a constant time implementation which is proven to be resistant to this attack. –  abbot Dec 28 '12 at 23:12
    
i want code that follows clear guidelines from standard authorities. as far as i can see that reference is clear and authoritative. you started by saying you were not an expert. now you are arguing against an article by someone who, as far as i can tell, is. in my (unexpert) opinion, either is fine. edit: hmm, although it looks like i may have screwed up my implementation. i will fix that later. –  andrew cooke Dec 28 '12 at 23:21
show 1 more comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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