Improve security & performance of Encrypt then MAC scheme. #23427
Conversation
Additionally make sure the IV is decoded from hexadecimal to binary for versions > 2.
`OC\Security\ISecureRandom` is no longer used.
Appending string `"a"` to the `$password` variable and using it as a cryptographic key is no longer needed, due to using a proper cryptographic hash key derivation function.
PHP has a built-in function for HMAC, as such there is little reason to use phpseclib's slower implementation. I specifically modified the wrapper around the HMAC function to retain the same input/output.
kesselb
left a comment
kesselb
left a comment
There was a problem hiding this comment.
Thanks for your pull request 👍
| } | ||
| $this->cipher->setPassword($password); | ||
|
|
||
| $keyMaterial = hash_hkdf('sha512', $password); |
There was a problem hiding this comment.
| $keyMaterial = hash_hkdf('sha512', $password); | |
| $keyMaterial = hash_hkdf('sha512', $password, 32); |
I'm not 100% sure. The documentation says "Desired output length in bytes" 🤷♂️
There was a problem hiding this comment.
@kesselb The documentation says hash_hkdf with a $length of 0 will use the provided hash function length (which is 64 bytes in this case). Also, 32 bytes is not enough. If anything, we'll explicitly ask for 64 bytes.
The failing tests at https://drone.nextcloud.com/nextcloud/server/34111 could be a start. |
|
It will take me a bit to set up my dev env, because my OS doesn't support stuff NextCloud needs. But ill dig into the unit tests as soon as i can. Obviously gonna have to modify the Crypto tests, but there appears to be issues with the Storage tests, which may be checking for ciphertext outputs too. |
https://github.com/juliushaertl/nextcloud-docker-dev/ might help with the dev setup |
|
@kesselb I managed to get my env set up, it would was good I did it anyways. i need to learn docker at some point. Ill try to figure out these unit tests fails tomorrow if i got time. |
We are currently looking into some failures that are unrelated. So if they are not obviously related in the first place, then maybe they are just false positives (see also #22305) |
|
But those seem related :D |
3 participants
First off, this will properly split a single symmetric key into multiple, to increase the complexity of the relationship between the encryption & MAC key.
I removed unnecessary code (resulting in less dependencies, increasing simplicity), and documentation.
This Pull request is not ready to be merged, due to the fact I have not implemented unit tests nor I have I signed my work. I'm requesting review of this draft, prior to submitting the PR that should be accepted.
With that said, how should we go about the unit tests?