4

> **Update (6 February 2015):** > It seems like the class in question has actually updated as a response to the answers here (through an issue, now closed, raised on github).

I was looking for an easy to use library for encrypting files in PHP. PHP provides encryption functionality through the [mcrypt library](http://php.net/manual/en/book.mcrypt.php). Yet, there is a lot of configuration options and choices, enough of them to make a lot of mistakes.

So, instead of making those mistakes ourself, I searched for a drop in class (open source license, no framework constraints) which has already figured out how to apply the functionality securely. There is a lot of examples out there, but I cannot seem to find any code which has been reviewed by anyone with any background knowledge on this.

The best candidate I have found so far is on github: Pixelfck/SymmetricEncryption
The code seems pretty well written, the class is quite compact and well commented, so that is at least a good sign. Yet I don't think ourselves qualified to judge the security part of it.

Could anyone with more background on this read through the code and see if there are any issues?

Bounty update

I've quite some questions I hope to see answered (don't hesitate to pick the code apart even more):


First: the class includes two 'userland-implementations' of cryptographic functions:

Are those two implementations actually correct?


Second: some 'odd' things I spotted:

  • The HKDF 'extract' step has been replaced by the use of PBKDF2, is this an accepted idea for the intended use?
  • The number of rounds used for PBKDF2 is added (unencrypted) to the output of the encrypted data and used without/before being authentication by the hmac. isn't this a risk?
  • PBKDF2 is limited to a single 'block' only, isn't this contrary to how PBKDF2 is intended to function?

Third: some 'minor' questions:

  • Is 2^12 (= 4096) indeed an acceptable minimum number of rounds for PBKDF2 when used in this fashion?
  • Is 128 bits of salt for PBKDF2 indeed enough 'with safety margin to spare'?
  • The hmac key is 256 bits long, is this enough in this case?

Maybe I'm just worrying too much about this, but I would rather not use a third-party library that is flawed.

Community
  • 1
Monika
  • 1,092
  • 1
  • 10
  • 21

2 Answers2

9

No, this is not secure.

  • The HMAC check is vulnerable to timing attacks. Since the author uses a standard string comparison, the check stops as soon as one character from the provided HMAC doesn't match the corresponding character from the expected HMAC. So the longer the common prefix, the longer the check takes. By carefully observing those time differences, an attacker may be able to reconstruct the expected HMAC and “forge” arbitrary data. This is a well-known error.
  • The length parameter of strlen() and substr() refers to characters rather than bytes in some PHP setups, which will break critical functionalities of the library. For example, the popular “mbstring” extension provides encoding-aware implementations of strlen() and substr(). In this case, multiple bytes may be counted as a single multi-byte character.
  • Encryption together with compression is known to be problematic in certain scenarios (see the CRIME and BREACH attack against SSL/TLS). The compression feature is turned off by default, though.
  • The cipher configuration is arcane. For example, the desired length of the PBKDF2 output is specified indirectly through the PBKDF2_BLOCK_COUNT constant which is the minimum multiple of the hash function output required for the target length. This creates a huge risk of user mistakes, especially when the library is marketed as an easy alternative to Mcrypt.
  • The author confuses the block size of AES with its various key sizes. MCRYPT_RIJNDAEL_128 is not AES-128. The numbers of the MCRYPT_RIJNDAEL_* constants refer to the block size of the Rijndael algorithm, whereas the numbers of the AES variants refer to different key lengths. AES is Rijndael with a block size of 128 bits and a key size of 128 bits, 192 bits or 256 bits. So to get AES in PHP, you use MCRYPT_RIJNDAEL_128 and a key string with either 16, 24 or 32 bytes. The library uses a 32-byte key by default, which means it employs AES-256, not AES-128.
  • The key derivation looks rather odd: Both the encryption key and the HMAC key are based on the same output of the PBKDF2 function. The only reason why they're different is because the literal strings "EncryptionKey" and "AuthenticationKey" are mixed into the HKDF call. This isn't necessarily wrong. In fact, the RFC specifically supports an extra input parameter. But it might be safer and more robust to generate enough key material for both applications and then split it.

The main problem, however, is that the library relies heavily on home-grown implementations when it should use standard solutions. For example, combining encryption and authentication should be done with AES-GCM. The PHP OpenSSL extension also provides a complete PBKDF2 implementation.

Fleche
  • 4,024
  • 1
  • 17
  • 20
  • Intersting. Your issue with the userland implementation are of course valid. Yet, I'm not aware of AES-GCM being available to PHP? So that leaves the PHP programmers with 'doing it ourselves' (see if we can find a well reviewed implementation). Compression with encryption is a real issue and apparently so is the timing attack vs the HMAC (although I don't see how, but that's not the point). I don't see how using AES-256 instead of AES-128 lowers the security of the implementation, yet it does show that the author made a mistake. – Monika Dec 08 '14 at 15:21
  • Also, I did some more searching on my own questions and came up with: Limiting PBKDF2 in its output length is rather common (and even [suggested here](http://security.stackexchange.com/questions/53115/how-to-know-which-output-length-to-request-from-pbkdf2)); The HKDF function called twice is with different 'info' parameter [seems to be as designed](http://crypto.stackexchange.com/a/17840). (My personal opinion is that anyone who uses mb_* function overloading should be fired. Yet people *can* do it, so some undoubtedly will.) – Monika Dec 08 '14 at 15:22
  • AES-GCM and PBKDF2 are available through the [PHP OpenSSL extension](http://php.net/manual/en/book.openssl.php). Of course that begs the question: Why not use OpenSSL in the first place and avoid this low-level Mcrypt stuff? And that's indeed the recommended approach. Mcrypt should only be used for very simple use cases (like plain AES). – Fleche Dec 08 '14 at 15:40
  • The reason why I pointed out the author's confusion and the key derivation scheme is because a security library has to meet much higher demands than other software. If this library only had to, say, generate fancy graphics, I wouldn't care about the author's confusion as long as the output is OK. But we're talking about sensitive data (otherwise we wouldn't need encryption in the first place). And that means it's not enough for the library to kinda sorta work by accident. It must be 100% correct *and* robust *and* easy to use. If it lacks any of those properties, then it's not a good library. – Fleche Dec 08 '14 at 16:44
  • This sucks. I thought I found a good cipher class for PHP. Is there any at all? – datasn.io Feb 05 '15 at 04:13
  • How about this one? http://php.net/manual/en/function.mcrypt-encrypt.php#78531 – datasn.io Feb 05 '15 at 04:14
  • The PHP comments are infamous for their nonsense, so this isn't really worth talking about. Anyway: The author clearly doesn't know what he/she is doing. There's the extremely insecure ECB mode, there's a dummy initialization vector which is never used (that's the whole idea of ECB), there's SHA-256 as a poor man's key derivation function, there's the confusion of MCRYPT_RIJNDAEL_256 and AES-256. It's simply a hack job written by somebody with no security background whatsoever. – Fleche Feb 05 '15 at 07:32
  • @Fleche openssl_php doesn't support AES-GCM. Even though it exists as a cipher mode, it doesn't work because the openssl_encypt doesn't return the authentication tag, nor does openssl_decrypt take one. – Xander Feb 06 '15 at 21:06
  • You're right, GCM is currently not supported. However, the PHP core developers [seem to be aware of this](http://www.serverphorums.com/read.php?7,1124092), so there's still hope to get GCM support in a future PHP version. – Fleche Feb 06 '15 at 23:55
2

The Pixelfck/SymmetricEncryption class seems to hit everything I would look for in a mcrypt abstraction system. It is using derived keys using what looks to be a standard algorithm (PBKDF2), employes AES128 in CFB mode, and uses a hash_hmac of SHA256.

I would say it is reasonable to expect this class provides Authenticated Encryption and doesn't appear to have any of the common mistakes.

The only thing I see which you should be aware of is it uses MCRYPT_DEV_URANDOM as a random source, which could lead to a weakening of security under extremely high use on a system without a hardware random source, but for most applications it should work fine.

I would use it.

DuneWalker
  • 61
  • 5
  • 1
    `MCRYPT_RIJNDAEL_128` is *not* AES-128, and the library actually uses AES-256. The numbers of the `MCRYPT_RIJNDAEL_*` constants refer to the block sizes of the Rijndael algorithm, whereas the numbers of the AES variants refer to different key sizes. AES is Rijndael with a block size of 128 bits and a key length of 128 bits (AES-128), 192 bits (AES-192) or 256 bits (AES-256). The library uses `MCRYPT_RIJNDAEL_128` together with a 32-byte key, so that's AES-256. – Fleche Dec 05 '14 at 03:27
  • As far as I can tell from the code, this is pulling in the key size with the function mcrypt_get_key_size() which has always matched the block size that I have seen. – DuneWalker Dec 05 '14 at 15:28
  • This question got me wondering, so I actually tested the case with `echo mcrypt_get_key_size(MCRYPT_RIJNDAEL_128, MCRYPT_MODE_CFB);` which yielded 32, not 16 which I was expecting. Very interesting, I hadn't heard about this distinction before. Thank you for pointing it out! – DuneWalker Dec 05 '14 at 15:40