CVE-2022-24868: Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting') (CWE-79)
value: GLPI is a Free Asset and IT Management Software package, that provides ITIL Service Desk features, licenses tracking and software auditing. In versions prior to 10.0.0 one can exploit a lack of sanitization on SVG file uploads and inject javascript into their user avatar. As a result any user viewing the avatar will be subject to a cross site scripting attack. Users of GLPI are advised to upgrade. Users unable to upgrade should disallow SVG avatars.
CWE description: The product does not neutralize or incorrectly neutralizes user-controllable input before it is placed in output that is used as a web page that is served to other users.
CVSS v3 base score: 5.4
Published: 2022-04-21T17:15Z
Repository: https://github.com/glpi-project/glpi
Language: PHP
Files changed: src/Document.php, src/User.php
Commit date: 2022-04-20 09:39:45 +0200
Commit message: Merge pull request from GHSA-9hg4-fpwv-gx78


Diff: diff --git a/src/Document.php b/src/Document.php
index 82555a9952b..d78ae305e3b 100644
--- a/src/Document.php
+++ b/src/Document.php
@@ -1265,10 +1265,11 @@ class Document extends CommonDBTM
         $sha1sum  = sha1_file($fullpath);
         $dir      = self::isValidDoc($filename);
         $new_path = self::getUploadFileValidLocationName($dir, $sha1sum);
 
         if (!$sha1sum || !$dir || !$new_path) {
+            @unlink($fullpath);
             return false;
         }
 
        // Delete old file (if not used by another doc)
         if (
@@ -1317,10 +1318,11 @@ class Document extends CommonDBTM
         } else { // Copy (will overwrite dest file is present)
             if (copy($fullpath, GLPI_DOC_DIR . "/" . $new_path)) {
                 Session::addMessageAfterRedirect(__('Document copy succeeded.'));
             } else {
                 Session::addMessageAfterRedirect(__('File move failed'), false, ERROR);
+                @unlink($fullpath);
                 return false;
             }
         }
 
        // For display
diff --git a/src/User.php b/src/User.php
index 2d41d679eb9..1c1171f1a30 100644
--- a/src/User.php
+++ b/src/User.php
@@ -827,11 +827,11 @@ class User extends CommonDBTM
             if (isset($input["_picture"]) && !empty($input["_picture"])) {
                 $newPicture = true;
             }
             if ($newPicture) {
                 $fullpath = GLPI_TMP_DIR . "/" . $input["_picture"];
-                if (Toolbox::getMime($fullpath, 'image')) {
+                if (Document::isImage($fullpath, 'image')) {
                    // Unlink old picture (clean on changing format)
                     self::dropPictureFiles($this->fields['picture']);
                    // Move uploaded file
                     $filename     = uniqid($this->fields['id'] . '_');
                     $sub          = substr($filename, -2); /* 2 hex digit */
@@ -844,34 +844,33 @@ class User extends CommonDBTM
 
                     @mkdir(GLPI_PICTURE_DIR . "/$sub");
                     $picture_path = GLPI_PICTURE_DIR  . "/$sub/${filename}.$extension";
                     self::dropPictureFiles("$sub/${filename}.$extension");
 
-                    if (
-                        Document::isImage($fullpath)
-                        && Document::renameForce($fullpath, $picture_path)
-                    ) {
+                    if (Document::renameForce($fullpath, $picture_path)) {
                         Session::addMessageAfterRedirect(__('The file is valid. Upload is successful.'));
                         // For display
                         $input['picture'] = "$sub/${filename}.$extension";
 
                         //prepare a thumbnail
                         $thumb_path = GLPI_PICTURE_DIR . "/$sub/${filename}_min.$extension";
                         Toolbox::resizePicture($picture_path, $thumb_path);
                     } else {
                         Session::addMessageAfterRedirect(
-                            __('Potential upload attack or file too large. Moving temporary file failed.'),
+                            __('Moving temporary file failed.'),
                             false,
                             ERROR
                         );
+                        @unlink($fullpath);
                     }
                 } else {
                     Session::addMessageAfterRedirect(
                         __('The file is not an image file.'),
                         false,
                         ERROR
                     );
+                    @unlink($fullpath);
                 }
             } else {
                //ldap jpegphoto synchronisation.
                 $picture = $this->syncLdapPhoto();
                 if (!empty($picture)) {

Vulnerable code:                 if (Toolbox::getMime($fullpath, 'image')) {
                    if (
                        Document::isImage($fullpath)
                        && Document::renameForce($fullpath, $picture_path)
                    ) {
                            __('Potential upload attack or file too large. Moving temporary file failed.'),
Fixed code:             @unlink($fullpath);
                @unlink($fullpath);
                if (Document::isImage($fullpath, 'image')) {
                    if (Document::renameForce($fullpath, $picture_path)) {
                            __('Moving temporary file failed.'),
                        @unlink($fullpath);
                    @unlink($fullpath);