Show HN: An idiot-proof crypto library for Python 3 (feedback, please)

[andrewcooke]

I really couldn't find a simple wrapper to pycrypto that would encrypt data correctly in Python 3.

So I tried to follow the instructions at http://www.daemonology.net/blog/2009-06-11-cryptographic-right-answers.html (written by HN's cperciva) and wrote https://github.com/andrewcooke/simple-crypt

Is that OK? Given the problems Tarsnap had with CTR mode, was it wrong to follow those instructions anyway? Have I got the offset handling right?

It seemed like it would be better to write this once, get it revised here, and then publish it (I'll make an egg for pypi once it seems OK) than quietly write my own and let others do the same (it was surprisingly complex - I mention the Tarsnap issues because after reading about them realised I had the same error...).

Apologies if I overlooked an existing package. And thanks for any constructive criticism.

[marshray]

    def encrypt(salt, password, data):
                ^^^^^^

You're the crypto library. Don't ask the user to choose the salt, they won't do it right.

    key = _expand_key(salt, password)

Most crypto people would advise you to generate two separate keys for encryption and MAC.

    hmac = data[-HMAC_HASH.digest_size:]
    hmac2 = HMAC.new(key, data[:-HMAC_HASH.digest_size], HMAC_HASH).digest()
    if hmac != hmac2: 
       ^^^^^^^^^^^^^

Possible timing attack. Consider doing another HMAC of each HMAC and comparing those instead. See https://www.isecpartners.com/news-events/news/2011/february/...

    offset = getrandbits(AES.block_size*8)
    Counter.new(AES.block_size*8, initial_value=offset, allow_wraparound=True)

I don't like that you allow wraparound counters, in fact, I'd go so far as to call it broken. This is not allowed by NIST-specified CTR mode.

Bad Things^TM will happen if multiple messages (or different parts of a single message) are ever encrypted with the same key. Seriously, screwups like this have shortened wars.

     return prefix + encrypted + hmac

Put some type of magic constant in front of this. You'll be glad you did later when you have to change the format or simply the PBKDF2 count.

     HMAC_HASH = SHA256

but later... def _expand_key(salt, password): ... return PBKDF2(....)

I don't see where you're telling PBKDF2 what hash to use for its HMAC. It's probably defaulting to something like SHA-1.

[andrewcooke → marshray]

whoa. thanks. about to go to bed(!) but will address that (dual hmacs) tomorrow am. thanks again!

edit: on the wraparound counters, i can replace it with a nonce (prefix) and smaller counter (and no wraparound). i thought they were equivalent, but people do seem to prefer a prefix (my reasoning was that if you combine nonce and counter with addition rather than concatenation then you need wraparound since you may start at the end of the counter).

edit2: so what's a reliable way of providing salt? i can't think of one that's context-independent except for simply generating some random junk and appending it to the message. is that ok? (i guess so)

ps, also, while here, i will drop this link which so far has no replies http://codereview.stackexchange.com/questions/19910/simple-c...

[marshray → andrewcooke]

Yeah, sorry for all the edits.

Just pick a secure random number of 128 bits for the salt.

And read the NIST docs on CTR mode. Merry Xmas.

[andrewcooke → marshray]

quick update (i need to review the code some more, but this is slipping down the page and i need to go do some errands) - i have fixed all the above, i believe.

the salt is now 128 bits of random data, included with the message (and covered by the HMAC, along with the header).

HMACs are compared using a second hash.

the counter is 1/2 a block of random nonce (taken from the salt) and 1/2 a block of incremented counter, as described in the NIST docs (their second option in section B2 of http://csrc.nist.gov/publications/nistpubs/800-38a/sp800-38a...).

the message includes a 4 byte header that is checked on decryption (in general, the code includes a bunch more checks and exceptions).

PBKDF2 now explicitly uses SHA256 (you were right; the default was SHA1).

thank-you again!

ps just fyi (i am not using it, but stumbled across this while checking things again) wraparound does seem to be ok - the standard incrementing function in section B2 is "mod 2^m" (so wraps; the example given actually includes a wrap!). but still, they recommend a separate nonce and incrementing block (rather than incrementing from a random offset) (which is what i have changed to).

[cperciva]

Given the problems Tarsnap had with CTR mode

FWIW, the Tarsnap bug wasn't a cryptographic mistake, it was a refactoring mistake -- I knew what the code should be doing, and the code was doing exactly what the code should be doing, right up to the point when I decided to tidy it up.

The lesson to be learned from that isn't "crypto is hard" or "CTR is dangerous", but rather "pay attention when you rewrite existing code".

[andrewcooke → cperciva]

ok; in my case i was confused by the way pycrypto handles ivs for ctr mode (it ignores the iv and expects an explicit offset or prefix to the counter). so initially i had the same encryption every time.

but anyway, ignoring that red herring, if you have the time, confirmation that it seems to do what you described would be appreciated...

[edit: fwiw, other concerns i have include: is there any reason why hmac shouldn't use the same key as the main cipher; is it better to use a random counter offset or a smaller counter and a prefix / nonce?]

[andrewcooke]

Clickable links:

http://www.daemonology.net/blog/2009-06-11-cryptographic-rig... (not me!)

https://github.com/andrewcooke/simple-crypt (main page, inc example)

https://github.com/andrewcooke/simple-crypt/blob/master/src/... (source)

[e1ven]

Very cool! I've been using m2Crypto, Tomcrypt, and PyCrypt manually over the last year or two - I've tried various variations, and changed it up a few times.

Most recently, though, I've moved to GPG, because frankly, I don't trust myself to do it right.

Your library looks cool, but with all respect, I don't trust you either yet ;)

Good luck with this - It seems really useful, and I hope it gets some more exposure, and can cook for a bit.

[mcherm]

As the saying goes: "Someone is always going to build a bigger picture idiot."