Merge pull request #972 from foodcoopshop/remotefile

Remotefile security fix
foodcoopshop · Nov 2, 2023 · 0d5bec5 · 0d5bec5
2 parents d3d10e3 + 9587958
commit 0d5bec5
Show file tree

Hide file tree

Showing 5 changed files with 48 additions and 12 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,13 @@
 
 Das Format basiert auf [keepachangelog.com](http://keepachangelog.com) und verwendet [Semantic Versioning](http://semver.org/).
 
+## v3.6.1
+
+### Security fix
+- Security-Fix für das Netzwerk-Modul [PR#972](https://github.com/foodcoopshop/foodcoopshop/pull/972)
+
+Datum: 02.11.2023 / [Alle Änderungen anzeigen](https://github.com/foodcoopshop/foodcoopshop/compare/v3.6.0...v3.6.1)
+
 ## v3.6.0
 
 ### Neue Datenschutz-Funktionen

diff --git a/VERSION.txt b/VERSION.txt
@@ -1 +1 @@
-3.6.0
+3.6.1
diff --git a/plugins/Admin/tests/TestCase/src/Model/Table/ProductsTableTest.php b/plugins/Admin/tests/TestCase/src/Model/Table/ProductsTableTest.php
@@ -88,19 +88,31 @@ public function testChangeImageValidImageAndDeleteImage()
 
     public function testChangeImageInvalidImage()
     {
+        $file = WWW_ROOT . '/css/global.css';
         $productId = 346;
         $products = [
-            [$productId => Configure::read('App.fullBaseUrl') . '/css/global.css']
+            [$productId => $file]
         ];
-        $exceptionThrown = false;
 
         try {
             $this->Product->changeImage($products);
-        } catch (InvalidParameterException $e) {
-            $exceptionThrown = true;
+        } catch (Exception $e) {
+            $this->assertEquals('file is not an image: ' . $file, $e->getMessage());
         }
+    }
 
-        $this->assertSame(true, $exceptionThrown);
+    public function testChangeImageInvalidDomain()
+    {
+        $productId = 346;
+        $products = [
+            [$productId => 'https://localhost:8080/img/tests/test-image.jpg']
+        ];
+
+        try {
+            $this->Product->changeImage($products);
+        } catch (Exception $e) {
+            $this->assertEquals('invalid host', $e->getMessage());
+        }
     }
 
     public function testChangeImageNonExistingFile()
@@ -113,7 +125,7 @@ public function testChangeImageNonExistingFile()
 
         try {
             $this->Product->changeImage($products);
-        } catch (InvalidParameterException $e) {
+        } catch (Exception $e) {
             $exceptionThrown = true;
         }
 

diff --git a/src/Lib/RemoteFile/RemoteFile.php b/src/Lib/RemoteFile/RemoteFile.php
@@ -18,9 +18,11 @@
 
 class RemoteFile
 {
-    public static function exists(string $remoteFile): bool
+    public static function exists(string $remoteFile, $allowedHosts = []): bool
     {
 
+        self::verifyAllowedHosts($allowedHosts, $remoteFile);
+
         $ch = curl_init($remoteFile);
         curl_setopt($ch, CURLOPT_NOBODY, true);
         curl_exec($ch);
@@ -35,4 +37,16 @@ public static function exists(string $remoteFile): bool
 
     }
 
+    private static function verifyAllowedHosts($allowedHosts, $remoteFile)
+    {
+        if (empty($allowedHosts)) {
+            throw new \Exception('allowedHosts must be set');
+        } else {
+            $host = parse_url($remoteFile, PHP_URL_HOST);
+            if (!in_array($host, $allowedHosts)) {
+                throw new \Exception('invalid host');
+             }
+        }
+    }
+
 }
diff --git a/src/Model/Table/ProductsTable.php b/src/Model/Table/ProductsTable.php
@@ -1370,7 +1370,10 @@ public function changeImage($products)
             }
 
             if (filter_var($imageFromRemoteServer, FILTER_VALIDATE_URL)) {
-                if (!RemoteFile::exists($imageFromRemoteServer)) {
+                $syncDomainsTable = FactoryLocator::get('Table')->get('Network.SyncDomains');
+                $syncDomains = $syncDomainsTable->getActiveSyncDomains()->toArray();
+                $syncDomains = Hash::extract($syncDomains, '{n}.domain');
+                if (!RemoteFile::exists($imageFromRemoteServer, $syncDomains)) {
                     throw new InvalidParameterException('remote image not existing: ' . $imageFromRemoteServer);
                 }
             } else {
@@ -1380,9 +1383,9 @@ public function changeImage($products)
                 }
             }
 
-            $imageSize = getimagesize($imageFromRemoteServer);
-            if ($imageSize === false) {
-                throw new InvalidParameterException('file is not not an image: ' . $imageFromRemoteServer);
+            $mimeContentType = mime_content_type($imageFromRemoteServer);
+            if (!in_array($mimeContentType, Configure::read('app.allowedImageMimeTypes'))) {
+                throw new InvalidParameterException('file is not an image: ' . $imageFromRemoteServer);
             }
         }