Solidate mime type guessing. (#4056)

* Improve security of mime type guessing.

* Update mime guessing changes in user guide and changelogs.

* Add ext-fileinfo to suggest section of composer.json.

Author: sfadschm

CHANGELOG.md

@@ -10,6 +10,12 @@
 - `CodeIgniter\Config\Config` is now deprecated in favor of `CodeIgniter\Config\Factories::config()`
 - HTTP Layer Refactor: Numerous deprecations have been made towards a transition to a PSR-compliant HTTP layer. [See the User Guide](user_guide_src/source/installation/upgrade_405.rst)
 
+**Mime Type Detection**
+
+- `Config\Mimes::guessExtensionFromType` now only reverse searches the `$mimes` array if no extension is proposed (i.e., usually not for uploaded files).
+- The fallback values of `UploadedFile->getExtension()` and `UploadedFile->guessExtension()` have been changed. `UploadedFile->getExtension()` now returns `$this->getClientExtension()` instead of `''`; `UploadedFile->guessExtension()` now returns `''` instead of `$this->getClientExtension()`.
+These changes increase security when handling uploaded files as the client can no longer force a wrong mime type on the application. However, these might affect how file extensions are detected in your application.
+
 ## [v4.0.4](https://github.com/codeigniter4/CodeIgniter4/tree/v4.0.4) (2020-07-15)
 
 [Full Changelog](https://github.com/codeigniter4/CodeIgniter4/compare/v4.0.3...v4.0.4)

admin/framework/composer.json

@@ -22,6 +22,9 @@
 		"predis/predis": "^1.1",
 		"squizlabs/php_codesniffer": "^3.3"
 	},
+	"suggest": {
+		"ext-fileinfo": "Improves mime type detection for files"
+	},
 	"autoload": {
 		"psr-4": {
 			"CodeIgniter\\": "system/"

admin/module/composer.json

@@ -12,6 +12,9 @@
 		"mikey179/vfsstream": "^1.6",
 		"phpunit/phpunit": "^8.5"
 	},
+	"suggest": {
+		"ext-fileinfo": "Improves mime type detection for files"
+	},
 	"autoload-dev": {
 		"psr-4": {
 			"Tests\\Support\\": "tests/_support"

admin/starter/composer.json

@@ -13,6 +13,9 @@
 		"mikey179/vfsstream": "^1.6",
 		"phpunit/phpunit": "^8.5"
 	},
+	"suggest": {
+		"ext-fileinfo": "Improves mime type detection for files"
+	},
 	"autoload": {
 		"psr-4": {
 			"App\\": "app",

app/Config/Mimes.php

@@ -12,6 +12,9 @@
  * the most common one should be first in the array to aid the guess*
  * methods. The same applies when more than one mime-type exists for a
  * single extension.
+ *
+ * When working with mime types, please make sure you have the ´fileinfo´
+ * extension enabled to reliably detect the media types.
  */
 
 class Mimes
@@ -33,7 +36,6 @@ class Mimes
 			'text/csv',
 			'text/x-comma-separated-values',
 			'text/comma-separated-values',
-			'application/octet-stream',
 			'application/vnd.ms-excel',
 			'application/x-csv',
 			'text/x-csv',
@@ -69,7 +71,6 @@ class Mimes
 			'application/pdf',
 			'application/force-download',
 			'application/x-download',
-			'binary/octet-stream',
 		],
 		'ai'    => [
 			'application/pdf',
@@ -462,12 +463,10 @@ class Mimes
 		'srt'   => [
 			'text/srt',
 			'text/plain',
-			'application/octet-stream',
 		],
 		'vtt'   => [
 			'text/vtt',
 			'text/plain',
-			'application/octet-stream',
 		],
 		'ico'   => [
 			'image/x-icon',
@@ -509,11 +508,20 @@ public static function guessExtensionFromType(string $type, string $proposedExte
 
 		$proposedExtension = trim(strtolower($proposedExtension));
 
-		if ($proposedExtension !== '' && array_key_exists($proposedExtension, static::$mimes) && in_array($type, is_string(static::$mimes[$proposedExtension]) ? [static::$mimes[$proposedExtension]] : static::$mimes[$proposedExtension], true))
+		if ($proposedExtension !== '')
 		{
-			return $proposedExtension;
+			if(array_key_exists($proposedExtension, static::$mimes) && in_array($type, is_string(static::$mimes[$proposedExtension]) ? [static::$mimes[$proposedExtension]] : static::$mimes[$proposedExtension], true))
+			{
+				// The detected mime type matches with the proposed extension.
+				return $proposedExtension;
+			}
+
+			// An extension was proposed, but the media type does not match the mime type list.
+			return null;
 		}
 
+		// Reverse check the mime type list if no extension was proposed.
+		// This search is order sensitive!
 		foreach (static::$mimes as $ext => $types)
 		{
 			if ((is_string($types) && $types === $type) || (is_array($types) && in_array($type, $types, true)))

composer.json

@@ -24,6 +24,9 @@
 		"rector/rector": "^0.8",
 		"squizlabs/php_codesniffer": "^3.3"
 	},
+	"suggest": {
+		"ext-fileinfo": "Improves mime type detection for files"
+	},
 	"config": {
 		"optimize-autoloader": true,
 		"preferred-install": "dist",

system/HTTP/Files/UploadedFile.php

@@ -308,24 +308,29 @@ public function getTempName(): string
 	 * Overrides SPLFileInfo's to work with uploaded files, since
 	 * the temp file that's been uploaded doesn't have an extension.
 	 *
-	 * Is simply an alias for guessExtension for a safer method
-	 * than simply relying on the provided extension.
-	 * Additionally it will return clientExtension in case if there are
-	 * other extensions with the same mime type.
+	 * This method tries to guess the extension from the files mime
+	 * type but will return the clientExtension if it fails to do so.
+	 *
+	 * This method will always return a more or less helpfull extension
+	 * but might be insecure if the mime type is not machted. Consider
+	 * using guessExtension for a more safe version.
 	 */
 	public function getExtension(): string
 	{
-		return $this->guessExtension();
+		return $this->guessExtension() ?: $this->getClientExtension();
 	}
 
 	/**
-	 * Attempts to determine the best file extension.
+	 * Attempts to determine the best file extension from the file's
+	 * mime type. In contrast to getExtension, this method will return
+	 * an empty string if it fails to determine an extension instead of
+	 * falling back to the unsecure clientExtension.
 	 *
 	 * @return string
 	 */
 	public function guessExtension(): string
 	{
-		return Mimes::guessExtensionFromType($this->getMimeType(), $this->getClientExtension()) ?? $this->getClientExtension();
+		return Mimes::guessExtensionFromType($this->getMimeType(), $this->getClientExtension()) ?? '';
 	}
 
 	//--------------------------------------------------------------------

system/Validation/FileRules.php

@@ -263,7 +263,7 @@ public function ext_in(?string $blank, string $params): bool
 				return true;
 			}
 
-			if (! in_array($file->getExtension(), $params, true))
+			if (! in_array($file->guessExtension(), $params, true))
 			{
 				return false;
 			}

tests/system/HTTP/Files/FileCollectionTest.php

@@ -185,11 +185,11 @@ public function testExtensionGuessing()
 		$this->assertEquals('txt', $file->getExtension());
 
 		// proposed extension matches finfo_open mime type (text/plain)
-		// but not client mime type
 		$file = $collection->getFile('userfile2');
 		$this->assertInstanceOf(UploadedFile::class, $file);
 		$this->assertEquals('txt', $file->getExtension());
-		$this->assertNotEquals('txt', \Config\Mimes::guessExtensionFromType($file->getClientMimeType(), $file->getClientExtension()) ?? $file->getClientExtension());
+		// but not client mime type
+		$this->assertEquals(null, \Config\Mimes::guessExtensionFromType($file->getClientMimeType(), $file->getClientExtension()));
 
 		// proposed extension does not match finfo_open mime type (text/plain)
 		// but can be resolved by reverse searching
@@ -206,8 +206,11 @@ public function testExtensionGuessing()
 		// this is a zip file (userFile4) but hat been renamed to 'rar'
 		$file = $collection->getFile('userfile5');
 		$this->assertInstanceOf(UploadedFile::class, $file);
-		$this->assertNotEquals('rar', $file->getExtension());
-		$this->assertEquals('rar', \Config\Mimes::guessExtensionFromType($file->getClientMimeType(), $file->getClientExtension()) ?? $file->getClientExtension());
+		// getExtension falls back to clientExtension (insecure)
+		$this->assertEquals('rar', $file->getExtension());
+		$this->assertEquals('rar', \Config\Mimes::guessExtensionFromType($file->getClientMimeType(), $file->getClientExtension()));
+		// guessExtension is secure and does not returns empty
+		$this->assertEquals('', $file->guessExtension());
 	}
 
 	//--------------------------------------------------------------------

user_guide_src/source/changelogs/v4.0.5.rst

@@ -12,6 +12,8 @@ Enhancements:
 - New URL helper function ``url_is()`` which allows you to check the current URL to see if matches the given string.
 - Services now have their config parameters strictly typehinted. This will ensure no one will pass a different config instance. If you need to pass a new config with additional properties, you need to extend that particular config.
 - Support for setting SameSite attribute on Session and CSRF cookies has been added. For security and compatibility with latest browser versions, the default setting is ``Lax``.
+- Guessing file extensions from mime type in ``Config\Mimes::guessExtensionFromType`` now only reverse searches the ``$mimes`` array if no extension is proposed (i.e., usually not for uploaded files).
+- The getter functions for file extensions of uploaded files now have different fallback values (``$this->getClientExtension()`` for ``UploadedFile->getExtension()`` and ``''`` for ``UploadedFile->guessExtension()``). This is a security fix and makes the process less dependent on the client.
 
 Changes: