From 7ac85a7284d172ab9866878baea95ed705419aac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?th=C3=A9odore=20cl=C3=A9ment?= <213218350+thclemnt@users.noreply.github.com> Date: Mon, 18 May 2026 16:53:06 +0200 Subject: [PATCH] fix(document): prevent filepath injection --- front/document.form.php | 7 +-- inc/document.class.php | 75 ++++++++++++++++++++++++------- inc/itsmnguploadhandler.class.php | 44 ++++++++++++++---- tests/functional/Document.php | 46 ++++++++++++++++--- 4 files changed, 136 insertions(+), 36 deletions(-) diff --git a/front/document.form.php b/front/document.form.php index ab791941bb..3a58ca8829 100644 --- a/front/document.form.php +++ b/front/document.form.php @@ -115,12 +115,7 @@ if ((isset($_POST['files']) && $_POST['files'] != '[]')) { $file = json_decode(stripslashes($_POST['files']), true)[0]; - $_POST['filename'] = $file['name']; - $_POST['filepath'] = ItsmngUploadHandler::uploadFile( - $file['path'], - $file['name'], - ItsmngUploadHandler::UPLOAD - ); + $_POST['_uploaded_file'] = ItsmngUploadHandler::getUploadedFileDescriptor($file); } if ($doc->update($_POST)) { Event::log( diff --git a/inc/document.class.php b/inc/document.class.php index 14438affaf..35fdda0b13 100644 --- a/inc/document.class.php +++ b/inc/document.class.php @@ -50,6 +50,7 @@ class Document extends CommonDBTM public static $rightname = 'document'; public static $tag_prefix = '#'; protected $usenotepad = true; + private $file_copy_source = null; public static function getTypeName($nb = 0) @@ -58,6 +59,12 @@ public static function getTypeName($nb = 0) } + public function setFileCopySource(Document $source): void + { + $this->file_copy_source = $source; + } + + /** * Check if given object can have Document * @@ -216,15 +223,24 @@ public function prepareInputForAdd($input) { global $CFG_GLPI; - // security (don't accept filename from $_REQUEST) - if (array_key_exists('filename', $_REQUEST)) { - unset($input['filename']); - } + $input = $this->filterFields($input); + + // current_filename is not necessary for a new item, but keeping a + // request-provided value can lead to wrong file deletion in move*(). + $input['current_filename'] = ''; if ($uid = Session::getLoginUserID()) { $input["users_id"] = Session::getLoginUserID(); } + if ($this->file_copy_source instanceof self) { + $input['filename'] = $this->file_copy_source->fields['filename']; + $input['filepath'] = $this->file_copy_source->fields['filepath']; + $input['sha1sum'] = $this->file_copy_source->fields['sha1sum']; + $input['mime'] = $this->file_copy_source->fields['mime']; + $this->file_copy_source = null; + } + // Create a doc only selecting a file from a item form $create_from_item = false; if ( @@ -252,14 +268,13 @@ public function prepareInputForAdd($input) } $upload_ok = false; - if (isset($input["_filename"]) && !(empty($input["_filename"]) == 1)) { + if (isset($input['_uploaded_file']) && is_array($input['_uploaded_file'])) { + $upload_ok = $this->uploadDocument($input, $input['_uploaded_file']); + } elseif (isset($input["_filename"]) && !(empty($input["_filename"]) == 1)) { $upload_ok = $this->moveDocument($input, stripslashes((string) array_shift($input["_filename"]))); } elseif (isset($input["upload_file"]) && !empty($input["upload_file"])) { // Move doc from upload dir $upload_ok = $this->moveUploadedDocument($input, $input["upload_file"]); - } elseif (isset($input['filepath']) && file_exists(GLPI_DOC_DIR . '/' . $input['filepath'])) { - // Document is created using an existing document file - $upload_ok = true; } // Tag @@ -285,6 +300,7 @@ public function prepareInputForAdd($input) } unset($input["upload_file"]); + unset($input['_uploaded_file']); // Don't add if no file if ( @@ -374,14 +390,22 @@ public function post_getFromDB() public function prepareInputForUpdate($input) { + $input = $this->filterFields($input); - // security (don't accept filename from $_REQUEST) - if (array_key_exists('filename', $_REQUEST)) { - unset($input['filename']); - } - - if (isset($input['current_filepath'])) { - if (isset($input["_filename"]) && !empty($input["_filename"]) == 1) { + if ( + isset($input['current_filepath']) + || isset($input['_uploaded_file']) + || isset($input['_filename']) + || isset($input['upload_file']) + ) { + // Always use the values stored in DB to prevent arbitrary file + // deletion or replacement using request-controlled paths. + $input['current_filepath'] = $this->fields['filepath']; + $input['current_filename'] = $this->fields['filename']; + + if (isset($input['_uploaded_file']) && is_array($input['_uploaded_file'])) { + $this->uploadDocument($input, $input['_uploaded_file']); + } elseif (isset($input["_filename"]) && !empty($input["_filename"]) == 1) { $this->moveDocument($input, stripslashes((string) array_shift($input["_filename"]))); } elseif (isset($input["upload_file"]) && !empty($input["upload_file"])) { // Move doc from upload dir @@ -391,6 +415,7 @@ public function prepareInputForUpdate($input) unset($input['current_filepath']); unset($input['current_filename']); + unset($input['_uploaded_file']); if (isset($input['link']) && !empty($input['link']) && !Toolbox::isValidWebUrl($input['link'])) { Session::addMessageAfterRedirect( @@ -404,6 +429,26 @@ public function prepareInputForUpdate($input) return $input; } + /** + * Remove fields that must only be produced by server-side upload handling. + * + * @param array $input + * @return array + **/ + private function filterFields(array $input): array + { + if (array_key_exists('filename', $_REQUEST)) { + unset($input['filename']); + } + + foreach (['filepath', 'sha1sum'] as $field) { + if (array_key_exists($field, $input)) { + unset($input[$field]); + } + } + + return $input; + } /** * Print the document form diff --git a/inc/itsmnguploadhandler.class.php b/inc/itsmnguploadhandler.class.php index 0610f7ea1f..fff5d012be 100644 --- a/inc/itsmnguploadhandler.class.php +++ b/inc/itsmnguploadhandler.class.php @@ -211,20 +211,48 @@ public static function addFileToDb($file, $name = null) $name = $file['name']; } $doc = new Document(); - $newDoc['filepath'] = ItsmngUploadHandler::uploadFile( - $file['path'], - $file['name'], - ItsmngUploadHandler::UPLOAD - ); - $newDoc['filename'] = $file['name']; $newDoc['name'] = $name; - $newDoc['mime'] = $file['format']; - $doc->add($newDoc); + $newDoc['_uploaded_file'] = self::getUploadedFileDescriptor($file); + if ($newDoc['_uploaded_file'] !== false) { + $doc->add($newDoc); + } return $doc; } + public static function getUploadedFileDescriptor(array $file) + { + $path = $file['path'] ?? ''; + $realpath = realpath($path); + $tmp_dir = realpath(GLPI_TMP_DIR); + + if ( + $realpath === false + || $tmp_dir === false + || !is_file($realpath) + || !Toolbox::startsWith($realpath, $tmp_dir . DIRECTORY_SEPARATOR) + ) { + Session::addMessageAfterRedirect( + __('Invalid uploaded file'), + false, + ERROR + ); + return false; + } + + return [ + 'name' => $file['name'] ?? basename($realpath), + 'tmp_name' => $realpath, + 'type' => $file['format'] ?? '', + 'error' => UPLOAD_ERR_OK, + ]; + } + public static function linkDocToItem($id, $entity, $isRecursive, $itemType, $itemId, $userId) { + if ((int) $id <= 0) { + return; + } + $docItem = new Document_Item(); $docItem->add([ 'documents_id' => $id,