From fb51880a4a5a2afb79b7aa1e9c041c300cf34e9c Mon Sep 17 00:00:00 2001 From: jknockaert Date: Wed, 20 May 2015 09:35:20 +0200 Subject: [PATCH 1/4] encrypted filesize calculation in flush() --- lib/private/files/stream/encryption.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/private/files/stream/encryption.php b/lib/private/files/stream/encryption.php index f2f5b9c9af7..7c8e6d4de83 100644 --- a/lib/private/files/stream/encryption.php +++ b/lib/private/files/stream/encryption.php @@ -412,7 +412,14 @@ class Encryption extends Wrapper { $encrypted = $this->encryptionModule->encrypt($this->cache); parent::stream_write($encrypted); $this->writeFlag = false; - $this->size = max($this->size, parent::stream_tell()); + // If the write concerns the last block then then update the encrypted filesize + // Note that the unencrypted pointer and filesize are NOT yet updated when flush() is called + // We recalculate the encrypted filesize as we do not know the context of calling flush() + if ((int)floor($this->unencryptedSize/$this->unencryptedBlockSize) === (int)floor($this->position/$this->unencryptedBlockSize)) { + $this->size = $this->util->getBlockSize() * (int)floor($this->unencryptedSize/$this->unencryptedBlockSize); + $this->size += strlen($encrypted); + $this->size += $this->headerSize; + } } // always empty the cache (otherwise readCache() will not fill it with the new block) $this->cache = ''; From 5a20edac8241590e1b21a8ad0abcd9d6af2f5112 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Wed, 20 May 2015 14:29:20 +0200 Subject: [PATCH 2/4] test to simulate a non-seekable stream wrapper --- lib/private/files/stream/encryption.php | 24 ++++++--- .../files/stream/dummyencryptionwrapper.php | 37 ++++++++++++++ tests/lib/files/stream/encryption.php | 50 +++++++++++++++++-- 3 files changed, 102 insertions(+), 9 deletions(-) create mode 100644 tests/lib/files/stream/dummyencryptionwrapper.php diff --git a/lib/private/files/stream/encryption.php b/lib/private/files/stream/encryption.php index 7c8e6d4de83..7394b84a5d3 100644 --- a/lib/private/files/stream/encryption.php +++ b/lib/private/files/stream/encryption.php @@ -130,6 +130,7 @@ class Encryption extends Wrapper { * @param int $size * @param int $unencryptedSize * @param int $headerSize + * @param string $wrapper stream wrapper class * @return resource * * @throws \BadMethodCallException @@ -144,7 +145,8 @@ class Encryption extends Wrapper { $mode, $size, $unencryptedSize, - $headerSize) { + $headerSize, + $wrapper = 'OC\Files\Stream\Encryption') { $context = stream_context_create(array( 'ocencryption' => array( @@ -164,7 +166,7 @@ class Encryption extends Wrapper { ) )); - return self::wrapSource($source, $mode, $context, 'ocencryption', 'OC\Files\Stream\Encryption'); + return self::wrapSource($source, $mode, $context, 'ocencryption', $wrapper); } /** @@ -309,7 +311,7 @@ class Encryption extends Wrapper { // flush will start writing there when the position moves to another block $positionInFile = (int)floor($this->position / $this->unencryptedBlockSize) * $this->util->getBlockSize() + $this->headerSize; - $resultFseek = parent::stream_seek($positionInFile); + $resultFseek = $this->parentStreamSeek($positionInFile); // only allow writes on seekable streams, or at the end of the encrypted stream if (!($this->readOnly) && ($resultFseek || $positionInFile === $this->size)) { @@ -376,10 +378,10 @@ class Encryption extends Wrapper { * $this->util->getBlockSize() + $this->headerSize; $oldFilePosition = parent::stream_tell(); - if (parent::stream_seek($newFilePosition)) { - parent::stream_seek($oldFilePosition); + if ($this->parentStreamSeek($newFilePosition)) { + $this->parentStreamSeek($oldFilePosition); $this->flush(); - parent::stream_seek($newFilePosition); + $this->parentStreamSeek($newFilePosition); $this->position = $newPosition; $return = true; } @@ -456,4 +458,14 @@ class Encryption extends Wrapper { parent::stream_read($this->headerSize); } + /** + * call stream_seek() from parent class + * + * @param integer $position + * @return bool + */ + protected function parentStreamSeek($position) { + return parent::stream_seek($position); + } + } diff --git a/tests/lib/files/stream/dummyencryptionwrapper.php b/tests/lib/files/stream/dummyencryptionwrapper.php new file mode 100644 index 00000000000..bb512d99c66 --- /dev/null +++ b/tests/lib/files/stream/dummyencryptionwrapper.php @@ -0,0 +1,37 @@ + + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + + +namespace Test\Files\Stream; + +class DummyEncryptionWrapper extends \OC\Files\Stream\Encryption { + + /** + * simulate a non-seekable stream wrapper by always return false + * + * @param int $position + * @return bool + */ + protected function parentStreamSeek($position) { + return false; + } + +} diff --git a/tests/lib/files/stream/encryption.php b/tests/lib/files/stream/encryption.php index c2388c7682a..802435eafc3 100644 --- a/tests/lib/files/stream/encryption.php +++ b/tests/lib/files/stream/encryption.php @@ -16,7 +16,7 @@ class Encryption extends \Test\TestCase { * @param integer $unencryptedSize * @return resource */ - protected function getStream($fileName, $mode, $unencryptedSize) { + protected function getStream($fileName, $mode, $unencryptedSize, $wrapper = '\OC\Files\Stream\Encryption') { clearstatcache(); $size = filesize($fileName); $source = fopen($fileName, $mode); @@ -45,9 +45,10 @@ class Encryption extends \Test\TestCase { ->method('getUidAndFilename') ->willReturn(['user1', $internalPath]); - return \OC\Files\Stream\Encryption::wrap($source, $internalPath, + + return $wrapper::wrap($source, $internalPath, $fullPath, $header, $uid, $this->encryptionModule, $storage, $encStorage, - $util, $file, $mode, $size, $unencryptedSize, 8192); + $util, $file, $mode, $size, $unencryptedSize, 8192, $wrapper); } /** @@ -256,6 +257,49 @@ class Encryption extends \Test\TestCase { unlink($fileName); } + /** + * simulate a non-seekable storage + * + * @dataProvider dataFilesProvider + */ + public function testWriteToNonSeekableStorage($testFile) { + + $wrapper = $this->getMockBuilder('\OC\Files\Stream\Encryption') + ->setMethods(['parentSeekStream'])->getMock(); + $wrapper->expects($this->any())->method('parentSeekStream')->willReturn(false); + + $expectedData = file_get_contents(\OC::$SERVERROOT . '/tests/data/' . $testFile); + // write it + $fileName = tempnam("/tmp", "FOO"); + $stream = $this->getStream($fileName, 'w+', 0, '\Test\Files\Stream\DummyEncryptionWrapper'); + // while writing the file from the beginning to the end we should never try + // to read parts of the file. This should only happen for write operations + // in the middle of a file + $this->encryptionModule->expects($this->never())->method('decrypt'); + fwrite($stream, $expectedData); + fclose($stream); + + // read it all + $stream = $this->getStream($fileName, 'r', strlen($expectedData), '\Test\Files\Stream\DummyEncryptionWrapper'); + $data = stream_get_contents($stream); + fclose($stream); + + $this->assertEquals($expectedData, $data); + + // another read test with a loop like we do in several places: + $stream = $this->getStream($fileName, 'r', strlen($expectedData)); + $data = ''; + while (!feof($stream)) { + $data .= fread($stream, 8192); + } + fclose($stream); + + $this->assertEquals($expectedData, $data); + + unlink($fileName); + + } + /** * @return \PHPUnit_Framework_MockObject_MockObject */ From bf6151e7994d50eeab65a50078d3c6a7c79a47cf Mon Sep 17 00:00:00 2001 From: jknockaert Date: Thu, 21 May 2015 07:51:07 +0200 Subject: [PATCH 3/4] fix calculation of $count, $count is always 8129 so we need to check this against the unencrypted file size --- lib/private/files/stream/encryption.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/files/stream/encryption.php b/lib/private/files/stream/encryption.php index 7394b84a5d3..dfffad3450a 100644 --- a/lib/private/files/stream/encryption.php +++ b/lib/private/files/stream/encryption.php @@ -273,7 +273,7 @@ class Encryption extends Wrapper { $result = ''; -// $count = min($count, $this->unencryptedSize - $this->position); + $count = min($count, $this->unencryptedSize - $this->position); while ($count > 0) { $remainingLength = $count; // update the cache of the current block From a577e723b0e46f5afcfd1cbee27215e832340509 Mon Sep 17 00:00:00 2001 From: jknockaert Date: Fri, 22 May 2015 22:13:27 +0200 Subject: [PATCH 4/4] flush() comments + perf opt --- lib/private/files/stream/encryption.php | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/private/files/stream/encryption.php b/lib/private/files/stream/encryption.php index dfffad3450a..22d230e7c86 100644 --- a/lib/private/files/stream/encryption.php +++ b/lib/private/files/stream/encryption.php @@ -412,14 +412,16 @@ class Encryption extends Wrapper { // we are handling that separately here and we don't want to // get into an infinite loop $encrypted = $this->encryptionModule->encrypt($this->cache); - parent::stream_write($encrypted); + $bytesWritten = parent::stream_write($encrypted); $this->writeFlag = false; - // If the write concerns the last block then then update the encrypted filesize + // Check whether the write concerns the last block + // If so then update the encrypted filesize // Note that the unencrypted pointer and filesize are NOT yet updated when flush() is called // We recalculate the encrypted filesize as we do not know the context of calling flush() - if ((int)floor($this->unencryptedSize/$this->unencryptedBlockSize) === (int)floor($this->position/$this->unencryptedBlockSize)) { - $this->size = $this->util->getBlockSize() * (int)floor($this->unencryptedSize/$this->unencryptedBlockSize); - $this->size += strlen($encrypted); + $completeBlocksInFile=(int)floor($this->unencryptedSize/$this->unencryptedBlockSize); + if ($completeBlocksInFile === (int)floor($this->position/$this->unencryptedBlockSize)) { + $this->size = $this->util->getBlockSize() * $completeBlocksInFile; + $this->size += $bytesWritten; $this->size += $this->headerSize; } }