use password hash to encrypt private key

[schiessle]

use password hash instead of the plain password to encrypt the private key as discussed with @LukasReschke .

What should be tested:

• setup ownCloud with master
• enable encryption
• create recovery key (private recovery key shouldn't have "keyFormat" in the header)
• re-login (user should have a private key which doesn't contain "keyFormat" in the header)
• upload a encrypted file
• switch to this branch
• create a new user (new user should have a private key which has "keyFormat:hash" in the header)
• check if both the new user and the old one can create new files and read existing ons
• change recovery key password (new key should have "keyFormat:hash" in the header)
• the old user changes his password in his personal settings (updated private key should have "keyFormat:hash" in the header) and he should still be able to read/write his files after re-login

[karlitschek]

What are the consequences regarding migration?

[schiessle]

What are the consequences regarding migration?

Nothing, we don't change existing keys.

• New keys will be encrypted with the hash instead of the login password
• If we change the password of a existing key it will use the new hash
• The header of the encrypted key will indicate which method was used (similar to what we did for the aes-128 vs aes-256 setting)

[LukasReschke]

This is a nifty one. Basically this will always return 0, the + operator is the wrong one here.

Use please . as otherwise the salt will always be 5feceb66ffc86f38d952786c6d696c79c2dbc239dd4e91b46729d73a27fb57e9, on every instance.

Going further, it might very much make sense to include the user id as well as another work factor:

$salt = hash('sha256', $uid . $instanceId . $instanceSecret, true);

Note that you should use $uid as first argument to prevent potential padding problems etc.

[schiessle]

Use please . as otherwise the salt will always be

Indeed this should have been a ., good catch. Will fix it and also add the uid.

[schiessle]

Hm, adding the uid is not that easy. We also have the "public link share"-key and the recovery-key. For them the uid isn't unique. Multiple admins could have access to the recovery key and the "public link share"-key gets created for whoever login first. Further at this point we have no idea which kind of key gets encrypted. So I would skip the uid for now.

[LukasReschke]

That's acceptable as well. The expectation here is "to make it a little bit harder" and not to offer the best possible security with regards to time-memory trade-off attacks. We can't anyways as the admin could intercept data on it's own.

[schiessle]

If we touch the code now then let's do it as good as possible. I had another look and added the uid for all regular users, for system users the uid is just a empty string

[LukasReschke]

When using in_array please always set the strict parameter to true.

in_array($keyFormat, $this->supportedKeyFormats, true)

[schiessle]

fixed and ready to push once Jenkins finished

[LukasReschke]

Code looks good to me. Basic functionality testing also did not find any bugs.

@bantu I'd appreciate your feedback on this one as well. Basically previously the encryption key of an user was only encrypted with the user password which limited the potential key space and also made brute-force attacks way easier than necessary. Using PBKDF2 should be a somewhat better approach. It's not perfect but better than before :-)

[ghost]

🚀 Test PASSed.🚀

[scrutinizer-notifier]

A new inspection was created.

[bantu]

@LukasReschke Without looking to deep into the code changes and from the point of interacting crypto primitives this is much better and more proper indeed.

Please note that you already depend on phpseclib and there is PBKDF2 support in it via the setPassword() method on Crypt objects. https://github.com/phpseclib/phpseclib/blob/2.0/phpseclib/Crypt/Base.php#L536

[LukasReschke]

Please note that you already depend on phpseclib and there is PBKDF2 support in it via the setPassword() method on Crypt objects. https://github.com/phpseclib/phpseclib/blob/2.0/phpseclib/Crypt/Base.php#L536

Mhm. Seems like adjusting this for our use-case would require also some more work. Considering that the embedded library also is somewhat widely used and only a fallback for PHP 5.4 I'd keep it as-is.

👍 from my side.

[ghost]

🚀 Test PASSed.🚀

[schiessle]

needs a second reviewer... maybe @nickvergessen or @PVince81 ? Thanks!

[nickvergessen]

👍