From 17254799d1069e9f67de14460264cda76395746f Mon Sep 17 00:00:00 2001 From: Andy Staudacher Date: Tue, 15 Sep 2009 20:27:04 -0700 Subject: Initial skeleton of Controller_Auth code audit test (non functional). --- modules/gallery/tests/controller_auth_data.txt | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 modules/gallery/tests/controller_auth_data.txt (limited to 'modules/gallery/tests/controller_auth_data.txt') diff --git a/modules/gallery/tests/controller_auth_data.txt b/modules/gallery/tests/controller_auth_data.txt new file mode 100644 index 00000000..e69de29b -- cgit v1.2.3 From e168e0dfae28bb56289b4debae8825c104ee69f9 Mon Sep 17 00:00:00 2001 From: Andy Staudacher Date: Tue, 15 Sep 2009 21:50:48 -0700 Subject: CSRF / auth fixes, golden data file checkpoint --- modules/gallery/controllers/permissions.php | 2 +- modules/gallery/controllers/simple_uploader.php | 1 + modules/gallery/tests/Controller_Auth_Test.php | 8 ++++++-- modules/gallery/tests/controller_auth_data.txt | 17 +++++++++++++++++ modules/tag/controllers/tags.php | 2 ++ modules/user/controllers/admin_users.php | 2 +- 6 files changed, 28 insertions(+), 4 deletions(-) (limited to 'modules/gallery/tests/controller_auth_data.txt') diff --git a/modules/gallery/controllers/permissions.php b/modules/gallery/controllers/permissions.php index 5f4620b2..8d75862e 100644 --- a/modules/gallery/controllers/permissions.php +++ b/modules/gallery/controllers/permissions.php @@ -81,7 +81,7 @@ class Permissions_Controller extends Controller { } } - function _get_form($item) { + private function _get_form($item) { $view = new View("permissions_form.html"); $view->item = $item; $view->groups = ORM::factory("group")->find_all(); diff --git a/modules/gallery/controllers/simple_uploader.php b/modules/gallery/controllers/simple_uploader.php index 156d18ac..bc508319 100644 --- a/modules/gallery/controllers/simple_uploader.php +++ b/modules/gallery/controllers/simple_uploader.php @@ -32,6 +32,7 @@ class Simple_Uploader_Controller extends Controller { } public function start() { + access::verify_csrf(); batch::start(); } diff --git a/modules/gallery/tests/Controller_Auth_Test.php b/modules/gallery/tests/Controller_Auth_Test.php index e3eb4eaf..50afae8f 100644 --- a/modules/gallery/tests/Controller_Auth_Test.php +++ b/modules/gallery/tests/Controller_Auth_Test.php @@ -21,6 +21,10 @@ class Controller_Auth_Test extends Unit_Test_Case { public function find_missing_auth_test() { $found = array(); foreach (glob("*/*/controllers/*.php") as $controller) { + if (strpos($controller, "modules/unit_test/") !== false) { + continue; + } + // List of all tokens without whitespace, simplifying parsing. $tokens = array(); foreach (token_get_all(file_get_contents($controller)) as $token) { @@ -150,12 +154,12 @@ class Controller_Auth_Test extends Unit_Test_Case { continue; } - fprintf($fd, "%-60s %-20s %-21s\n", + fprintf($fd, "%-60s %-20s %s\n", $controller, $function->name, implode("|", $flags)); } if (strpos(basename($controller), "admin_") === 0 && !$is_admin_controller) { - fprintf($fd, "%-60s %-20s %-21s\n", + fprintf($fd, "%-60s %-20s %s\n", $controller, basename($controller), "NO_ADMIN_CONTROLLER"); } } diff --git a/modules/gallery/tests/controller_auth_data.txt b/modules/gallery/tests/controller_auth_data.txt index e69de29b..aabd2863 100644 --- a/modules/gallery/tests/controller_auth_data.txt +++ b/modules/gallery/tests/controller_auth_data.txt @@ -0,0 +1,17 @@ +modules/comment/controllers/admin_comments.php queue DIRTY_CSRF +modules/digibug/controllers/digibug.php close_window DIRTY_AUTH +modules/gallery/controllers/combined.php javascript DIRTY_AUTH +modules/gallery/controllers/combined.php css DIRTY_AUTH +modules/gallery/controllers/maintenance.php index DIRTY_AUTH +modules/gallery/controllers/rest.php form_add DIRTY_AUTH +modules/gallery/controllers/rest.php _index DIRTY_AUTH +modules/gallery/controllers/rest.php _create DIRTY_AUTH +modules/gallery/controllers/rest.php _show DIRTY_AUTH +modules/gallery/controllers/rest.php _update DIRTY_AUTH +modules/gallery/controllers/rest.php _delete DIRTY_AUTH +modules/gallery/controllers/rest.php _form_add DIRTY_AUTH +modules/gallery/controllers/rest.php _form_edit DIRTY_AUTH +modules/gallery/controllers/simple_uploader.php start DIRTY_AUTH +modules/gallery/controllers/simple_uploader.php finish DIRTY_AUTH +modules/user/controllers/login.php ajax DIRTY_AUTH +modules/user/controllers/login.php html DIRTY_AUTH diff --git a/modules/tag/controllers/tags.php b/modules/tag/controllers/tags.php index c993e374..f4f98090 100644 --- a/modules/tag/controllers/tags.php +++ b/modules/tag/controllers/tags.php @@ -43,6 +43,8 @@ class Tags_Controller extends REST_Controller { } public function _index() { + // Far from perfection, but at least require view permission for the root album + access::required("view", 1); print tag::cloud(30); } diff --git a/modules/user/controllers/admin_users.php b/modules/user/controllers/admin_users.php index 521f82fa..0b748955 100644 --- a/modules/user/controllers/admin_users.php +++ b/modules/user/controllers/admin_users.php @@ -17,7 +17,7 @@ * along with this program; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street - Fifth Floor, Boston, MA 02110-1301, USA. */ -class Admin_Users_Controller extends Controller { +class Admin_Users_Controller extends Admin_Controller { public function index() { $view = new Admin_View("admin.html"); $view->content = new View("admin_users.html"); -- cgit v1.2.3 From 7608870537503ec571f45a175c8486d7945e7c63 Mon Sep 17 00:00:00 2001 From: Andy Staudacher Date: Tue, 15 Sep 2009 22:51:49 -0700 Subject: Controller auth / CSRF fixes --- modules/gallery/tests/Controller_Auth_Test.php | 19 +++++++++++-------- modules/gallery/tests/controller_auth_data.txt | 22 ++++++++++++++++++++++ modules/organize/controllers/organize.php | 4 ++++ modules/rss/controllers/rss.php | 4 ++-- modules/tag/controllers/tags.php | 3 ++- 5 files changed, 41 insertions(+), 11 deletions(-) (limited to 'modules/gallery/tests/controller_auth_data.txt') diff --git a/modules/gallery/tests/Controller_Auth_Test.php b/modules/gallery/tests/Controller_Auth_Test.php index c4dc915b..cd4abe07 100644 --- a/modules/gallery/tests/Controller_Auth_Test.php +++ b/modules/gallery/tests/Controller_Auth_Test.php @@ -26,7 +26,7 @@ class Controller_Auth_Test extends Unit_Test_Case { public function find_missing_auth_test() { $found = array(); foreach (glob("*/*/controllers/*.php") as $controller) { - if (strpos($controller, "modules/unit_test/") !== false) { + if (preg_match("{modules/(gallery_)?unit_test/}", $controller)) { continue; } @@ -54,11 +54,11 @@ class Controller_Auth_Test extends Unit_Test_Case { $open_braces--; if ($open_braces == 1 && $function) { $found[$controller][] = $function; + $function = null; } else if ($open_braces == 0) { $is_admin_controller = false; $is_rest_controller = false; } - $function = null; } else if ($token == "{") { $open_braces++; } @@ -80,6 +80,7 @@ class Controller_Auth_Test extends Unit_Test_Case { $previous_2 = $tokens[$token_number - 2][0]; $is_private = in_array($previous, array(T_PRIVATE, T_PROTECTED)) || in_array($previous_2, array(T_PRIVATE, T_PROTECTED)); + $is_static = $previous == T_STATIC || $previous_2 == T_STATIC; // Search forward to get function name do { @@ -91,7 +92,9 @@ class Controller_Auth_Test extends Unit_Test_Case { } } while ($token_number < count($tokens)); - if (!$is_private || ($is_rest_controller && in_array($name, self::$rest_methods))) { + if (!$is_static && + (!$is_private || + ($is_rest_controller && in_array($name, self::$rest_methods)))) { $function = self::_function($name, $line, $is_admin_controller); if ($is_rest_controller && in_array($name, self::$rest_methods_with_csrf_check)) { $function->checks_csrf(true); @@ -110,7 +113,8 @@ class Controller_Auth_Test extends Unit_Test_Case { if ($token[0] == T_STRING) { if ($token[1] == "access" && self::_token_matches(array(T_DOUBLE_COLON, "::"), $tokens, $token_number + 1) && - self::_token_matches(array(T_STRING, "required"), $tokens, $token_number + 2) && + self::_token_matches(array(T_STRING), $tokens, $token_number + 2) && + in_array($tokens[$token_number + 2][1], array("forbidden", "required")) && self::_token_matches("(", $tokens, $token_number + 3)) { $token_number += 3; $function->checks_authorization(true); @@ -125,7 +129,7 @@ class Controller_Auth_Test extends Unit_Test_Case { $token_number++; $function->uses_input(true); } - } else if ($token == T_VARIABLE) { + } else if ($token[0] == T_VARIABLE) { if ($token[1] == '$this' && self::_token_matches(array(T_OBJECT_OPERATOR), $tokens, $token_number + 1) && self::_token_matches(array(T_STRING, "input"), $tokens, $token_number + 2)) { @@ -152,7 +156,6 @@ class Controller_Auth_Test extends Unit_Test_Case { $is_admin_controller = true; foreach ($functions as $function) { $is_admin_controller &= $function->is_admin_controller; - $flags = array(); if ($function->uses_input() && !$function->checks_csrf()) { $flags[] = "DIRTY_CSRF"; @@ -224,14 +227,14 @@ class Controller_Auth_Test_Function { function uses_input($val=null) { if ($val !== null) { - $this->_uses_input = $val; + $this->_uses_input = (bool) $val; } return $this->_uses_input; } function checks_authorization($val=null) { if ($val !== null) { - $this->_checks_authorization = $val; + $this->_checks_authorization = (bool) $val; } return $this->_checks_authorization; } diff --git a/modules/gallery/tests/controller_auth_data.txt b/modules/gallery/tests/controller_auth_data.txt index aabd2863..fcb977e4 100644 --- a/modules/gallery/tests/controller_auth_data.txt +++ b/modules/gallery/tests/controller_auth_data.txt @@ -1,8 +1,17 @@ modules/comment/controllers/admin_comments.php queue DIRTY_CSRF +modules/comment/controllers/comments.php _index DIRTY_CSRF +modules/digibug/controllers/digibug.php print_proxy DIRTY_CSRF|DIRTY_AUTH modules/digibug/controllers/digibug.php close_window DIRTY_AUTH +modules/gallery/controllers/admin.php __call DIRTY_AUTH +modules/gallery/controllers/albums.php _show DIRTY_CSRF +modules/gallery/controllers/albums.php _form_add DIRTY_CSRF modules/gallery/controllers/combined.php javascript DIRTY_AUTH modules/gallery/controllers/combined.php css DIRTY_AUTH +modules/gallery/controllers/file_proxy.php __call DIRTY_CSRF|DIRTY_AUTH modules/gallery/controllers/maintenance.php index DIRTY_AUTH +modules/gallery/controllers/rest.php __construct DIRTY_AUTH +modules/gallery/controllers/rest.php __call DIRTY_AUTH +modules/gallery/controllers/rest.php form_edit DIRTY_AUTH modules/gallery/controllers/rest.php form_add DIRTY_AUTH modules/gallery/controllers/rest.php _index DIRTY_AUTH modules/gallery/controllers/rest.php _create DIRTY_AUTH @@ -13,5 +22,18 @@ modules/gallery/controllers/rest.php _form_add modules/gallery/controllers/rest.php _form_edit DIRTY_AUTH modules/gallery/controllers/simple_uploader.php start DIRTY_AUTH modules/gallery/controllers/simple_uploader.php finish DIRTY_AUTH +modules/gallery/controllers/upgrader.php index DIRTY_AUTH +modules/gallery/controllers/welcome_message.php index DIRTY_AUTH +modules/rss/controllers/rss.php feed DIRTY_CSRF|DIRTY_AUTH +modules/search/controllers/search.php index DIRTY_CSRF|DIRTY_AUTH +modules/server_add/controllers/admin_server_add.php autocomplete DIRTY_CSRF +modules/server_add/controllers/server_add.php children DIRTY_CSRF +modules/tag/controllers/admin_tags.php index DIRTY_CSRF +modules/tag/controllers/tags.php _show DIRTY_CSRF|DIRTY_AUTH modules/user/controllers/login.php ajax DIRTY_AUTH +modules/user/controllers/login.php auth_ajax DIRTY_AUTH modules/user/controllers/login.php html DIRTY_AUTH +modules/user/controllers/login.php auth_html DIRTY_AUTH +modules/user/controllers/logout.php index DIRTY_CSRF|DIRTY_AUTH +modules/user/controllers/password.php reset DIRTY_AUTH +modules/user/controllers/password.php do_reset DIRTY_CSRF|DIRTY_AUTH diff --git a/modules/organize/controllers/organize.php b/modules/organize/controllers/organize.php index 259c94e7..08c80de3 100644 --- a/modules/organize/controllers/organize.php +++ b/modules/organize/controllers/organize.php @@ -45,9 +45,13 @@ class Organize_Controller extends Controller { access::verify_csrf(); $target_album = ORM::factory("item", $target_album_id); + access::required("view", $target_album); + access::required("add", $target_album); + foreach ($this->input->post("source_ids") as $source_id) { $source = ORM::factory("item", $source_id); if (!$source->contains($target_album)) { + access::required("edit", $source); item::move($source, $target_album); } } diff --git a/modules/rss/controllers/rss.php b/modules/rss/controllers/rss.php index b89bed40..015d6032 100644 --- a/modules/rss/controllers/rss.php +++ b/modules/rss/controllers/rss.php @@ -21,13 +21,13 @@ class Rss_Controller extends Controller { public static $page_size = 20; public function feed($module_id, $feed_id, $id=null) { - $page = $this->input->get("page", 1); + $page = (int) $this->input->get("page", 1); if ($page < 1) { url::redirect(url::merge(array("page" => 1))); } // Configurable page size between 1 and 100, default 20 - $page_size = max(1, min(100, $this->input->get("page_size", self::$page_size))); + $page_size = max(1, min(100, (int) $this->input->get("page_size", self::$page_size))); // Run the appropriate feed callback if (module::is_active($module_id)) { diff --git a/modules/tag/controllers/tags.php b/modules/tag/controllers/tags.php index f4f98090..b9f2c61c 100644 --- a/modules/tag/controllers/tags.php +++ b/modules/tag/controllers/tags.php @@ -44,7 +44,8 @@ class Tags_Controller extends REST_Controller { public function _index() { // Far from perfection, but at least require view permission for the root album - access::required("view", 1); + $album = ORM::factory("item", 1); + access::required("view", $album); print tag::cloud(30); } -- cgit v1.2.3 From 39632c4689842b3e3bb0715c0e9be757149c257d Mon Sep 17 00:00:00 2001 From: Andy Staudacher Date: Tue, 15 Sep 2009 23:01:26 -0700 Subject: Also check for rss feeds in controller auth check --- modules/gallery/tests/Controller_Auth_Test.php | 8 ++++++-- modules/gallery/tests/controller_auth_data.txt | 1 + 2 files changed, 7 insertions(+), 2 deletions(-) (limited to 'modules/gallery/tests/controller_auth_data.txt') diff --git a/modules/gallery/tests/Controller_Auth_Test.php b/modules/gallery/tests/Controller_Auth_Test.php index cd4abe07..caf6d8f2 100644 --- a/modules/gallery/tests/Controller_Auth_Test.php +++ b/modules/gallery/tests/Controller_Auth_Test.php @@ -25,7 +25,9 @@ class Controller_Auth_Test extends Unit_Test_Case { public function find_missing_auth_test() { $found = array(); - foreach (glob("*/*/controllers/*.php") as $controller) { + $controllers = glob("*/*/controllers/*.php"); + $feeds = glob("*/*/helpers/*_rss.php"); + foreach (array_merge($controllers, $feeds) as $controller) { if (preg_match("{modules/(gallery_)?unit_test/}", $controller)) { continue; } @@ -92,7 +94,9 @@ class Controller_Auth_Test extends Unit_Test_Case { } } while ($token_number < count($tokens)); - if (!$is_static && + $is_rss_feed = $name == "feed" && strpos(basename($controller), "_rss.php"); + + if ((!$is_static || $is_rss_feed) && (!$is_private || ($is_rest_controller && in_array($name, self::$rest_methods)))) { $function = self::_function($name, $line, $is_admin_controller); diff --git a/modules/gallery/tests/controller_auth_data.txt b/modules/gallery/tests/controller_auth_data.txt index fcb977e4..fdf00c5e 100644 --- a/modules/gallery/tests/controller_auth_data.txt +++ b/modules/gallery/tests/controller_auth_data.txt @@ -1,5 +1,6 @@ modules/comment/controllers/admin_comments.php queue DIRTY_CSRF modules/comment/controllers/comments.php _index DIRTY_CSRF +modules/comment/helpers/comment_rss.php feed DIRTY_AUTH modules/digibug/controllers/digibug.php print_proxy DIRTY_CSRF|DIRTY_AUTH modules/digibug/controllers/digibug.php close_window DIRTY_AUTH modules/gallery/controllers/admin.php __call DIRTY_AUTH -- cgit v1.2.3