Hacker News new | comments | show | ask | jobs | submit login

    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.




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


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.


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).




Guidelines | FAQ | Support | API | Security | Lists | Bookmarklet | DMCA | Apply to YC | Contact

Search: