Issues in proxy.php - All Issues - Inspection of "Enabled PG convert html entities to utf8 (#4690)" - SimpleMachines/SMF2.1 - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Push — release-2.1 ( 61d523...503c62 )

by Colin

created 2018-06-21 18:45 UTC

proxy.php (8 issues)

Labels

Severity

Upgrade to new PHP Analysis Engine

These results are based on our legacy PHP analysis, consider migrating to our new PHP analysis engine instead. Learn more

<?php

/**
 * This is a lightweight proxy for serving images, generally meant to be used alongside SSL
 *
 * Simple Machines Forum (SMF)
 *
 * @package SMF
 * @author Simple Machines http://www.simplemachines.org
 * @copyright 2018 Simple Machines and individual contributors
 * @license http://www.simplemachines.org/about/smf/license.php BSD
 *
 * @version 2.1 Beta 4
 */

if (!defined('SMF'))
	define('SMF', 'proxy');

global $proxyhousekeeping;
function myFunction($a, $b) {
    // Do something
}

/**
 * Class ProxyServer
 */
class ProxyServer
{
	/** @var bool $enabled Whether or not this is enabled */
	protected $enabled;

	/** @var int $maxSize The maximum size for files to cache */
	protected $maxSize;

	/** @var string $secret A secret code used for hashing */
	protected $secret;

	/** @var string The cache directory */
	protected $cache;

	/** @var int $maxDays until enties get deleted */
	protected $maxDays;

	/**
	 * Constructor, loads up the Settings for the proxy
	 *
	 * @access public
	 */
	public function __construct()
	{
		global $image_proxy_enabled, $image_proxy_maxsize, $image_proxy_secret, $cachedir, $sourcedir;
function myFunction($a, $b) {
    // Do something
}

		require_once(dirname(__FILE__) . '/Settings.php');
		require_once($sourcedir . '/Subs.php');

		// Turn off all error reporting; any extra junk makes for an invalid image.
		error_reporting(0);

		$this->enabled = (bool) $image_proxy_enabled;
		$this->maxSize = (int) $image_proxy_maxsize;
		$this->secret = (string) $image_proxy_secret;
		$this->cache = $cachedir . '/images';
		$this->maxDays = 5;
	}

	/**
	 * Checks whether the request is valid or not
	 *
	 * @access public
	 * @return bool Whether the request is valid

	 */
	public function checkRequest()
	{
		if (!$this->enabled)
			return false;

		// Try to create the image cache directory if it doesn't exist
		if (!file_exists($this->cache))
			if (!mkdir($this->cache) || !copy(dirname($this->cache) . '/index.php', $this->cache . '/index.php'))
				return false;

		// Basic sanity check
		$_GET['request'] = validate_iri($_GET['request']);

		// We aren't going anywhere without these
		if (empty($_GET['hash']) || empty($_GET['request']))
			return false;

		$hash = $_GET['hash'];
		$request = $_GET['request'];

		if (md5($request . $this->secret) != $hash)
			return false;

		// Ensure any non-ASCII characters in the URL are encoded correctly
		$request = iri_to_url($request);

		// Attempt to cache the request if it doesn't exist
		if (!$this->isCached($request))
/**
 * @return array|string
 */
function returnsDifferentValues($x) {
    if ($x) {
        return 'foo';
    }

    return array();
}

$x = returnsDifferentValues($y);
if (is_array($x)) {
    // $x is an array.
}
			return $this->cacheImage($request);
/**
 * @return array|string
 */
function returnsDifferentValues($x) {
    if ($x) {
        return 'foo';
    }

    return array();
}

$x = returnsDifferentValues($y);
if (is_array($x)) {
    // $x is an array.
}

		return true;
	}

	/**
	 * Serves the request
	 *
	 * @access public
	 * @return void
	 */
	public function serve()
	{
		$request = $_GET['request'];
		$cached_file = $this->getCachedPath($request);
		$cached = json_decode(file_get_contents($cached_file), true);

		// Did we get an error when trying to fetch the image
		$response = $this->checkRequest();
		if ($response === -1)
		{
			// Throw a 404
			header('HTTP/1.0 404 Not Found');
			exit;
		}
		// Right, image not cached? Simply redirect, then.
		if ($response === 0)
		{
			$this::redirectexit($request);
		}

		$time = time();

		// Is the cache expired?
		if (!$cached || $time - $cached['time'] > ($this->maxDays * 86400))
		{
			@unlink($cached_file);
// For example instead of
@mkdir($dir);

// Better use
if (@mkdir($dir) === false) {
    throw new \RuntimeException('The directory '.$dir.' could not be created.');
}
			if ($this->checkRequest())
				$this->serve();
			$this::redirectexit($request);
		}

		$eTag = '"' . substr(sha1($request) . $cached['time'], 0, 64) . '"';
		if (!empty($_SERVER['HTTP_IF_NONE_MATCH']) && strpos($_SERVER['HTTP_IF_NONE_MATCH'], $eTag) !== false)
		{
			header('HTTP/1.1 304 Not Modified');
			exit;
		}

		// Make sure we're serving an image
		$contentParts = explode('/', !empty($cached['content_type']) ? $cached['content_type'] : '');
		if ($contentParts[0] != 'image')
			exit;

		$max_age = $time - $cached['time'] + (5 * 86400);
		header('content-type: ' . $cached['content_type']);
		header('content-length: ' . $cached['size']);
		header('cache-control: public, max-age=' . $max_age );
		header('last-modified: ' . gmdate('D, d M Y H:i:s', $cached['time']) . ' GMT');
		header('etag: ' . $eTag);
		echo base64_decode($cached['body']);
	}

	/**
	 * Returns the request's hashed filepath
	 *
	 * @access public
	 * @param string $request The request to get the path for
	 * @return string The hashed filepath for the specified request
	 */
	protected function getCachedPath($request)
	{
		return $this->cache . '/' . sha1($request . $this->secret);
	}

	/**
	 * Check whether the image exists in local cache or not
	 *
	 * @access protected
	 * @param string $request The image to check for in the cache
	 * @return bool Whether or not the requested image is cached
	 */
	protected function isCached($request)
	{
		return file_exists($this->getCachedPath($request));
	}

	/**
	 * Attempts to cache the image while validating it
	 *
	 * @access protected
	 * @param string $request The image to cache/validate
	 * @return int -1 error, 0 too big, 1 valid image
	 */
	protected function cacheImage($request)
	{
		$dest = $this->getCachedPath($request);
		$ext = strtolower(pathinfo(parse_url($request, PHP_URL_PATH), PATHINFO_EXTENSION));

		$image = fetch_web_data($request);

		// Looks like nobody was home
		if (empty($image))
			return -1;

		// What kind of file did they give us?
		$finfo = finfo_open(FILEINFO_MIME_TYPE);
		$mime_type = finfo_buffer($finfo, $image);

		// SVG needs a little extra care
		if ($ext == 'svg' && $mime_type == 'text/plain')
			$mime_type = 'image/svg+xml';

		// Make sure the url is returning an image
		if (strpos($mime_type, 'image/') !== 0)
			return -1;

		// Validate the filesize
		$size = strlen($image);
		if ($size > ($this->maxSize * 1024))
			return 0;

		// Cache it for later
		return file_put_contents($dest, json_encode(array(
			'content_type' => $mime_type,
			'size' => $size,
			'time' => time(),
			'body' => base64_encode($image),
		))) === false ? -1 : 1;
	}

	/**
	 * Static helper function to redirect a request
	 *
	 * @access public
	 * @param type $request
	 * @return void
	 */
	static public function redirectexit($request)

	{
		header('Location: ' . $request, false, 301);
if ( ! in_array($value, array('this-is-allowed', 'and-this-too'), true)) {
    throw new \InvalidArgumentException('This input is not allowed.');
}
		exit;
	}

	/**
	 * Delete all old entries
	 *
	 * @access public
	 * @return void
	 */
	public function housekeeping()
	{
		$path = $this->cache . '/';
		if ($handle = opendir($path))
		{
			while (false !== ($file = readdir($handle)))
			{
				$filelastmodified = filemtime($path . $file);

				if ((time() - $filelastmodified) > ($this->maxDays * 86400))
				{
				   unlink($path . $file);
				}
			}

			closedir($handle);
		}
	}
}

if (empty($proxyhousekeeping))
{
	$proxy = new ProxyServer();
	$proxy->serve();
}

?>

Push — release-2.1 ( 61d523...503c62 )

proxy.php (8 issues)

Labels

Severity

Introduced By

Upgrade to new PHP Analysis Engine

1. Pass all data via parameters

2. Create a class that maintains your state

1. Pass all data via parameters

2. Create a class that maintains your state

1 path for user data to reach this point

Response Splitting Attacks

General Strategies to prevent injection

1		<?php
2
3		/**
4		* This is a lightweight proxy for serving images, generally meant to be used alongside SSL
5		*
6		* Simple Machines Forum (SMF)
7		*
8		* @package SMF
9		* @author Simple Machines http://www.simplemachines.org
10		* @copyright 2018 Simple Machines and individual contributors
11		* @license http://www.simplemachines.org/about/smf/license.php BSD
12		*
13		* @version 2.1 Beta 4
14		*/
15
16		if (!defined('SMF'))
17		define('SMF', 'proxy');
18
19		global $proxyhousekeeping;
		0 ignored issues – show Compatibility Best Practice introduced 2018-02-09 18:24 UTC by Report Bug Copy Issue Report Show Similar Issues like this Use of `global` functionality is not recommended; it makes your code harder to test, and less reusable. Instead of relying on `global` state, we recommend one of these alternatives: 1. Pass all data via parameters function myFunction($a, $b) { // Do something } 2. Create a class that maintains your state class MyClass { private $a; private $b; public function __construct($a, $b) { $this->a = $a; $this->b = $b; } public function myFunction() { // Do something } } Loading history...
20
21		/**
22		* Class ProxyServer
23		*/
24		class ProxyServer
25		{
26		/** @var bool $enabled Whether or not this is enabled */
27		protected $enabled;
28
29		/** @var int $maxSize The maximum size for files to cache */
30		protected $maxSize;
31
32		/** @var string $secret A secret code used for hashing */
33		protected $secret;
34
35		/** @var string The cache directory */
36		protected $cache;
37
38		/** @var int $maxDays until enties get deleted */
39		protected $maxDays;
40
41		/**
42		* Constructor, loads up the Settings for the proxy
43		*
44		* @access public
45		*/
46		public function __construct()
47		{
48		global $image_proxy_enabled, $image_proxy_maxsize, $image_proxy_secret, $cachedir, $sourcedir;
		0 ignored issues – show Compatibility Best Practice introduced 2017-08-25 21:51 UTC by Report Bug Copy Issue Report Show Similar Issues like this Use of `global` functionality is not recommended; it makes your code harder to test, and less reusable. Instead of relying on `global` state, we recommend one of these alternatives: 1. Pass all data via parameters function myFunction($a, $b) { // Do something } 2. Create a class that maintains your state class MyClass { private $a; private $b; public function __construct($a, $b) { $this->a = $a; $this->b = $b; } public function myFunction() { // Do something } } Loading history...
49
50		require_once(dirname(__FILE__) . '/Settings.php');
51		require_once($sourcedir . '/Subs.php');
52
53		// Turn off all error reporting; any extra junk makes for an invalid image.
54		error_reporting(0);
55
56		$this->enabled = (bool) $image_proxy_enabled;
57		$this->maxSize = (int) $image_proxy_maxsize;
58		$this->secret = (string) $image_proxy_secret;
59		$this->cache = $cachedir . '/images';
60		$this->maxDays = 5;
61		}
62
63		/**
64		* Checks whether the request is valid or not
65		*
66		* @access public
67		* @return bool Whether the request is valid
		0 ignored issues – show Documentation introduced 2018-02-16 12:33 UTC by Report Bug Copy Issue Report Show Similar Issues like this Should the return type not be `integer\|boolean`? This check compares the return type specified in the `@return` annotation of a function or method doc comment with the types returned by the function and raises an issue if they mismatch. Loading history...
68		*/
69		public function checkRequest()
70		{
71		if (!$this->enabled)
72		return false;
73
74		// Try to create the image cache directory if it doesn't exist
75		if (!file_exists($this->cache))
76		if (!mkdir($this->cache) \|\| !copy(dirname($this->cache) . '/index.php', $this->cache . '/index.php'))
77		return false;
78
79		// Basic sanity check
80		$_GET['request'] = validate_iri($_GET['request']);
81
82		// We aren't going anywhere without these
83		if (empty($_GET['hash']) \|\| empty($_GET['request']))
84		return false;
85
86		$hash = $_GET['hash'];
87		$request = $_GET['request'];
88
89		if (md5($request . $this->secret) != $hash)
90		return false;
91
92		// Ensure any non-ASCII characters in the URL are encoded correctly
93		$request = iri_to_url($request);
94
95		// Attempt to cache the request if it doesn't exist
96		if (!$this->isCached($request))
		0 ignored issues – show Bug introduced 2018-03-11 08:18 UTC by Report Bug Copy Issue Report Show Similar Issues like this It seems like `$request` defined by `iri_to_url($request)` on line `93` can also be of type `boolean`; however, `ProxyServer::isCached()` does only seem to accept `string`, maybe add an additional type check? If a method or function can return multiple different values and unless you are sure that you only can receive a single value in this context, we recommend to add an additional type check: /** * @return array\|string */ function returnsDifferentValues($x) { if ($x) { return 'foo'; } return array(); } $x = returnsDifferentValues($y); if (is_array($x)) { // $x is an array. } If this a common case that PHP Analyzer should handle natively, please let us know by opening an issue. Loading history...
97		return $this->cacheImage($request);
		0 ignored issues – show Bug introduced 2018-03-11 08:18 UTC by Report Bug Copy Issue Report Show Similar Issues like this It seems like `$request` defined by `iri_to_url($request)` on line `93` can also be of type `boolean`; however, `ProxyServer::cacheImage()` does only seem to accept `string`, maybe add an additional type check? If a method or function can return multiple different values and unless you are sure that you only can receive a single value in this context, we recommend to add an additional type check: /** * @return array\|string */ function returnsDifferentValues($x) { if ($x) { return 'foo'; } return array(); } $x = returnsDifferentValues($y); if (is_array($x)) { // $x is an array. } If this a common case that PHP Analyzer should handle natively, please let us know by opening an issue. Loading history...
98
99		return true;
100		}
101
102		/**
103		* Serves the request
104		*
105		* @access public
106		* @return void
107		*/
108		public function serve()
109		{
110		$request = $_GET['request'];
111		$cached_file = $this->getCachedPath($request);
112		$cached = json_decode(file_get_contents($cached_file), true);
113
114		// Did we get an error when trying to fetch the image
115		$response = $this->checkRequest();
116		if ($response === -1)
117		{
118		// Throw a 404
119		header('HTTP/1.0 404 Not Found');
120		exit;
121		}
122		// Right, image not cached? Simply redirect, then.
123		if ($response === 0)
124		{
125		$this::redirectexit($request);
126		}
127
128		$time = time();
129
130		// Is the cache expired?
131		if (!$cached \|\| $time - $cached['time'] > ($this->maxDays * 86400))
132		{
133		@unlink($cached_file);
		0 ignored issues – show Security Best Practice introduced 2016-12-27 02:48 UTC by Report Bug Copy Issue Report Show Similar Issues like this It seems like you do not handle an error condition here. This can introduce security issues, and is generally not recommended. If you suppress an error, we recommend checking for the error condition explicitly: // For example instead of @mkdir($dir); // Better use if (@mkdir($dir) === false) { throw new \RuntimeException('The directory '.$dir.' could not be created.'); } Loading history...
134		if ($this->checkRequest())
135		$this->serve();
136		$this::redirectexit($request);
137		}
138
139		$eTag = '"' . substr(sha1($request) . $cached['time'], 0, 64) . '"';
140	View Code Duplication	if (!empty($_SERVER['HTTP_IF_NONE_MATCH']) && strpos($_SERVER['HTTP_IF_NONE_MATCH'], $eTag) !== false)
141		{
142		header('HTTP/1.1 304 Not Modified');
143		exit;
144		}
145
146		// Make sure we're serving an image
147		$contentParts = explode('/', !empty($cached['content_type']) ? $cached['content_type'] : '');
148		if ($contentParts[0] != 'image')
149		exit;
150
151		$max_age = $time - $cached['time'] + (5 * 86400);
152		header('content-type: ' . $cached['content_type']);
153		header('content-length: ' . $cached['size']);
154		header('cache-control: public, max-age=' . $max_age );
155		header('last-modified: ' . gmdate('D, d M Y H:i:s', $cached['time']) . ' GMT');
156		header('etag: ' . $eTag);
157		echo base64_decode($cached['body']);
158		}
159
160		/**
161		* Returns the request's hashed filepath
162		*
163		* @access public
164		* @param string $request The request to get the path for
165		* @return string The hashed filepath for the specified request
166		*/
167		protected function getCachedPath($request)
168		{
169		return $this->cache . '/' . sha1($request . $this->secret);
170		}
171
172		/**
173		* Check whether the image exists in local cache or not
174		*
175		* @access protected
176		* @param string $request The image to check for in the cache
177		* @return bool Whether or not the requested image is cached
178		*/
179		protected function isCached($request)
180		{
181		return file_exists($this->getCachedPath($request));
182		}
183
184		/**
185		* Attempts to cache the image while validating it
186		*
187		* @access protected
188		* @param string $request The image to cache/validate
189		* @return int -1 error, 0 too big, 1 valid image
190		*/
191		protected function cacheImage($request)
192		{
193		$dest = $this->getCachedPath($request);
194		$ext = strtolower(pathinfo(parse_url($request, PHP_URL_PATH), PATHINFO_EXTENSION));
195
196		$image = fetch_web_data($request);
197
198		// Looks like nobody was home
199		if (empty($image))
200		return -1;
201
202		// What kind of file did they give us?
203		$finfo = finfo_open(FILEINFO_MIME_TYPE);
204		$mime_type = finfo_buffer($finfo, $image);
205
206		// SVG needs a little extra care
207		if ($ext == 'svg' && $mime_type == 'text/plain')
208		$mime_type = 'image/svg+xml';
209
210		// Make sure the url is returning an image
211		if (strpos($mime_type, 'image/') !== 0)
212		return -1;
213
214		// Validate the filesize
215		$size = strlen($image);
216		if ($size > ($this->maxSize * 1024))
217		return 0;
218
219		// Cache it for later
220		return file_put_contents($dest, json_encode(array(
221		'content_type' => $mime_type,
222		'size' => $size,
223		'time' => time(),
224		'body' => base64_encode($image),
225		))) === false ? -1 : 1;
226		}
227
228		/**
229		* Static helper function to redirect a request
230		*
231		* @access public
232		* @param type $request
233		* @return void
234		*/
235		static public function redirectexit($request)
		0 ignored issues – show Coding Style introduced 2018-02-16 12:33 UTC by Report Bug Copy Issue Report Show Similar Issues like this As per PSR2, the `static` declaration should come after the visibility declaration. Loading history...
236		{
237		header('Location: ' . $request, false, 301);
		0 ignored issues – show Security Response Splitting introduced 2018-02-16 12:33 UTC by Report Bug Copy Issue Report Show Similar Issues like this `'Location: ' . $request` can contain request data and is used in response header context(s) leading to a potential security vulnerability. 1 path for user data to reach this point Read from `$_GET,` and `$request` is assigned in proxy.php on line 110 `$request` is passed to ProxyServer::redirectexit() in proxy.php on line 125 Response Splitting Attacks Allowing an attacker to set a response header, opens your application to response splitting attacks; effectively allowing an attacker to send any response, he would like. General Strategies to prevent injection In general, it is advisable to prevent any user-data to reach this point. This can be done by white-listing certain values: if ( ! in_array($value, array('this-is-allowed', 'and-this-too'), true)) { throw new \InvalidArgumentException('This input is not allowed.'); } For numeric data, we recommend to explicitly cast the data: $sanitized = (integer) $tainted; Loading history...
238		exit;
239		}
240
241		/**
242		* Delete all old entries
243		*
244		* @access public
245		* @return void
246		*/
247		public function housekeeping()
248		{
249		$path = $this->cache . '/';
250		if ($handle = opendir($path))
251		{
252		while (false !== ($file = readdir($handle)))
253		{
254		$filelastmodified = filemtime($path . $file);
255
256		if ((time() - $filelastmodified) > ($this->maxDays * 86400))
257		{
258		unlink($path . $file);
259		}
260		}
261
262		closedir($handle);
263		}
264		}
265		}
266
267		if (empty($proxyhousekeeping))
268		{
269		$proxy = new ProxyServer();
270		$proxy->serve();
271		}
272
273		?>

SimpleMachines / SMF2.1

Push — release-2.1 ( 61d523...503c62 )

proxy.php (8 issues)

Labels

Severity

Introduced By

Upgrade to new PHP Analysis Engine

1. Pass all data via parameters

2. Create a class that maintains your state

1. Pass all data via parameters

2. Create a class that maintains your state

1 path for user data to reach this point

Response Splitting Attacks

General Strategies to prevent injection

Duplication Side-by-Side

Filter issues like