From 3e6e9d2e06b25f0fecdf2d5cb1cf0ff80a83db53 Mon Sep 17 00:00:00 2001 From: Alan Hardman Date: Sun, 8 Aug 2021 12:12:08 -0600 Subject: [PATCH 1/4] Limiting cookie access to same-origin --- index.php | 1 + 1 file changed, 1 insertion(+) diff --git a/index.php b/index.php index 0f74e86c..78fc70b3 100644 --- a/index.php +++ b/index.php @@ -14,6 +14,7 @@ "FALLBACK" => "en", "CACHE" => true, "AUTOLOAD" => "app/;lib/vendor/", + "JAR.samesite" => "Strict", "PACKAGE" => "Phproject", "TZ" => "UTC", "microtime" => microtime(true), From c05563328060a3cca6f8463c9d3e6cf37fb3dc56 Mon Sep 17 00:00:00 2001 From: Alan Hardman Date: Fri, 10 Sep 2021 13:00:05 -0600 Subject: [PATCH 2/4] Adding CSRF token protection This adds a CSRF token to the session, and requires/validates that token for all web-based POST requests. It is sent by default as a header with each jQuery XHR and available as a tag in the templating engine for use in forms. --- app/controller.php | 8 ++++ app/controller/admin.php | 33 +++++++------ app/controller/backlog.php | 3 ++ app/controller/index.php | 12 ++++- app/controller/issues.php | 26 ++++++++-- app/controller/taskboard.php | 3 ++ app/controller/user.php | 3 ++ app/helper/security.php | 30 ++++++++++++ app/helper/template.php | 11 +++++ app/routes.ini | 27 +++++------ app/view/admin/config.html | 1 + app/view/admin/groups.html | 10 +++- app/view/admin/groups/edit.html | 16 +++++-- app/view/admin/index.html | 1 + app/view/admin/sprints/edit.html | 1 + app/view/admin/sprints/new.html | 1 + app/view/admin/users.html | 7 +-- app/view/admin/users/deleted.html | 16 +++++-- app/view/admin/users/edit.html | 1 + app/view/blocks/footer.html | 7 ++- app/view/blocks/head.html | 1 + app/view/blocks/issue-list/bulk-update.html | 1 + app/view/blocks/navbar-public.html | 1 + app/view/blocks/navbar.html | 12 ++++- app/view/index/login.html | 2 + app/view/index/reset.html | 1 + app/view/index/reset_complete.html | 1 + app/view/index/reset_forced.html | 1 + app/view/issues/edit-form.html | 1 + app/view/issues/single.html | 53 ++++++++++++++------- app/view/taskboard/index.html | 1 + app/view/user/account.html | 4 ++ app/view/user/dashboard-widget-modal.html | 1 + index.php | 6 +++ js/global.js | 7 +++ 35 files changed, 245 insertions(+), 65 deletions(-) create mode 100644 app/helper/template.php diff --git a/app/controller.php b/app/controller.php index 0c77aaa9..3a841e9c 100644 --- a/app/controller.php +++ b/app/controller.php @@ -91,4 +91,12 @@ public function now($time = true) { return $time ? date("Y-m-d H:i:s") : date("Y-m-d"); } + + /** + * Validate the request's CSRF token, exiting if invalid + */ + protected function validateCsrf() + { + \Helper\Security::instance()->validateCsrfToken(); + } } diff --git a/app/controller/admin.php b/app/controller/admin.php index 612743f4..406ada7f 100644 --- a/app/controller/admin.php +++ b/app/controller/admin.php @@ -22,6 +22,8 @@ public function index(\Base $f3) $f3->set("menuitem", "admin"); if ($f3->get("POST.action") == "clearcache") { + $this->validateCsrf(); + $cache = \Cache::instance(); // Clear configured cache @@ -153,6 +155,7 @@ public function config(\Base $f3) */ public function config_post_saveattribute(\Base $f3) { + $this->validateCsrf(); $this->_requireAdmin(\Model\User::RANK_SUPER); $attribute = str_replace("-", ".", $f3->get("POST.attribute")); @@ -304,6 +307,7 @@ public function user_new(\Base $f3) */ public function user_save(\Base $f3) { + $this->validateCsrf(); $security = \Helper\Security::instance(); $user = new \Model\User(); $user_id = $f3->get("POST.user_id"); @@ -399,6 +403,7 @@ public function user_save(\Base $f3) */ public function user_delete(\Base $f3, array $params) { + $this->validateCsrf(); $user = new \Model\User(); $user->load($params["id"]); if (!$user->id) { @@ -434,6 +439,7 @@ public function user_delete(\Base $f3, array $params) */ public function user_undelete(\Base $f3, array $params) { + $this->validateCsrf(); $user = new \Model\User(); $user->load($params["id"]); if (!$user->id) { @@ -485,20 +491,15 @@ public function groups(\Base $f3) */ public function group_new(\Base $f3) { - $f3->set("title", $f3->get("dict.groups")); - - if ($f3->get("POST")) { - $group = new \Model\User(); - $group->name = $f3->get("POST.name"); - $group->username = \Web::instance()->slug($group->name); - $group->role = "group"; - $group->task_color = sprintf("%02X%02X%02X", mt_rand(0, 0xFF), mt_rand(0, 0xFF), mt_rand(0, 0xFF)); - $group->created_date = $this->now(); - $group->save(); - $f3->reroute("/admin/groups"); - } else { - $f3->error(405); - } + $this->validateCsrf(); + $group = new \Model\User(); + $group->name = $f3->get("POST.name"); + $group->username = \Web::instance()->slug($group->name); + $group->role = "group"; + $group->task_color = sprintf("%02X%02X%02X", mt_rand(0, 0xFF), mt_rand(0, 0xFF), mt_rand(0, 0xFF)); + $group->created_date = $this->now(); + $group->save(); + $f3->reroute("/admin/groups"); } /** @@ -532,6 +533,7 @@ public function group_edit(\Base $f3, array $params) */ public function group_delete(\Base $f3, array $params) { + $this->validateCsrf(); $group = new \Model\User(); $group->load($params["id"]); $group->delete(); @@ -549,6 +551,7 @@ public function group_delete(\Base $f3, array $params) */ public function group_ajax(\Base $f3) { + $this->validateCsrf(); if (!$f3->get("AJAX")) { $f3->error(400); } @@ -608,6 +611,7 @@ public function group_ajax(\Base $f3) */ public function group_setmanager(\Base $f3, array $params) { + $this->validateCsrf(); $db = $f3->get("db.instance"); $group = new \Model\User(); @@ -645,6 +649,7 @@ public function sprints(\Base $f3) */ public function sprint_new(\Base $f3) { + $this->validateCsrf(); $f3->set("title", $f3->get("dict.sprints")); if ($post = $f3->get("POST")) { diff --git a/app/controller/backlog.php b/app/controller/backlog.php index dbf2b56c..935af134 100644 --- a/app/controller/backlog.php +++ b/app/controller/backlog.php @@ -141,6 +141,8 @@ public function redirect(\Base $f3) */ public function edit($f3) { + $this->validateCsrf(); + // Move project $post = $f3->get("POST"); $issue = new \Model\Issue(); @@ -166,6 +168,7 @@ public function edit($f3) */ public function sort($f3) { + $this->validateCsrf(); $this->_requireLogin(\Model\User::RANK_MANAGER); $backlog = new \Model\Issue\Backlog(); if ($f3->get("POST.sprint_id")) { diff --git a/app/controller/index.php b/app/controller/index.php index 0728e3a1..a515fbd8 100644 --- a/app/controller/index.php +++ b/app/controller/index.php @@ -68,6 +68,7 @@ public function login($f3) */ public function loginpost($f3) { + $this->validateCsrf(); $user = new \Model\User(); // Load user by username or email address @@ -111,6 +112,7 @@ public function loginpost($f3) */ public function registerpost($f3) { + $this->validateCsrf(); // Exit immediately if public registrations are disabled if (!$f3->get("site.public_registration")) { @@ -184,6 +186,7 @@ public function reset($f3) $f3->reroute("/"); } else { if ($f3->get("POST.email")) { + $this->validateCsrf(); $user = new \Model\User(); $user->load(array("email = ?", $f3->get("POST.email"))); if ($user->id && !$user->deleted_date) { @@ -233,6 +236,8 @@ public function reset_complete($f3, $params) } if ($f3->get("POST.password1")) { + $this->validateCsrf(); + // Validate new password if ($f3->get("POST.password1") != $f3->get("POST.password2")) { $f3->set("reset.error", "The given passwords don't match."); @@ -263,6 +268,10 @@ public function reset_forced($f3) $user = new \Model\User(); $user->loadCurrent(); + if ($f3->get('POST')) { + $this->validateCsrf(); + } + if ($f3->get("POST.password1") != $f3->get("POST.password2")) { $f3->set("reset.error", "The given passwords don't match."); } elseif (strlen($f3->get("POST.password1")) < 6) { @@ -280,12 +289,13 @@ public function reset_forced($f3) } /** - * GET|POST /logout + * POST /logout * * @param \Base $f3 */ public function logout($f3) { + $this->validateCsrf(); $session = new \Model\Session(); $session->loadCurrent(); $session->delete(); diff --git a/app/controller/issues.php b/app/controller/issues.php index df8d00dc..1c1471c4 100644 --- a/app/controller/issues.php +++ b/app/controller/issues.php @@ -239,6 +239,7 @@ public function index($f3) */ public function bulk_update($f3) { + $this->validateCsrf(); $post = $f3->get("POST"); $issue = new \Model\Issue(); @@ -526,7 +527,7 @@ protected function loadIssueMeta(\Model\Issue $issue) } /** - * GET /issues/close/@id + * POST /issues/close/@id * Close an issue * * @param \Base $f3 @@ -535,6 +536,7 @@ protected function loadIssueMeta(\Model\Issue $issue) */ public function close($f3, $params) { + $this->validateCsrf(); $issue = new \Model\Issue(); $issue->load($params["id"]); @@ -554,7 +556,7 @@ public function close($f3, $params) } /** - * GET /issues/reopen/@id + * POST /issues/reopen/@id * Reopen an issue * * @param \Base $f3 @@ -563,6 +565,7 @@ public function close($f3, $params) */ public function reopen($f3, $params) { + $this->validateCsrf(); $issue = new \Model\Issue(); $issue->load($params["id"]); @@ -588,7 +591,7 @@ public function reopen($f3, $params) } /** - * GET /issues/copy/@id + * POST /issues/copy/@id * Copy an issue * * @param \Base $f3 @@ -597,6 +600,7 @@ public function reopen($f3, $params) */ public function copy($f3, $params) { + $this->validateCsrf(); $issue = new \Model\Issue(); $issue->load($params["id"]); @@ -760,6 +764,7 @@ protected function _saveNew() */ public function save($f3) { + $this->validateCsrf(); if ($f3->get("POST.id")) { // Updating existing issue. $issue = $this->_saveUpdate(); @@ -849,6 +854,8 @@ public function single($f3, $params) */ public function add_watcher($f3, $params) { + $this->validateCsrf(); + $issue = new \Model\Issue(); $issue->load(array("id=?", $params["id"])); if (!$issue->id) { @@ -875,6 +882,8 @@ public function add_watcher($f3, $params) */ public function delete_watcher($f3, $params) { + $this->validateCsrf(); + $issue = new \Model\Issue(); $issue->load(array("id=?", $params["id"])); if (!$issue->id) { @@ -896,6 +905,8 @@ public function delete_watcher($f3, $params) */ public function add_dependency($f3, $params) { + $this->validateCsrf(); + $issue = new \Model\Issue(); $issue->load(array("id=?", $params["id"])); if (!$issue->id) { @@ -921,6 +932,8 @@ public function add_dependency($f3, $params) */ public function delete_dependency($f3, $params) { + $this->validateCsrf(); + $issue = new \Model\Issue(); $issue->load(array("id=?", $params["id"])); if (!$issue->id) { @@ -1060,6 +1073,7 @@ public function single_dependencies($f3, $params) */ public function single_delete($f3, $params) { + $this->validateCsrf(); $issue = new \Model\Issue(); $issue->load($params["id"]); $user = $f3->get("user_obj"); @@ -1081,6 +1095,7 @@ public function single_delete($f3, $params) */ public function single_undelete($f3, $params) { + $this->validateCsrf(); $issue = new \Model\Issue(); $issue->load($params["id"]); $user = $f3->get("user_obj"); @@ -1101,6 +1116,7 @@ public function single_undelete($f3, $params) */ public function comment_save($f3) { + $this->validateCsrf(); $post = $f3->get("POST"); $issue = new \Model\Issue(); @@ -1152,6 +1168,7 @@ public function comment_save($f3) */ public function comment_delete($f3) { + $this->validateCsrf(); $this->_requireAdmin(); $comment = new \Model\Issue\Comment(); $comment->load($f3->get("POST.id")); @@ -1168,6 +1185,7 @@ public function comment_delete($f3) */ public function file_delete($f3) { + $this->validateCsrf(); $file = new \Model\Issue\File(); $file->load($f3->get("POST.id")); $file->delete(); @@ -1183,6 +1201,7 @@ public function file_delete($f3) */ public function file_undelete($f3) { + $this->validateCsrf(); $file = new \Model\Issue\File(); $file->load($f3->get("POST.id")); $file->deleted_date = null; @@ -1289,6 +1308,7 @@ public function search($f3) */ public function upload($f3) { + $this->validateCsrf(); $user_id = $this->_userId; $issue = new \Model\Issue(); diff --git a/app/controller/taskboard.php b/app/controller/taskboard.php index b13a0999..c68d998f 100644 --- a/app/controller/taskboard.php +++ b/app/controller/taskboard.php @@ -305,6 +305,7 @@ public function burndown($f3, $params) */ public function saveManHours($f3) { + $this->validateCsrf(); $user = new \Model\User(); $user->load(array("id = ?", $f3->get("POST.user_id"))); if (!$user->id) { @@ -324,6 +325,7 @@ public function saveManHours($f3) */ public function add($f3) { + $this->validateCsrf(); $post = $f3->get("POST"); $post['sprint_id'] = $post['sprintId']; $post['name'] = $post['title']; @@ -341,6 +343,7 @@ public function add($f3) */ public function edit($f3) { + $this->validateCsrf(); $post = $f3->get("POST"); $issue = new \Model\Issue(); $issue->load($post["taskId"]); diff --git a/app/controller/user.php b/app/controller/user.php index 8be269fe..cac72afc 100644 --- a/app/controller/user.php +++ b/app/controller/user.php @@ -82,6 +82,7 @@ public function dashboard($f3) */ public function dashboardPost($f3) { + $this->validateCsrf(); $helper = \Helper\Dashboard::instance(); $widgets = json_decode($f3->get("POST.widgets")); $allWidgets = $helper->allWidgets; @@ -145,6 +146,7 @@ public function account($f3) */ public function save($f3) { + $this->validateCsrf(); $f3 = \Base::instance(); $post = array_map("trim", $f3->get("POST")); @@ -234,6 +236,7 @@ public function save($f3) */ public function avatar($f3) { + $this->validateCsrf(); $f3 = \Base::instance(); $user = new \Model\User(); diff --git a/app/helper/security.php b/app/helper/security.php index 43ab67d2..51aa3805 100644 --- a/app/helper/security.php +++ b/app/helper/security.php @@ -113,6 +113,36 @@ public function updateDatabase($version) } } + /** + * Initialize a CSRF token + */ + public function initCsrfToken() + { + $f3 = \Base::instance(); + if (!($token = $f3->get('COOKIE.XSRF-TOKEN'))) { + $token = $this->salt_sha2(); + $f3->set('COOKIE.XSRF-TOKEN', $token); + } + $f3->set('csrf_token', $token); + } + + /** + * Validate a CSRF token, exiting if invalid + */ + public function validateCsrfToken() + { + $f3 = \Base::instance(); + $cookieToken = $f3->get('COOKIE.XSRF-TOKEN'); + $requestToken = $f3->get('POST.csrf-token'); + if (!$requestToken) { + $requestToken = $f3->get('HEADERS.X-Xsrf-Token'); + } + if (!$cookieToken || !$requestToken || !hash_equals($cookieToken, $requestToken)) { + $f3->error(400, 'Invalid CSRF token'); + exit; + } + } + /** * Check if two hashes are equal, safe against timing attacks * diff --git a/app/helper/template.php b/app/helper/template.php new file mode 100644 index 00000000..d6d12112 --- /dev/null +++ b/app/helper/template.php @@ -0,0 +1,11 @@ +esc($csrf_token) ?>" />'; + } +} diff --git a/app/routes.ini b/app/routes.ini index 54c24e8a..032723c5 100644 --- a/app/routes.ini +++ b/app/routes.ini @@ -8,8 +8,7 @@ GET|POST /reset/forced = Controller\Index->reset_forced GET|POST /reset/@token = Controller\Index->reset_complete POST /login = Controller\Index->loginpost POST /register = Controller\Index->registerpost -GET|POST /logout = Controller\Index->logout -GET|POST /ping = Controller\Index->ping +POST /logout = Controller\Index->logout GET /opensearch.xml = Controller\Index->opensearch ; Issues @@ -21,9 +20,9 @@ GET /issues/edit/@id = Controller\Issues->edit POST /issues/save = Controller\Issues->save POST /issues/bulk_update = Controller\Issues->bulk_update GET /issues/export = Controller\Issues->export -GET|POST /issues/@id = Controller\Issues->single -GET|POST /issues/delete/@id = Controller\Issues->single_delete -GET|POST /issues/undelete/@id = Controller\Issues->single_undelete +GET /issues/@id = Controller\Issues->single +POST /issues/delete/@id = Controller\Issues->single_delete +POST /issues/undelete/@id = Controller\Issues->single_undelete POST /issues/comment/save = Controller\Issues->comment_save POST /issues/comment/delete = Controller\Issues->comment_delete POST /issues/file/delete = Controller\Issues->file_delete @@ -40,9 +39,9 @@ GET /issues/project/@id = Controller\Issues\Project->overview GET /issues/project/@id/files = Controller\Issues\Project->files GET /search = Controller\Issues->search POST /issues/upload = Controller\Issues->upload -GET /issues/close/@id = Controller\Issues->close -GET /issues/reopen/@id = Controller\Issues->reopen -GET /issues/copy/@id = Controller\Issues->copy +POST /issues/close/@id = Controller\Issues->close +POST /issues/reopen/@id = Controller\Issues->reopen +POST /issues/copy/@id = Controller\Issues->copy GET /issues/parent_ajax = Controller\Issues->parent_ajax ; Tags @@ -74,19 +73,15 @@ GET /admin/users/deleted = Controller\Admin->deleted_users GET /admin/users/new = Controller\Admin->user_new POST /admin/users/save = Controller\Admin->user_save GET /admin/users/@id = Controller\Admin->user_edit -GET|POST /admin/users/@id/delete = Controller\Admin->user_delete -GET|POST /admin/users/@id/undelete = Controller\Admin->user_undelete +POST /admin/users/@id/delete = Controller\Admin->user_delete +POST /admin/users/@id/undelete = Controller\Admin->user_undelete POST /admin/groups/new = Controller\Admin->group_new -GET|POST /admin/groups/@id = Controller\Admin->group_edit -GET|POST /admin/groups/@id/delete = Controller\Admin->group_delete +GET /admin/groups/@id = Controller\Admin->group_edit +POST /admin/groups/@id/delete = Controller\Admin->group_delete POST /admin/groups/ajax = Controller\Admin->group_ajax GET|POST /admin/groups/@id/setmanager/@user_group_id = Controller\Admin->group_setmanager -GET|POST /admin/attributes/new = Controller\Admin->attribute_new -GET|POST /admin/attributes/@id = Controller\Admin->attribute_edit -GET|POST /admin/attributes/@id/delete = Controller\Admin->attribute_delete - GET|POST /admin/sprints/new = Controller\Admin->sprint_new GET|POST /admin/sprints/@id = Controller\Admin->sprint_edit diff --git a/app/view/admin/config.html b/app/view/admin/config.html index 7a1040ec..97521ac1 100644 --- a/app/view/admin/config.html +++ b/app/view/admin/config.html @@ -8,6 +8,7 @@
+
diff --git a/app/view/admin/groups.html b/app/view/admin/groups.html index e560ef64..91b3fd50 100644 --- a/app/view/admin/groups.html +++ b/app/view/admin/groups.html @@ -32,7 +32,14 @@ {{ @group.name | esc }} {{ @group.count }}   #{{ @group.task_color }} - + + + + + + @@ -52,6 +59,7 @@