Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: is_image validation is vulnerable #3184

Closed
mmrtonmoybd opened this issue Jun 29, 2020 · 1 comment
Closed

Bug: is_image validation is vulnerable #3184

mmrtonmoybd opened this issue Jun 29, 2020 · 1 comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them

Comments

@mmrtonmoybd
Copy link
Contributor

Codeigniter is_image validation method is vulnerable. Codeigniter only check mime type to determine an image. An attacker can change uploaded file mime type that they want. I asked my friend who is a cyber security specialist more than 4 years. He said “In Kali Linux has a tool to change uploaded mime type” .

But codeigniter is_image only check mime type.

I change some code in is_image method to secure it.

In before :

public function is_image(string $blank = null, string $params): bool
	{
		// Grab the file name off the top of the $params
		// after we split it.
		$params = explode(',', $params);
		$name   = array_shift($params);

		if (! ($files = $this->request->getFileMultiple($name)))
		{
			$files = [$this->request->getFile($name)];
		}

		foreach ($files as $file)
		{
			if (is_null($file))
			{
				return false;
			}

			if ($file->getError() === UPLOAD_ERR_NO_FILE)
			{
				return true;
			}

			// We know that our mimes list always has the first mime
			// start with `image` even when then are multiple accepted types.
			$type = \Config\Mimes::guessTypeFromExtension($file->getExtension());

			if (mb_strpos($type, 'image') !== 0)
			{
				return false;
			}
		}

		return true;
	}

I added some code. I give a comment for understand other what I do.
In after

public function is_image(string $blank = null, string $params): bool
	{
		// Grab the file name off the top of the $params
		// after we split it.
		$params = explode(',', $params);
		$name   = array_shift($params);

		if (! ($files = $this->request->getFileMultiple($name)))
		{
			$files = [$this->request->getFile($name)];
		}

		foreach ($files as $file)
		{
			if (is_null($file))
			{
				return false;
			}
			/*
			first I check the image size with the help of getimagesize php build in function. We know that any valid image has a size and it bigger than 0. So we check if the image size is is greater then 0 then it works otherwise false. Now we imagecreatefrompng, imagecreatefromgif, imagecreatefromjpeg, and imagecreatefromwbmp for high security purpose to determine that uploaded image is an valid image not any malices code.
			
			So we check mime type for image if mime type matches in if condition then it run. Example: I am an attacker. I want to upload c99 shell. I change my upload mime type to image/jpeg. Now I pass of the check, but whene we check imagecreatefromjpeg. Then I did not uploaded my shell. Because imagecreatefromjpeg can expect only jpeg/jpg formate OTHERWISE false.
			*/
            $tempfile = $file->getTempName();
			if (getimagesize($tempfile) > 0) {
				if ($file->getMimeType() == 'image/jpeg' || $file->getMimeType() == 'image/pjpeg') {
			if (!imagecreatefromjpeg($tempfile)) {
				return false;
			}
			}
			if ($file->getMimeType() == 'image/png' || $file->getMimeType() == 'image/x-png') {
			if (!imagecreatefrompng($tempfile)) {
				return false;
			}
			}
			if ($file->getMimeType() == 'image/gif') {
			if (!imagecreatefromgif($tempfile)) {
				return false;
			}
			}
			if ($file->getMimeType() == 'image/bmp' || $file->getMimeType() == 'image/x-bmp' || $file->getMimeType() == 'image/x-bitmap' 
			|| $file->getMimeType() == 'image/x-xbitmap' || $file->getMimeType() == 'image/x-win-bitmap' || $file->getMimeType() == 'image/x-windows-bmp' 
			|| $file->getMimeType() == 'image/ms-bmp' || $file->getMimeType() == 'image/x-ms-bmp') {
			if (!imagecreatefromwbmp($tempfile)) {
				return false;
			}
			}
			} else {
				return false;
			}
			//my code is end
			if ($file->getError() === UPLOAD_ERR_NO_FILE)
			{
				return true;
			}

			// We know that our mimes list always has the first mime
			// start with `image` even when then are multiple accepted types.
			$type = \Config\Mimes::guessTypeFromExtension($file->getExtension());

			if (mb_strpos($type, 'image') !== 0)
			{
				return false;
			}
		}

		return true;
	}

Please read the commented text in 2nd code to understand.
Now the is_image method is secure.
If I do any worng please comment on the issue.

@mmrtonmoybd mmrtonmoybd added the bug Verified issues on the current code behavior or pull requests that will fix them label Jun 29, 2020
@michalsn
Copy link
Member

I wouldn't call myself a security expert, but this is not a bug.

I'm pretty sure that we clearly say in the user guide that is_image rule relies on a mime-type check. Additionally, as you can see in the code, we also take into accout the file extension. So simply changing mime-type to image for PHP file won't work. Even if you will try to inject some malicious code to the image file, you will end up with .jpg (or other) image extension - which won't be executed as a PHP file.

That being said, this rule seems fine to me, but we always welcome improvements. If you would like to add another rule for checking images (more restrictive, designed for the most common extensions?), we will be happy to see a PR from you. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

No branches or pull requests

2 participants