From 2ab1c1902711c8b0caf5c3e8f2fa825d72f6755d Mon Sep 17 00:00:00 2001 From: Andrey Andreev Date: Wed, 4 Jan 2017 15:26:35 +0200 Subject: Fix an XSS vulnerability --- system/core/Security.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'system') diff --git a/system/core/Security.php b/system/core/Security.php index 8b313a9a2..d198b663b 100644 --- a/system/core/Security.php +++ b/system/core/Security.php @@ -499,7 +499,7 @@ class CI_Security { * Becomes: <blink> */ $pattern = '#' - .'<((?/*\s*)(?[a-z0-9]+)(?=[^a-z0-9]|$)' // tag start and name, followed by a non-tag character + .'<((?/*\s*)((?[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 .'(?(?:[\s\042\047/=]*' // non-attribute characters, excluding > (tag close) for obvious reasons -- cgit v1.2.3-24-g4f1b From 5a2390d4d6287f2ce35cadae4713b7dcd10fdc9b Mon Sep 17 00:00:00 2001 From: Andrey Andreev Date: Wed, 4 Jan 2017 16:01:27 +0200 Subject: [ci skip] Protect CSRF verification from timing side-channel attacks --- system/core/Security.php | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) (limited to 'system') diff --git a/system/core/Security.php b/system/core/Security.php index d198b663b..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; } -- cgit v1.2.3-24-g4f1b From cfd52edad6a4ae84b0c34755455b5b7b164878be Mon Sep 17 00:00:00 2001 From: Andrey Andreev Date: Wed, 4 Jan 2017 16:58:08 +0200 Subject: [ci skip] Try to mitigate BREACH attacks against CSRF tokens --- system/helpers/form_helper.php | 41 +++++++++++++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 6 deletions(-) (limited to 'system') 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 = '
\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%s%s', + $prepend, + $CI->security->get_csrf_token_name(), + $CI->security->get_csrf_hash(), + $append, + "\n" + ); + } + return $form; } } -- cgit v1.2.3-24-g4f1b From ec8dbbb79bb083acd1cf6beff5abea055b583db5 Mon Sep 17 00:00:00 2001 From: Andrey Andreev Date: Wed, 4 Jan 2017 17:01:44 +0200 Subject: Fix a possible file inclusion vulnerability in CI_Loader::vars() --- system/core/Loader.php | 58 ++++++++++++++++++++++++-------------------------- 1 file changed, 28 insertions(+), 30 deletions(-) (limited to 'system') 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; } // -------------------------------------------------------------------- -- cgit v1.2.3-24-g4f1b From d9367b6acf96cbc407147bd4422c2cbb941ccaa1 Mon Sep 17 00:00:00 2001 From: Andrey Andreev Date: Thu, 5 Jan 2017 17:35:28 +0200 Subject: Address #4963 Would supersede PR #4966 --- system/libraries/Email.php | 54 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 47 insertions(+), 7 deletions(-) (limited to 'system') diff --git a/system/libraries/Email.php b/system/libraries/Email.php index 4ab873586..e25015450 100644 --- a/system/libraries/Email.php +++ b/system/libraries/Email.php @@ -1842,6 +1842,33 @@ class CI_Email { // -------------------------------------------------------------------- + /** + * Validate email for shell + * + * Applies stricter, shell-safe validation to email addresses. + * Introduced to prevent RCE via sendmail's -f option. + * + * @see https://github.com/bcit-ci/CodeIgniter/issues/4963 + * @see https://gist.github.com/Zenexer/40d02da5e07f151adeaeeaa11af9ab36 + * @license https://creativecommons.org/publicdomain/zero/1.0/ CC0 1.0, Public Domain + * + * Credits for the base concept go to Paul Buonopane + * + * @param string $email + * @return bool + */ + protected function _validate_email_for_shell(&$email) + { + if (function_exists('idn_to_ascii') && $atpos = strpos($email, '@')) + { + $email = self::substr($email, 0, ++$atpos).idn_to_ascii(self::substr($email, $atpos)); + } + + return (filter_var($email, FILTER_VALIDATE_EMAIL) === $email && preg_match('#\A[a-z0-9._+-]+@[a-z0-9.-]{1,253}\z#i', $email)); + } + + // -------------------------------------------------------------------- + /** * Send using mail() * @@ -1854,7 +1881,11 @@ class CI_Email { $this->_recipients = implode(', ', $this->_recipients); } - if ($this->_safe_mode === TRUE) + // _validate_email_for_shell() below accepts by reference, + // so this needs to be assigned to a variable + $from = $this->clean_email($this->_headers['Return-Path']); + + if ($this->_safe_mode === TRUE || ! $this->_validate_email_for_shell($from)) { return mail($this->_recipients, $this->_subject, $this->_finalbody, $this->_header_str); } @@ -1862,7 +1893,7 @@ class CI_Email { { // most documentation of sendmail using the "-f" flag lacks a space after it, however // we've encountered servers that seem to require it to be in place. - return mail($this->_recipients, $this->_subject, $this->_finalbody, $this->_header_str, '-f '.$this->clean_email($this->_headers['Return-Path'])); + return mail($this->_recipients, $this->_subject, $this->_finalbody, $this->_header_str, '-f '.$from); } } @@ -1875,13 +1906,22 @@ class CI_Email { */ protected function _send_with_sendmail() { + // _validate_email_for_shell() below accepts by reference, + // so this needs to be assigned to a variable + $from = $this->clean_email($this->_headers['From']); + if ($this->_validate_email_for_shell($from)) + { + $from = '-f '.$from; + } + else + { + $from = ''; + } + // is popen() enabled? - if ( ! function_usable('popen') - OR FALSE === ($fp = @popen( - $this->mailpath.' -oi -f '.escapeshellarg($this->clean_email($this->_headers['From'])).' -t' - , 'w')) - ) // server probably has popen disabled, so nothing we can do to get a verbose error. + if ( ! function_usable('popen') OR FALSE === ($fp = @popen($this->mailpath.' -oi '.$from.' -t', 'w'))) { + // server probably has popen disabled, so nothing we can do to get a verbose error. return FALSE; } -- cgit v1.2.3-24-g4f1b