Merge branch 'security' into 3.1-stable

5 files changed, 85 insertions, 43 deletions

diff --git a/system/core/Loader.php b/system/core/Loader.php
index 0515723b4..17ff2362c 100644
--- a/system/core/Loader.php
+++ b/system/core/Loader.php
@@ -486,7 +486,7 @@ class CI_Loader {
 	 */
 	public function view($view, $vars = array(), $return = FALSE)
 	{
-		return $this->_ci_load(array('_ci_view' => $view, '_ci_vars' => $this->_ci_object_to_array($vars), '_ci_return' => $return));
+		return $this->_ci_load(array('_ci_view' => $view, '_ci_vars' => $this->_ci_prepare_view_vars($vars), '_ci_return' => $return));
 	}
 
 	// --------------------------------------------------------------------
@@ -519,19 +519,13 @@ class CI_Loader {
 	 */
 	public function vars($vars, $val = '')
 	{
-		if (is_string($vars))
-		{
-			$vars = array($vars => $val);
-		}
-
-		$vars = $this->_ci_object_to_array($vars);
+		$vars = is_string($vars)
+			? array($vars => $val)
+			: $this->_ci_prepare_view_vars($vars);
 
-		if (is_array($vars) && count($vars) > 0)
+		foreach ($vars as $key => $val)
 		{
-			foreach ($vars as $key => $val)
-			{
-				$this->_ci_cached_vars[$key] = $val;
-			}
+			$this->_ci_cached_vars[$key] = $val;
 		}
 
 		return $this;
@@ -940,18 +934,7 @@ class CI_Loader {
 		 * the two types and cache them so that views that are embedded within
 		 * other views can have access to these variables.
 		 */
-		if (is_array($_ci_vars))
-		{
-			foreach (array_keys($_ci_vars) as $key)
-			{
-				if (strncmp($key, '_ci_', 4) === 0)
-				{
-					unset($_ci_vars[$key]);
-				}
-			}
-
-			$this->_ci_cached_vars = array_merge($this->_ci_cached_vars, $_ci_vars);
-		}
+		empty($_ci_vars) OR $this->_ci_cached_vars = array_merge($this->_ci_cached_vars, $_ci_vars);
 		extract($this->_ci_cached_vars);
 
 		/*
@@ -1382,17 +1365,32 @@ class CI_Loader {
 	// --------------------------------------------------------------------
 
 	/**
-	 * CI Object to Array translator
+	 * Prepare variables for _ci_vars, to be later extract()-ed inside views
 	 *
-	 * Takes an object as input and converts the class variables to
-	 * an associative array with key/value pairs.
+	 * Converts objects to associative arrays and filters-out internal
+	 * variable names (i.e. keys prexied with '_ci_').
 	 *
-	 * @param	object	$object	Object data to translate
+	 * @param	mixed	$vars
 	 * @return	array
 	 */
-	protected function _ci_object_to_array($object)
+	protected function _ci_prepare_view_vars($vars)
 	{
-		return is_object($object) ? get_object_vars($object) : $object;
+		if ( ! is_array($vars))
+		{
+			$vars = is_object($vars)
+				? get_object_vars($object)
+				: array();
+		}
+
+		foreach (array_keys($vars) as $key)
+		{
+			if (strncmp($key, '_ci_', 4) === 0)
+			{
+				unset($vars[$key]);
+			}
+		}
+
+		return $vars;
 	}
 
 	// --------------------------------------------------------------------
diff --git a/system/core/Security.php b/system/core/Security.php
index 8b313a9a2..585ed90ec 100644
--- a/system/core/Security.php
+++ b/system/core/Security.php
@@ -224,12 +224,9 @@ class CI_Security {
 			}
 		}
 
-		// Do the tokens exist in both the _POST and _COOKIE arrays?
-		if ( ! isset($_POST[$this->_csrf_token_name], $_COOKIE[$this->_csrf_cookie_name])
-			OR $_POST[$this->_csrf_token_name] !== $_COOKIE[$this->_csrf_cookie_name]) // Do the tokens match?
-		{
-			$this->csrf_show_error();
-		}
+		// Check CSRF token validity, but don't error on mismatch just yet - we'll want to regenerate
+		$valid = isset($_POST[$this->_csrf_token_name], $_COOKIE[$this->_csrf_cookie_name])
+			&& hash_equals($_POST[$this->_csrf_token_name], $_COOKIE[$this->_csrf_cookie_name]);
 
 		// We kill this since we're done and we don't want to pollute the _POST array
 		unset($_POST[$this->_csrf_token_name]);
@@ -245,6 +242,11 @@ class CI_Security {
 		$this->_csrf_set_hash();
 		$this->csrf_set_cookie();
 
+		if ($valid !== TRUE)
+		{
+			$this->csrf_show_error();
+		}
+
 		log_message('info', 'CSRF token verified');
 		return $this;
 	}
@@ -499,7 +501,7 @@ class CI_Security {
 		 * Becomes: <blink>
 		 */
 		$pattern = '#'
-			.'<((?<slash>/*\s*)(?<tagName>[a-z0-9]+)(?=[^a-z0-9]|$)' // tag start and name, followed by a non-tag character
+			.'<((?<slash>/*\s*)((?<tagName>[a-z0-9]+)(?=[^a-z0-9]|$)|.+)' // tag start and name, followed by a non-tag character
 			.'[^\s\042\047a-z0-9>/=]*' // a valid attribute character immediately after the tag would count as a separator
 			// optional attributes
 			.'(?<attributes>(?:[\s\042\047/=]*' // non-attribute characters, excluding > (tag close) for obvious reasons
diff --git a/system/helpers/form_helper.php b/system/helpers/form_helper.php
index fc7d2a6a0..a49eea803 100644
--- a/system/helpers/form_helper.php
+++ b/system/helpers/form_helper.php
@@ -90,12 +90,6 @@ if ( ! function_exists('form_open'))
 
 		$form = '<form action="'.$action.'"'.$attributes.">\n";
 
-		// Add CSRF field if enabled, but leave it out for GET requests and requests to external websites
-		if ($CI->config->item('csrf_protection') === TRUE && strpos($action, $CI->config->base_url()) !== FALSE && ! stripos($form, 'method="get"'))
-		{
-			$hidden[$CI->security->get_csrf_token_name()] = $CI->security->get_csrf_hash();
-		}
-
 		if (is_array($hidden))
 		{
 			foreach ($hidden as $name => $value)
@@ -104,6 +98,41 @@ if ( ! function_exists('form_open'))
 			}
 		}
 
+		// Add CSRF field if enabled, but leave it out for GET requests and requests to external websites
+		if ($CI->config->item('csrf_protection') === TRUE && strpos($action, $CI->config->base_url()) !== FALSE && ! stripos($form, 'method="get"'))
+		{
+			// Prepend/append random-length "white noise" around the CSRF
+			// token input, as a form of protection against BREACH attacks
+			if (FALSE !== ($noise = $CI->security->get_random_bytes(1)))
+			{
+				list(, $noise) = unpack('c', $noise);
+			}
+			else
+			{
+				$noise = mt_rand(-128, 127);
+			}
+
+			// Prepend if $noise has a negative value, append if positive, do nothing for zero
+			$prepend = $append = '';
+			if ($noise < 0)
+			{
+				$prepend = str_repeat(" ", abs($noise));
+			}
+			elseif ($noise > 0)
+			{
+				$append  = str_repeat(" ", $noise);
+			}
+
+			$form .= sprintf(
+				'%s<input type="hidden" name="%s" value="%s" />%s%s',
+				$prepend,
+				$CI->security->get_csrf_token_name(),
+				$CI->security->get_csrf_hash(),
+				$append,
+				"\n"
+			);
+		}
+
 		return $form;
 	}
 }
diff --git a/tests/codeigniter/core/Security_test.php b/tests/codeigniter/core/Security_test.php
index cbf0285ec..4c54ec9fa 100644
--- a/tests/codeigniter/core/Security_test.php
+++ b/tests/codeigniter/core/Security_test.php
@@ -154,6 +154,11 @@ class Security_test extends CI_TestCase {
 			'<img src="b on=">on=">"x onerror="alert&#40;1&#41;">',
 			$this->security->xss_clean('<img src="b on="<x">on=">"x onerror="alert(1)">')
 		);
+
+		$this->assertEquals(
+			"\n>&lt;!-\n<b d=\"'e><iframe onload=alert&#40;1&#41; src=x>\n<a HREF=\">\n",
+			$this->security->xss_clean("\n><!-\n<b\n<c d=\"'e><iframe onload=alert(1) src=x>\n<a HREF=\"\">\n")
+		);
 	}
 
 	// --------------------------------------------------------------------
diff --git a/user_guide_src/source/changelog.rst b/user_guide_src/source/changelog.rst
index 91a59c4cf..ca1696c42 100644
--- a/user_guide_src/source/changelog.rst
+++ b/user_guide_src/source/changelog.rst
@@ -7,6 +7,14 @@ Version 3.1.3
 
 Release Date: Not Released
 
+-  **Security**
+
+   -  Fixed an XSS vulnerability in :doc:`Security Library <libraries/security>` method ``xss_clean()``.
+   -  Fixed a possible file inclusion vulnerability in :doc:`Loader Library <libraries/loader>` method ``vars()``.
+   -  Fixed a possible remote code execution vulnerability in the :doc:`Email Library <libraries/email>` when 'mail' or 'sendmail' are used (thanks to Paul Buonopane from `NamePros <https://www.namepros.com/>`_).
+   -  Added protection against timing side-channel attacks in :doc:`Security Library <libraries/security>` method ``csrf_verify()``.
+   -  Added protection against BREACH attacks targeting the CSRF token field generated by :doc:`Form Helper <helpers/form_helper>` function :php:func:`form_open()`.
+
 -  General Changes
 
    -  Deprecated ``$config['allow_get_array']``.