From 0dc184e99f0ca607774a68257432a9a981f4d5b7 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 18 Jan 2010 11:10:37 -0800 Subject: Overload url::current() and url::merge() to make the current url XSS safe. Add tests to make sure that it doesn't relapse with future Kohana changes. Fixes ticket #983. Ref: http://gallery.menalto.com/node/93738 --- modules/gallery/helpers/MY_url.php | 14 ++++++++++ modules/gallery/tests/Url_Security_Test.php | 43 +++++++++++++++++++++++++++++ modules/rss/controllers/rss.php | 8 ++---- 3 files changed, 60 insertions(+), 5 deletions(-) create mode 100644 modules/gallery/tests/Url_Security_Test.php (limited to 'modules') diff --git a/modules/gallery/helpers/MY_url.php b/modules/gallery/helpers/MY_url.php index 74284951..8a7909b6 100644 --- a/modules/gallery/helpers/MY_url.php +++ b/modules/gallery/helpers/MY_url.php @@ -89,4 +89,18 @@ class url extends url_Core { static function abs_current($qs=false) { return self::abs_site(url::current($qs)); } + + /** + * Just like url::merge except that it escapes any XSS in the path. + */ + static function merge($params) { + return htmlspecialchars(parent::merge($params)); + } + + /** + * Just like url::current except that it escapes any XSS in the path. + */ + static function current($qs=false, $suffix=false) { + return htmlspecialchars(parent::current($qs, $suffix)); + } } diff --git a/modules/gallery/tests/Url_Security_Test.php b/modules/gallery/tests/Url_Security_Test.php new file mode 100644 index 00000000..de25880f --- /dev/null +++ b/modules/gallery/tests/Url_Security_Test.php @@ -0,0 +1,43 @@ +save = array(Router::$current_uri, Router::$complete_uri, $_GET); + } + + public function teardown() { + list(Router::$current_uri, Router::$complete_uri, $_GET) = $this->save; + } + + public function xss_in_current_url_test() { + Router::$current_uri = "foo//bar"; + Router::$complete_uri = "foo//bar?foo=bar"; + $this->assert_same("foo/<xss>/bar", url::current()); + $this->assert_same("foo/<xss>/bar?foo=bar", url::current(true)); + } + + public function xss_in_merged_url_test() { + Router::$current_uri = "foo//bar"; + Router::$complete_uri = "foo//bar?foo=bar"; + $_GET = array("foo" => "bar"); + $this->assert_same("foo/<xss>/bar?foo=bar", url::merge(array())); + $this->assert_same("foo/<xss>/bar?foo=bar&a=b", url::merge(array("a" => "b"))); + } +} \ No newline at end of file diff --git a/modules/rss/controllers/rss.php b/modules/rss/controllers/rss.php index 41c781d9..3066ba16 100644 --- a/modules/rss/controllers/rss.php +++ b/modules/rss/controllers/rss.php @@ -52,14 +52,12 @@ class Rss_Controller extends Controller { $view->feed = $feed; $view->pub_date = date("D, d M Y H:i:s T"); - $feed->uri = url::abs_site(str_replace("&", "&", url::merge($_GET))); + $feed->uri = url::abs_site(url::merge($_GET)); if ($page > 1) { - $feed->previous_page_uri = - url::abs_site(str_replace("&", "&", url::merge(array("page" => $page - 1)))); + $feed->previous_page_uri = url::abs_site(url::merge(array("page" => $page - 1))); } if ($page < $feed->max_pages) { - $feed->next_page_uri = - url::abs_site(str_replace("&", "&", url::merge(array("page" => $page + 1)))); + $feed->next_page_uri = url::abs_site(url::merge(array("page" => $page + 1))); } header("Content-Type: application/rss+xml"); -- cgit v1.2.3 From 4eafe97b4858cb8d1b3367574f7e7ef473127d7c Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 18 Jan 2010 11:57:23 -0800 Subject: Reload $item after removing view permissions. --- modules/gallery/tests/Access_Helper_Test.php | 1 + 1 file changed, 1 insertion(+) (limited to 'modules') diff --git a/modules/gallery/tests/Access_Helper_Test.php b/modules/gallery/tests/Access_Helper_Test.php index 084bfb47..b2244766 100644 --- a/modules/gallery/tests/Access_Helper_Test.php +++ b/modules/gallery/tests/Access_Helper_Test.php @@ -74,6 +74,7 @@ class Access_Helper_Test extends Unit_Test_Case { access::deny(identity::everybody(), "view", $item); access::deny(identity::registered_users(), "view", $item); + $item->reload(); $user = identity::create_user("access_test", "Access Test", ""); foreach ($user->groups() as $group) { -- cgit v1.2.3 From 41a392611c0e602d2e14859e5c0d5bf9e61d0073 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 18 Jan 2010 12:08:05 -0800 Subject: Change DENY and ALLOW to "0" and "1" to match the fact that ORM no longer typecasts values as of http://dev.kohanaphp.com/issues/2459 --- modules/gallery/helpers/access.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'modules') diff --git a/modules/gallery/helpers/access.php b/modules/gallery/helpers/access.php index 8ce7e436..e0a0e979 100644 --- a/modules/gallery/helpers/access.php +++ b/modules/gallery/helpers/access.php @@ -66,8 +66,8 @@ * the Access_Intent_Model */ class access_Core { - const DENY = false; - const ALLOW = true; + const DENY = "0"; + const ALLOW = "1"; const INHERIT = null; // access_intent const UNKNOWN = null; // cache (access_cache, items) -- cgit v1.2.3 From 284788d964688385f77b18bc063a841d0dbcdcd8 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 18 Jan 2010 12:08:39 -0800 Subject: Switch from stdClass to arrays which works around issues caused in http://dev.kohanaphp.com/issues/2459 -- I don't exactly know why, but the solutions are equivalent so I'm not going to dig too far. --- modules/gallery/helpers/model_cache.php | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) (limited to 'modules') diff --git a/modules/gallery/helpers/model_cache.php b/modules/gallery/helpers/model_cache.php index 302e42d9..88756407 100644 --- a/modules/gallery/helpers/model_cache.php +++ b/modules/gallery/helpers/model_cache.php @@ -18,27 +18,25 @@ * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA. */ class model_cache_Core { - private static $cache; + private static $cache = array(); static function get($model_name, $id, $field_name="id") { - if (TEST_MODE || empty(self::$cache->$model_name->$field_name->$id)) { + if (TEST_MODE || empty(self::$cache[$model_name][$field_name][$id])) { $model = ORM::factory($model_name)->where($field_name, "=", $id)->find(); if (!$model->loaded()) { throw new Exception("@todo MISSING_MODEL $model_name:$id"); } - self::$cache->$model_name->$field_name->$id = $model; + self::$cache[$model_name][$field_name][$id] = $model; } - return self::$cache->$model_name->$field_name->$id; + return self::$cache[$model_name][$field_name][$id]; } static function clear() { - self::$cache = new stdClass(); + self::$cache = array(); } static function set($model) { - self::$cache->{$model->object_name} - ->{$model->primary_key} - ->{$model->{$model->primary_key}} = $model; + self::$cache[$model->object_name][$model->primary_key][$model->{$model->primary_key}] = $model; } } -- cgit v1.2.3 From 51427d540464ffa5b4663a9c3b794cefc637925a Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 18 Jan 2010 12:21:57 -0800 Subject: Verified --- modules/gallery/tests/xss_data.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'modules') diff --git a/modules/gallery/tests/xss_data.txt b/modules/gallery/tests/xss_data.txt index a264286c..1530c73e 100644 --- a/modules/gallery/tests/xss_data.txt +++ b/modules/gallery/tests/xss_data.txt @@ -137,7 +137,7 @@ modules/gallery/views/l10n_client.html.php 26 DIRTY $strin modules/gallery/views/l10n_client.html.php 32 DIRTY $l10n_search_form modules/gallery/views/l10n_client.html.php 41 DIRTY access::csrf_form_field() modules/gallery/views/l10n_client.html.php 42 DIRTY form::hidden("l10n-message-key") -modules/gallery/views/l10n_client.html.php 43 DIRTY form::textarea("l10n-edit-translation","",' rows="5" class="translationField"') +modules/gallery/views/l10n_client.html.php 43 DIRTY form::textarea("l10n-edit-translation","",' id="l10n-edit-translation" rows="5" class="translationField"') modules/gallery/views/l10n_client.html.php 46 DIRTY form::textarea("l10n-edit-plural-translation-zero","",' rows="2"') modules/gallery/views/l10n_client.html.php 50 DIRTY form::textarea("l10n-edit-plural-translation-one","",' rows="2"') modules/gallery/views/l10n_client.html.php 54 DIRTY form::textarea("l10n-edit-plural-translation-two","",' rows="2"') -- cgit v1.2.3 From cac4692510d6b5da0d0a63f2ec54783df6ca86e4 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 18 Jan 2010 12:35:26 -0800 Subject: Don't use rand() as the name. Now that ORM::load_types() is gone, it won't get coerced to a string, and then we wind up comparing: 12345 != 12345-12321 In the old approach, they'd both be strings so they'd be inequal. But in the new approach the first value is an integer (sinced it came from rand()) so the second value is typecast to an integer which drops everything after the - sign so they appear equal. --- modules/gallery/tests/Album_Helper_Test.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'modules') diff --git a/modules/gallery/tests/Album_Helper_Test.php b/modules/gallery/tests/Album_Helper_Test.php index 1284b8cc..ef0905da 100644 --- a/modules/gallery/tests/Album_Helper_Test.php +++ b/modules/gallery/tests/Album_Helper_Test.php @@ -38,7 +38,7 @@ class Album_Helper_Test extends Unit_Test_Case { } public function create_conflicting_album_test() { - $rand = rand(); + $rand = "name_" . rand(); $root = ORM::factory("item", 1); $album1 = album::create($root, $rand, $rand, $rand); $album2 = album::create($root, $rand, $rand, $rand); -- cgit v1.2.3 From cf236a228a8ea3316506f6d855bcade92676674c Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 18 Jan 2010 12:37:20 -0800 Subject: Don't assert_same() now that typecasting is gone from ORM. --- modules/gallery/tests/Item_Model_Test.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'modules') diff --git a/modules/gallery/tests/Item_Model_Test.php b/modules/gallery/tests/Item_Model_Test.php index bf5fca1a..d03a03f4 100644 --- a/modules/gallery/tests/Item_Model_Test.php +++ b/modules/gallery/tests/Item_Model_Test.php @@ -52,7 +52,7 @@ class Item_Model_Test extends Unit_Test_Case { public function updating_view_count_only_doesnt_change_updated_date_test() { $item = self::_create_random_item(); $item->reload(); - $this->assert_same(0, $item->view_count); + $this->assert_equal(0, $item->view_count); // Force the updated date to something well known db::build() -- cgit v1.2.3 From a9f07986f64faf5062c2d86aa34710892d8ac8b3 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 18 Jan 2010 12:39:42 -0800 Subject: The root parent id is 0, not null (this deviation exposed by the new lack of typecasting in ORM). --- modules/gallery/tests/Gallery_Installer_Test.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'modules') diff --git a/modules/gallery/tests/Gallery_Installer_Test.php b/modules/gallery/tests/Gallery_Installer_Test.php index 43399fb4..74a07b1a 100644 --- a/modules/gallery/tests/Gallery_Installer_Test.php +++ b/modules/gallery/tests/Gallery_Installer_Test.php @@ -41,7 +41,7 @@ class Gallery_Installer_Test extends Unit_Test_Case { $this->assert_equal("Gallery", $root->title); $this->assert_equal(1, $root->left_ptr); $this->assert_equal($max_right_ptr, $root->right_ptr); - $this->assert_equal(null, $root->parent_id); + $this->assert_equal(0, $root->parent_id); $this->assert_equal(1, $root->level); } } -- cgit v1.2.3 From 06cabecd76781d7075cbacb5cf433042b4296dc5 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 18 Jan 2010 12:49:58 -0800 Subject: Coerce some integers to strings now that ORM isn't typecasting anymore. --- modules/gallery/tests/Gallery_Rest_Helper_Test.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) (limited to 'modules') diff --git a/modules/gallery/tests/Gallery_Rest_Helper_Test.php b/modules/gallery/tests/Gallery_Rest_Helper_Test.php index cd0aabae..f8cf6190 100644 --- a/modules/gallery/tests/Gallery_Rest_Helper_Test.php +++ b/modules/gallery/tests/Gallery_Rest_Helper_Test.php @@ -94,8 +94,8 @@ class Gallery_Rest_Helper_Test extends Unit_Test_Case { "path" => $photo->relative_url(), "thumb_url" => $photo->thumb_url(), "thumb_dimensions" => array( - "width" => $photo->thumb_width, - "height" => $photo->thumb_height), + "width" => (string)$photo->thumb_width, + "height" => (string)$photo->thumb_height), "has_thumb" => true, "title" => $photo->title))))), gallery_rest::get($request)); @@ -115,14 +115,14 @@ class Gallery_Rest_Helper_Test extends Unit_Test_Case { "parent_path" => $child->relative_url(), "title" => $photo->title, "thumb_url" => $photo->thumb_url(), - "thumb_size" => array("height" => $photo->thumb_height, - "width" => $photo->thumb_width), + "thumb_size" => array("height" => (string)$photo->thumb_height, + "width" => (string)$photo->thumb_width), "resize_url" => $photo->resize_url(), "resize_size" => array("height" => $photo->resize_height, "width" => $photo->resize_width), "url" => $photo->file_url(), - "size" => array("height" => $photo->height, - "width" => $photo->width), + "size" => array("height" => (string)$photo->height, + "width" => (string)$photo->width), "description" => $photo->description, "slug" => $photo->slug))), gallery_rest::get($request)); -- cgit v1.2.3 From 9384f987bb96d0d39787ff9d3d16a70c01cd76e0 Mon Sep 17 00:00:00 2001 From: Bharat Mediratta Date: Mon, 18 Jan 2010 12:52:52 -0800 Subject: Coerce some integers to strings now that ORM isn't typecasting anymore. --- modules/tag/tests/Tag_Rest_Helper_Test.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'modules') diff --git a/modules/tag/tests/Tag_Rest_Helper_Test.php b/modules/tag/tests/Tag_Rest_Helper_Test.php index 4e8dd527..514538d4 100644 --- a/modules/tag/tests/Tag_Rest_Helper_Test.php +++ b/modules/tag/tests/Tag_Rest_Helper_Test.php @@ -85,8 +85,8 @@ class Tag_Rest_Helper_Test extends Unit_Test_Case { $this->assert_equal( json_encode(array("status" => "OK", - "tags" => array(array("name" => "albums", "count" => 2), - array("name" => "photos", "count" => 2)))), + "tags" => array(array("name" => "albums", "count" => "2"), + array("name" => "photos", "count" => "2")))), tag_rest::get($request)); } -- cgit v1.2.3