Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Encrypting/decrypting very slow due to use of de-/encryptWithPassword() #812

Closed
SunMar opened this issue Nov 16, 2017 · 7 comments
Closed

Comments

@SunMar
Copy link
Contributor

SunMar commented Nov 16, 2017

Hi,

During tests we discovered that encrypting and decrypting is very slow. This is caused by the use of the PBKDF2 algorithm in KeyOrPassword (from the defuse/php-encryption dependency), which can be traced back to CryptTrait where the encryptWithPassword() and decryptWithPassword() methods are used. The use of this algorithm is secure if you're unsure about how good your encryption key is, however if you know you have a secure encryption key, then PBKDF2 is not needed.

An easy way to get a big performance boost here is by allowing a Key object to be used as encryption key instead of a string. If it's a Key object then encrypt() instead of encryptWithPassword() and decrypt() instead of decryptWithPassword() can be used. With a Key the encryption library trusts the key and skips PBKDF2 using only the much faster HKDF algorithm.

This could be implemented backwards compatible by checking in CryptTrait if $this->encryptionKey is an instance of Key or not. I can see to make a pull request for this but would first like to know if something like this would be accepted.

@Sephster
Copy link
Member

In principle I can't think of an issue with what you are proposing as long as it is backwards compatible but it would be good to get more information. If you can provide any links that can back up your assertions that would be appreciated. I noted from the php-encryption lib that they note PBKDF2 is slower but I didn't find much in the way of pros and cons between password vs key other than that.

Is the use of encryptWithPassword adversely affecting your production system? If you could provide some more context about the issue being faced and any benchmarks you have got from your tests as well that would really help us to make a decision. Thanks for your time

@SunMar
Copy link
Contributor Author

SunMar commented Nov 16, 2017

See defuse/php-encryption#359 for information about PBKDF2 being slow vs using a Key. Unfortunately I can't give any benchmarks but our performance tests showed that the PBKDF2 hashing algorithm was forming a big bottleneck, taking up to 50% of a request's total processing time. Once we put some hacks in place to use a Key instead, the bottleneck disappeared. The HKDF hashing did not take significantly more time than any other code being run during the request.

@SunMar
Copy link
Contributor Author

SunMar commented Nov 16, 2017

Also see defuse/php-encryption#161 which is the original use case for implementing the PBKDF2 algorithm. From what I understand it's for cases where you don't store the key on the server, but have it supplied by the user in the form of a password. PBKDF2 is then used to turn the password into an encryption key and it's meant to be slow to protect against attacks like brute force. However if you are storing the encryption key on the server, and it's a strong encryption key then PBKDF2 is not necessary, only HKDF should be secure enough. The Key object by default generates strong keys that are 32 bytes long and because it uses random_bytes() it's not limited to human readable characters like a password is. The Key object even forces you to use its createNewRandomKey() function by not allowing the class to be changed through final and only accepting its own custom format for import/export of keys. So I think it might even be better practice anyways to use a Key instead of a password for the encryption key via AuthorizationServer. The only con I can think of is that you need to store the Key somewhere, you can't use a key you already have because you can't import just anything as a Key object, but that is an issue I guess that would need to be addressed by the person implementing thephpleague/oauth2-server and doesn't need to be a concern of this package itself.

@SunMar
Copy link
Contributor Author

SunMar commented Nov 20, 2017

Perhaps when this is merged and in a release, the following documentation should be updated too to reflect the ability to use a Key:

V5 Security Improvements#6.0.0

Installation#Generating encryption keys

From skimming through he documentation I couldn't find other places referencing encryption keys.

@SunMar
Copy link
Contributor Author

SunMar commented Nov 23, 2017

Ok, I found the gh-pages branch and also created a PR #820 for that branch to reflect the changes in the documentation, if they are accepted.

@sg3s
Copy link

sg3s commented Jan 25, 2018

Hi,

I just wanted to note that I encountered this exact issue too, when I was implementing the (php-encryption) library directly in my own project for different purposes (after having found it in this project).

In my case when decrypting just one item while using decryptWithPassword the PBKDF2 overhead accounted for almost 200ms of the process time for a request that took ~260ms total, which was ~60ms without decrypting the value (profiled with xdebug / qcachegrind).

After switching to the methods described in the defuse/php-encryption documentation this slowdown essentially disappeared.

Checkout the neat bin script the lib provides to generate a key, might want to include that in the docs maybe?

The performance hit is significant, I came here to see if it was a known problem. Good to see someone already beat me to it. Would be good to see this updated upstream :)

Sephster added a commit that referenced this issue Feb 28, 2018
Allow CryptTrait to accept a \Defuse\Crypto\Key as encryption key #812
@Sephster
Copy link
Member

Sephster commented Apr 2, 2018

Closing this issue as @SunMar has produced a fix which will be added to the next release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants