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

fix encryption stream for 2.0 #15305

Closed
wants to merge 2 commits into from
Closed

Conversation

DeepDiver1975
Copy link
Member

follow up of #15303

@schiesbn @nickvergessen @jknockaert

@DeepDiver1975 DeepDiver1975 added this to the 8.1-current milestone Mar 30, 2015
@jknockaert
Copy link
Contributor

@DeepDiver1975 That's quite a bit more changes than I submit in #15303

// get into an infinite loop
$encrypted = $this->encryptionModule->encrypt($this->cache);
parent::stream_write($encrypted);
$this->writeFlag = false;
$this->encryptionStorage->updateUnencryptedSize($this->fullPath, $this->unencryptedSize);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line updating the ecryptionStorage should be moved to stream_close() right after $this->flush().

@DeepDiver1975
Copy link
Member Author

@jknockaert your changes are in here: b57f469

I added unit testing on top

@jknockaert
Copy link
Contributor

@DeepDiver1975 OK

@DeepDiver1975
Copy link
Member Author

@jknockaert see e4ec4ca

@jknockaert
Copy link
Contributor

@DeepDiver1975 Thanks

@DeepDiver1975
Copy link
Member Author

I added unit testing on top

I'll move this to it's own pr

@jknockaert
Copy link
Contributor

@DeepDiver1975 Ah, that's why it's gone again; I already wondered...

@jknockaert
Copy link
Contributor

@DeepDiver1975 I think this PR could be labelled for review now. @schiesbn Could you test the big file issue? I think there also needs to be done some testing with unseekable encrypted streams; I don't have such a setup here.

@jknockaert
Copy link
Contributor

@DeepDiver1975 Reviewing my own code I noticed that I forgot to initialise the protected variables I added; so could you pls add $this->cache = ''; and $this->writeFlag = false; somewhere in stream_open(), perhaps after $this->position = 0;? Thanks.

@DeepDiver1975
Copy link
Member Author

@jknockaert quick testing this with a big file shows that this doesn't really help yet

@DeepDiver1975
Copy link
Member Author

there is something missing on the chunk border

bildschirmfoto von 2015-03-30 15 04 34

@DeepDiver1975
Copy link
Member Author

The issue is within the calcs of $positionInFile .... this change doesn't seem to help

@jknockaert
Copy link
Contributor

@DeepDiver1975 Not entirely sure what's happening here. Does this only happen with big files? What is big? Bigger than 6kb? Or bigger than 6MB? I would suggest to add the initialisation commented above; on top of that I am still unsure whether $this->sizeand this->unencryptedSize get initialised correctly for new files; you may want to try uncommenting these lines.
For testing it may be an idea to single out if the bug is in reading or writing. Could you test reading on a big file that was correctly encoded (i.e. with an older version of the code)?

@DeepDiver1975
Copy link
Member Author

that file has a size of 1.7MB

@jknockaert
Copy link
Contributor

@DeepDiver1975 Pls also add in stream_write(): $this->size+=$this->util->getBlockSize() right after $this->writeHeader();. I guess this is what causes the problem you observed (but pls still add the initialisations suggested in #15305 (comment))

DeepDiver1975 added a commit that referenced this pull request Mar 30, 2015
Conflicts:
	lib/private/files/stream/encryption.php
@schiessle
Copy link
Contributor

I cherry-picked this state over to #14472

I tested it with some large files and it worked great here. But we should still investigate the problem at the chonk border mentioned by @DeepDiver1975

@jknockaert Thanks a lot for your contribution!

There is another issue which might or might not be related to the stream wrapper.
If I share a text file with user2, user2 can read it but if user2 edits the file with the internal text editor the file gets corrupted. It looks like it starts to encrypt already the header and then continues with encrtypting already encrypted content. For the owner it works just fine.

@jknockaert jknockaert mentioned this pull request Mar 30, 2015
12 tasks
DeepDiver1975 added a commit that referenced this pull request Mar 30, 2015
Conflicts:
	lib/private/files/stream/encryption.php
@jknockaert
Copy link
Contributor

Maybe this PR should be closed now that it has been ported to #14472?

@schiessle
Copy link
Contributor

agree

@schiessle schiessle closed this Mar 30, 2015
@schiessle schiessle deleted the jknockaert-newbranch branch March 30, 2015 15:12
DeepDiver1975 added a commit that referenced this pull request Mar 31, 2015
Conflicts:
	lib/private/files/stream/encryption.php
DeepDiver1975 added a commit that referenced this pull request Apr 1, 2015
Conflicts:
	lib/private/files/stream/encryption.php
DeepDiver1975 added a commit that referenced this pull request Apr 2, 2015
Conflicts:
	lib/private/files/stream/encryption.php
DeepDiver1975 added a commit that referenced this pull request Apr 7, 2015
Conflicts:
	lib/private/files/stream/encryption.php
@lock lock bot locked as resolved and limited conversation to collaborators Aug 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants