Vulnerability: OpenCATS through 0.9.6 allows remote attackers to execute arbitrary code by uploading an executable file via lib/FileUtility.php.
Vulnerability ID: CVE-2021-41560
CWE: CWE-434
Severity score: 10
Project: https://github.com/opencats/OpenCATS
Published: 2021-12-15
File: lib/FileUtility.php
Commit message: Fixed critical upload vulnerability (#552)

Co-authored-by: Nicholas Ferreira <nickguitar.dll@hotmail.com>
Commit author: Nicholas Ferreira

Vulnerable version (before fix):
 public static function makeSafeFilename($filename)          /* Is the file extension safe? */         $fileExtension = self::getFileExtension($filename);         if (in_array($fileExtension, $GLOBALS['badFileExtensions']))         {             $filename .= '.txt';         }          return $filename;     }  public static function getUploadFileFromPost($siteID, $subDirectory, $id)             if (!eval(Hooks::get('FILE_UTILITY_SPACE_CHECK'))) return;              $uploadPath = FileUtility::getUploadPath($siteID, $subDirectory);             $newFileName = $_FILES[$id]['name'];              // Could just while(file_exists) it, but I'm paranoid of infinate loops             // Shouldn't have 1000 files of the same name anyway

Fixed version (after fix):
 public static function makeSafeFilename($filename)          /* Is the file extension safe? */         $fileExtension = self::getFileExtension($filename);                  /* Use a whitelist instead of a blacklist to prevent possible bypasses */         if (!preg_match("/(?i)\.(pdf|docx?|rtf|odt?g?|txt|wpd|jpe?g|png|csv|xlsx?|ppt|msg|heic|tiff?|html?|bmp|wps|xps)$/i", $fileExtension))         {             $filename .= ".txt";         } /*        if (in_array($fileExtension, $GLOBALS['badFileExtensions']))         {             $filename .= '.txt';         } */         return $filename;     }  public static function getUploadFileFromPost($siteID, $subDirectory, $id)             if (!eval(Hooks::get('FILE_UTILITY_SPACE_CHECK'))) return;              $uploadPath = FileUtility::getUploadPath($siteID, $subDirectory);             $newFileName = FileUtility::makeSafeFilename($_FILES[$id]['name']);              // Could just while(file_exists) it, but I'm paranoid of infinate loops             // Shouldn't have 1000 files of the same name anyway

Patch:
patch: @@ -184,11 +184,17 @@ public static function makeSafeFilename($filename)
 
         /* Is the file extension safe? */
         $fileExtension = self::getFileExtension($filename);
-        if (in_array($fileExtension, $GLOBALS['badFileExtensions']))
+        
+        /* Use a whitelist instead of a blacklist to prevent possible bypasses */
+        if (!preg_match("/(?i)\.(pdf|docx?|rtf|odt?g?|txt|wpd|jpe?g|png|csv|xlsx?|ppt|msg|heic|tiff?|html?|bmp|wps|xps)$/i", $fileExtension))
+        {
+            $filename .= ".txt";
+        }
+/*        if (in_array($fileExtension, $GLOBALS['badFileExtensions']))
         {
             $filename .= '.txt';
         }
-
+*/
         return $filename;
     }
 
@@ -563,7 +569,7 @@ public static function getUploadFileFromPost($siteID, $subDirectory, $id)
             if (!eval(Hooks::get('FILE_UTILITY_SPACE_CHECK'))) return;
 
             $uploadPath = FileUtility::getUploadPath($siteID, $subDirectory);
-            $newFileName = $_FILES[$id]['name'];
+            $newFileName = FileUtility::makeSafeFilename($_FILES[$id]['name']);
 
             // Could just while(file_exists) it, but I'm paranoid of infinate loops
             // Shouldn't have 1000 files of the same name anyway
path: lib/FileUtility.php