[stable8.2] Heal unencrypted file sizes at download time #22626

Merged
merged 1 commit into from Feb 25, 2016

Projects

None yet

5 participants

@schiessle
Member

approved backport of #22579

@PVince81 @MorrisJobke please test and review... Thanks!

@schiessle schiessle added this to the 8.2.3-current-maintenance milestone Feb 24, 2016
@mention-bot

By analyzing the blame information on this pull request, we identified @DeepDiver1975, @nickvergessen and @LukasReschke to be potential reviewers

@schiessle
Member

@jknockaert maybe you can also review the backports.... Thanks!

@schiessle
Member

failing tests are unrelated, please ignore:

08:45:42 Installing ....
08:45:43 ownCloud is not installed - only a limited number of commands are available
08:45:45 
08:45:45 
08:45:45                                  
08:45:45   [OCP\Files\NotFoundException]  
08:45:45                                  
08:45:45 
08:45:45 
08:45:45 maintenance:install [--database="..."] [--database-name="..."] [--database-host="..."] [--database-user="..."] [--database-pass[="..."]] [--database-table-prefix[="..."]] [--admin-user="..."] [--admin-pass="..."] [--data-dir="..."]
08:45:45 
08:45:45 
08:45:45 Kill the swift docker
@PVince81
Collaborator

Tested, works πŸ‘

@jknockaert jknockaert and 1 other commented on an outdated diff Feb 24, 2016
lib/private/files/storage/wrapper/encryption.php
+ while ($count > 0) {
+ $data=fread($stream, $blockSize);
+ $count=strlen($data);
+ $lastChunkContentEncrypted .= $data;
+ if(strlen($lastChunkContentEncrypted) > $blockSize) {
+ $newUnencryptedSize += $unencryptedBlockSize;
+ $lastChunkContentEncrypted=substr($lastChunkContentEncrypted, $blockSize);
+ }
+ }
+
+ fclose($stream);
+
+ // we have to decrypt the last chunk to get it actual size
+ $encryptionModule->begin($this->getFullPath($path), $this->uid, 'r', $header, []);
+ $decryptedLastChunk = $encryptionModule->decrypt($lastChunkContentEncrypted, $lastChunkNr . 'end');
+ $decryptedLastChunk .= $encryptionModule->end($this->getFullPath($path), $lastChunkNr . 'end');
@jknockaert
jknockaert Feb 24, 2016 Contributor

How can this work? decryp() and end() seem to be defined to take exactly one argument in 8.2 (and 8.1 as well):
https://github.com/owncloud/core/blob/fix_broken_unencrypted_size_8.2/apps/encryption/lib/crypto/encryption.php#L212
https://github.com/owncloud/core/blob/fix_broken_unencrypted_size_8.2/apps/encryption/lib/crypto/encryption.php#L318
Has this really been tested in a meaningful way?

@schiessle
schiessle Feb 25, 2016 Member

you are right, this arguments doesn't exists in oC < 9.0... Unfortunately PHP doesn't care if you add more arguments then expected by the method that's why unit tests and manual tests passed. I will remove it

@schiessle
schiessle Feb 25, 2016 Member

@jknockaert fixed... looking forward to your +1 πŸ˜‰

@schiessle schiessle recalculate unencrypted size if we assume that the size stored in the…
… db is not correct
f8e2541
@jknockaert
Contributor

@schiesbn Has this backport actually been properly tested?

@schiessle
Member

Has this backport actually been properly tested?

yes, by me and by @PVince81. As said, we didn't noticed the additional parameter because PHP don't care if you call a method with more parameters than expected. It just ignores the additional parameters.

@jknockaert jknockaert commented on the diff Feb 25, 2016
lib/private/files/storage/wrapper/encryption.php
+ * @param string $path internal path relative to the storage root
+ * @param int $unencryptedSize size of the unencrypted file
+ *
+ * @return int unencrypted size
+ */
+ protected function verifyUnencryptedSize($path, $unencryptedSize) {
+
+ $size = $this->storage->filesize($path);
+ $result = $unencryptedSize;
+
+ if ($unencryptedSize < 0 ||
+ ($size > 0 && $unencryptedSize === $size)
+ ) {
+ // check if we already calculate the unencrypted size for the
+ // given path to avoid recursions
+ if (isset($this->fixUnencryptedSizeOf[$this->getFullPath($path)]) === false) {
@jknockaert
jknockaert Feb 25, 2016 Contributor

If the analysis #22579 (comment) is right we may be able to do without the recursion test in 8.1 and 8.2, as there are no versions to be determined. Would we want to give it a try just to see?

@schiessle
schiessle Feb 25, 2016 Member

no, people can also write their own encryption modul and we don't want and we can't forbid to use certain file system operation within the encryption module. For now, that's the safe way to do it.

@jknockaert
Contributor

This one is good πŸ‘

@schiessle schiessle merged commit 75f9587 into stable8.2 Feb 25, 2016

20 of 21 checks passed

Scrutinizer Timed out
Details
cla-bot-core Build #1691 succeeded in 48 sec
Details
core-ci-linux-jsunit/database=sqlite,label=SLAVE Build #58511 succeeded in 3 min 38 sec
Details
core-ci-linux-swift-primary-storage/database=mysql,label=SLAVE Build #52446 succeeded in 13 min
Details
core-ci-linux/database=mysql,label=SLAVE Build #27228 succeeded in 22 min
Details
core-ci-linux/database=oci,label=SLAVE Build #27229 succeeded in 31 min
Details
core-ci-linux/database=pgsql,label=SLAVE Build #27229 succeeded in 18 min
Details
core-ci-linux/database=sqlite,label=SLAVE Build #27229 succeeded in 12 min
Details
ocs-api-integration-tests-ci Build #7791 succeeded in 38 sec
Details
server-master-linux-externals-ci/database=sqlite,external=smb-silvershell,label=SLAVE Build #7554 succeeded in 1 min 30 sec
Details
server-master-linux-externals-ci/database=sqlite,external=swift-ceph,label=SLAVE Build #7554 succeeded in 48 sec
Details
server-master-linux-externals-ci/database=sqlite,external=webdav-ownCloud,label=SLAVE Build #7554 succeeded in 3 min 42 sec
Details
server-master-linux-externals-smb-windows-ext-ci/database=sqlite,external=smb-windows,label=master Build #7868 succeeded in 5 min 15 sec
Details
server-master-linux-php7-ci/database=sqlite,label=SLAVE Build #35238 succeeded in 6 min 41 sec
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=litmus,mirallBranch=v2.0.2,slave=SMASH Build #11870 succeeded in 4 min 51 sec
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_basicSync@0,mirallBranch=v2.0.2,slave=SMASH Build #11870 succeeded in 6 min 36 sec
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_basicSync@1,mirallBranch=v2.0.2,slave=SMASH Build #11870 succeeded in 30 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_shareLink,mirallBranch=v2.0.2,slave=SMASH Build #11870 succeeded in 28 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePermissions,mirallBranch=v2.0.2,slave=SMASH Build #11870 succeeded in 23 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePropagationGroups,mirallBranch=v2.0.2,slave=SMASH Build #11870 succeeded in 32 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePropagationInsideGroups,mirallBranch=v2.0.2,slave=SMASH Build #11870 succeeded in 9 min 5 sec
Details
@schiessle schiessle deleted the fix_broken_unencrypted_size_8.2 branch Feb 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment