image: improve sanitation

Signed-off-by: Varun Patil <[email protected]>
pulsejet · Apr 8, 2024 · 0c499ee · 0c499ee
1 parent e774f6b
commit 0c499ee
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 22 deletions.
diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php
@@ -45,6 +45,7 @@ class Application extends App implements IBootstrap
 {
     public const APPNAME = 'memories';
 
+    // Remember to update IMAGICK_SAFE if this is updated
     public const IMAGE_MIMES = [
         'image/png',
         'image/jpeg',
@@ -56,7 +57,7 @@ class Application extends App implements IBootstrap
         'image/bmp',
         'image/x-dcraw',        // RAW
         // 'image/x-xbitmap',   // too rarely used for photos
-        // 'image/svg+xml',     // too rarely used for photos
+        // 'image/svg+xml',     // unsafe
     ];
 
     public const VIDEO_MIMES = [

diff --git a/lib/Controller/ImageController.php b/lib/Controller/ImageController.php
@@ -32,6 +32,8 @@
 use OCP\AppFramework\Http\JSONResponse;
 use OCP\Files\IRootFolder;
 
+const IMAGICK_SAFE = '/^image\/(x-)?(png|jpeg|gif|bmp|tiff|webp|hei(f|c)|avif|dcraw)$/';
+
 class ImageController extends GenericApiController
 {
     /**
@@ -312,7 +314,7 @@ public function decodable(string $id): Http\Response
             $name = $file->getName();
 
             // Convert image to JPEG if required
-            if (!\in_array($mimetype, ['image/png', 'image/webp', 'image/jpeg', 'image/gif'], true)) {
+            if (!preg_match('/^image\/(png|webp|jpeg|gif)$/', $mimetype)) {
                 [$blob, $mimetype] = $this->getImageJPEG($blob, $mimetype);
                 $name .= '.jpg';
             }
@@ -354,14 +356,8 @@ public function editImage(
                 throw Exceptions::ForbiddenFileUpdate($name);
             }
 
-            // Check if we have imagick
-            if (!class_exists('Imagick')) {
-                throw Exceptions::Forbidden('Imagick extension is not available');
-            }
-
             // Read the image
-            $image = new \Imagick();
-            $image->readImageBlob($file->getContent());
+            $image = self::getImagick($file->getContent());
 
             // Due to a bug in filerobot, the provided width and height may be swapped
             // 1. If the user does not rotate the image, we're fine
@@ -420,20 +416,10 @@ public function editImage(
      */
     private function getImageJPEG($blob, $mimetype): array
     {
-        // TODO: Use imaginary if available
+        // TODO: Use imaginary if available (once HEIC isn't broken)
 
-        // Check if Imagick is available
-        if (!class_exists('Imagick')) {
-            throw Exceptions::Forbidden('Imagick extension is not available');
-        }
-
-        // Read original image
-        try {
-            $image = new \Imagick();
-            $image->readImageBlob($blob);
-        } catch (\ImagickException $e) {
-            throw Exceptions::Forbidden('Imagick failed to read image: '.$e->getMessage());
-        }
+        // Get an instance of Imagick
+        $image = self::getImagick($blob);
 
         // Convert to JPEG
         try {
@@ -501,4 +487,36 @@ private function refreshPreviews(\OCP\Files\File $file): void
             return;
         }
     }
+
+    /**
+     * Get an instance of Imagick for the given blob.
+     *
+     * @param string $blob Blob of image data
+     *
+     * @return \Imagick
+     */
+    private static function getImagick(string $blob)
+    {
+        // Check if Imagick is available
+        if (!class_exists('Imagick')) {
+            throw Exceptions::Forbidden('Imagick extension is not available');
+        }
+
+        try {
+            $image = new \Imagick();
+
+            // Check if image is safe
+            $image->pingImageBlob($blob);
+            if (!preg_match(IMAGICK_SAFE, $mime = $image->getImageMimeType())) {
+                throw Exceptions::Forbidden("Image type {$mime} not allowed");
+            }
+
+            // Read the image blob
+            $image->readImageBlob($blob);
+
+            return $image;
+        } catch (\ImagickException $e) {
+            throw Exceptions::Forbidden('Imagick failed to read image: '.$e->getMessage());
+        }
+    }
 }