From 9cdb012ca64ff1f719f8120d5fd162cd5ef1013f Mon Sep 17 00:00:00 2001 From: Mariusz Krzaczkowski Date: Tue, 14 Dec 2021 14:20:27 +0100 Subject: [PATCH] Improved validation of uploaded files --- app/Fields/File.php | 75 +++++++++++++++++---------- config/version.php | 2 +- languages/en-US/Other/Exceptions.json | 3 +- 3 files changed, 52 insertions(+), 28 deletions(-) diff --git a/app/Fields/File.php b/app/Fields/File.php index d3fd25ac3b44..0cc183db2159 100644 --- a/app/Fields/File.php +++ b/app/Fields/File.php @@ -125,6 +125,9 @@ public static function loadFromInfo($fileInfo) foreach ($fileInfo as $key => $value) { $instance->{$key} = $fileInfo[$key]; } + if (isset($instance->name)) { + $instance->name = trim(\App\Purifier::purify($instance->name)); + } return $instance; } @@ -148,14 +151,14 @@ public static function loadFromRequest($file) /** * Load file instance from file path. * - * @param array $path + * @param string $path * * @return \self */ - public static function loadFromPath($path) + public static function loadFromPath(string $path) { $instance = new self(); - $instance->name = basename($path); + $instance->name = trim(\App\Purifier::purify(basename($path))); $instance->path = $path; return $instance; } @@ -202,7 +205,7 @@ public static function loadFromContent(string $contents, $name = false, array $p $name = \App\TextParser::textTruncate($name, 180, false) . '_' . uniqid() . ".$extension"; } $instance = new self(); - $instance->name = $name; + $instance->name = trim(\App\Purifier::purify($name)); $instance->path = $path; $instance->ext = $extension; foreach ($param as $key => $value) { @@ -377,13 +380,13 @@ public function getDirectoryPath() /** * Validate whether the file is safe. * - * @param bool|string $type + * @param string|null $type * * @throws \Exception * * @return bool */ - public function validate($type = false) + public function validate(?string $type = null): bool { $return = true; try { @@ -408,7 +411,7 @@ public function validate($type = false) $message = \call_user_func_array('vsprintf', [\App\Language::translateSingleMod(array_shift($params), 'Other.Exceptions'), $params]); } $this->validateError = $message; - Log::error("Error: {$e->getMessage()} | {$this->getName()} | {$this->getSize()}", __CLASS__); + Log::error("Error during file validation: {$this->getName()} | Size: {$this->getSize()}\n {$e->__toString()}", __CLASS__); } return $return; } @@ -416,17 +419,17 @@ public function validate($type = false) /** * Validate and secure the file. * - * @param bool|string $type + * @param string|null $type * * @return bool */ - public function validateAndSecure($type = false): bool + public function validateAndSecure(?string $type = null): bool { if ($this->validate($type)) { return true; } $reValidate = false; - if ('image' === $this->getShortMimeType(0) && static::secureImage($this)) { + if (static::secureFile($this)) { $this->size = filesize($this->path); $this->content = file_get_contents($this->path); $reValidate = true; @@ -480,6 +483,9 @@ private function checkFile() if (empty($this->name)) { throw new \App\Exceptions\DangerousFile('ERR_FILE_EMPTY_NAME'); } + if (!$this->validateInjection($this->name)) { + throw new \App\Exceptions\DangerousFile('ERR_FILE_ILLEGAL_NAME'); + } if (0 === $this->getSize()) { throw new \App\Exceptions\DangerousFile('ERR_FILE_WRONG_SIZE'); } @@ -533,7 +539,7 @@ private function validateCodeInjection() || false !== stripos($contents, 'searchCodeInjection() ) { - throw new \App\Exceptions\DangerousFile('ERR_FILE_PHP_CODE_INJECTION'); + throw new \App\Exceptions\DangerousFile('ERR_FILE_CODE_INJECTION'); } } } @@ -636,43 +642,57 @@ private function searchCodeInjection(): bool */ private function validateCodeInjectionInMetadata() { - if ( + if (\extension_loaded('imagick')) { + try { + $img = new \imagick($this->path); + $this->validateInjection($img->getImageProperties()); + } catch (\Throwable $e) { + throw new \App\Exceptions\DangerousFile('ERR_FILE_CODE_INJECTION', $e->getCode(), $e); + } + } elseif ( \function_exists('exif_read_data') && \in_array($this->getMimeType(), ['image/jpeg', 'image/tiff']) && \in_array(exif_imagetype($this->path), [IMAGETYPE_JPEG, IMAGETYPE_TIFF_II, IMAGETYPE_TIFF_MM]) ) { $imageSize = getimagesize($this->path, $imageInfo); - if ( - $imageSize - && (empty($imageInfo['APP1']) || 0 === strpos($imageInfo['APP1'], 'Exif')) - && ($exifdata = exif_read_data($this->path)) && !$this->validateImageMetadata($exifdata) - ) { - throw new \App\Exceptions\DangerousFile('ERR_FILE_PHP_CODE_INJECTION'); + try { + if ( + $imageSize + && (empty($imageInfo['APP1']) || 0 === strpos($imageInfo['APP1'], 'Exif')) + && ($exifData = exif_read_data($this->path)) && !$this->validateInjection($exifData) + ) { + throw new \App\Exceptions\DangerousFile('ERR_FILE_CODE_INJECTION'); + } + } catch (\Throwable $e) { + throw new \App\Exceptions\DangerousFile('ERR_FILE_CODE_INJECTION', $e->getCode(), $e); } } } /** - * Validate image metadata. + * Validate injection. * - * @param mixed $data + * @param string|array $data * * @return bool */ - private function validateImageMetadata($data) + private function validateInjection($data): bool { + $return = true; if (\is_array($data)) { foreach ($data as $value) { - if (!$this->validateImageMetadata($value)) { + if (!$this->validateInjection($value)) { return false; } } } else { if (1 === preg_match('/(<\?php?(.*?))/i', $data) || false !== stripos($data, 'validate($type)) { - if (!static::secureImage($file)) { + if (!static::secureFile($file)) { $attach[] = ['name' => $file->getName(), 'error' => $file->validateError, 'hash' => $request->getByType('hash', 'Alnum')]; continue; } @@ -1301,8 +1321,11 @@ public static function cleanTemp($keys) * * @return bool */ - public static function secureImage(self $file): bool + public static function secureFile(self $file): bool { + if ('image' !== $file->getShortMimeType(0)) { + return false; + } $result = false; if (\extension_loaded('imagick')) { try { diff --git a/config/version.php b/config/version.php index 366143bbae2a..34aa642feb1c 100644 --- a/config/version.php +++ b/config/version.php @@ -1,7 +1,7 @@ '6.3.5', + 'appVersion' => '6.3.6', 'patchVersion' => '2021.12.14', 'lib_roundcube' => '0.2.3', ]; diff --git a/languages/en-US/Other/Exceptions.json b/languages/en-US/Other/Exceptions.json index 92e50d572227..ae68b0886ff8 100644 --- a/languages/en-US/Other/Exceptions.json +++ b/languages/en-US/Other/Exceptions.json @@ -22,8 +22,9 @@ "ERR_FILE_EMPTY_NAME": "File name is empty", "ERR_FILE_ERROR_REQUEST": "File transfer error: %s", "ERR_FILE_ILLEGAL_FORMAT": "Illegal file format", + "ERR_FILE_ILLEGAL_NAME": "Illegal file name", "ERR_FILE_WRONG_IMAGE": "Wrong image file", - "ERR_FILE_PHP_CODE_INJECTION": "File contains dangerous PHP code and was rejected", + "ERR_FILE_CODE_INJECTION": "File contains dangerous code and was rejected", "ERR_FILE_XPACKET_CODE_INJECTION": "File contains dangerous XPACKET code and was rejected", "ERR_NO_PERMISSIONS_TO_CREATE_DIRECTORIES": "No permissions to create directories", "ERR_DIRECTORY_CANNOT_BE_CREATED": "The directory cannot be created",