

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.<p>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<p>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?<p>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...).<p>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/...](https://www.isecpartners.com/news-
events/news/2011/february/double-hmac-verification.aspx)

    
    
        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
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...](http://codereview.stackexchange.com/questions/19910/simple-
crypto-library-in-python-correct-and-secure)

~~~
marshray
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.

------
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
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...](http://www.daemonology.net/blog/2009-06-11-cryptographic-right-
answers.html) (not me!)

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

[https://github.com/andrewcooke/simple-
crypt/blob/master/src/...](https://github.com/andrewcooke/simple-
crypt/blob/master/src/simplecrypt/__init__.py) (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."

