5 ms·

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

13 points by andrewcooke 14y ago

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.

andrewcooke 14y ago
Clickable links: http://www.daemonology.net/blog/2009-06-11-cryptographic-right-answers.html http://www.daemonology.net/blog/2009-06-11-cryptographic-rig... (not me!) https://github.com/andrewcooke/simple-crypt https://github.com/andrewcooke/simple-crypt (main page, inc example) https://github.com/andrewcooke/simple-crypt/blob/master/src/simplecrypt/__init__.py https://github.com/andrewcooke/simple-crypt/blob/master/src/... (source)
e1ven 14y ago
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 14y ago
As the saying goes: "Someone is always going to build a bigger picture idiot."
cperciva 14y ago
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 14y ago
  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?]
marshray 14y ago
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/double-hmac-verification.aspx 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 14y ago
  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-crypto-library-in-python-correct-and-secure http://codereview.stackexchange.com/questions/19910/simple-c...
  - 14y ago

13 points by andrewcooke 14y ago

andrewcooke 14y ago

Clickable links: http://www.daemonology.net/blog/2009-06-11-cryptographic-right-answers.html http://www.daemonology.net/blog/2009-06-11-cryptographic-rig... (not me!) https://github.com/andrewcooke/simple-crypt https://github.com/andrewcooke/simple-crypt (main page, inc example) https://github.com/andrewcooke/simple-crypt/blob/master/src/simplecrypt/__init__.py https://github.com/andrewcooke/simple-crypt/blob/master/src/... (source)

e1ven 14y ago

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 14y ago

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

cperciva 14y ago

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 14y ago
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?]

marshray 14y ago

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/double-hmac-verification.aspx 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 14y ago
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-crypto-library-in-python-correct-and-secure http://codereview.stackexchange.com/questions/19910/simple-c...
- 14y ago