Restrict Choices for Encrypter's Ciphers
Closed this issue · 18 comments
Currently, the encrypter class lets you select broken ciphers. I propose restricting the ciphers you're allowed to select. If a broken cipher is selected, an \InvalidArgumentException should be thrown.
Do you have a list of valid cyphers then?
I'm working on it.
If you come up with a list, I can look in to implementing this if you want
Will do, thanks!
My suggestion is to use only AES either in CTR or CBC mode. However, if you just defaulted to AES-256-CTR (you can go with CTR after the IV generation is fixed away from openssl_random_pseudo_bytes) and removed setCipher() altogether it would be the most simple approach (end user would not need to tinker with different available options).
@timoh6 Thanks! As for the feedback you provided on reddit about using different keys for authentication and encryption, how would you handle that? Where would you pass in the authentication key?
One simple approach is to use PBKDF2 to derive enough material for the encryption key and MAC key from a single source. I wrote a bit about it here: http://timoh6.github.io/2014/06/16/PHP-data-encryption-cheatsheet.html#encryption-and-authentication-keys
In practice, if you go using PBKDF2, you stretch the key material first (i.e. use high enough iteration count in PBKDF2 using output length the same as the underlying hash function output size). Then when you have the "stretched" key bytes, you can run it through PBKDF2 again using 512 bits output and only a single iteration. After that you can use, say, the first 256 bits for the encryption key and the last 256 bits for the authentication key.
Doing the PBKDF2 run two times avoids a security issue in PBKDF2 algorithm which occurs if you use it to output longer lengths than the underlying hash function outputs. In other words, you stretch the keying material first and after that make it appropriate length.
Another popular approach is to use HKDF (but there is no built in support for it in PHP). But see https://github.com/defuse/php-encryption for more information about how it is handled there.
I'm going to move out the encryption key and auth key into a new issue. I've got a fix in place for restricting the ciphers.
I've committed changes based on your suggestions: https://github.com/opulencephp/Opulence/tree/develop/src/Opulence/Cryptography/Encryption
Looks better to me now. One important change should be made through: add the PBKDF2 salt under the MAC (unless I misread and it is there already).
As the PBKDF2 iteration count default is rather low, I'd stress in the corresponding documentation that the "key" should be long enough, i.e. something like "bang hands on the keyboard multiple times to create it".
Actually in the first version of TCrypto I demanded that the "key" must be at least 40 characters long (that way I avoided the need to use key stretching altogether).
Another nitpick of mine is to check the return values of random_bytes().
I salt the encryption and authentication keys when deriving them. The salt is also stored with the IV alongside the HMAC.
I'll add something to the docs about suitably long password. In your opinion, should I also permit users to specify an actual key (trust it is of high entropy), which won't force key stretching? For example, should I let users do:
$encrypter = new Encrypter(new Key("someLongRandomKey"));
// Or for passwords
$encrypter = new Encrypter(new Password("someLowEntropyPassword"));I tried an iteration count of 100,000, but it was taking 200 milliseconds on my development server, which feels unacceptably slow for a Web app.
What do you mean check the return value of random_bytes()? I don't think it returns null or false or anything if it fails to generate a strong value.
I think it makes sense to allow bypassing the key stretching phrase by using a proper encryption key (your example looks reasonable). As there is possibility to choose AES with different key lengths, maybe the "keying process" should be designed with "automatic key length adjust" in mind.
I.e. the "Password" key deriver should return the encryption key precisely as long as the underlying AES version needs (128/192/256 bits) and the "Key" key deriver should just use single iteration of PBKDF2 with appropriate length output size (128/192/256 bits).
In addition to that, as the cipher can be changed, it should be also encoded (appended) in the payload and make the MAC check cover it too.
200 ms is, in general, indeed too much. The lack of heavier stretching can be easily mitigated by using a longer password, so I think this is more a documentation issue. Maybe defaulting to 50 000 iterations would be fine?
Regarding the PBKDF2, (I almost forgot) it could be sensible to use SHA-512 as the underlying HMAC's hash algorithm. In general, it is a bit more resistant to GPU crackers than SHA-256, but on the otherhand, it also slows down honest users on 32-bit systems. But maybe in 2016 it is reasonable to assume that majority of Opulence installs will be on 64-bit systems.
The return value check I mentioned is just a habit of mine to make sure you get the right amount of data back (not an empty string for example). I admit such edge situations are extremely unlikely (if not impossible), but dealing with security sensitive code it's better be safe than sorry.
Thanks for the review! I'll implement something and get back to you.
I've made the appropriate changes: https://github.com/opulencephp/Opulence/tree/develop/src/Opulence/Cryptography/Encryption
Starts to look good to me. At first I thought the "version" tag was not included in the MAC, but that was a mistake of mine (it's a wise move to use a version tag).
Maybe the version should be extracted from the payload instead of class variable (so that backwards compatibility is easier to maintain, if the encryption protocol ever needs to be updated)?
One more thing to improve would be to add length checks to getPieces(), i.e. MAC must be exactly X bytes etc.
Now we could use more eyes to look at the code, pinging @paragonie-scott
I've added validation to the pieces: https://github.com/opulencephp/Opulence/blob/develop/src/Opulence/Cryptography/Encryption/Encrypter.php. Right now, since I only have version 1.0.0, I don't extract the version when decrypting. If/when I update the encryption protocol, I'll start reading that version number and decrypt it using the correct version.
If this is being used on raw binary, you probably want mb_substr() with '8bit'. If you pull in our constant-time encoding library, you can use the Binary class which is always mbstring.func_overload safe.
Thanks Scott! I really appreciate the feedback. I have restricted all PHP functions in the crypto library to be in the global namespace.