Merge pull request #892 from narfbg/develop-core-security

Improve the core Security library

1 files changed, 106 insertions, 173 deletions

diff --git a/system/core/Security.php b/system/core/Security.php
index f7998da60..1007f61f4 100755
--- a/system/core/Security.php
+++ b/system/core/Security.php
@@ -1,13 +1,13 @@
-<?php  if ( ! defined('BASEPATH')) exit('No direct script access allowed');
+<?php if ( ! defined('BASEPATH')) exit('No direct script access allowed');
 /**
  * CodeIgniter
  *
  * An open source application development framework for PHP 5.1.6 or newer
  *
  * NOTICE OF LICENSE
- * 
+ *
  * Licensed under the Open Software License version 3.0
- * 
+ *
  * This source file is subject to the Open Software License (OSL 3.0) that is
  * bundled with this package in the files license.txt / license.rst.  It is
  * also available through the world wide web at this URL:
@@ -42,7 +42,6 @@ class CI_Security {
 	 * Random Hash for protecting URLs
 	 *
 	 * @var string
-	 * @access protected
 	 */
 	protected $_xss_hash			= '';
 
@@ -50,7 +49,6 @@ class CI_Security {
 	 * Random Hash for Cross Site Request Forgery Protection Cookie
 	 *
 	 * @var string
-	 * @access protected
 	 */
 	protected $_csrf_hash			= '';
 
@@ -59,7 +57,6 @@ class CI_Security {
 	 * Defaults to two hours (in seconds)
 	 *
 	 * @var int
-	 * @access protected
 	 */
 	protected $_csrf_expire			= 7200;
 
@@ -67,7 +64,6 @@ class CI_Security {
 	 * Token name for Cross Site Request Forgery Protection Cookie
 	 *
 	 * @var string
-	 * @access protected
 	 */
 	protected $_csrf_token_name		= 'ci_csrf_token';
 
@@ -75,46 +71,39 @@ class CI_Security {
 	 * Cookie name for Cross Site Request Forgery Protection Cookie
 	 *
 	 * @var string
-	 * @access protected
 	 */
-	protected $_csrf_cookie_name	= 'ci_csrf_token';
+	protected $_csrf_cookie_name		= 'ci_csrf_token';
 
 	/**
 	 * List of never allowed strings
 	 *
 	 * @var array
-	 * @access protected
 	 */
-
 	protected $_never_allowed_str = array(
-					'document.cookie'	=> '[removed]',
-					'document.write'	=> '[removed]',
-					'.parentNode'		=> '[removed]',
-					'.innerHTML'		=> '[removed]',
-					'window.location'	=> '[removed]',
-					'-moz-binding'		=> '[removed]',
-					'<!--'				=> '&lt;!--',
-					'-->'				=> '--&gt;',
-					'<![CDATA['			=> '&lt;![CDATA[',
-					'<comment>'			=> '&lt;comment&gt;'
-	);
+						'document.cookie'	=> '[removed]',
+						'document.write'	=> '[removed]',
+						'.parentNode'		=> '[removed]',
+						'.innerHTML'		=> '[removed]',
+						'window.location'	=> '[removed]',
+						'-moz-binding'		=> '[removed]',
+						'<!--'				=> '&lt;!--',
+						'-->'				=> '--&gt;',
+						'<![CDATA['			=> '&lt;![CDATA[',
+						'<comment>'			=> '&lt;comment&gt;'
+					);
 
 	/**
 	 * List of never allowed regex replacement
 	 *
 	 * @var array
-	 * @access protected
 	 */
 	protected $_never_allowed_regex = array(
-					"javascript\s*:"			=> '[removed]',
-					"expression\s*(\(|&\#40;)"	=> '[removed]', // CSS and IE
-					"vbscript\s*:"				=> '[removed]', // IE, surprise!
-					"Redirect\s+302"			=> '[removed]'
-	);
+						'javascript\s*:',
+						'expression\s*(\(|&\#40;)', // CSS and IE
+						'vbscript\s*:', // IE, surprise!
+						'Redirect\s+302'
+					);
 
-	/**
-	 * Constructor
-	 */
 	public function __construct()
 	{
 		// CSRF config
@@ -135,7 +124,7 @@ class CI_Security {
 		// Set the CSRF hash
 		$this->_csrf_set_hash();
 
-		log_message('debug', "Security Class Initialized");
+		log_message('debug', 'Security Class Initialized');
 	}
 
 	// --------------------------------------------------------------------
@@ -148,7 +137,7 @@ class CI_Security {
 	public function csrf_verify()
 	{
 		// If no POST data exists we will set the CSRF cookie
-		if (count($_POST) == 0)
+		if (count($_POST) === 0)
 		{
 			return $this->csrf_set_cookie();
 		}
@@ -164,20 +153,13 @@ class CI_Security {
 		}
 
 		// Do the tokens exist in both the _POST and _COOKIE arrays?
-		if ( ! isset($_POST[$this->_csrf_token_name]) OR
-			 ! isset($_COOKIE[$this->_csrf_cookie_name]))
+		if ( ! isset($_POST[$this->_csrf_token_name]) OR ! isset($_COOKIE[$this->_csrf_cookie_name])
+			OR $_POST[$this->_csrf_token_name] != $_COOKIE[$this->_csrf_cookie_name]) // Do the tokens match?
 		{
 			$this->csrf_show_error();
 		}
 
-		// Do the tokens match?
-		if ($_POST[$this->_csrf_token_name] != $_COOKIE[$this->_csrf_cookie_name])
-		{
-			$this->csrf_show_error();
-		}
-
-		// We kill this since we're done and we don't want to
-		// polute the _POST array
+		// We kill this since we're done and we don't want to polute the _POST array
 		unset($_POST[$this->_csrf_token_name]);
 
 		// Regenerate on every submission?
@@ -187,12 +169,11 @@ class CI_Security {
 			unset($_COOKIE[$this->_csrf_cookie_name]);
 			$this->_csrf_hash = '';
 		}
-		
+
 		$this->_csrf_set_hash();
 		$this->csrf_set_cookie();
 
-		log_message('debug', "CSRF token verified");
-
+		log_message('debug', 'CSRF token verified');
 		return $this;
 	}
 
@@ -208,19 +189,13 @@ class CI_Security {
 		$expire = time() + $this->_csrf_expire;
 		$secure_cookie = (bool) config_item('cookie_secure');
 
-		if ($secure_cookie)
+		if ($secure_cookie && ( ! isset($_SERVER['HTTPS']) OR $_SERVER['HTTPS'] == 'off' OR ! $_SERVER['HTTPS']))
 		{
-			$req = isset($_SERVER['HTTPS']) ? $_SERVER['HTTPS'] : FALSE;
-
-			if ( ! $req OR $req == 'off')
-			{
-				return FALSE;
-			}
+			return FALSE;
 		}
 
 		setcookie($this->_csrf_cookie_name, $this->_csrf_hash, $expire, config_item('cookie_path'), config_item('cookie_domain'), $secure_cookie);
-
-		log_message('debug', "CRSF cookie Set");
+		log_message('debug', 'CRSF cookie Set');
 
 		return $this;
 	}
@@ -258,7 +233,7 @@ class CI_Security {
 	 *
 	 * Getter Method
 	 *
-	 * @return 	string 	self::csrf_token_name
+	 * @return 	string 	self::_csrf_token_name
 	 */
 	public function get_csrf_token_name()
 	{
@@ -278,7 +253,7 @@ class CI_Security {
 	 * the filter.
 	 *
 	 * Note: This function should only be used to deal with data
-	 * upon submission.  It's not something that should
+	 * upon submission. It's not something that should
 	 * be used for general runtime processing.
 	 *
 	 * This function was based in part on some code and ideas I
@@ -295,10 +270,7 @@ class CI_Security {
 	 */
 	public function xss_clean($str, $is_image = FALSE)
 	{
-		/*
-		 * Is the string an array?
-		 *
-		 */
+		// Is the string an array?
 		if (is_array($str))
 		{
 			while (list($key) = each($str))
@@ -309,13 +281,8 @@ class CI_Security {
 			return $str;
 		}
 
-		/*
-		 * Remove Invisible Characters
-		 */
-		$str = remove_invisible_characters($str);
-
-		// Validate Entities in URLs
-		$str = $this->_validate_entities($str);
+		// Remove Invisible Characters and validate entities in URLs
+		$str = $this->_validate_entities(remove_invisible_characters($str));
 
 		/*
 		 * URL Decode
@@ -325,7 +292,6 @@ class CI_Security {
 		 * <a href="http://%77%77%77%2E%67%6F%6F%67%6C%65%2E%63%6F%6D">Google</a>
 		 *
 		 * Note: Use rawurldecode() so it does not remove plus signs
-		 *
 		 */
 		$str = rawurldecode($str);
 
@@ -335,16 +301,11 @@ class CI_Security {
 		 * This permits our tests below to work reliably.
 		 * We only convert entities that are within tags since
 		 * these are the ones that will pose security problems.
-		 *
 		 */
-
 		$str = preg_replace_callback("/[a-z]+=([\'\"]).*?\\1/si", array($this, '_convert_attribute'), $str);
+		$str = preg_replace_callback('/<\w+.*?(?=>|<|$)/si', array($this, '_decode_entity'), $str);
 
-		$str = preg_replace_callback("/<\w+.*?(?=>|<|$)/si", array($this, '_decode_entity'), $str);
-
-		/*
-		 * Remove Invisible Characters Again!
-		 */
+		// Remove Invisible Characters Again!
 		$str = remove_invisible_characters($str);
 
 		/*
@@ -355,15 +316,9 @@ class CI_Security {
 		 * NOTE: preg_replace was found to be amazingly slow here on
 		 * large blocks of data, so we use str_replace.
 		 */
+		$str = str_replace("\t", ' ', $str);
 
-		if (strpos($str, "\t") !== FALSE)
-		{
-			$str = str_replace("\t", ' ', $str);
-		}
-
-		/*
-		 * Capture converted string for later comparison
-		 */
+		// Capture converted string for later comparison
 		$converted_string = $str;
 
 		// Remove Strings that are never allowed
@@ -383,7 +338,7 @@ class CI_Security {
 			// Images have a tendency to have the PHP short opening and
 			// closing tags every so often so we skip those and only
 			// do the long opening tags.
-			$str = preg_replace('/<\?(php)/i', "&lt;?\\1", $str);
+			$str = preg_replace('/<\?(php)/i', '&lt;?\\1', $str);
 		}
 		else
 		{
@@ -420,19 +375,19 @@ class CI_Security {
 		{
 			$original = $str;
 
-			if (preg_match("/<a/i", $str))
+			if (preg_match('/<a/i', $str))
 			{
-				$str = preg_replace_callback("#<a\s+([^>]*?)(>|$)#si", array($this, '_js_link_removal'), $str);
+				$str = preg_replace_callback('#<a\s+([^>]*?)(>|$)#si', array($this, '_js_link_removal'), $str);
 			}
 
-			if (preg_match("/<img/i", $str))
+			if (preg_match('/<img/i', $str))
 			{
-				$str = preg_replace_callback("#<img\s+([^>]*?)(\s?/?>|$)#si", array($this, '_js_img_removal'), $str);
+				$str = preg_replace_callback('#<img\s+([^>]*?)(\s?/?>|$)#si', array($this, '_js_img_removal'), $str);
 			}
 
-			if (preg_match("/script/i", $str) OR preg_match("/xss/i", $str))
+			if (preg_match('/(script|xss)/i', $str))
 			{
-				$str = preg_replace("#<(/*)(script|xss)(.*?)\>#si", '[removed]', $str);
+				$str = preg_replace('#<(/*)(script|xss)(.*?)\>#si', '[removed]', $str);
 			}
 		}
 		while($original != $str);
@@ -459,14 +414,16 @@ class CI_Security {
 		 *
 		 * Similar to above, only instead of looking for
 		 * tags it looks for PHP and JavaScript commands
-		 * that are disallowed.  Rather than removing the
+		 * that are disallowed. Rather than removing the
 		 * code, it simply converts the parenthesis to entities
 		 * rendering the code un-executable.
 		 *
 		 * For example:	eval('some code')
-		 * Becomes:		eval&#40;'some code'&#41;
+		 * Becomes:	eval&#40;'some code'&#41;
 		 */
-		$str = preg_replace('#(alert|cmd|passthru|eval|exec|expression|system|fopen|fsockopen|file|file_get_contents|readfile|unlink)(\s*)\((.*?)\)#si', "\\1\\2&#40;\\3&#41;", $str);
+		$str = preg_replace('#(alert|cmd|passthru|eval|exec|expression|system|fopen|fsockopen|file|file_get_contents|readfile|unlink)(\s*)\((.*?)\)#si',
+					'\\1\\2&#40;\\3&#41;',
+					$str);
 
 
 		// Final clean up
@@ -483,13 +440,12 @@ class CI_Security {
 		 * string post-removal of XSS, then it fails, as there was unwanted XSS
 		 * code found and removed/changed during processing.
 		 */
-
 		if ($is_image === TRUE)
 		{
-			return ($str === $converted_string) ? TRUE : FALSE;
+			return ($str === $converted_string);
 		}
 
-		log_message('debug', "XSS Filtering completed");
+		log_message('debug', 'XSS Filtering completed');
 		return $str;
 	}
 
@@ -521,7 +477,7 @@ class CI_Security {
 	 * The reason we are not using html_entity_decode() by itself is because
 	 * while it is not technically correct to leave out the semicolon
 	 * at the end of an entity most browsers will still interpret the entity
-	 * correctly.  html_entity_decode() does not convert entities without
+	 * correctly. html_entity_decode() does not convert entities without
 	 * semicolons, so we are left with our own little solution here. Bummer.
 	 *
 	 * @param	string
@@ -557,38 +513,23 @@ class CI_Security {
 	public function sanitize_filename($str, $relative_path = FALSE)
 	{
 		$bad = array(
-						"../",
-						"<!--",
-						"-->",
-						"<",
-						">",
-						"'",
-						'"',
-						'&',
-						'$',
-						'#',
-						'{',
-						'}',
-						'[',
-						']',
-						'=',
-						';',
-						'?',
-						"%20",
-						"%22",
-						"%3c",		// <
-						"%253c",	// <
-						"%3e",		// >
-						"%0e",		// >
-						"%28",		// (
-						"%29",		// )
-						"%2528",	// (
-						"%26",		// &
-						"%24",		// $
-						"%3f",		// ?
-						"%3b",		// ;
-						"%3d"		// =
-					);
+				'../', '<!--', '-->', '<', '>',
+				"'", '"', '&', '$', '#',
+				'{', '}', '[', ']', '=',
+				';', '?', '%20', '%22',
+				'%3c',		// <
+				'%253c',	// <
+				'%3e',		// >
+				'%0e',		// >
+				'%28',		// (
+				'%29',		// )
+				'%2528',	// (
+				'%26',		// &
+				'%24',		// $
+				'%3f',		// ?
+				'%3b',		// ;
+				'%3d'		// =
+			);
 
 		if ( ! $relative_path)
 		{
@@ -641,26 +582,26 @@ class CI_Security {
 		if ($is_image === TRUE)
 		{
 			/*
-			 * Adobe Photoshop puts XML metadata into JFIF images, 
+			 * Adobe Photoshop puts XML metadata into JFIF images,
 			 * including namespacing, so we have to allow this for images.
 			 */
 			unset($evil_attributes[array_search('xmlns', $evil_attributes)]);
 		}
-		
+
 		do {
 			$count = 0;
 			$attribs = array();
-			
+
 			// find occurrences of illegal attribute strings without quotes
-			preg_match_all("/(".implode('|', $evil_attributes).")\s*=\s*([^\s]*)/is",  $str, $matches, PREG_SET_ORDER);
-			
+			preg_match_all('/('.implode('|', $evil_attributes).')\s*=\s*([^\s]*)/is', $str, $matches, PREG_SET_ORDER);
+
 			foreach ($matches as $attr)
 			{
 				$attribs[] = preg_quote($attr[0], '/');
 			}
-			
+
 			// find occurrences of illegal attribute strings with quotes (042 and 047 are octal quotes)
-			preg_match_all("/(".implode('|', $evil_attributes).")\s*=\s*(\042|\047)([^\\2]*?)(\\2)/is",  $str, $matches, PREG_SET_ORDER);
+			preg_match_all('/('.implode('|', $evil_attributes).')\s*=\s*(\042|\047)([^\\2]*?)(\\2)/is',  $str, $matches, PREG_SET_ORDER);
 
 			foreach ($matches as $attr)
 			{
@@ -670,11 +611,11 @@ class CI_Security {
 			// replace illegal attribute strings that are inside an html tag
 			if (count($attribs) > 0)
 			{
-				$str = preg_replace("/<(\/?[^><]+?)([^A-Za-z\-])(".implode('|', $attribs).")([\s><])([><]*)/i", '<$1$2$4$5', $str, -1, $count);
+				$str = preg_replace('/<(\/?[^><]+?)([^A-Za-z\-])('.implode('|', $attribs).')([\s><])([><]*)/i', '<$1$2$4$5', $str, -1, $count);
 			}
-			
+
 		} while ($count);
-		
+
 		return $str;
 	}
 
@@ -690,14 +631,9 @@ class CI_Security {
 	 */
 	protected function _sanitize_naughty_html($matches)
 	{
-		// encode opening brace
-		$str = '&lt;'.$matches[1].$matches[2].$matches[3];
-
-		// encode captured opening or closing brace to prevent recursive vectors
-		$str .= str_replace(array('>', '<'), array('&gt;', '&lt;'),
-							$matches[4]);
-
-		return $str;
+		return '&lt;'.$matches[1].$matches[2].$matches[3] // encode opening brace
+			// encode captured opening or closing brace to prevent recursive vectors:
+			. str_replace(array('>', '<'), array('&gt;', '&lt;'), $matches[4]);
 	}
 
 	// --------------------------------------------------------------------
@@ -715,9 +651,12 @@ class CI_Security {
 	 */
 	protected function _js_link_removal($match)
 	{
-		$attributes = $this->_filter_attributes(str_replace(array('<', '>'), '', $match[1]));
-
-		return str_replace($match[1], preg_replace("#href=.*?(alert\(|alert&\#40;|javascript\:|livescript\:|mocha\:|charset\=|window\.|document\.|\.cookie|<script|<xss|base64\s*,)#si", "", $attributes), $match[0]);
+		return str_replace($match[1],
+					preg_replace('#href=.*?(alert\(|alert&\#40;|javascript\:|livescript\:|mocha\:|charset\=|window\.|document\.|\.cookie|<script|<xss|base64\s*,)#si',
+							'',
+							$this->_filter_attributes(str_replace(array('<', '>'), '', $match[1]))
+					),
+					$match[0]);
 	}
 
 	// --------------------------------------------------------------------
@@ -735,9 +674,12 @@ class CI_Security {
 	 */
 	protected function _js_img_removal($match)
 	{
-		$attributes = $this->_filter_attributes(str_replace(array('<', '>'), '', $match[1]));
-
-		return str_replace($match[1], preg_replace("#src=.*?(alert\(|alert&\#40;|javascript\:|livescript\:|mocha\:|charset\=|window\.|document\.|\.cookie|<script|<xss|base64\s*,)#si", "", $attributes), $match[0]);
+		return str_replace($match[1],
+					preg_replace('#src=.*?(alert\(|alert&\#40;|javascript\:|livescript\:|mocha\:|charset\=|window\.|document\.|\.cookie|<script|<xss|base64\s*,)#si',
+							'',
+							$this->_filter_attributes(str_replace(array('<', '>'), '', $match[1]))
+					),
+					$match[0]);
 	}
 
 	// --------------------------------------------------------------------
@@ -768,12 +710,11 @@ class CI_Security {
 	protected function _filter_attributes($str)
 	{
 		$out = '';
-
 		if (preg_match_all('#\s*[a-z\-]+\s*=\s*(\042|\047)([^\\1]*?)\\1#is', $str, $matches))
 		{
 			foreach ($matches[0] as $match)
 			{
-				$out .= preg_replace("#/\*.*?\*/#s", '', $match);
+				$out .= preg_replace('#/\*.*?\*/#s', '', $match);
 			}
 		}
 
@@ -811,33 +752,28 @@ class CI_Security {
 		 * Protect GET variables in URLs
 		 */
 
-		 // 901119URL5918AMP18930PROTECT8198
-
-		$str = preg_replace('|\&([a-z\_0-9\-]+)\=([a-z\_0-9\-]+)|i', $this->xss_hash()."\\1=\\2", $str);
+		// 901119URL5918AMP18930PROTECT8198
+		$str = preg_replace('|\&([a-z\_0-9\-]+)\=([a-z\_0-9\-]+)|i', $this->xss_hash().'\\1=\\2', $str);
 
 		/*
 		 * Validate standard character entities
 		 *
 		 * Add a semicolon if missing.  We do this to enable
 		 * the conversion of entities to ASCII later.
-		 *
 		 */
-		$str = preg_replace('#(&\#?[0-9a-z]{2,})([\x00-\x20])*;?#i', "\\1;\\2", $str);
+		$str = preg_replace('#(&\#?[0-9a-z]{2,})([\x00-\x20])*;?#i', '\\1;\\2', $str);
 
 		/*
 		 * Validate UTF16 two byte encoding (x00)
 		 *
 		 * Just as above, adds a semicolon if missing.
-		 *
 		 */
-		$str = preg_replace('#(&\#x?)([0-9A-F]+);?#i',"\\1\\2;",$str);
+		$str = preg_replace('#(&\#x?)([0-9A-F]+);?#i', '\\1\\2;', $str);
 
 		/*
 		 * Un-Protect GET variables in URLs
 		 */
-		$str = str_replace($this->xss_hash(), '&', $str);
-
-		return $str;
+		return str_replace($this->xss_hash(), '&', $str);
 	}
 
 	// ----------------------------------------------------------------------
@@ -852,14 +788,11 @@ class CI_Security {
 	 */
 	protected function _do_never_allowed($str)
 	{
-		foreach ($this->_never_allowed_str as $key => $val)
-		{
-			$str = str_replace($key, $val, $str);
-		}
+		$str = str_replace(array_keys($this->_never_allowed_str), $this->_never_allowed_str, $str);
 
-		foreach ($this->_never_allowed_regex as $key => $val)
+		foreach ($this->_never_allowed_regex as $regex)
 		{
-			$str = preg_replace("#".$key."#i", $val, $str);
+			$str = preg_replace('#'.$regex.'#i', '[removed]', $str);
 		}
 
 		return $str;
@@ -896,4 +829,4 @@ class CI_Security {
 }
 
 /* End of file Security.php */
-/* Location: ./system/core/Security.php */
\ No newline at end of file
+/* Location: ./system/core/Security.php */