search

slimphp / Slim

Slim is a PHP micro framework that helps you quickly write simple yet powerful web applications and APIs.
http://slimframework.com
MIT License
11.98k stars 1.95k forks source link

Mini Encryption Audit #1037

Closed defuse closed 9 years ago

defuse commented 9 years ago

Looking at https://github.com/slimphp/Slim/blob/develop/Slim/Crypt.php...

  1. The IV is not included in the HMAC. It needs to be, otherwise you can modify the IV to modify the plaintext.
  2. It uses MCRYPT_RIJNDAEL_256. This is not AES, it's the 256-bit block version of rijndael. You want MCRYPT_RIJNDAEL_128.
  3. The same key is used for encryption and authentication. This is a bad practice. You need to use something like HKDF to make two keys.
  4. It's not mbstring.func_overload-safe (the strlen() in validateKeyLength()).
  5. It uses zero-byte padding. If the plaintext ends in zero bytes they will get stripped, violating integrity. Use PKCS#5 padding. Actually, it's much worse than this, since it's replacing all zero bytes, not just the ones at the end.
  6. Exceptions leak the key: https://gist.github.com/defuse/c197a18a7b5a0667c92d

I only spent 10 minutes looking, so there are almost certainly more problems. Moral of the story: Please, please, please, please don't write yet another broken PHP encryption library. I've already written one, and have taken the time to get everything right. Please, just use that one, or make sure you carefully go through it and address all of the things that can go wrong.

padraic commented 9 years ago

@defuse Could this not have been reported privately?

defuse commented 9 years ago

@padraic I saw no need. This is the development branch and my understanding is that it's not what everyone is using. The crypto everyone is using is actually worse, see #1035.

silentworks commented 9 years ago

I somewhat feel this would have been better disclosed privately first. Also parading it on Twitter doesn't help either, we know you probably know more about security than most of us, but don't think trying to make others work look bad is the way to prove that.

Anyway back to the original point, please share privately first next time, if you realise no-one is taking note of it after 30 days then by all means disclose it publicly.

sarciszewski commented 9 years ago

Please post a security@ contact address and PGP key on the project website then.

padraic commented 9 years ago

It just took me 10 seconds to locate an email for Josh. Not all communications require a security@ email address or a PGP key. A normal unencrypted email still has some reasonable degree of privacy that is better than a public github issue. A DM over Twitter can often solicit an email contact.

The proverbial cat is running far from the bag, so I'll return the topic to the issue reported.

defuse commented 9 years ago

@silentworks I tweeted this to raise awareness about this common problem. Slim is not the first framework I've seen trying to implement this exact same thing, and they are invariably insecure in the exact same ways. I don't "try to make others work look bad." I care only about the technical facts and what I can do to make things more secure, not about the people behind it. If I see something wrong, I will point out the technical details; it's never intended as an attack against the author.

codeguy commented 9 years ago

Just migrated the develop branch over to Zend\Crypt.

https://github.com/slimphp/Slim/commit/c5b6c76a2bba90dfab300280ed9258e2263edb52