Shopping Basket

 x 
Cart empty


Popular Items

Find Us Elsewhere

Nasty Trap Lurking Within Joomla JFile Class

I am glad to say that this article is now out of date, because a fix for the issue described has now been included in releases 3.3.0, 3.2.4 and 2.5.20 of Joomla. However I am leaving the article in place for the convenience of sites that are not able to update to the latest release.

 

There have been reports on various Joomla-related forums of a mysterious security issue: a user deletes a file, and the result is that the permissions of the root Joomla site folder are changed to 0777. Depending on the server, this can cause the website to crash, and can make it vulnerable to hackers. Often the JomSocial extension is implicated in this problem, but opinions differ, and it may be that other extensions also feature this vulnerability.

The purpose of this article is to explain why this happens, and how it can be fixed.

Some examples of the reports are http://www.jomsocial.com/forum/technical-issues/5146-public-html-folder-is-changing-permission-into-777 or http://forum.joomla.org/viewtopic.php?t=699264.The common factor in the reports are that a user deletes a file. Looking at JomSocial, like most Joomla extensions, it uses the Joomla core JFile class to handle file deletion, ie

JFile::delete($somefile);

If we look at the JFile class, the code for JFile::delete() is as follows (this is taken from Joomla 3.2 but 2.5 and 1.5 are substantially the same)

	public static function delete($file)
	{
		$FTPOptions = JClientHelper::getCredentials('ftp');
		if (is_array($file))
		{
			$files = $file;
		}
		else
		{
			$files[] = $file;
		}
		// Do NOT use ftp if it is not enabled
		if ($FTPOptions['enabled'] == 1)
		{
			// Connect the FTP client
			$ftp = JClientFtp::getInstance($FTPOptions['host'], $FTPOptions['port'], array(), $FTPOptions['user'], $FTPOptions['pass']);
		}
		foreach ($files as $file)
		{
			$file = JPath::clean($file);
			// Try making the file writable first. If it's read-only, it can't be deleted
			// on Windows, even if the parent folder is writable
			@chmod($file, 0777);
			// In case of restricted permissions we zap it one way or the other
			// as long as the owner is either the webserver or the ftp
			if (@unlink($file))
			{
				// Do nothing
			}
			elseif ($FTPOptions['enabled'] == 1)
			{
				$file = JPath::clean(str_replace(JPATH_ROOT, $FTPOptions['root'], $file), '/');
				if (!$ftp->delete($file))
				{
					// FTP connector throws an error
					return false;
				}
			}
			else
			{
				$filename = basename($file);
				JLog::add(JText::sprintf('JLIB_FILESYSTEM_DELETE_FAILED', $filename), JLog::WARNING, 'jerror');
				return false;
			}
		}
		return true;
	}

You will notice the @chmod($file, 0777). This is fine if the file exists,. the problem arises if $somefile is empty. It could be either an empty string or an array that contains an element that is emtpy. This could happen for example if a user tries to delete a non-existent cover photo. Perhaps the photo has already been deleted, and the code to delete the file is accidentally called twice. The extension looks up the database entry for the user's cover photo, and finds a blank. Unfortunately the programmer has forgotten to test for this possibility, and tries to delete it anyway, with a call that amounts to:-

jimport('joomla.filesystem.file');
JFile::delete('');

This eventually reaches the line

@chmod($file, 0777);

The problem is that when $file is an empty string PHP interprests this as the current working directory, which on a Joomla site is the root directory, and so the permissions of the root directory are changed to 0777. The attempt to delete the root directory (in other words the entire Joomla site) fails (thankfully) because unlink() only works on files. However we are left with a root folder that is now writeable.

I tested this on my own testing site, for the sake of testing I edited the latest news module and put this code at the beginning:-

jimport('joomla.filesystem.file');
JFile::delete('');

This does indeed change the root folder permissions to 0777 and due to the security settings on my server immediately crashes the site. On other servers the site may not crash, instead the root folder will be left writeable and hence more easily hackable.

A similar problem can arise when the value passed to JFile::delete() is a path to a directory rather than a file. Again the end result will be that the directory permissions are changed to 0777 without it being deleted. This may be a more insidious problem because it may well pass unnoticed for some time.

I don't have actual proof but I am pretty certain that this is what is causing the reported problem, it would explain why it seems to appear sporadically and why there is some uncertainty about which extension is actually responsible, because more than one could generate this error. The particularly worrying thing about this is that, if an extension is vulnerable to this problem, it can often be triggered as a result of ordinary user actions. For example many extensions that allow some sort of social interaction will allow users to add and then delete photos.

I think it is a bad thing for Joomla to include such a nasty trap lurking within the JFile class, because it can be easily fixed just by checking within JFile::delete() whether the item being operated on is actually a file before attempting to modify its permissions. I reported this issue to the Joomla Security Strike Team (JSST) several months ago, as yet there has been no patch. This is particularly frustrating because the patch is very, very simple. I explain below how you can fix the problem yourself if you are experiencing it. I can only assume that the lack of action is due to a widespread perception amongst PHP programmers that 0777 folder permissions are not a security risk. I think that this belief is dangerously misguided, I explain why here.

In order to be exploitable this issue does require some sloppy programming. It could be argued that programmers should be careful about what they are deleting, and I would agree. However it is an easy mistake to let slip through, particularly because JFile::delete() will accept an array argument, it can be quite easy to lose track of what is in an array and accidentally send one where an element is an empty string or a directory path. I don't think many programmers would realise that the consequences of not checking what they are deleting could be so dire: the natural assumption is that, if you pass an empty string to a delete function, it will just do nothing..

How to Fix It

Unfortunately, in the absence of an official fix, the only way to fix this problem yourself is to hack the Joomla core, by editing the JFile class. I would normally strongly advise against doing this, however in this case it may be the lesser of two evils. The file that you need to edit is libraries/joomla/filesystem/file.php. You can open it up in a text editor such as notepad or textpad. Do not use a word-processor, that will garble the code. Immediately after

		foreach ($files as $file)
		{

in the code add the following:-

        if(empty($file) || is_dir($file))
        {
           continue;
        }

In Joomla 3.2.3 this should be added immediately after line 187. In earlier versions of Joomla the line number is slightly different, but it should be easy enough to find because it needs to be just before the chmod(0777) line.

This should solve the worst problems. It is not a good idea to just delete the chmod(0777), because this may lead to problems with deleting files that are read-only. For clarity, the complete code for the JFile::delete() method should now be.-

	public static function delete($file)
	{
		$FTPOptions = JClientHelper::getCredentials('ftp');
		if (is_array($file))
		{
			$files = $file;
		}
		else
		{
			$files[] = $file;
		}
		// Do NOT use ftp if it is not enabled
		if ($FTPOptions['enabled'] == 1)
		{
			// Connect the FTP client
			$ftp = JClientFtp::getInstance($FTPOptions['host'], $FTPOptions['port'], array(), $FTPOptions['user'], $FTPOptions['pass']);
		}
		foreach ($files as $file)
		{
            if(empty($file) || is_dir($file))
            {
               continue;
            }
			$file = JPath::clean($file);
			// Try making the file writable first. If it's read-only, it can't be deleted
			// on Windows, even if the parent folder is writable
			@chmod($file, 0777);
			// In case of restricted permissions we zap it one way or the other
			// as long as the owner is either the webserver or the ftp
			if (@unlink($file))
			{
				// Do nothing
			}
			elseif ($FTPOptions['enabled'] == 1)
			{
				$file = JPath::clean(str_replace(JPATH_ROOT, $FTPOptions['root'], $file), '/');
				if (!$ftp->delete($file))
				{
					// FTP connector throws an error
					return false;
				}
			}
			else
			{
				$filename = basename($file);
				JLog::add(JText::sprintf('JLIB_FILESYSTEM_DELETE_FAILED', $filename), JLog::WARNING, 'jerror');
				return false;
			}
		}
		return true;
	}