From f565212c5aa07a8016394a3bc66874be83c73d4d Mon Sep 17 00:00:00 2001 From: Andrey Andreev Date: Thu, 19 Jan 2017 15:17:00 +0200 Subject: Fix byte-safety issues & actually test for them --- .gitignore | 2 + .travis.yml | 3 +- composer.json | 5 ++- system/helpers/text_helper.php | 7 ++- system/libraries/Encrypt.php | 59 +++++++++++++++++++++---- system/libraries/Encryption.php | 10 ++--- tests/codeigniter/libraries/Encryption_test.php | 16 ++++++- tests/phpunit.xml | 10 ++--- user_guide_src/source/changelog.rst | 11 ++++- 9 files changed, 93 insertions(+), 30 deletions(-) diff --git a/.gitignore b/.gitignore index 5982f9bad..eb45390e9 100644 --- a/.gitignore +++ b/.gitignore @@ -8,6 +8,8 @@ application/logs/* !application/logs/index.html !application/logs/.htaccess +composer.lock + user_guide_src/build/* user_guide_src/cilexer/build/* user_guide_src/cilexer/dist/* diff --git a/.travis.yml b/.travis.yml index 6b90a5f64..2a0be4ddc 100644 --- a/.travis.yml +++ b/.travis.yml @@ -26,11 +26,10 @@ before_script: - sh -c "if [ '$DB' = 'pgsql' ] || [ '$DB' = 'pdo/pgsql' ]; then psql -c 'create database ci_test;' -U postgres; fi" - sh -c "if [ '$DB' = 'mysql' ] || [ '$DB' = 'mysqli' ] || [ '$DB' = 'pdo/mysql' ]; then mysql -e 'create database IF NOT EXISTS ci_test;'; fi" -script: phpunit -d zend.enable_gc=0 -d date.timezone=UTC --coverage-text --configuration tests/travis/$DB.phpunit.xml +script: phpunit -d zend.enable_gc=0 -d date.timezone=UTF -d mbstring.func_overload=7 -d mbstring.internal_encoding=UTF-8 vendor/bin/phpunit --coverage-text --configuration tests/travis/$DB.phpunit.xml matrix: allow_failures: - - php: 5.3 - php: hhvm exclude: - php: hhvm diff --git a/composer.json b/composer.json index 64d1be155..0a898d2c1 100644 --- a/composer.json +++ b/composer.json @@ -17,6 +17,7 @@ "paragonie/random_compat": "Provides better randomness in PHP 5.x" }, "require-dev": { - "mikey179/vfsStream": "1.1.*" + "mikey179/vfsStream": "1.1.*", + "phpunit/phpunit": "4.* || 5.*" } -} \ No newline at end of file +} diff --git a/system/helpers/text_helper.php b/system/helpers/text_helper.php index 07c01c3af..217729b70 100644 --- a/system/helpers/text_helper.php +++ b/system/helpers/text_helper.php @@ -138,7 +138,10 @@ if ( ! function_exists('ascii_to_entities')) function ascii_to_entities($str) { $out = ''; - for ($i = 0, $s = strlen($str) - 1, $count = 1, $temp = array(); $i <= $s; $i++) + $length = defined('MB_OVERLOAD_STRING') + ? mb_strlen($str, '8bit') - 1 + : strlen($str) - 1; + for ($i = 0, $count = 1, $temp = array(); $i <= $length; $i++) { $ordinal = ord($str[$i]); @@ -176,7 +179,7 @@ if ( ! function_exists('ascii_to_entities')) $temp = array(); } // If this is the last iteration, just output whatever we have - elseif ($i === $s) + elseif ($i === $length) { $out .= '&#'.implode(';', $temp).';'; } diff --git a/system/libraries/Encrypt.php b/system/libraries/Encrypt.php index 46f374726..ebcc6e8c6 100644 --- a/system/libraries/Encrypt.php +++ b/system/libraries/Encrypt.php @@ -122,7 +122,7 @@ class CI_Encrypt { $key = config_item('encryption_key'); - if ( ! strlen($key)) + if ( ! self::strlen($key)) { show_error('In order to use the encryption class requires that you set an encryption key in your config file.'); } @@ -252,7 +252,7 @@ class CI_Encrypt { $string = $this->_xor_merge($string, $key); $dec = ''; - for ($i = 0, $l = strlen($string); $i < $l; $i++) + for ($i = 0, $l = self::strlen($string); $i < $l; $i++) { $dec .= ($string[$i++] ^ $string[$i]); } @@ -275,7 +275,8 @@ class CI_Encrypt { { $hash = $this->hash($key); $str = ''; - for ($i = 0, $ls = strlen($string), $lh = strlen($hash); $i < $ls; $i++) + + for ($i = 0, $ls = self::strlen($string), $lh = self::strlen($hash); $i < $ls; $i++) { $str .= $string[$i] ^ $hash[($i % $lh)]; } @@ -295,7 +296,7 @@ class CI_Encrypt { public function mcrypt_encode($data, $key) { $init_size = mcrypt_get_iv_size($this->_get_cipher(), $this->_get_mode()); - $init_vect = mcrypt_create_iv($init_size, MCRYPT_RAND); + $init_vect = mcrypt_create_iv($init_size, MCRYPT_DEV_URANDOM); return $this->_add_cipher_noise($init_vect.mcrypt_encrypt($this->_get_cipher(), $key, $data, $this->_get_mode(), $init_vect), $key); } @@ -313,13 +314,14 @@ class CI_Encrypt { $data = $this->_remove_cipher_noise($data, $key); $init_size = mcrypt_get_iv_size($this->_get_cipher(), $this->_get_mode()); - if ($init_size > strlen($data)) + if ($init_size > self::strlen($data)) { return FALSE; } - $init_vect = substr($data, 0, $init_size); - $data = substr($data, $init_size); + $init_vect = self::substr($data, 0, $init_size); + $data = self::substr($data, $init_size); + return rtrim(mcrypt_decrypt($this->_get_cipher(), $key, $data, $this->_get_mode(), $init_vect), "\0"); } @@ -339,7 +341,7 @@ class CI_Encrypt { $key = $this->hash($key); $str = ''; - for ($i = 0, $j = 0, $ld = strlen($data), $lk = strlen($key); $i < $ld; ++$i, ++$j) + for ($i = 0, $j = 0, $ld = self::strlen($data), $lk = self::strlen($key); $i < $ld; ++$i, ++$j) { if ($j >= $lk) { @@ -369,7 +371,7 @@ class CI_Encrypt { $key = $this->hash($key); $str = ''; - for ($i = 0, $j = 0, $ld = strlen($data), $lk = strlen($key); $i < $ld; ++$i, ++$j) + for ($i = 0, $j = 0, $ld = self::strlen($data), $lk = self::strlen($key); $i < $ld; ++$i, ++$j) { if ($j >= $lk) { @@ -477,4 +479,43 @@ class CI_Encrypt { return hash($this->_hash_type, $str); } + // -------------------------------------------------------------------- + + /** + * Byte-safe strlen() + * + * @param string $str + * @return int + */ + protected static function strlen($str) + { + return defined('MB_OVERLOAD_STRING') + ? mb_strlen($str, '8bit') + : strlen($str); + } + + // -------------------------------------------------------------------- + + /** + * Byte-safe substr() + * + * @param string $str + * @param int $start + * @param int $length + * @return string + */ + protected static function substr($str, $start, $length = NULL) + { + if (defined('MB_OVERLOAD_STRING')) + { + // mb_substr($str, $start, null, '8bit') returns an empty + // string on PHP 5.3 + isset($length) OR $length = ($start >= 0 ? self::strlen($str) - $start : -$start); + return mb_substr($str, $start, $length, '8bit'); + } + + return isset($length) + ? substr($str, $start, $length) + : substr($str, $start); + } } diff --git a/system/libraries/Encryption.php b/system/libraries/Encryption.php index 74832ede6..c1e454dda 100644 --- a/system/libraries/Encryption.php +++ b/system/libraries/Encryption.php @@ -135,11 +135,11 @@ class CI_Encryption { ); /** - * mbstring.func_override flag + * mbstring.func_overload flag * * @var bool */ - protected static $func_override; + protected static $func_overload; // -------------------------------------------------------------------- @@ -161,7 +161,7 @@ class CI_Encryption { show_error('Encryption: Unable to find an available encryption driver.'); } - isset(self::$func_override) OR self::$func_override = (extension_loaded('mbstring') && ini_get('mbstring.func_override')); + isset(self::$func_overload) OR self::$func_overload = (extension_loaded('mbstring') && ini_get('mbstring.func_overload')); $this->initialize($params); if ( ! isset($this->_key) && self::strlen($key = config_item('encryption_key')) > 0) @@ -911,7 +911,7 @@ class CI_Encryption { */ protected static function strlen($str) { - return (self::$func_override) + return (self::$func_overload) ? mb_strlen($str, '8bit') : strlen($str); } @@ -928,7 +928,7 @@ class CI_Encryption { */ protected static function substr($str, $start, $length = NULL) { - if (self::$func_override) + if (self::$func_overload) { // mb_substr($str, $start, null, '8bit') returns an empty // string on PHP 5.3 diff --git a/tests/codeigniter/libraries/Encryption_test.php b/tests/codeigniter/libraries/Encryption_test.php index 96e52ada8..99c5d4b9d 100644 --- a/tests/codeigniter/libraries/Encryption_test.php +++ b/tests/codeigniter/libraries/Encryption_test.php @@ -94,10 +94,22 @@ class Encryption_test extends CI_TestCase { } // Test default length, it must match the digest size - $this->assertEquals(64, strlen($this->encryption->hkdf('foobar', 'sha512'))); + $hkdf_result = $this->encryption->hkdf('foobar', 'sha512'); + $this->assertEquals( + 64, + defined('MB_OVERLOAD_STRING') + ? mb_strlen($hkdf_result, '8bit') + : strlen($hkdf_result) + ); // Test maximum length (RFC5869 says that it must be up to 255 times the digest size) - $this->assertEquals(12240, strlen($this->encryption->hkdf('foobar', 'sha384', NULL, 48 * 255))); + $hkdf_result = $this->encryption->hkdf('foobar', 'sha384', NULL, 48 * 255); + $this->assertEquals( + 12240, + defined('MB_OVERLOAD_STRING') + ? mb_strlen($hkdf_result, '8bit') + : strlen($hkdf_result) + ); $this->assertFalse($this->encryption->hkdf('foobar', 'sha224', NULL, 28 * 255 + 1)); // CI-specific test for an invalid digest diff --git a/tests/phpunit.xml b/tests/phpunit.xml index 96c3af9bb..875198c4e 100644 --- a/tests/phpunit.xml +++ b/tests/phpunit.xml @@ -17,10 +17,8 @@ - - PEAR_INSTALL_DIR - PHP_LIBDIR - ../vendor - + + ../system/ + - \ No newline at end of file + diff --git a/user_guide_src/source/changelog.rst b/user_guide_src/source/changelog.rst index 2769990f8..17069ca32 100644 --- a/user_guide_src/source/changelog.rst +++ b/user_guide_src/source/changelog.rst @@ -7,6 +7,12 @@ Version 3.1.4 Release Date: Not Released +- **Security** + + - Updated :doc:`Encrypt Library ` (DEPRECATED) to call ``mcrypt_create_iv()`` with ``MCRYPT_DEV_URANDOM``. + - Fixed byte-safety issues in :doc:`Encrypt Library ` (DEPRECATED) when ``mbstring.func_overload`` is enabled. + - Fixed byte-safety issues in :doc:`Encryption Library ` when ``mbstring.func_overload`` is enabled. + - General Changes - Updated the :doc:`Image Manipulation Library ` to work-around an issue with some JPEGs when using GD. @@ -18,6 +24,7 @@ Bug fixes for 3.1.4 - Fixed a bug (#4977) - :doc:`Loader Library ` method ``helper()`` could accept any character as a filename extension separator. - Fixed a regression where the :doc:`Session Library ` would fail on a ``session_regenerate_id(TRUE)`` call with the 'database' driver. - Fixed a bug (#4987) - :doc:`Query Builder ` caching didn't keep track of table aliases. +- Fixed a bug where :doc:`Text Helper ` function ``ascii_to_entities()`` wasn't byte-safe when ``mbstring.func_overload`` is enabled. Version 3.1.3 ============= @@ -82,7 +89,7 @@ Bug fixes for 3.1.2 - Fixed a regression (#4874) - :doc:`Session Library ` didn't take into account ``session.hash_bits_per_character`` when validating session IDs. - Fixed a bug (#4871) - :doc:`Query Builder ` method ``update_batch()`` didn't properly handle identifier escaping. - Fixed a bug (#4884) - :doc:`Query Builder ` didn't properly parse field names ending in 'is' when used inside WHERE and HAVING statements. -- Fixed a bug where ``CI_Log``, ``CI_Output``, ``CI_Email`` and ``CI_Zip`` didn't handle strings in a byte-safe manner when ``mbstring.func_override`` is enabled. +- Fixed a bug where ``CI_Log``, ``CI_Output``, ``CI_Email`` and ``CI_Zip`` didn't handle strings in a byte-safe manner when ``mbstring.func_overload`` is enabled. Version 3.1.1 ============= @@ -119,7 +126,7 @@ Bug fixes for 3.1.1 - Fixed a bug where :doc:`Query Builder ` method ``insert_batch()`` tried to execute an unsupported SQL query with the 'ibase' and 'pdo/firebird' drivers. - Fixed a bug (#4809) - :doc:`Database ` driver 'pdo/mysql' didn't turn off ``AUTOCOMMIT`` when starting a transaction. - Fixed a bug (#4822) - :doc:`CAPTCHA Helper ` didn't clear expired PNG images. -- Fixed a bug (#4823) - :doc:`Session Library ` 'files' driver could enter an infinite loop if ``mbstring.func_override`` is enabled. +- Fixed a bug (#4823) - :doc:`Session Library ` 'files' driver could enter an infinite loop if ``mbstring.func_overload`` is enabled. - Fixed a bug (#4851) - :doc:`Database Forge ` didn't quote schema names passed to its ``create_database()`` method. - Fixed a bug (#4863) - :doc:`HTML Table Library ` method ``set_caption()`` was missing method chaining support. - Fixed a bug (#4843) - :doc:`XML-RPC Library ` client class didn't set a read/write socket timeout. -- cgit v1.2.3-24-g4f1b