Getting rid of openssl_seal and rc4 in server side encryption #37243

come-nc · 2023-03-15T17:11:36Z

Resolves: #

Summary

Get rid of the intermediate fileKey and the obsolete RC4 encryption used on it.
Also used the opportunity to change the padding used to OAEP.
Files encrypted with previous version can still be read but upon write they will get migrated to the new encryption.

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)

apps/encryption/lib/KeyManager.php

apps/encryption/lib/Recovery.php

 				$this->addRecoveryKeys($filePath . '/');
 			} else {
-				$fileKey = $this->keyManager->getFileKey($filePath, $this->user->getUID());
+				$fileKey = $this->keyManager->getFileKey($filePath, $this->user->getUID(), null);


apps/encryption/lib/Crypto/Encryption.php

 				$chunk = substr($data, 0, $this->getUnencryptedBlockSize(true));

-				$encrypted .= $this->crypt->symmetricEncryptFileContent($chunk, $this->fileKey, $this->version + 1, $position);
+				$encrypted .= $this->crypt->symmetricEncryptFileContent($chunk, $this->fileKey, $this->version + 1, (string)$position);


Signed-off-by: Côme Chilliet <[email protected]>

fileKey gets deleted upon save as it’s stored in shareKeys instead now. We use presence of a fileKey to detect if a file is using the legacy system or the new one, because we do not always have access to header data. Signed-off-by: Côme Chilliet <[email protected]>

Signed-off-by: Côme Chilliet <[email protected]>

weizenspreu · 2023-03-17T12:28:21Z

@come-nc Had a first look at the code. Switching to OAEP is nice to see, as well as RC-4 being gone. If I could wish for another improvement, it would be that the file version should be written into the file header as well (and not just to the database).

Considerations:

At the moment, the files on disk are not self-contained, meaning that a backup of the files is insufficient to restore encrypted files. The restore breaks because without the version number the integrity of the files cannot be checked. That's a problem I often see in real-life requests. (see also chapter 5.4 of my whitepaper)
Writing the version into the file header would allow a to-be-improved restore process to read that version (e.g. during an ./occ files:scan run) and recover those encrypted files as long as the corresponding key files are available as well.
The version information in the database could still be used to prevent replay attacks (replacing a file with a newer version with a file with an older version) while the system admin could still replay files from backups via the aforementioned restore process.

come-nc · 2023-03-20T10:55:46Z

@come-nc Had a first look at the code. Switching to OAEP is nice to see, as well as RC-4 being gone. If I could wish for another improvement, it would be that the file version should be written into the file header as well (and not just to the database).

I need to look into that, because for the RC4 drop I added a useLegacyFileKey in the header, and in the end I can only use it when decoding, there are some code path where we do not read the header, like when resharing or adding recovery keys.
So I do not know if all code path needing the version parses the header.

At the moment, the files on disk are not self-contained, meaning that a backup of the files is insufficient to restore encrypted files. The restore breaks because without the version number the integrity of the files cannot be checked. That's a problem I often see in real-life requests. (see also chapter 5.4 of my whitepaper)

Writing the version into the file header would allow a to-be-improved restore process to read that version (e.g. during an ./occ files:scan run) and recover those encrypted files as long as the corresponding key files are available as well.

The version information in the database could still be used to prevent replay attacks (replacing a file with a newer version with a file with an older version) while the system admin could still replay files from backups via the aforementioned restore process.

So the version in the header would only be written by the normal process, and would be read only for recovery scenarios?

But in any case I think this would need to go in a follow-up PR as it is not tied to this one.

weizenspreu · 2023-03-20T11:45:03Z

So the version in the header would only be written by the normal process, and would be read only for recovery scenarios?

Yes, exactly. I remember that someone wrote a fix-file-version-in-database command which tests through a bunch of version numbers to find the correct one. I wrote one such script myself in the past as well. Having the version number within the file header would obsolete that brute-force approach. And as the file header is rewritten with every re-encryption anyway...

But in any case I think this would need to go in a follow-up PR as it is not tied to this one.

Fine for me.

Signed-off-by: Côme Chilliet <[email protected]>

come-nc · 2023-03-30T15:56:22Z

Follow-up task: write an occ command to migrate all files to the new key format, when master key is used. (it only needs to open/write/close all encrypted files.)

AkechiShiro · 2023-04-25T07:48:56Z

@come-nc Hi and thanks for this work, could the follow-up task be tracked by an issue or draft PR ?

come-nc · 2023-04-25T10:15:50Z

@come-nc Hi and thanks for this work, could the follow-up task be tracked by an issue or draft PR ?

#37918

biboc · 2023-07-12T11:41:31Z

Hi,

Does it break recover-tool?
https://github.com/nextcloud/encryption-recovery-tools

New files added with Nextcloud 27 have no fileKey and the script does not work anymore

biboc · 2023-07-12T11:55:17Z

Also, the documentation needs to be updated: https://docs.nextcloud.com/server/latest/admin_manual/configuration_files/encryption_configuration.html

weizenspreu · 2023-07-13T06:52:06Z

Does it break recover-tool? https://github.com/nextcloud/encryption-recovery-tools

New files added with Nextcloud 27 have no fileKey and the script does not work anymore

This should be fixed.

come-nc · 2023-07-18T09:14:21Z

Also, the documentation needs to be updated: https://docs.nextcloud.com/server/latest/admin_manual/configuration_files/encryption_configuration.html

I read this page and I am not sure which part you are referring to which should be updated?
What I’ve read is still true I think.

biboc · 2023-08-22T16:22:45Z

I don't remember, someone may have removed the part I mentioned. I should have link to a specific Nextcloud version

come-nc · 2024-09-17T14:12:38Z

@joshtrichards Can you explain why you added the pending documentation label back?

come-nc · 2025-04-03T12:13:48Z

@joshtrichards Can you explain why you added the pending documentation label back?

No answer, removing.

joshtrichards · 2025-04-03T13:11:05Z

Oops missed this first time around. Not sure now. I seem to remember fixing the encryption tools link, but that was a different issue. Fine by me.

come-nc self-assigned this Mar 15, 2023

come-nc added enhancement 2. developing Work in progress technical debt labels Mar 15, 2023

come-nc added this to the Nextcloud 27 milestone Mar 15, 2023

github-advanced-security bot found potential problems Mar 15, 2023

View reviewed changes

apps/encryption/lib/KeyManager.php Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Mar 16, 2023

View reviewed changes

github-advanced-security bot found potential problems Mar 17, 2023

View reviewed changes

come-nc added 5 commits March 17, 2023 11:08

Getting rid of openssl_seal and rc4 in server side encryption

fbe282c

Signed-off-by: Côme Chilliet <[email protected]>

Adapt encryption tests to the new system

26d8fb0

Signed-off-by: Côme Chilliet <[email protected]>

Adapt encryption recovery tests to new system

0750481

Signed-off-by: Côme Chilliet <[email protected]>

Fix encryption file key management

24e762c

Signed-off-by: Côme Chilliet <[email protected]>

come-nc changed the title ~~WIP - getting rid of openssl_seal and rc4 in server side encryption~~ Getting rid of openssl_seal and rc4 in server side encryption Mar 17, 2023

come-nc force-pushed the enh/openssl-get-rid-of-rc4 branch from c512e86 to 24e762c Compare March 17, 2023 10:09

come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 17, 2023

come-nc requested review from artonge, szaimen and weizenspreu March 17, 2023 11:56

come-nc marked this pull request as ready for review March 17, 2023 11:58

Add a test for multiKeyEncrypt/Decrypt methods

430009b

Signed-off-by: Côme Chilliet <[email protected]>

come-nc requested review from a team, ArtificialOwl, blizzz and icewind1991 and removed request for a team March 27, 2023 15:29

artonge approved these changes Mar 30, 2023

View reviewed changes

skjnldsv approved these changes Apr 4, 2023

View reviewed changes

come-nc merged commit 7fdb239 into master Apr 4, 2023

come-nc deleted the enh/openssl-get-rid-of-rc4 branch April 4, 2023 08:38

This was referenced Apr 25, 2023

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

Merged

Get rid of openssl_seal in encryption code #2182

Closed

come-nc mentioned this pull request Apr 25, 2023

Add an occ command to migrate all files to the new key format, when master key is used #37918

Closed

juliusknorr mentioned this pull request Jun 20, 2023

fix: Remove broken way of updating the encryption access list nextcloud/richdocuments#3028

Closed

3 tasks

biboc mentioned this pull request Jul 12, 2023

Can't recover a file added with Nextcloud 27 nextcloud/encryption-recovery-tools#4

Closed

artonge added the pending documentation This pull request needs an associated documentation update label Jul 12, 2023

weizenspreu mentioned this pull request Jul 12, 2023

overhaul server-side-encryption support nextcloud/encryption-recovery-tools#7

Merged

come-nc removed the pending documentation This pull request needs an associated documentation update label Nov 13, 2023

joshtrichards added feature: encryption (server-side) pending documentation This pull request needs an associated documentation update labels Feb 13, 2024

weizenspreu mentioned this pull request Apr 1, 2024

Recovering SSE files fails nextcloud/encryption-recovery-tools#42

Closed

joshtrichards mentioned this pull request Aug 29, 2024

[Bug]: Nextcloud 29 - No fileKey created after enabling server side encryption module #47594

Closed

8 tasks

joshtrichards mentioned this pull request Nov 18, 2024

503 Encryption not ready: multikeydecrypt with share key failed #8349

Closed

come-nc removed the pending documentation This pull request needs an associated documentation update label Apr 3, 2025

Uh oh!

Getting rid of openssl_seal and rc4 in server side encryption #37243

Getting rid of openssl_seal and rc4 in server side encryption #37243

Uh oh!

Conversation

come-nc commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

Uh oh!

Check notice

Check notice

weizenspreu commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

come-nc commented Mar 20, 2023

Uh oh!

weizenspreu commented Mar 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

come-nc commented Mar 30, 2023

Uh oh!

AkechiShiro commented Apr 25, 2023

Uh oh!

come-nc commented Apr 25, 2023

Uh oh!

biboc commented Jul 12, 2023

Uh oh!

biboc commented Jul 12, 2023

Uh oh!

weizenspreu commented Jul 13, 2023

Uh oh!

come-nc commented Jul 18, 2023

Uh oh!

biboc commented Aug 22, 2023

Uh oh!

come-nc commented Sep 17, 2024

Uh oh!

come-nc commented Apr 3, 2025

Uh oh!

joshtrichards commented Apr 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

come-nc commented Mar 15, 2023 •

edited

Loading

weizenspreu commented Mar 17, 2023 •

edited

Loading

weizenspreu commented Mar 20, 2023 •

edited

Loading