Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixed moving of master key encrypted and versioned files between shared folders #16696

Open
wants to merge 1 commit into
base: master
from

Conversation

@yahesh
Copy link
Member

yahesh commented Aug 8, 2019

This fixes #16419 for master key encrypted files.

Signed-off-by: Kenny <k.niehage@syseleven.de>
@yahesh yahesh changed the title fixed moving of encrypted and versioned files between shared folders fixed moving of master key encrypted and versioned files between shared folders Aug 8, 2019
@yahesh

This comment has been minimized.

Copy link
Member Author

yahesh commented Aug 8, 2019

This fixes the problem for master key encrypted files and improves the situation a bit for user key encrypted files.

@yahesh

This comment has been minimized.

Copy link
Member Author

yahesh commented Aug 8, 2019

Copied over from #16419 as suggested by @kesselb:

It seems that moving user key encrypted folders leads into some weird locking issues in the $this->file->getAccessList() call of lib/private/Files/Stream/Encryption.php:stream_open() (which we can reproduce reliably but haven't fully understood yet). At least, this locking issue doesn't break files. Copying user key encrypted folders works.

This is a write-down of what the general problem was:
Let's shortly re-iterate on what happens when a file is moved from one shared folder to another shared folder:

  • a copy of the file is put into the target folder
  • a copy of the file is put into the trashbin of the user moving the file
  • the original copy of the file is put into the trashbin of the file owner

The general logic can be found in lib/private/Files/Storage/Wrapper/Encryption.php: moveFromStorage() which first copies the file/folder with the help of lib/private/Files/Storage/Wrapper/Encryption.php:copyBetweenStorage() and then deletes the file/folder.

One task that has to be done is to update the oc_filecache table. Most of this is done by underlying classes but the encrypted value in the database is handled here in lib/private/Files/Storage/Wrapper/Encryption.php:updateEncryptedVersion() and this is also where the problem is buried.

When copyBetweenStorage is first called in moveFromStorage it provides true as the $isRename value. This later prompts updateEncryptedVersion to overwrite the encrypted value of the source file with 1. So back to the process:

  • When the source file is copied to the target file, the encrypted value of the source file is changed into a wrong value.
  • When the source file is copied to the trashbin of the user moving the file, the encrypted value of the source file doesn't match and the file copy fails because the signature checks fails while reading the content.

What we did for now to remedy this problem:

  • At the start of copyBetweenStorage we backup the original encrypted value.
  • We let the rest of the program logic untouched because some underlying code seems to rely on this broken behaviour.
  • At the end of copyBetweenStorage we restore the original encrypted value so that it is the correct value for the next copy task.
@jknockaert

This comment has been minimized.

Copy link
Contributor

jknockaert commented Aug 12, 2019

@yahesh Thanks for the PR. Ideally there would be one or more tests added to the codebase in order to avoid future regressions (and they also help to some extent in documenting the PR).
@nextcloud/encryption Reviews needed. Any other groups to invite?

@yahesh

This comment has been minimized.

Copy link
Member Author

yahesh commented Aug 12, 2019

@jknockaert As you may have read in the corresponding issue, this problem occurs some 32 levels down into the call stack. I really have no idea how to properly mock this misbehaviour so that the requested test isn't much more than a dummy placeholder.

@jknockaert

This comment has been minimized.

Copy link
Contributor

jknockaert commented Aug 12, 2019

@yahesh Ja this is pretty low-level stuff.
@schiessle Do you have any advise as to how to design a proper test for this fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.