xss: Draft Files

This mitigates an XSS vulnerability with files uploaded through drafts.
osTicket · Mar 8, 2023 · 86f9693 · 86f9693
1 parent e43cc79
commit 86f9693
Show file tree

Hide file tree

Showing 4 changed files with 84 additions and 66 deletions.
diff --git a/bootstrap.php b/bootstrap.php
@@ -54,6 +54,15 @@ function exif_imagetype ($filename) {
             }
         }
 
+        if (!function_exists('exif_imagetype')) {
+            function exif_imagetype ($filename) {
+                if ((list($width,$height,$type,) = getimagesize($filename)) !== false)
+                    return $type;
+
+                return false;
+            }
+        }
+
         if (!isset($_SERVER['REMOTE_ADDR']))
             $_SERVER['REMOTE_ADDR'] = '';
     }

diff --git a/include/ajax.draft.php b/include/ajax.draft.php
@@ -53,78 +53,72 @@ static function _updateDraft($draft) {
     static function _uploadInlineImage($draft) {
         global $cfg;
 
-        if (!isset($_POST['data']) && !isset($_FILES['file']))
+        if (!isset($_FILES['file']))
             Http::response(422, "File not included properly");
 
         # Fixup for expected multiple attachments
-        if (isset($_FILES['file'])) {
-            $file = AttachmentFile::format($_FILES['file']);
-
-            # Allow for data-uri uploaded files
-            $fp = fopen($file[0]['tmp_name'], 'rb');
-            if (fread($fp, 5) == 'data:') {
-                $data = 'data:';
-                while ($block = fread($fp, 8192))
-                  $data .= $block;
-                $file[0] = Format::parseRfc2397($data);
-                list(,$ext) = explode('/', $file[0]['type'], 2);
-                $file[0] += array(
-                    'name' => Misc::randCode(8).'.'.$ext,
-                    'size' => strlen($file[0]['data']),
-                );
-            }
-            fclose($fp);
+        $file = AttachmentFile::format($_FILES['file']);
+
+        # Allow for data-uri uploaded files
+        $fp = fopen($file[0]['tmp_name'], 'rb');
+        if (fread($fp, 5) == 'data:') {
+            $data = 'data:';
+            while ($block = fread($fp, 8192))
+              $data .= $block;
+            $file[0] = Format::parseRfc2397($data);
+            list(,$ext) = explode('/', $file[0]['type'], 2);
+            $file[0] += array(
+                'name' => Misc::randCode(8).'.'.$ext,
+                'size' => strlen($file[0]['data']),
+            );
+        }
+        fclose($fp);
+
+        // Check file type to ensure image
+        $type = $file[0]['type'];
+        if (strpos($file[0]['type'], 'image/') !== 0)
+            return Http::response(403,
+                JsonDataEncoder::encode(array(
+                    'error' => 'File type is not allowed',
+                ))
+            );
 
-            # TODO: Detect unacceptable attachment extension
-            # TODO: Verify content-type and check file-content to ensure image
-            $type = $file[0]['type'];
-            if (strpos($file[0]['type'], 'image/') !== 0)
-                return Http::response(403,
-                    JsonDataEncoder::encode(array(
-                        'error' => 'File type is not allowed',
-                    ))
-                );
+        // Check if file is truly an image
+        if (!FileUploadField::isValidFile($file[0]))
+            return Http::response(403,
+                JsonDataEncoder::encode(array(
+                    'error' => 'File is not valid',
+                ))
+            );
 
-            # TODO: Verify file size is acceptable
-            if ($file[0]['size'] > $cfg->getMaxFileSize())
+        // Verify file size is acceptable
+        if ($file[0]['size'] > $cfg->getMaxFileSize())
+            return Http::response(403,
+                JsonDataEncoder::encode(array(
+                    'error' => 'File is too large',
+                ))
+            );
+
+        // Paste uploads in Chrome will have a name of 'blob'
+        if ($file[0]['name'] == 'blob')
+            $file[0]['name'] = 'screenshot-'.Misc::randCode(4);
+
+        $ids = $draft->attachments->upload($file);
+
+        if (!$ids) {
+            if ($file[0]['error']) {
                 return Http::response(403,
                     JsonDataEncoder::encode(array(
-                        'error' => 'File is too large',
+                        'error' => $file[0]['error'],
                     ))
                 );
 
-            // Paste uploads in Chrome will have a name of 'blob'
-            if ($file[0]['name'] == 'blob')
-                $file[0]['name'] = 'screenshot-'.Misc::randCode(4);
-
-            $ids = $draft->attachments->upload($file);
-
-            if (!$ids) {
-                if ($file[0]['error']) {
-                    return Http::response(403,
-                        JsonDataEncoder::encode(array(
-                            'error' => $file[0]['error'],
-                        ))
-                    );
-                }
-                else
-                    return Http::response(500, 'Unable to attach image');
             }
-
-            $id = (is_array($ids)) ? $ids[0] : $ids;
-        }
-        else {
-            $type = explode('/', $_POST['contentType']);
-            $info = array(
-                'data' => base64_decode($_POST['data']),
-                'name' => Misc::randCode(10).'.'.$type[1],
-                // TODO: Ensure _POST['contentType']
-                'type' => $_POST['contentType'],
-            );
-            // TODO: Detect unacceptable filetype
-            // TODO: Verify content-type and check file-content to ensure image
-            $id = $draft->attachments->save($info);
+            else
+                return Http::response(500, 'Unable to attach image');
         }
+
+        $id = (is_array($ids)) ? $ids[0] : $ids;
         if (!($f = AttachmentFile::lookup($id)))
             return Http::response(500, 'Unable to attach image');
 

diff --git a/include/class.file.php b/include/class.file.php
@@ -1054,12 +1054,8 @@ function setMimeType($type) {
     }
 
     function getMimeType() {
-        if (!isset($this->_mimetype)) {
-            // Try to to auto-detect mime type
-            $finfo = new finfo(FILEINFO_MIME);
-            $this->_mimetype = $finfo->buffer($this->getContents(),
-                    FILEINFO_MIME_TYPE);
-        }
+        if (!isset($this->_mimetype))
+            $this->_mimetype = self::mime_type($this->getRealPath());
 
         return $this->_mimetype;
     }
@@ -1075,6 +1071,21 @@ function getContents() {
     function getData() {
         return $this->getContents();
     }
+
+    /*
+     * Given a filepath - auto detect the mime type
+     *
+     */
+    static function mime_type($filepath) {
+        // Try to to auto-detect mime type
+        $type = null;
+        if (function_exists('finfo_open')) {
+            $finfo = finfo_open(FILEINFO_MIME_TYPE);
+            $type = finfo_file($finfo, $filepath);
+            finfo_close($finfo);
+        }
+        return $type ?: mime_content_type($filepath);
+    }
 }
 
 ?>
diff --git a/include/class.forms.php b/include/class.forms.php
@@ -3911,6 +3911,10 @@ function uploadAttachment(&$file) {
     }
 
     static function isValidFile($file) {
+        // Make sure mime type is valid
+        if (strcasecmp(FileObject::mime_type($file['tmp_name']),
+                    $file['type']) !== 0)
+            return false;
 
         // Check invalid image hacks
         if ($file['tmp_name']