diff options
| -rw-r--r-- | application/config/config.php | 9 | ||||
| -rw-r--r-- | application/config/user_agents.php | 1 | ||||
| -rw-r--r-- | system/core/Log.php | 14 | ||||
| -rw-r--r-- | system/core/Output.php | 44 | ||||
| -rw-r--r-- | system/core/Router.php | 6 | ||||
| -rw-r--r-- | system/core/URI.php | 35 | ||||
| -rw-r--r-- | system/core/Utf8.php | 2 | ||||
| -rw-r--r-- | system/helpers/file_helper.php | 12 | ||||
| -rw-r--r-- | system/libraries/Email.php | 11 | ||||
| -rw-r--r-- | system/libraries/Xmlrpc.php | 10 | ||||
| -rw-r--r-- | system/libraries/Zip.php | 12 | ||||
| -rw-r--r-- | tests/codeigniter/core/Input_test.php | 47 | ||||
| -rw-r--r-- | tests/codeigniter/core/URI_test.php | 15 | ||||
| -rw-r--r-- | tests/codeigniter/core/Utf8_test.php | 13 | ||||
| -rw-r--r-- | tests/mocks/core/uri.php | 15 | ||||
| -rw-r--r-- | tests/mocks/core/utf8.php | 5 | ||||
| -rw-r--r-- | user_guide_src/source/changelog.rst | 3 |
17 files changed, 195 insertions, 59 deletions
diff --git a/application/config/config.php b/application/config/config.php index cd2ca479b..5240f6c26 100644 --- a/application/config/config.php +++ b/application/config/config.php @@ -141,15 +141,18 @@ $config['subclass_prefix'] = 'MY_'; | Allowed URL Characters |-------------------------------------------------------------------------- | -| This lets you specify with a regular expression which characters are permitted -| within your URLs. When someone tries to submit a URL with disallowed -| characters they will get a warning message. +| This lets you specify which characters are permitted within your URLs. +| When someone tries to submit a URL with disallowed characters they will +| get a warning message. | | As a security measure you are STRONGLY encouraged to restrict URLs to | as few characters as possible. By default only these are allowed: a-z 0-9~%.:_- | | Leave blank to allow all characters -- but only if you are insane. | +| The configured value is actually a regular expression character group +| and it will be executed as: ! preg_match('/^[<permitted_uri_chars>]+$/i +| | DO NOT CHANGE THIS UNLESS YOU FULLY UNDERSTAND THE REPERCUSSIONS!! | */ diff --git a/application/config/user_agents.php b/application/config/user_agents.php index 2af70bf9c..819e42b69 100644 --- a/application/config/user_agents.php +++ b/application/config/user_agents.php @@ -90,6 +90,7 @@ $browsers = array( 'Opera' => 'Opera', 'MSIE' => 'Internet Explorer', 'Internet Explorer' => 'Internet Explorer', + 'Trident.* rv' => 'Internet Explorer', 'Shiira' => 'Shiira', 'Firefox' => 'Firefox', 'Chimera' => 'Chimera', diff --git a/system/core/Log.php b/system/core/Log.php index b2327b8f0..63fef2088 100644 --- a/system/core/Log.php +++ b/system/core/Log.php @@ -175,10 +175,18 @@ class CI_Log { return FALSE; } - $message .= $level.' '.($level === 'INFO' ? ' -' : '-').' '.date($this->_date_fmt).' --> '.$msg."\n"; + $message .= $level.' - '.date($this->_date_fmt).' --> '.$msg."\n"; flock($fp, LOCK_EX); - fwrite($fp, $message); + + for ($written = 0, $length = strlen($message); $written < $length; $written += $result) + { + if (($result = fwrite($fp, substr($message, $written))) === FALSE) + { + break; + } + } + flock($fp, LOCK_UN); fclose($fp); @@ -187,7 +195,7 @@ class CI_Log { @chmod($filepath, FILE_WRITE_MODE); } - return TRUE; + return is_int($result); } } diff --git a/system/core/Output.php b/system/core/Output.php index 10332f0d8..a7680b3d0 100644 --- a/system/core/Output.php +++ b/system/core/Output.php @@ -542,17 +542,26 @@ class CI_Output { return; } - $expire = time() + ($this->cache_expiration * 60); - - // Put together our serialized info. - $cache_info = serialize(array( - 'expire' => $expire, - 'headers' => $this->headers - )); - if (flock($fp, LOCK_EX)) { - fwrite($fp, $cache_info.'ENDCI--->'.$output); + $expire = time() + ($this->cache_expiration * 60); + + // Put together our serialized info. + $cache_info = serialize(array( + 'expire' => $expire, + 'headers' => $this->headers + )); + + $output = $cache_info.'ENDCI--->'.$output; + + for ($written = 0, $length = strlen($output); $written < $length; $written += $result) + { + if (($result = fwrite($fp, substr($output, $written))) === FALSE) + { + break; + } + } + flock($fp, LOCK_UN); } else @@ -560,13 +569,22 @@ class CI_Output { log_message('error', 'Unable to secure a file lock for file at: '.$cache_path); return; } + fclose($fp); - @chmod($cache_path, FILE_WRITE_MODE); - log_message('debug', 'Cache file written: '.$cache_path); + if (is_int($result)) + { + @chmod($cache_path, FILE_WRITE_MODE); + log_message('debug', 'Cache file written: '.$cache_path); - // Send HTTP cache-control headers to browser to match file cache settings. - $this->set_cache_header($_SERVER['REQUEST_TIME'], $expire); + // Send HTTP cache-control headers to browser to match file cache settings. + $this->set_cache_header($_SERVER['REQUEST_TIME'], $expire); + } + else + { + @unlink($cache_path); + log_message('error', 'Unable to write the complete cache content at: '.$cache_path); + } } // -------------------------------------------------------------------- diff --git a/system/core/Router.php b/system/core/Router.php index cb44a3ce9..71530ff07 100644 --- a/system/core/Router.php +++ b/system/core/Router.php @@ -154,16 +154,16 @@ class CI_Router { { if (isset($_GET[$this->config->item('directory_trigger')]) && is_string($_GET[$this->config->item('directory_trigger')])) { - $this->set_directory(trim($this->uri->_filter_uri($_GET[$this->config->item('directory_trigger')]))); + $this->set_directory(trim($this->uri->filter_uri($_GET[$this->config->item('directory_trigger')]))); $segments[] = $this->directory; } - $this->set_class(trim($this->uri->_filter_uri($_GET[$this->config->item('controller_trigger')]))); + $this->set_class(trim($this->uri->filter_uri($_GET[$this->config->item('controller_trigger')]))); $segments[] = $this->class; if ( ! empty($_GET[$this->config->item('function_trigger')]) && is_string($_GET[$this->config->item('function_trigger')])) { - $this->set_method(trim($this->uri->_filter_uri($_GET[$this->config->item('function_trigger')]))); + $this->set_method(trim($this->uri->filter_uri($_GET[$this->config->item('function_trigger')]))); $segments[] = $this->method; } } diff --git a/system/core/URI.php b/system/core/URI.php index 5e4c80a00..c83b7a74f 100644 --- a/system/core/URI.php +++ b/system/core/URI.php @@ -70,6 +70,15 @@ class CI_URI { public $rsegments = array(); /** + * Permitted URI chars + * + * PCRE character group allowed in URI segments + * + * @var string + */ + protected $_permitted_uri_chars; + + /** * Class constructor * * Simply globalizes the $RTR object. The front @@ -81,6 +90,12 @@ class CI_URI { public function __construct() { $this->config =& load_class('Config', 'core'); + + if ($this->config->item('enable_query_strings') !== TRUE OR is_cli()) + { + $this->_permitted_uri_chars = $this->config->item('permitted_uri_chars'); + } + log_message('debug', 'URI Class Initialized'); } @@ -303,23 +318,19 @@ class CI_URI { * @param string $str * @return string */ - public function _filter_uri($str) + public function filter_uri($str) { - if ($str !== '' && $this->config->item('permitted_uri_chars') != '' && $this->config->item('enable_query_strings') === FALSE) + if ( ! empty($str) && ! empty($this->_permitted_uri_chars) && ! preg_match('/^['.$this->_permitted_uri_chars.']+$/i'.(UTF8_ENABLED ? 'u' : ''), $str)) { - // preg_quote() in PHP 5.3 escapes -, so the str_replace() and addition of - to preg_quote() is to maintain backwards - // compatibility as many are unaware of how characters in the permitted_uri_chars will be parsed as a regex pattern - if ( ! preg_match('|^['.str_replace(array('\\-', '\-'), '-', preg_quote($this->config->item('permitted_uri_chars'), '-')).']+$|i', $str)) - { - show_error('The URI you submitted has disallowed characters.', 400); - } + show_error('The URI you submitted has disallowed characters.', 400); } // Convert programatic characters to entities and return return str_replace( - array('$', '(', ')', '%28', '%29'), // Bad - array('$', '(', ')', '(', ')'), // Good - $str); + array('$', '(', ')', '%28', '%29'), // Bad + array('$', '(', ')', '(', ')'), // Good + $str + ); } // -------------------------------------------------------------------- @@ -365,7 +376,7 @@ class CI_URI { foreach (explode('/', preg_replace('|/*(.+?)/*$|', '\\1', $this->uri_string)) as $val) { // Filter segments for security - $val = trim($this->_filter_uri($val)); + $val = trim($this->filter_uri($val)); if ($val !== '') { diff --git a/system/core/Utf8.php b/system/core/Utf8.php index a78616d40..828a8aeba 100644 --- a/system/core/Utf8.php +++ b/system/core/Utf8.php @@ -66,7 +66,7 @@ class CI_Utf8 { } if ( - @preg_match('/./u', 'é') === 1 // PCRE must support UTF-8 + defined('PREG_BAD_UTF8_ERROR') // PCRE must support UTF-8 && function_exists('iconv') // iconv must be installed && MB_ENABLED === TRUE // mbstring must be enabled && $charset === 'UTF-8' // Application charset must be UTF-8 diff --git a/system/helpers/file_helper.php b/system/helpers/file_helper.php index 4b45a62d0..0587740b1 100644 --- a/system/helpers/file_helper.php +++ b/system/helpers/file_helper.php @@ -79,11 +79,19 @@ if ( ! function_exists('write_file')) } flock($fp, LOCK_EX); - fwrite($fp, $data); + + for ($written = 0, $length = strlen($data); $written < $length; $written += $result) + { + if (($result = fwrite($fp, substr($data, $written))) === FALSE) + { + break; + } + } + flock($fp, LOCK_UN); fclose($fp); - return TRUE; + return is_int($result); } } diff --git a/system/libraries/Email.php b/system/libraries/Email.php index 7e80ffbca..f4efff882 100644 --- a/system/libraries/Email.php +++ b/system/libraries/Email.php @@ -2097,7 +2097,16 @@ class CI_Email { */ protected function _send_data($data) { - if ( ! fwrite($this->_smtp_connect, $data.$this->newline)) + $data .= $this->newline; + for ($written = 0, $length = strlen($data); $written < $length; $written += $result) + { + if (($result = fwrite($this->_smtp_connect, substr($data, $written))) === FALSE) + { + break; + } + } + + if ($result === FALSE) { $this->_set_error_message('lang:email_smtp_data_failure', $data); return FALSE; diff --git a/system/libraries/Xmlrpc.php b/system/libraries/Xmlrpc.php index 2fd12599e..ab907e706 100644 --- a/system/libraries/Xmlrpc.php +++ b/system/libraries/Xmlrpc.php @@ -724,7 +724,15 @@ class XML_RPC_Client extends CI_Xmlrpc .'Content-Length: '.strlen($msg->payload).$r.$r .$msg->payload; - if ( ! fwrite($fp, $op, strlen($op))) + for ($written = 0, $length = strlen($op); $written < $length; $written += $result) + { + if (($result = fwrite($fp, substr($op, $written))) === FALSE) + { + break; + } + } + + if ($result === FALSE) { error_log($this->xmlrpcstr['http_error']); return new XML_RPC_Response(0, $this->xmlrpcerr['http_error'], $this->xmlrpcstr['http_error']); diff --git a/system/libraries/Zip.php b/system/libraries/Zip.php index 250ee02cd..b10b0bb0f 100644 --- a/system/libraries/Zip.php +++ b/system/libraries/Zip.php @@ -403,11 +403,19 @@ class CI_Zip { } flock($fp, LOCK_EX); - fwrite($fp, $this->get_zip()); + + for ($written = 0, $data = $this->get_zip(), $length = strlen($data); $written < $length; $written += $result) + { + if (($result = fwrite($fp, substr($data, $written))) === FALSE) + { + break; + } + } + flock($fp, LOCK_UN); fclose($fp); - return TRUE; + return is_int($result); } // -------------------------------------------------------------------- diff --git a/tests/codeigniter/core/Input_test.php b/tests/codeigniter/core/Input_test.php index 0a98e556c..95833fc91 100644 --- a/tests/codeigniter/core/Input_test.php +++ b/tests/codeigniter/core/Input_test.php @@ -138,13 +138,24 @@ class Input_test extends CI_TestCase { public function test_valid_ip() { - $ip_v4 = '192.18.0.1'; - $this->assertTrue($this->input->valid_ip($ip_v4)); + $this->assertTrue($this->input->valid_ip('192.18.0.1')); + $this->assertTrue($this->input->valid_ip('192.18.0.1', 'ipv4')); + $this->assertFalse($this->input->valid_ip('555.0.0.0')); + $this->assertFalse($this->input->valid_ip('2001:db8:0:85a3::ac1f:8001', 'ipv4')); + + // v6 tests + $this->assertFalse($this->input->valid_ip('192.18.0.1', 'ipv6')); + + $ip_v6 = array( + '2001:0db8:0000:85a3:0000:0000:ac1f:8001', + '2001:db8:0:85a3:0:0:ac1f:8001', + '2001:db8:0:85a3::ac1f:8001' + ); - $ip_v6 = array('2001:0db8:0000:85a3:0000:0000:ac1f:8001', '2001:db8:0:85a3:0:0:ac1f:8001', '2001:db8:0:85a3::ac1f:8001'); foreach ($ip_v6 as $ip) { $this->assertTrue($this->input->valid_ip($ip)); + $this->assertTrue($this->input->valid_ip($ip, 'ipv6')); } } @@ -171,4 +182,34 @@ class Input_test extends CI_TestCase { $this->assertTrue($this->input->is_ajax_request()); } + // -------------------------------------------------------------------- + + public function test_input_stream() + { + $this->markTestSkipped('TODO: Find a way to test input://'); + } + + // -------------------------------------------------------------------- + + public function test_set_cookie() + { + $this->markTestSkipped('TODO: Find a way to test HTTP headers'); + } + + public function test_ip_address() + { + // 127.0.0.1 is set in our Bootstrap file + $this->assertEquals('127.0.0.1', $this->input->ip_address()); + + // Invalid + $_SERVER['REMOTE_ADDR'] = 'invalid_ip_address'; + $this->input->ip_address = FALSE; // reset cached value + $this->assertEquals('0.0.0.0', $this->input->ip_address()); + + // TODO: Add proxy_ips tests + + // Back to reality + $_SERVER['REMOTE_ADDR'] = '127.0.0.1'; // back to reality + } + }
\ No newline at end of file diff --git a/tests/codeigniter/core/URI_test.php b/tests/codeigniter/core/URI_test.php index 7fa0e6265..99d79bbd2 100644 --- a/tests/codeigniter/core/URI_test.php +++ b/tests/codeigniter/core/URI_test.php @@ -112,11 +112,10 @@ class URI_test extends CI_TestCase { public function test_filter_uri() { - $this->uri->config->set_item('enable_query_strings', FALSE); - $this->uri->config->set_item('permitted_uri_chars', 'a-z 0-9~%.:_\-'); + $this->uri->_set_permitted_uri_chars('a-z 0-9~%.:_\-'); $str_in = 'abc01239~%.:_-'; - $str = $this->uri->_filter_uri($str_in); + $str = $this->uri->filter_uri($str_in); $this->assertEquals($str, $str_in); } @@ -126,11 +125,9 @@ class URI_test extends CI_TestCase { public function test_filter_uri_escaping() { // ensure escaping even if dodgey characters are permitted + $this->uri->_set_permitted_uri_chars('a-z 0-9~%.:_\-()$'); - $this->uri->config->set_item('enable_query_strings', FALSE); - $this->uri->config->set_item('permitted_uri_chars', 'a-z 0-9~%.:_\-()$'); - - $str = $this->uri->_filter_uri('$destroy_app(foo)'); + $str = $this->uri->filter_uri('$destroy_app(foo)'); $this->assertEquals($str, '$destroy_app(foo)'); } @@ -142,8 +139,8 @@ class URI_test extends CI_TestCase { $this->setExpectedException('RuntimeException'); $this->uri->config->set_item('enable_query_strings', FALSE); - $this->uri->config->set_item('permitted_uri_chars', 'a-z 0-9~%.:_\-'); - $this->uri->_filter_uri('$this()'); + $this->uri->_set_permitted_uri_chars('a-z 0-9~%.:_\-'); + $this->uri->filter_uri('$this()'); } // -------------------------------------------------------------------- diff --git a/tests/codeigniter/core/Utf8_test.php b/tests/codeigniter/core/Utf8_test.php index caa7b6986..71299134e 100644 --- a/tests/codeigniter/core/Utf8_test.php +++ b/tests/codeigniter/core/Utf8_test.php @@ -11,10 +11,15 @@ class Utf8_test extends CI_TestCase { public function test_convert_to_utf8() { - $this->assertEquals( - $this->utf8->convert_to_utf8('', 'WINDOWS-1251'), - 'тест' - ); + $this->assertEquals('тест', $this->utf8->convert_to_utf8('', 'WINDOWS-1251')); + } + + // -------------------------------------------------------------------- + + public function test_is_ascii() + { + $this->assertTrue($this->utf8->is_ascii_test('foo bar')); + $this->assertFalse($this->utf8->is_ascii_test('тест')); } }
\ No newline at end of file diff --git a/tests/mocks/core/uri.php b/tests/mocks/core/uri.php index 11078587b..96ec5afa1 100644 --- a/tests/mocks/core/uri.php +++ b/tests/mocks/core/uri.php @@ -10,12 +10,23 @@ class Mock_Core_URI extends CI_URI { // set predictable config values $test->ci_set_config(array( 'index_page' => 'index.php', - 'base_url' => 'http://example.com/', - 'subclass_prefix' => 'MY_' + 'base_url' => 'http://example.com/', + 'subclass_prefix' => 'MY_', + 'enable_query_strings' => FALSE, + 'permitted_uri_chars' => 'a-z 0-9~%.:_\-' )); $this->config = new $cls; + if ($this->config->item('enable_query_strings') !== TRUE OR is_cli()) + { + $this->_permitted_uri_chars = $this->config->item('permitted_uri_chars'); + } + } + + public function _set_permitted_uri_chars($value) + { + $this->_permitted_uri_chars = $value; } }
\ No newline at end of file diff --git a/tests/mocks/core/utf8.php b/tests/mocks/core/utf8.php index 068e74ac1..a43138fbc 100644 --- a/tests/mocks/core/utf8.php +++ b/tests/mocks/core/utf8.php @@ -23,4 +23,9 @@ class Mock_Core_Utf8 extends CI_Utf8 { } } + public function is_ascii_test($str) + { + return $this->_is_ascii($str); + } + }
\ No newline at end of file diff --git a/user_guide_src/source/changelog.rst b/user_guide_src/source/changelog.rst index 43e7be41f..4712ed87d 100644 --- a/user_guide_src/source/changelog.rst +++ b/user_guide_src/source/changelog.rst @@ -391,6 +391,8 @@ Release Date: Not Released - :doc:`URI Library <libraries/uri>` changes include: + - Added conditional PCRE UTF-8 support to the "invalid URI characters" check and removed the ``preg_quote()`` call from it to allow more flexibility. + - Renamed method ``_filter_uri()`` to ``filter_uri()``. - Changed private methods to protected so that MY_URI can override them. - Renamed internal method ``_parse_cli_args()`` to ``_parse_argv()``. - Renamed internal method ``_detect_uri()`` to ``_parse_request_uri()``. @@ -668,6 +670,7 @@ Bug fixes for 3.0 - Fixed a bug (#346) - with ``$config['global_xss_filtering']`` turned on, the ``$_GET``, ``$_POST``, ``$_COOKIE`` and ``$_SERVER`` superglobals were overwritten during initialization time, resulting in XSS filtering being either performed twice or there was no possible way to get the original data, even though options for this do exist. - Fixed an edge case (#555) - incorrect browser version was reported for Opera 10+ due to a non-standard user-agent string. - Fixed a bug (#133) - :doc:`Text Helper <helpers/text_helper>` :func:`ascii_to_entities()` stripped the last character if it happens to be in the extended ASCII group. +- Fixed a bug (#2822) - ``fwrite()`` was used incorrectly throughout the whole framework, allowing incomplete writes when writing to a network stream and possibly a few other edge cases. Version 2.1.4 ============= |