Here we go! New data format for the file ACM module:

- No longer PHP files that are included
- Contain a simple PHP header to stop people attempting to read them
- Read using file system functions only reading as much data as required
Result is:
- Better performance by minimising file system reads
- Injected HTML from nasty scripts no longer contaminates the board
- Opcode caches are no longer thrashed and fragmented by the many files we generate



git-svn-id: file:///svn/phpbb/branches/phpBB-3_0_0@9543 89ea8834-ac86-4346-8a33-228a782c2dd0

phpBB/docs/CHANGELOG.html

@@ -91,6 +91,7 @@
 		<li>[Fix] Allow whitespaces in avatar gallery names. (Bug #44955)</li>
 		<li>[Fix] Sorting by author or subject on viewtopic now preserves the order. (Bug #44875)</li>
 		<li>[Fix] Correctly determine writable status of files on Windows operating system. (Bug #39035)</li>
+		<li>[Change] Change the data format of the default file ACM to be more secure from tampering and have better performance.</li>
 		<li>[Feature] Backported 3.2 captcha plugins.</li>
 		<li>[Feature] Introduced new ACM plugins:
 			<ul>

phpBB/includes/acm/acm_file.php

@@ -3,7 +3,7 @@
 *
 * @package acm
 * @version $Id$
-* @copyright (c) 2005 phpBB Group
+* @copyright (c) 2005, 2009 phpBB Group
 * @license http://opensource.org/licenses/gpl-license.php GNU Public License
 *
 */
@@ -44,17 +44,7 @@ class acm
 	*/
 	function load()
 	{
-		global $phpEx;
+		return $this->_read('data_global');
-		if (file_exists($this->cache_dir . 'data_global.' . $phpEx))
-		{
-			@include($this->cache_dir . 'data_global.' . $phpEx);
-		}
-		else
-		{
-			return false;
-		}
-
-		return true;
 	}
 
 	/**
@@ -86,22 +76,7 @@ class acm
 
 		global $phpEx;
 
-		if ($fp = @fopen($this->cache_dir . 'data_global.' . $phpEx, 'wb'))
+		if (!$this->_write('data_global'))
-		{
-			@flock($fp, LOCK_EX);
-			fwrite($fp, "<?php\nif (!defined('IN_PHPBB')) exit;\n\$this->vars = " . var_export($this->vars, true) . ";\n\n\$this->var_expires = " . var_export($this->var_expires, true) . "\n?>");
-			@flock($fp, LOCK_UN);
-			fclose($fp);
-
-			if (!function_exists('phpbb_chmod'))
-			{
-				global $phpbb_root_path;
-				include($phpbb_root_path . 'includes/functions.' . $phpEx);
-			}
-
-			phpbb_chmod($this->cache_dir . 'data_global.' . $phpEx, CHMOD_READ | CHMOD_WRITE);
-		}
-		else
 		{
 			// Now, this occurred how often? ... phew, just tell the user then...
 			if (!@is_writable($this->cache_dir))
@@ -132,6 +107,8 @@ class acm
 			return;
 		}
 
+		$time = time();
+
 		while (($entry = readdir($dir)) !== false)
 		{
 			if (!preg_match('/^(sql_|data_(?!global))/', $entry))
@@ -139,9 +116,20 @@ class acm
 				continue;
 			}
 
-			$expired = true;
+			if (!($handle = @fopen($this->cache_dir . $entry, 'rb')))
-			@include($this->cache_dir . $entry);
+			{
-			if ($expired)
+				continue;
+			}
+
+			// Skip the PHP header
+			fgets($handle);
+
+			// Skip expiration
+			$expires = (int) fgets($handle);
+
+			fclose($handle);
+
+			if ($time >= $expires)
 			{
 				$this->remove_file($this->cache_dir . $entry);
 			}
@@ -157,7 +145,7 @@ class acm
 
 			foreach ($this->var_expires as $var_name => $expires)
 			{
-				if (time() > $expires)
+				if ($time >= $expires)
 				{
 					$this->destroy($var_name);
 				}
@@ -181,8 +169,7 @@ class acm
 				return false;
 			}
 
-			@include($this->cache_dir . "data{$var_name}.$phpEx");
+			return $this->_read('data' . $var_name);
-			return (isset($data)) ? $data : false;
 		}
 		else
 		{
@@ -197,23 +184,7 @@ class acm
 	{
 		if ($var_name[0] == '_')
 		{
-			global $phpEx;
+			$this->_write('data' . $var_name, $var, time() + $ttl);
-
-			if ($fp = @fopen($this->cache_dir . "data{$var_name}.$phpEx", 'wb'))
-			{
-				@flock($fp, LOCK_EX);
-				fwrite($fp, "<?php\nif (!defined('IN_PHPBB')) exit;\n\$expired = (time() > " . (time() + $ttl) . ") ? true : false;\nif (\$expired) { return; }\n\n\$data =  " . (sizeof($var) ? "unserialize(" . var_export(serialize($var), true) . ");" : 'array();') . "\n\n?>");
-				@flock($fp, LOCK_UN);
-				fclose($fp);
-
-				if (!function_exists('phpbb_chmod'))
-				{
-					global $phpbb_root_path;
-					include($phpbb_root_path . 'includes/functions.' . $phpEx);
-				}
-
-				phpbb_chmod($this->cache_dir . "data{$var_name}.$phpEx", CHMOD_READ | CHMOD_WRITE);
-			}
 		}
 		else
 		{
@@ -288,31 +259,30 @@ class acm
 					continue;
 				}
 
-				// The following method is more failproof than simply assuming the query is on line 3 (which it should be)
+				if (!($handle = @fopen($this->cache_dir . $entry, 'rb')))
-				$check_line = @file_get_contents($this->cache_dir . $entry);
-
-				if (empty($check_line))
 				{
 					continue;
 				}
 
-				// Now get the contents between /* and */
+				// Skip the PHP header
-				$check_line = substr($check_line, strpos($check_line, '/* ') + 3, strpos($check_line, ' */') - strpos($check_line, '/* ') - 3);
+				fgets($handle);
+
+				// Skip expiration
+				fgets($handle);
+
+				// Grab the query, remove the LF
+				$query = substr(fgets($handle), 0, -1);
+
+				fclose($handle);
 
-				$found = false;
 				foreach ($table as $check_table)
 				{
 					// Better catch partial table names than no table names. ;)
-					if (strpos($check_line, $check_table) !== false)
+					if (strpos($query, $check_table) !== false)
-					{
-						$found = true;
-						break;
-					}
-				}
-
-				if ($found)
 					{
 						$this->remove_file($this->cache_dir . $entry);
+						break;
+					}
 				}
 			}
 			closedir($dir);
@@ -371,29 +341,16 @@ class acm
 	*/
 	function sql_load($query)
 	{
-		global $phpEx;
-
 		// Remove extra spaces and tabs
 		$query = preg_replace('/[\n\r\s\t]+/', ' ', $query);
+
+		if (($rowset = $this->_read('sql_' . md5($query))) === false)
+		{
+			return false;
+		}
+
 		$query_id = sizeof($this->sql_rowset);
-
+		$this->sql_rowset[$query_id] = $rowset;
-		if (!file_exists($this->cache_dir . 'sql_' . md5($query) . ".$phpEx"))
-		{
-			return false;
-		}
-
-		@include($this->cache_dir . 'sql_' . md5($query) . ".$phpEx");
-
-		if (!isset($expired))
-		{
-			return false;
-		}
-		else if ($expired)
-		{
-			$this->remove_file($this->cache_dir . 'sql_' . md5($query) . ".$phpEx", true);
-			return false;
-		}
-
 		$this->sql_row_pointer[$query_id] = 0;
 
 		return $query_id;
@@ -404,15 +361,10 @@ class acm
 	*/
 	function sql_save($query, &$query_result, $ttl)
 	{
-		global $db, $phpEx;
+		global $db;
 
 		// Remove extra spaces and tabs
 		$query = preg_replace('/[\n\r\s\t]+/', ' ', $query);
-		$filename = $this->cache_dir . 'sql_' . md5($query) . '.' . $phpEx;
-
-		if ($fp = @fopen($filename, 'wb'))
-		{
-			@flock($fp, LOCK_EX);
 
 		$query_id = sizeof($this->sql_rowset);
 		$this->sql_rowset[$query_id] = array();
@@ -424,21 +376,8 @@ class acm
 		}
 		$db->sql_freeresult($query_result);
 
-			$file = "<?php\nif (!defined('IN_PHPBB')) exit;\n\n/* " . str_replace('*/', '*\/', $query) . " */\n";
+		if ($this->_write('sql_' . md5($query), $this->sql_rowset[$query_id], $ttl + time(), $query))
-			$file .= "\n\$expired = (time() > " . (time() + $ttl) . ") ? true : false;\nif (\$expired) { return; }\n";
-
-			fwrite($fp, $file . "\n\$this->sql_rowset[\$query_id] = " . (sizeof($this->sql_rowset[$query_id]) ? "unserialize(" . var_export(serialize($this->sql_rowset[$query_id]), true) . ");" : 'array();') . "\n\n?>");
-			@flock($fp, LOCK_UN);
-			fclose($fp);
-
-			if (!function_exists('phpbb_chmod'))
 		{
-				global $phpbb_root_path;
-				include($phpbb_root_path . 'includes/functions.' . $phpEx);
-			}
-
-			phpbb_chmod($filename, CHMOD_READ | CHMOD_WRITE);
-
 			$query_result = $query_id;
 		}
 	}
@@ -507,6 +446,262 @@ class acm
 		return true;
 	}
 
+	/**
+	* Read cached data from a specified file
+	*
+	* @access private
+	* @param string $filename Filename to write
+	* @return mixed False if an error was encountered, otherwise the data type of the cached data
+	*/
+	function _read($filename)
+	{
+		global $phpEx;
+
+		$file = "{$this->cache_dir}$filename.$phpEx";
+
+		$type = substr($filename, 0, strpos($filename, '_'));
+
+		if (!file_exists($file))
+		{
+			return false;
+		}
+
+		if (!($handle = @fopen($file, 'rb')))
+		{
+			return false;
+		}
+
+		// Skip the PHP header
+		fgets($handle);
+
+		if ($filename == 'data_global')
+		{
+			$this->vars = $this->var_expires = array();
+
+			$time = time();
+
+			while (($expires = (int) fgets($handle)) && !feof($handle))
+			{
+				// Number of bytes of data
+				$bytes = substr(fgets($handle), 0, -1);
+
+				if (!is_numeric($bytes) || ($bytes = (int) $bytes) === 0)
+				{
+					// We cannot process the file without a valid number of bytes
+					// so we discard it
+					fclose($handle);
+
+					$this->vars = $this->var_expires = array();
+					$this->is_modified = false;
+
+					$this->remove_file($file);
+
+					return false;
+				}
+
+				if ($time >= $expires)
+				{
+					fseek($handle, $bytes, SEEK_CUR);
+
+					continue;
+				}
+
+				$var_name = substr(fgets($handle), 0, -1);
+
+				// Read the length of bytes that consists of data.
+				$data = fread($handle, $bytes - strlen($var_name));
+				$data = @unserialize($data);
+
+				// Don't use the data if it was invalid
+				if ($data !== false)
+				{
+					$this->vars[$var_name] = $data;
+					$this->var_expires[$var_name] = $expires;
+				}
+
+				// Absorb the LF
+				fgets($handle);
+			}
+
+			fclose($handle);
+
+			$this->is_modified = false;
+
+			return true;
+		}
+		else
+		{
+			$data = false;
+			$line = 0;
+
+			while (($buffer = fgets($handle)) && !feof($handle))
+			{
+				$buffer = substr($buffer, 0, -1); // Remove the LF
+
+				// $buffer is only used to read integers
+				// if it is non numeric we have an invalid
+				// cache file, which we will now remove.
+				if (!is_numeric($buffer))
+				{
+					break;
+				}
+
+				if ($line == 0)
+				{
+					$expires = (int) $buffer;
+
+					if (time() >= $expires)
+					{
+						break;
+					}
+
+					if ($type == 'sql')
+					{
+						// Skip the query
+						fgets($handle);
+					}
+				}
+				else if ($line == 1)
+				{
+					$bytes = (int) $buffer;
+
+					// Never should have 0 bytes
+					if (!$bytes)
+					{
+						break;
+					}
+
+					// Grab the serialized data
+					$data = fread($handle, $bytes);
+
+					// Read 1 byte, to trigger EOF
+					fread($handle, 1);
+
+					if (!feof($handle))
+					{
+						// Somebody tampered with our data
+						$data = false;
+					}
+					break;
+				}
+				else
+				{
+					// Something went wrong
+					break;
+				}
+				$line++;
+			}
+			fclose($handle);
+
+			// unserialize if we got some data
+			$data = ($data !== false) ? @unserialize($data) : $data;
+
+			if ($data === false)
+			{
+				$this->remove_file($file);
+				return false;
+			}
+
+			return $data;
+		}
+	}
+
+	/**
+	* Write cache data to a specified file
+	*
+	* 'data_global' is a special case and the generated format is different for this file:
+	* <code>
+	* <?php exit; ?>
+	* (expiration)
+	* (length of var and serialised data)
+	* (var)
+	* (serialised data)
+	* ... (repeat)
+	* </code>
+	*
+	* The other files have a similar format:
+	* <code>
+	* <?php exit; ?>
+	* (expiration)
+	* (query) [SQL files only]
+	* (length of serialised data)
+	* (serialised data)
+	* </code>
+	*
+	* @access private
+	* @param string $filename Filename to write
+	* @param mixed $data Data to store
+	* @param int $expires Timestamp when the data expires
+	* @param string $query Query when caching SQL queries
+	* @return bool True if the file was successfully created, otherwise false
+	*/
+	function _write($filename, $data = null, $expires = 0, $query = '')
+	{
+		global $phpEx;
+
+		$file = "{$this->cache_dir}$filename.$phpEx";
+
+		if ($handle = @fopen($file, 'wb'))
+		{
+			@flock($handle, LOCK_EX);
+
+			// File header
+			fwrite($handle, '<' . '?php exit; ?' . '>');
+
+			if ($filename == 'data_global')
+			{
+				// Global data is a different format
+				foreach ($this->vars as $var => $data)
+				{
+					if (strpos($var, "\r") !== false || strpos($var, "\n") !== false)
+					{
+						// CR/LF would cause fgets() to read the cache file incorrectly
+						// do not cache test entries, they probably won't be read back
+						// the cache keys should really be alphanumeric with a few symbols.
+						continue;
+					}
+					$data = serialize($data);
+
+					// Write out the expiration time
+					fwrite($handle, "\n" . $this->var_expires[$var] . "\n");
+
+					// Length of the remaining data for this var (ignoring two LF's)
+					fwrite($handle, strlen($data . $var) . "\n");
+					fwrite($handle, $var . "\n");
+					fwrite($handle, $data);
+				}
+			}
+			else
+			{
+				fwrite($handle, "\n" . $expires . "\n");
+
+				if (strpos($filename, 'sql_') === 0)
+				{
+					fwrite($handle, $query . "\n");
+				}
+				$data = serialize($data);
+
+				fwrite($handle, strlen($data) . "\n");
+				fwrite($handle, $data);
+			}
+
+			@flock($handle, LOCK_UN);
+			fclose($handle);
+
+			if (!function_exists('phpbb_chmod'))
+			{
+				global $phpbb_root_path;
+				include($phpbb_root_path . 'includes/functions.' . $phpEx);
+			}
+
+			phpbb_chmod($file, CHMOD_READ | CHMOD_WRITE);
+
+			return true;
+		}
+
+		return false;
+	}
+
 	/**
 	* Removes/unlinks file
 	*/