Fix truncation of files upon read when using object store and encryption #28389

alanmeeson · 2021-08-11T22:42:17Z

When using and object store as primary storage and using the default encryption module at the same time, any encrypted file would be truncated when read, and a text error message added to the end.

This was caused by a combination of the reliance of the read functions on on knowing the unencrypted file size, and a bug in the function which calculated the unencrypted file size for a given file.

In order to calculate the unencrypted file size, the function would first skip the header block, then use fseek to skip to the last encrypted block in the file. Because there was a correspondence between the encrypted and unencrypted blocks, this would also be the last encrypted block. It would then read the final block and decrypt it to get the unencrypted length of the last block. With that, the number of blocks, and the unencrypted block size, it could calculate the unencrypted file size.

The trouble was that when using an object store, an fread call doesn't always get you the number of bytes you asked for, even if they are available. To resolve this I adapted the stream_read_block function from lib/private/Files/Streams/Encryption.php to work here. This function wraps the fread call in a loop and repeats until it has the entire set of bytes that were requested, or there are no more to get. (The same sort of fix as in the PR mentioned later on).

This fixes the immediate bug, and should (with luck) allow people to read their encrypted files now. (The problem was purely on the decryption side). In the future it would be nice to do some refactoring here.

I have done some testing of this with image files ranging from 1kb to 10mb using Nextcloud version 22.1.0 (the nextcloud:22.1-apache docker image), with sqlite and a Linode object store as the primary storage.

Does anyone have time to take a look and help nudge this PR into contribution shape?

I think @szaimen has been keeping an eye on the related issue, and @kesselb and @rullzer were involved in a previous related PR.

PVince81

thanks a lot for the in-depth analysis!

👍 code looks fine to me, it makes sense

PVince81 · 2021-08-18T09:28:49Z

can you run composer install && composer run cs:fix lib/private/Files/Storage/Wrapper/Encryption.php ?

this should fix CI

blizzz

one small remark, but looks good as far as i can say.

blizzz · 2021-08-18T09:39:50Z

lib/private/Files/Storage/Wrapper/Encryption.php

+ * @param int $blockSize Length of requested data block in bytes
+ * @return string Data fetched from stream.
+ */
+ private function fread_block($handle, $blockSize): string {


Suggested change

private function fread_block($handle, $blockSize): string {

private function fread_block($handle, int $blockSize): string {

add type hint for blockSize?

@alanmeeson, can you address those? :)

Type hint and cs:fix comments addressed.

mashedkeyboard · 2021-08-20T09:38:04Z

Could I suggest that a test of the encryption functionality be included within https://github.com/nextcloud/server/blob/master/tests/lib/Files/ObjectStore/ObjectStoreTest.php along with this PR to prevent regression? I can try and put one together as a separate PR, but it seems to make more sense to include it in this one. I don't think there are any tests currently that cover object stores with encryption - though, if there are, they clearly need fixing!

juliushaertl · 2021-08-20T10:09:26Z

The test cases can be in the encryption related classes and could be annotated with @group PRIMARY-s3 to run then.

alanmeeson · 2021-08-22T08:37:19Z

Thanks all for the reviews and suggestions. Now that I am back to having internet again I can start doing something about them.

I'll get on with setting up an actual development environment (rather than using nano over ssh into a VPS) and sort out the code standards and type hints issues. If anyone has any suggestions for what a good PHP development setup looks like these days I'd be very happy to hear about them; it's been quite a while since I last did anything in PHP.

Regarding the unit tests: I think the simple approach would be to write a test which uses the encryption module with an S3-equivalent store, however that doesn't guarantee that the issue this PR fixes would always occur in the tests. My ideal approach would be to add a unit test to the Encryption tests in which we mock out the fread function so that it artificially hits each of the edge cases described in the fread documentation:

fread reads more bytes than requested, as though due to buffer magic.
fread reads fewer bytes than requested, despite not being at the end of the stream.
fread reads fewer bytes than requested, due to EOF.

Is there a standard approach to mocking used in this project?

On another note: The fix in this PR addresses correctly estimating the unencrypted file size; it doesn't do anything in cases where a file has already had the wrong file size estimated for it. This means that if people have uploaded files with the old version of the code, and had incorrect file size estimates captured in the meta data, then this fix won't help them. I can see of a potential hacky fix that could resolve this (if we fail to decrypt a block, try the "last block" encryption key format, in case we are at the last block but didn't know it). The alternative is for users to do a one off wipe of their file size estimates somehow, so that they are re-estimated with the new code. Does this edge case matter enough to take pre-emptive action, or is it something to just tackle if someone shouts about it?

alanmeeson · 2021-08-30T14:11:16Z

I've added the type hint as suggested and applied composer cs:fix as requested.

I'm not going to have time to figure out the testing framework within the next month or so.

If anyone, (eg: @mashedkeyboard ;-) ) wants to have a go, please have at it.

szaimen · 2021-08-31T01:38:01Z

CI is unfortuneately failing again

skjnldsv · 2021-10-23T09:18:37Z

@alanmeeson please rebase or allow us to edit this PR

alanmeeson · 2021-10-23T09:34:41Z

Allow edits by maintainers is already set, I think you should have permissions to edit.

Just in case that doesn't work, I've invited you as a collaborator to my fork of the repo.

skjnldsv · 2021-10-23T10:52:12Z

Can you then please rebase your branch?

…ion. When using and object store as primary storage and using the default encryption module at the same time, any encrypted file would be truncated when read, and a text error message added to the end. This was caused by a combination of the reliance of the read functions on on knowing the unencrypted file size, and a bug in the function which calculated the unencrypted file size for a given file. In order to calculate the unencrypted file size, the function would first skip the header block, then use fseek to skip to the last encrypted block in the file. Because there was a corresponence between the encrypted and unencrypted blocks, this would also be the last encrypted block. It would then read the final block and decrypt it to get the unencrypted length of the last block. With that, the number of blocks, and the unencrypted block size, it could calculate the unencrypted file size. The trouble was that when using an object store, an fread call doesn't always get you the number of bytes you asked for, even if they are available. To resolve this I adapted the stream_read_block function from lib/private/Files/Streams/Encryption.php to work here. This function wraps the fread call in a loop and repeats until it has the entire set of bytes that were requested, or there are no more to get. This fixes the imediate bug, and should (with luck) allow people to get their encrypted files out of Nextcloud now. (The problem was purely on the decryption side). In the future it would be nice to do some refactoring here. I have tested this with image files ranging from 1kb to 10mb using Nextcloud version 22.1.0 (the nextcloud:22.1-apache docker image), with sqlite and a Linode object store as the primary storage. Signed-off-by: Alan Meeson <alan@carefullycalculated.co.uk>

Signed-off-by: alanmeeson <alan@carefullycalculated.co.uk>

alanmeeson · 2021-10-23T14:22:11Z

Rebased on top of nextcloud:master & pushed.

I haven't squashed the commits - I couldn't find any indication as to the what the project preference is regarding that in the docs, and in absence of other guidance I tend to err towards avoiding re-writing repo history.

Hopefully that covers the required bits, and you should have ability to change things yourselves if needed so that I'm not a blocker.

skjnldsv · 2021-10-23T16:29:17Z

! [remote rejected]       fork/bugfix/22077/default-encryption-module -> bugfix/22077/default-encryption-module
(refusing to allow a Personal Access Token to create or update workflow `.github/workflows/ftp.yml` without `workflow` scope)

Ah, got it! The issue is that rebasing contains a new workflow, which is against github security policy :)
Thanks for rebasing 😉

welcome · 2021-10-23T17:10:36Z

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

juliushaertl requested review from PVince81, icewind1991 and juliushaertl August 12, 2021 10:35

juliushaertl added 3. to review Waiting for reviews bug feature: encryption (server-side) feature: object storage labels Aug 12, 2021

szaimen added this to the Nextcloud 23 milestone Aug 12, 2021

PVince81 approved these changes Aug 13, 2021

View reviewed changes

CarlSchwan approved these changes Aug 18, 2021

View reviewed changes

skjnldsv approved these changes Aug 18, 2021

View reviewed changes

skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Aug 18, 2021

blizzz reviewed Aug 18, 2021

View reviewed changes

szaimen added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Aug 31, 2021

skjnldsv mentioned this pull request Oct 13, 2021

23.0.0 beta 1 #29202

Merged

This comment has been minimized.

Sign in to view

skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Oct 22, 2021

This comment has been minimized.

Sign in to view

alanmeeson added 2 commits October 23, 2021 15:11

Add type hint to fread_block $blockSize param & apply cs:fix.

16f70e8

Signed-off-by: alanmeeson <alan@carefullycalculated.co.uk>

alanmeeson force-pushed the bugfix/22077/default-encryption-module branch from a5a6b7a to 16f70e8 Compare October 23, 2021 14:12

skjnldsv merged commit 623ac8c into nextcloud:master Oct 23, 2021

skjnldsv mentioned this pull request Oct 25, 2021

23.0.0 beta 2 #29431

Merged

25 tasks

PVince81 mentioned this pull request Nov 18, 2021

Server-side encryption is incompatible with using an object store as a primary data store #22077

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix truncation of files upon read when using object store and encryption #28389

Fix truncation of files upon read when using object store and encryption #28389

alanmeeson commented Aug 11, 2021 •

edited

Loading

PVince81 left a comment

PVince81 commented Aug 18, 2021

blizzz left a comment

blizzz Aug 18, 2021

skjnldsv Aug 25, 2021

alanmeeson Aug 30, 2021

mashedkeyboard commented Aug 20, 2021

juliushaertl commented Aug 20, 2021

alanmeeson commented Aug 22, 2021

alanmeeson commented Aug 30, 2021

szaimen commented Aug 31, 2021

This comment has been minimized.

skjnldsv commented Oct 23, 2021

alanmeeson commented Oct 23, 2021 •

edited

Loading

skjnldsv commented Oct 23, 2021

This comment has been minimized.

alanmeeson commented Oct 23, 2021

skjnldsv commented Oct 23, 2021 •

edited

Loading

welcome bot commented Oct 23, 2021

	private function fread_block($handle, $blockSize): string {
	private function fread_block($handle, int $blockSize): string {

Fix truncation of files upon read when using object store and encryption #28389

Fix truncation of files upon read when using object store and encryption #28389

Conversation

alanmeeson commented Aug 11, 2021 • edited Loading

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 commented Aug 18, 2021

blizzz left a comment

Choose a reason for hiding this comment

blizzz Aug 18, 2021

Choose a reason for hiding this comment

skjnldsv Aug 25, 2021

Choose a reason for hiding this comment

alanmeeson Aug 30, 2021

Choose a reason for hiding this comment

mashedkeyboard commented Aug 20, 2021

juliushaertl commented Aug 20, 2021

alanmeeson commented Aug 22, 2021

alanmeeson commented Aug 30, 2021

szaimen commented Aug 31, 2021

This comment has been minimized.

skjnldsv commented Oct 23, 2021

alanmeeson commented Oct 23, 2021 • edited Loading

skjnldsv commented Oct 23, 2021

This comment has been minimized.

alanmeeson commented Oct 23, 2021

skjnldsv commented Oct 23, 2021 • edited Loading

welcome bot commented Oct 23, 2021

alanmeeson commented Aug 11, 2021 •

edited

Loading

alanmeeson commented Oct 23, 2021 •

edited

Loading

skjnldsv commented Oct 23, 2021 •

edited

Loading