From 735f6cc0371f5e5c5d84a5ea4189f23104302ed1 Mon Sep 17 00:00:00 2001 From: jknockaert Date: Tue, 21 Apr 2015 14:21:53 +0200 Subject: fix encryption header error When moving back the pointer to position 0 (using stream_seek), the pointer on the encrypted stream will be moved to the position immediately after the header. Reading the header again (invoked by stream_read) will cause an error, writing the header again (invoked by stream_write) will corrupt the file. Reading/writing the header should therefore happen when opening the file rather than upon read or write. Note that a side-effect of this PR is that empty files will still get an encryption header; I think that is OK, but it is different from how it was originally implemented. --- lib/private/files/stream/encryption.php | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) (limited to 'lib/private') diff --git a/lib/private/files/stream/encryption.php b/lib/private/files/stream/encryption.php index 910357eef45..936bcb71e71 100644 --- a/lib/private/files/stream/encryption.php +++ b/lib/private/files/stream/encryption.php @@ -221,15 +221,28 @@ class Encryption extends Wrapper { || $mode === 'w+' || $mode === 'wb' || $mode === 'wb+' + || $mode === 'r+' + || $mode === 'rb+' ) { - // We're writing a new file so start write counter with 0 bytes - $this->unencryptedSize = 0; - $this->size = 0; $this->readOnly = false; } else { $this->readOnly = true; } + if ( + $mode === 'w' + || $mode === 'w+' + || $mode === 'wb' + || $mode === 'wb+' + ) { + // We're writing a new file so start write counter with 0 bytes + $this->unencryptedSize = 0; + $this->writeHeader(); + $this->size = $this->util->getHeaderSize(); + } else { + parent::stream_read($this->util->getHeaderSize()); + } + $sharePath = $this->fullPath; if (!$this->storage->file_exists($this->internalPath)) { $sharePath = dirname($sharePath); @@ -250,11 +263,6 @@ class Encryption extends Wrapper { $result = ''; - // skip the header if we read the file from the beginning - if ($this->position === 0) { - parent::stream_read($this->util->getHeaderSize()); - } - // $count = min($count, $this->unencryptedSize - $this->position); while ($count > 0) { $remainingLength = $count; @@ -281,11 +289,6 @@ class Encryption extends Wrapper { public function stream_write($data) { - if ($this->position === 0) { - $this->writeHeader(); - $this->size = $this->util->getHeaderSize(); - } - $length = 0; // loop over $data to fit it in 6126 sized unencrypted blocks while (strlen($data) > 0) { -- cgit v1.2.3 From 1756562501e3201ed21a6f8a26b53c6bfdf12934 Mon Sep 17 00:00:00 2001 From: jknockaert Date: Tue, 21 Apr 2015 14:43:08 +0200 Subject: Update encryption.php --- lib/private/files/stream/encryption.php | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) (limited to 'lib/private') diff --git a/lib/private/files/stream/encryption.php b/lib/private/files/stream/encryption.php index 936bcb71e71..2e73e2927a7 100644 --- a/lib/private/files/stream/encryption.php +++ b/lib/private/files/stream/encryption.php @@ -229,6 +229,14 @@ class Encryption extends Wrapper { $this->readOnly = true; } + $sharePath = $this->fullPath; + if (!$this->storage->file_exists($this->internalPath)) { + $sharePath = dirname($sharePath); + } + + $accessList = $this->file->getAccessList($sharePath); + $this->newHeader = $this->encryptionModule->begin($this->fullPath, $this->uid, $this->header, $accessList); + if ( $mode === 'w' || $mode === 'w+' @@ -243,14 +251,6 @@ class Encryption extends Wrapper { parent::stream_read($this->util->getHeaderSize()); } - $sharePath = $this->fullPath; - if (!$this->storage->file_exists($this->internalPath)) { - $sharePath = dirname($sharePath); - } - - $accessList = $this->file->getAccessList($sharePath); - $this->newHeader = $this->encryptionModule->begin($this->fullPath, $this->uid, $this->header, $accessList); - return true; } -- cgit v1.2.3 From 238302ee7daa26c6940460cf4c9f293b413c8f6f Mon Sep 17 00:00:00 2001 From: jknockaert Date: Tue, 21 Apr 2015 17:46:43 +0200 Subject: fixed name --- lib/private/files/stream/encryption.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'lib/private') diff --git a/lib/private/files/stream/encryption.php b/lib/private/files/stream/encryption.php index 2e73e2927a7..6fe205d352c 100644 --- a/lib/private/files/stream/encryption.php +++ b/lib/private/files/stream/encryption.php @@ -1,7 +1,7 @@ - * @author jknockaert + * @author Jasper Knockaert * @author Thomas Müller * * @copyright Copyright (c) 2015, ownCloud, Inc. -- cgit v1.2.3 From 49df8ef525c11e6468a65fd705fef9e0e3a71374 Mon Sep 17 00:00:00 2001 From: jknockaert Date: Wed, 22 Apr 2015 12:28:07 +0200 Subject: Update encryption.php --- lib/private/files/stream/encryption.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'lib/private') diff --git a/lib/private/files/stream/encryption.php b/lib/private/files/stream/encryption.php index 6fe205d352c..52660270763 100644 --- a/lib/private/files/stream/encryption.php +++ b/lib/private/files/stream/encryption.php @@ -237,6 +237,7 @@ class Encryption extends Wrapper { $accessList = $this->file->getAccessList($sharePath); $this->newHeader = $this->encryptionModule->begin($this->fullPath, $this->uid, $this->header, $accessList); + if (!($path==='')){ if ( $mode === 'w' || $mode === 'w+' @@ -250,7 +251,7 @@ class Encryption extends Wrapper { } else { parent::stream_read($this->util->getHeaderSize()); } - + } return true; } -- cgit v1.2.3 From 9a5783b28434762aeb05ce62627a5adb675e5560 Mon Sep 17 00:00:00 2001 From: Bjoern Schiessle Date: Fri, 24 Apr 2015 16:47:27 +0200 Subject: fix unit tests --- lib/private/files/stream/encryption.php | 14 ++++++++++---- tests/lib/files/stream/encryption.php | 21 ++++++++++++++++----- 2 files changed, 26 insertions(+), 9 deletions(-) (limited to 'lib/private') diff --git a/lib/private/files/stream/encryption.php b/lib/private/files/stream/encryption.php index 52660270763..5f39207db87 100644 --- a/lib/private/files/stream/encryption.php +++ b/lib/private/files/stream/encryption.php @@ -237,7 +237,6 @@ class Encryption extends Wrapper { $accessList = $this->file->getAccessList($sharePath); $this->newHeader = $this->encryptionModule->begin($this->fullPath, $this->uid, $this->header, $accessList); - if (!($path==='')){ if ( $mode === 'w' || $mode === 'w+' @@ -249,9 +248,9 @@ class Encryption extends Wrapper { $this->writeHeader(); $this->size = $this->util->getHeaderSize(); } else { - parent::stream_read($this->util->getHeaderSize()); - } + $this->skipHeader(); } + return true; } @@ -432,9 +431,16 @@ class Encryption extends Wrapper { * @return integer * @throws EncryptionHeaderKeyExistsException if header key is already in use */ - private function writeHeader() { + protected function writeHeader() { $header = $this->util->createHeader($this->newHeader, $this->encryptionModule); return parent::stream_write($header); } + /** + * read first block to skip the header + */ + protected function skipHeader() { + parent::stream_read($this->util->getHeaderSize()); + } + } diff --git a/tests/lib/files/stream/encryption.php b/tests/lib/files/stream/encryption.php index 024185b34c8..0b34de8ae12 100644 --- a/tests/lib/files/stream/encryption.php +++ b/tests/lib/files/stream/encryption.php @@ -55,6 +55,7 @@ class Encryption extends \Test\TestCase { $fileExists, $expectedSharePath, $expectedSize, + $expectedUnencryptedSize, $expectedReadOnly) { // build mocks @@ -77,9 +78,15 @@ class Encryption extends \Test\TestCase { return array(); })); + $utilMock = $this->getMockBuilder('\OC\Encryption\Util') + ->disableOriginalConstructor()->getMock(); + $utilMock->expects($this->any()) + ->method('getHeaderSize') + ->willReturn(8192); + // get a instance of the stream wrapper $streamWrapper = $this->getMockBuilder('\OC\Files\Stream\Encryption') - ->setMethods(['loadContext'])->disableOriginalConstructor()->getMock(); + ->setMethods(['loadContext', 'writeHeader', 'skipHeader'])->disableOriginalConstructor()->getMock(); // set internal properties of the stream wrapper $stream = new \ReflectionClass('\OC\Files\Stream\Encryption'); @@ -95,6 +102,10 @@ class Encryption extends \Test\TestCase { $file->setAccessible(true); $file->setValue($streamWrapper, $fileMock); $file->setAccessible(false); + $util = $stream->getProperty('util'); + $util->setAccessible(true); + $util->setValue($streamWrapper, $utilMock); + $util->setAccessible(false); $fullPathP = $stream->getProperty('fullPath'); $fullPathP->setAccessible(true); $fullPathP->setValue($streamWrapper, $fullPath); @@ -118,7 +129,7 @@ class Encryption extends \Test\TestCase { $unencryptedSize = $stream->getProperty('unencryptedSize'); $unencryptedSize->setAccessible(true); - $this->assertSame($expectedSize, + $this->assertSame($expectedUnencryptedSize, $unencryptedSize->getValue($streamWrapper) ); $unencryptedSize->setAccessible(false); @@ -133,9 +144,9 @@ class Encryption extends \Test\TestCase { public function dataProviderStreamOpen() { return array( - array('r', '/foo/bar/test.txt', true, '/foo/bar/test.txt', null, true), - array('r', '/foo/bar/test.txt', false, '/foo/bar', null, true), - array('w', '/foo/bar/test.txt', true, '/foo/bar/test.txt', 0, false), + array('r', '/foo/bar/test.txt', true, '/foo/bar/test.txt', null, null, true), + array('r', '/foo/bar/test.txt', false, '/foo/bar', null, null, true), + array('w', '/foo/bar/test.txt', true, '/foo/bar/test.txt', 8192, 0, false), ); } -- cgit v1.2.3