diff options
| -rw-r--r-- | system/core/Loader.php | 58 | ||||
| -rw-r--r-- | system/core/Security.php | 16 | ||||
| -rw-r--r-- | system/helpers/form_helper.php | 41 | ||||
| -rw-r--r-- | tests/codeigniter/core/Security_test.php | 5 | ||||
| -rw-r--r-- | user_guide_src/source/changelog.rst | 8 |
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(1)">', $this->security->xss_clean('<img src="b on="<x">on=">"x onerror="alert(1)">') ); + + $this->assertEquals( + "\n><!-\n<b d=\"'e><iframe onload=alert(1) 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']``. |