From 2612fc9586041ae3a91fc8b9d4bed988f500bd66 Mon Sep 17 00:00:00 2001 From: Louis Chemineau Date: Thu, 14 Mar 2024 09:34:45 +0100 Subject: [PATCH] fix: SSE files being mistaken for E2EE Signed-off-by: Alyx Signed-off-by: Louis Chemineau --- lib/E2EEnabledPathCache.php | 69 +++++++++++++------ lib/EncryptionManager.php | 8 ++- tests/Unit/Connector/Sabre/LockPluginTest.php | 27 ++------ tests/Unit/EncryptionManagerTest.php | 4 +- 4 files changed, 64 insertions(+), 44 deletions(-) diff --git a/lib/E2EEnabledPathCache.php b/lib/E2EEnabledPathCache.php index 6583349d..1577eb26 100644 --- a/lib/E2EEnabledPathCache.php +++ b/lib/E2EEnabledPathCache.php @@ -25,11 +25,11 @@ namespace OCA\EndToEndEncryption; use OCP\Cache\CappedMemoryCache; -use OCP\Files\Cache\ICache; +use OCP\Files\Folder; use OCP\Files\IHomeStorage; +use OCP\Files\InvalidPathException; use OCP\Files\Node; use OCP\Files\NotFoundException; -use OCP\Files\Storage\IStorage; class E2EEnabledPathCache { /** @@ -45,53 +45,82 @@ public function __construct() { } /** - * Checks if the path is an E2E folder or inside an E2E folder + * Checks if the node is an E2EE folder or inside an E2EE folder. + * + * @return bool true, if the node is valid and E2EE, false otherwise */ public function isE2EEnabledPath(Node $node): bool { - if ($node->isEncrypted()) { + // only checking for $node->isEncrypted() will lead to false-positives for SSE files, + // as they have the encryption flag set but are not E2E, so we need to check the folder's flag, + // as folders having the encryption flag set are always E2E + if ($node instanceof Folder && $node->isEncrypted()) { return true; } - $storage = $node->getStorage(); - $cache = $storage->getCache(); - return $this->getEncryptedStates($cache, $node, $storage); + + try { + $storage = $node->getStorage(); + } catch (NotFoundException $e) { + return false; + } + // if not home storage, fallback to EncryptionManager + if (!$storage->instanceOfStorage(IHomeStorage::class)) { + return EncryptionManager::isEncryptedFile($node); + } + + // walk path backwards while caching each node's state + return $this->getEncryptedStates((string) $storage->getCache()->getNumericStorageId(), $node); } /** - * Get the encryption state for the path + * Determines a node's E2E encryption state by walking up the tree. Caches each node's state on the way. + * + * @param string $storageId the node's storage id, used for caching + * @param Node $node the node to check + * + * @return bool true, if the node is valid and E2EE, false otherwise */ - protected function getEncryptedStates(ICache $cache, Node $node, IStorage $storage): bool { - if (!$storage->instanceOfStorage(IHomeStorage::class)) { + protected function getEncryptedStates(string $storageId, Node $node): bool { + try { + $nodeId = $node->getId(); + } catch (InvalidPathException|NotFoundException $e) { return false; } - $storageId = (string)$cache->getNumericStorageId(); + // initialize array for storage id if necessary if (!isset($this->perStorageEncryptedStateCache[$storageId])) { $this->perStorageEncryptedStateCache[$storageId] = []; } - if (isset($this->perStorageEncryptedStateCache[$storageId][$node->getId()])) { - return $this->perStorageEncryptedStateCache[$storageId][$node->getId()]; + // return cached state if available + elseif (isset($this->perStorageEncryptedStateCache[$storageId][$nodeId])) { + return $this->perStorageEncryptedStateCache[$storageId][$nodeId]; } if ($node->getPath() === '/') { // root is never encrypted - $this->perStorageEncryptedStateCache[$storageId][$node->getId()] = false; + $this->perStorageEncryptedStateCache[$storageId][$nodeId] = false; return false; } - if ($node->isEncrypted()) { - // no need to go further down in the tree - $this->perStorageEncryptedStateCache[$storageId][$node->getId()] = true; + // checking for folder for the same reason as above + if ($node instanceof Folder && $node->isEncrypted()) { + // no need to go further up in the tree + $this->perStorageEncryptedStateCache[$storageId][$nodeId] = true; return true; } try { $parentNode = $node->getParent(); } catch (NotFoundException $e) { - $this->perStorageEncryptedStateCache[$storageId][$node->getId()] = false; + // node not encrypted and no parent that could be E2EE, so node is not E2EE + $this->perStorageEncryptedStateCache[$storageId][$nodeId] = false; return false; } - $encrypted = $this->getEncryptedStates($cache, $parentNode, $storage); - $this->perStorageEncryptedStateCache[$storageId][$node->getId()] = $encrypted; + + // check parent's state + $encrypted = $this->getEncryptedStates($storageId, $parentNode); + + // if any parent is E2EE, this node is E2EE as well + $this->perStorageEncryptedStateCache[$storageId][$nodeId] = $encrypted; return $encrypted; } } diff --git a/lib/EncryptionManager.php b/lib/EncryptionManager.php index b48134e3..590d7117 100644 --- a/lib/EncryptionManager.php +++ b/lib/EncryptionManager.php @@ -84,7 +84,13 @@ public function removeEncryptionFlag(int $id): void { /** * Check if a file is in a folder marked as encrypted */ - public function isEncryptedFile(Node $node): bool { + public static function isEncryptedFile(Node $node): bool { + // traverse up if node is not a folder to prevent false positives for SSE files + // (see E2EEnabledPathCache#isE2EEnabledPath) + if (!($node instanceof Folder)) { + $node = $node->getParent(); + } + do { if ($node->isEncrypted()) { return true; diff --git a/tests/Unit/Connector/Sabre/LockPluginTest.php b/tests/Unit/Connector/Sabre/LockPluginTest.php index b2d3eb87..df2a44e9 100644 --- a/tests/Unit/Connector/Sabre/LockPluginTest.php +++ b/tests/Unit/Connector/Sabre/LockPluginTest.php @@ -34,6 +34,7 @@ use OCA\EndToEndEncryption\LockManager; use OCA\EndToEndEncryption\UserAgentManager; use OCP\Files\Cache\ICache; +use OCP\Files\Folder; use OCP\Files\IRootFolder; use OCP\Files\Storage\IStorage; use OCP\IUserSession; @@ -563,7 +564,7 @@ public function testIsE2EEnabledPathEncryptedFolder():void { ]) ->getMock(); - $node = $this->createMock(Node::class); + $node = $this->createMock(Folder::class); $node->expects($this->once()) ->method('isEncrypted') ->willReturn(true); @@ -586,16 +587,14 @@ public function testIsE2EEnabledPathParentEncrypted():void { ]) ->getMock(); - $encryptedParentParentNode = $this->createMock(Node::class); + $encryptedParentParentNode = $this->createMock(Folder::class); $encryptedParentParentNode->expects($this->once()) ->method('isEncrypted') ->willReturn(true); $encryptedParentParentNode->method('getId') ->willReturn(1); - $encryptedParentParentNode->method('getFileInfo') - ->willReturn(['parent' => 0]); - $parentNode = $this->createMock(Node::class); + $parentNode = $this->createMock(Folder::class); $parentNode->expects($this->once()) ->method('isEncrypted') ->willReturn(false); @@ -604,13 +603,8 @@ public function testIsE2EEnabledPathParentEncrypted():void { ->willReturn($encryptedParentParentNode); $parentNode->method('getId') ->willReturn(2); - $parentNode->method('getFileInfo') - ->willReturn(['parent' => 1]); $fileNode = $this->createMock(Node::class); - $fileNode->expects($this->atLeastOnce()) - ->method('isEncrypted') - ->willReturn(false); $cache = $this->createMock(ICache::class); $cache->method('getNumericStorageId') ->willReturn(1); @@ -649,16 +643,14 @@ public function testIsE2EEnabledPathNonEncrypted():void { ]) ->getMock(); - $encryptedParentParentNode = $this->createMock(Node::class); + $encryptedParentParentNode = $this->createMock(Folder::class); $encryptedParentParentNode->method('getId') ->willReturn(1); - $encryptedParentParentNode->method('getFileInfo') - ->willReturn(['parent' => 0]); $encryptedParentParentNode->expects($this->once()) ->method('getPath') ->willReturn('/'); - $parentNode = $this->createMock(Node::class); + $parentNode = $this->createMock(Folder::class); $parentNode->expects($this->once()) ->method('isEncrypted') ->willReturn(false); @@ -667,8 +659,6 @@ public function testIsE2EEnabledPathNonEncrypted():void { ->willReturn($encryptedParentParentNode); $parentNode->method('getId') ->willReturn(2); - $parentNode->method('getFileInfo') - ->willReturn(['parent' => 1]); $cache = $this->createMock(ICache::class); $cache->method('getNumericStorageId') @@ -680,9 +670,6 @@ public function testIsE2EEnabledPathNonEncrypted():void { ->willReturn($cache); $fileNode = $this->createMock(Node::class); - $fileNode->expects($this->atLeastOnce()) - ->method('isEncrypted') - ->willReturn(false); $fileNode->expects($this->once()) ->method('getParent') ->willReturn($parentNode); @@ -691,8 +678,6 @@ public function testIsE2EEnabledPathNonEncrypted():void { ->willReturn('/data/rere/re'); $fileNode->method('getId') ->willReturn(3); - $fileNode->method('getFileInfo') - ->willReturn(['parent' => 2]); $fileNode->method('getStorage') ->willReturn($storage); diff --git a/tests/Unit/EncryptionManagerTest.php b/tests/Unit/EncryptionManagerTest.php index f7fe24fa..0d33ffc1 100644 --- a/tests/Unit/EncryptionManagerTest.php +++ b/tests/Unit/EncryptionManagerTest.php @@ -173,8 +173,8 @@ public function dataTestIsEncryptedFile(): array { * @return PHPUnit_Framework_MockObject_MockObject[] */ public function constructNestedNodes(): array { - $node1 = $this->getMockBuilder(Node::class)->disableOriginalConstructor()->getMock(); - $node2 = $this->getMockBuilder(Node::class)->disableOriginalConstructor()->getMock(); + $node1 = $this->getMockBuilder(Folder::class)->disableOriginalConstructor()->getMock(); + $node2 = $this->getMockBuilder(Folder::class)->disableOriginalConstructor()->getMock(); $node3 = $this->getMockBuilder(Node::class)->disableOriginalConstructor()->getMock(); $node1->expects($this->any())->method('getParent')->willReturn($node2); $node1->expects($this->any())->method('getPath')->willReturn('/data/user');