Use a PHP implementation of openssl_seal that allows to use modern ciphers

[come-nc]

Follow-up on #35916

Summary

PHP openssl_seal do not support any cipher.
We currently use rc4 which is not available with modern openssl version and default configuration.
Using our own implementation for openssl_seal allows:

• As a first step to still be able to read rc4 on modern system, to be able to read encrypted data from previous versions without turning on legacy ciphers
• To move to a more modern cipher instead of rc4 for new data, ideally one with an auth tag like AES-256-GCM
• Things should be made so it’s easy to switch again in the future to something more modern when needed. Encryption ciphers will always keep evolving.

TODO

• Strong type
• Replace rc4 custom implementation by call to seclib
• Remove fallback to php openssl_seal, always use the same implementation
• Add support for newer cipher (may be done in followup PR)

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[yahesh]

I'm not quite sure why you'd rather throw an exception in opensslSeal() and opensslOpen() than fall back to the PHP implementation. Is there a specific rationale behind that decision?

[come-nc]

I'm not quite sure why you'd rather throw an exception in opensslSeal() and opensslOpen() than fall back to the PHP implementation. Is there a specific rationale behind that decision?

This is to avoid strange bugs we do not understand by having several implementations of the same thing.
If an unexpected value is passed as cipher I do not want the code to silently accept it.

[come-nc]

@yahesh How come your code calls the same method rc4 for decryption and encryption ?

I wanted to switch to using phpseclib implementation but their implementation has one method for decryption and one for encryption, it does not appear to be the same operation?

[yahesh]

@yahesh How come your code calls the same method rc4 for decryption and encryption ?

I wanted to switch to using phpseclib implementation but their implementation has one method for decryption and one for encryption, it does not appear to be the same operation?

RC4 is a synchronous stream cipher, meaning that the key scheduler acts as a mere CSPRNG. The output of the key scheduler (aka. keystream) is XORed with the plaintext to retrieve the ciphertext. Likewise, the ciphertext needs to be XORed with the identical keystream to be decrypted again. Thus, the same function can be used for encryption and decryption as it just calculates the keystream and then XORs the input with that keystream.

[come-nc]

After reading the code a bit more indepth, it appears that we only use these multiKeyEncrypt/multiKeyDecrypt functions to encypt/decrypt a filekey which is in turn used to encrypt/decrypt with symmetricEncryptFileContent, which does not use RC4.

So, we already have something similar to what openssl_open/seal does, but then we use open/seal on the key.
It seems to me that we could get rid of the random intermediate key and the rc4 altogether and just encrypt the filekey with openssl_public_encrypt directly.

@yahesh Does that makes sense to you? Or did I miss something?

[come-nc]

/rebase

[yahesh]

After reading the code a bit more indepth, it appears that we only use these multiKeyEncrypt/multiKeyDecrypt functions to encypt/decrypt a filekey which is in turn used to encrypt/decrypt with symmetricEncryptFileContent, which does not use RC4.

So, we already have something similar to what openssl_open/seal does, but then we use open/seal on the key. It seems to me that we could get rid of the random intermediate key and the rc4 altogether and just encrypt the filekey with openssl_public_encrypt directly.

@yahesh Does that makes sense to you? Or did I miss something?

Yes, I'm totally with you that the current intermediate key is not needed to reach the design goals of the encryption scheme (see also chapter 5.3 of my corresponding whitepaper). However, removing the intermediate key would result in an implementation that's not a pure drop-in replacement. That means that all files would have to be migrated so that the actual file key is stored directly in the multi-encrypted sharekey files.

[come-nc]

Yes, I'm totally with you that the current intermediate key is not needed to reach the design goals of the encryption scheme (see also chapter 5.3 of my corresponding whitepaper). However, removing the intermediate key would result in an implementation that's not a pure drop-in replacement. That means that all files would have to be migrated so that the actual file key is stored directly in the multi-encrypted sharekey files.

Thank you for confirming that I am not crazy, and thanks a lot for the whitepaper, lots of information in there :-)

I agree that removing the intermediate key means having a migration, but so does switching from RC4 to something else, so I think we should still attempt to get rid of the intermediate key for 27.

I will push the current state of this PR for review and merge in 26 if possible, to support openssl without legacy ciphers, and to have control over the seal/open process and be able to refactor in the future.

[yahesh]

I agree that removing the intermediate key means having a migration, but so does switching from RC4 to something else, so I think we should still attempt to get rid of the intermediate key for 27.

@come-nc Would be great if you could mention me in the corresponding pull request then so that I can prepare my rescue script ahead of time.

[st3iny]

Tested and works (on a machine with OpenSSL 1.1.1).

I created a fresh instance without this PR, enabled encryption, encrypted all files and then applied this PR.

Handling and uploading files still works. There were no errors logged.

[icewind1991]

Tested that files encrypted on master can be opened with this PR and vice-versa.

Code looks ok

[kesselb]

camelCase please ;)

[yahesh]

Looks fine to me. Most of the code is already in active use elsewhere.

[AkechiShiro]

Is there any follow-up draft PR about adding AES-256-GCM ciphers for server side encryption ?

• Add support for newer cipher (may be done in followup PR)

Is it planned in any roadmaps for NC 27 or 28 ?

I've searched issues and pull requests about that, but all were closed as if that issue was fixed, but it isn't at the moment, I believe (at least moving to AES-256-GCM isn't acheived by this PR, but only a step towards it/facilitating this move).

Also, what is the rationale behind using RC4 for server side encryption, how safe is RC4 for that purpose ?
Also RC4, being deprecated by OpenSSL just doesn't make it more solid in terms of security.

However, I'm not particularly knowledgeable in cryptography, I've seen old news outlet criticizing RC4 usage and they did in 2013.
There are also academic papers, pointing at some flaws, but since Nextcloud's use and implementation of RC4 is probably not the same, the concerns may or may not apply.

I do not know if there is any disclaimer in Nextcloud's official documentation warning users that this cipher is being used despite being deprecated by lots of others projects and justifying its use or explaining the risks, that server holders exposes their users to by using this feature.

[yahesh]

@AkechiShiro This pull request handled preparatory work that was necessary so that the existing encryption scheme worked with new OpenSSLv3 installations. What you're looking for is pull request #37243 which got rid of RC-4 entirely.

[AkechiShiro]

Thanks @yahesh for mentionning the following PR here, but I didn't see much announced in release notes of Nextcloud 26 about server side encryption changes and RC4 being removed, I saw that RC4 seclib implementation was going to be used in order to be compatible with OpenSSLv3, I should have searched better I guess.

The PR you linked states padding is changed to OAEP but there doesn't seem to be a mention towards the new cipher used.

I had to look into the commits file changes to see that it's now using most likely RSA used at least for the intermidate key ? Is that correct ? And is RSA used as well for any files ? (Multikey encryption means a key for each files?)

What is the new default cipher agreed on for server side encryption ?

[st3iny]

@AkechiShiro As you can see from the milestone of the linked PR it is scheduled for Nextcloud 27.

[yahesh]

I had to look into the commits file changes to see that it's now using most likely RSA used at least for the intermidate key ? Is that correct ? And is RSA used as well for any files ? (Multikey encryption means a key for each files?)

What is the new default cipher agreed on for server side encryption ?

Not quite. The pull request removes the intermediate keys altogether. That means that the file key is protected through RSA with OAEP padding. The encryption within the files currently stays as Encrypt-then-MAC with AES-256-CTR for the encryption and HMAC-SHA-256 for the MACs.

P.S.: Which is okay for now. The chosen encryption is not bad. It could be replaced with more modern ciphers or block cipher modes. However, these often aren't (properly) supported by PHP.