Merge pull request #593 from nextcloud/artonge/fluse1367/master/impro…

…ve_e2e_encrypted_file_detection fix: SSE files being mistaken for E2EE
nextcloud · Mar 18, 2024 · 38b49bc · 38b49bc
2 parents dd69c7e + 2612fc9
commit 38b49bc
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 44 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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');