From 71123b0f378425229db6911984a90f5ee9953420 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Guillot?= Date: Sat, 5 Jun 2021 14:33:19 -0700 Subject: [PATCH] Add missing CSRF checks --- SECURITY.md | 2 ++ app/Controller/BaseController.php | 7 +++++++ app/Controller/BoardAjaxController.php | 1 + app/Controller/ColumnController.php | 1 + app/Controller/CommentController.php | 1 + app/Controller/ProjectViewController.php | 6 +++++- app/Controller/SubtaskStatusController.php | 1 + app/Controller/SwimlaneController.php | 1 + app/Controller/TaskListController.php | 9 ++++++++- app/Controller/TaskModificationController.php | 2 ++ app/Controller/TaskMovePositionController.php | 1 + app/Controller/WebNotificationController.php | 2 ++ app/Core/Http/Request.php | 10 ++++++++++ app/Helper/SubtaskHelper.php | 1 + app/Template/board/table_container.php | 6 +++--- app/Template/column/index.php | 2 +- app/Template/swimlane/table.php | 2 +- app/Template/task/details.php | 4 ++-- app/Template/task/dropdown.php | 4 ++-- app/Template/task_comments/show.php | 2 +- app/Template/task_list/header.php | 4 ++-- app/Template/task_move_position/show.php | 2 +- app/Template/web_notification/show.php | 4 ++-- 23 files changed, 58 insertions(+), 17 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index e4850537c..f0bc6b42e 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -9,3 +9,5 @@ Only the latest stable version is supported. Do not open a new GitHub issue if the bug is a security vulnerability. Send an email to `security AT kanboard DOT net` with all the steps to reproduce the problem. + +**This software is in maintenance mode**. Low severity or harmless issues won't be fixed. diff --git a/app/Controller/BaseController.php b/app/Controller/BaseController.php index 49f07e6b9..c0a51c8da 100644 --- a/app/Controller/BaseController.php +++ b/app/Controller/BaseController.php @@ -33,6 +33,13 @@ abstract class BaseController extends Base } } + protected function checkReusableGETCSRFParam() + { + if (! $this->token->validateReusableCSRFToken($this->request->getStringParam('csrf_token'))) { + throw new AccessForbiddenException(); + } + } + protected function checkCSRFForm() { if (! $this->token->validateCSRFToken($this->request->getRawValue('csrf_token'))) { diff --git a/app/Controller/BoardAjaxController.php b/app/Controller/BoardAjaxController.php index 323743940..3f726b6f7 100644 --- a/app/Controller/BoardAjaxController.php +++ b/app/Controller/BoardAjaxController.php @@ -21,6 +21,7 @@ class BoardAjaxController extends BaseController */ public function save() { + $this->checkReusableGETCSRFParam(); $project_id = $this->request->getIntegerParam('project_id'); if (! $project_id || ! $this->request->isAjax()) { diff --git a/app/Controller/ColumnController.php b/app/Controller/ColumnController.php index 8e4712d97..73a466789 100644 --- a/app/Controller/ColumnController.php +++ b/app/Controller/ColumnController.php @@ -150,6 +150,7 @@ class ColumnController extends BaseController */ public function move() { + $this->checkReusableGETCSRFParam(); $project = $this->getProject(); $values = $this->request->getJson(); diff --git a/app/Controller/CommentController.php b/app/Controller/CommentController.php index a29491a31..8360c641c 100644 --- a/app/Controller/CommentController.php +++ b/app/Controller/CommentController.php @@ -166,6 +166,7 @@ class CommentController extends BaseController */ public function toggleSorting() { + $this->checkReusableGETCSRFParam(); $task = $this->getTask(); $this->helper->comment->toggleSorting(); diff --git a/app/Controller/ProjectViewController.php b/app/Controller/ProjectViewController.php index 8ff793435..df9e45665 100644 --- a/app/Controller/ProjectViewController.php +++ b/app/Controller/ProjectViewController.php @@ -149,8 +149,12 @@ class ProjectViewController extends BaseController */ public function doDuplication() { + $this->checkCSRFForm(); + $project = $this->getProject(); - $project_id = $this->projectDuplicationModel->duplicate($project['id'], array_keys($this->request->getValues()), $this->userSession->getId()); + $values = $this->request->getRawFormValues(); + + $project_id = $this->projectDuplicationModel->duplicate($project['id'], array_keys($values), $this->userSession->getId()); if ($project_id !== false) { $this->flash->success(t('Project cloned successfully.')); diff --git a/app/Controller/SubtaskStatusController.php b/app/Controller/SubtaskStatusController.php index c912848e7..c5542b3f6 100644 --- a/app/Controller/SubtaskStatusController.php +++ b/app/Controller/SubtaskStatusController.php @@ -17,6 +17,7 @@ class SubtaskStatusController extends BaseController */ public function change() { + $this->checkReusableGETCSRFParam(); $task = $this->getTask(); $subtask = $this->getSubtask($task); $fragment = $this->request->getStringParam('fragment'); diff --git a/app/Controller/SwimlaneController.php b/app/Controller/SwimlaneController.php index 9e07c1629..e0fc40734 100644 --- a/app/Controller/SwimlaneController.php +++ b/app/Controller/SwimlaneController.php @@ -205,6 +205,7 @@ class SwimlaneController extends BaseController */ public function move() { + $this->checkReusableGETCSRFParam(); $project = $this->getProject(); $values = $this->request->getJson(); diff --git a/app/Controller/TaskListController.php b/app/Controller/TaskListController.php index 7128b91ec..f976c6071 100644 --- a/app/Controller/TaskListController.php +++ b/app/Controller/TaskListController.php @@ -23,6 +23,13 @@ class TaskListController extends BaseController $project = $this->getProject(); $search = $this->helper->projectHeader->getSearchQuery($project); + if ($this->request->getIntegerParam('show_subtasks') !== 0 || + $this->request->getIntegerParam('hide_subtasks') !== 0 || + $this->request->getStringParam('direction') !== '' || + $this->request->getStringParam('order') !== '') { + $this->checkReusableGETCSRFParam(); + } + if ($this->request->getIntegerParam('show_subtasks')) { session_set('subtaskListToggle', true); } elseif ($this->request->getIntegerParam('hide_subtasks')) { @@ -41,7 +48,7 @@ class TaskListController extends BaseController $this->userSession->setListOrder($project['id'], $order, $direction); $paginator = $this->paginator - ->setUrl('TaskListController', 'show', array('project_id' => $project['id'])) + ->setUrl('TaskListController', 'show', array('project_id' => $project['id'], 'csrf_token' => $this->token->getReusableCSRFToken())) ->setMax(30) ->setOrder($order) ->setDirection($direction) diff --git a/app/Controller/TaskModificationController.php b/app/Controller/TaskModificationController.php index 7164ce0cd..b2b94c3d5 100644 --- a/app/Controller/TaskModificationController.php +++ b/app/Controller/TaskModificationController.php @@ -16,6 +16,7 @@ class TaskModificationController extends BaseController { public function assignToMe() { + $this->checkReusableGETCSRFParam(); $task = $this->getTask(); $values = ['id' => $task['id'], 'owner_id' => $this->userSession->getId()]; @@ -38,6 +39,7 @@ class TaskModificationController extends BaseController */ public function start() { + $this->checkReusableGETCSRFParam(); $task = $this->getTask(); $values = ['id' => $task['id'], 'date_started' => time()]; diff --git a/app/Controller/TaskMovePositionController.php b/app/Controller/TaskMovePositionController.php index 39687b799..802eadeda 100644 --- a/app/Controller/TaskMovePositionController.php +++ b/app/Controller/TaskMovePositionController.php @@ -31,6 +31,7 @@ class TaskMovePositionController extends BaseController public function save() { + $this->checkReusableGETCSRFParam(); $task = $this->getTask(); $values = $this->request->getJson(); diff --git a/app/Controller/WebNotificationController.php b/app/Controller/WebNotificationController.php index 02827ab57..95624744a 100644 --- a/app/Controller/WebNotificationController.php +++ b/app/Controller/WebNotificationController.php @@ -34,6 +34,7 @@ class WebNotificationController extends BaseController */ public function flush() { + $this->checkReusableGETCSRFParam(); $userId = $this->getUserId(); $this->userUnreadNotificationModel->markAllAsRead($userId); $this->show(); @@ -46,6 +47,7 @@ class WebNotificationController extends BaseController */ public function remove() { + $this->checkReusableGETCSRFParam(); $user_id = $this->getUserId(); $notification_id = $this->request->getIntegerParam('notification_id'); $this->userUnreadNotificationModel->markAsRead($user_id, $notification_id); diff --git a/app/Core/Http/Request.php b/app/Core/Http/Request.php index 5fd96b722..b9907d067 100644 --- a/app/Core/Http/Request.php +++ b/app/Core/Http/Request.php @@ -111,6 +111,16 @@ class Request extends Base return array(); } + /** + * Get POST values without modification + * + * @return array + */ + public function getRawFormValues() + { + return $this->post; + } + /** * Get POST value without modification * diff --git a/app/Helper/SubtaskHelper.php b/app/Helper/SubtaskHelper.php index 138a824b5..c1c92aa0d 100644 --- a/app/Helper/SubtaskHelper.php +++ b/app/Helper/SubtaskHelper.php @@ -64,6 +64,7 @@ class SubtaskHelper extends Base 'subtask_id' => $subtask['id'], 'user_id' => $userId, 'fragment' => $fragment, + 'csrf_token' => $this->token->getReusableCSRFToken(), ); if ($subtask['status'] == 0 && $this->hasSubtaskInProgress()) { diff --git a/app/Template/board/table_container.php b/app/Template/board/table_container.php index 54c7a6ad5..cb1533b43 100644 --- a/app/Template/board/table_container.php +++ b/app/Template/board/table_container.php @@ -11,9 +11,9 @@ class="board-project-" data-project-id="" data-check-interval="" - data-save-url="url->href('BoardAjaxController', 'save', array('project_id' => $project['id'])) ?>" - data-reload-url="url->href('BoardAjaxController', 'reload', array('project_id' => $project['id'])) ?>" - data-check-url="url->href('BoardAjaxController', 'check', array('project_id' => $project['id'], 'timestamp' => time())) ?>" + data-save-url="url->href('BoardAjaxController', 'save', array('project_id' => $project['id'], 'csrf_token' => $this->app->getToken()->getReusableCSRFToken())) ?>" + data-reload-url="url->href('BoardAjaxController', 'reload', array('project_id' => $project['id'], 'csrf_token' => $this->app->getToken()->getReusableCSRFToken())) ?>" + data-check-url="url->href('BoardAjaxController', 'check', array('project_id' => $project['id'], 'timestamp' => time(), 'csrf_token' => $this->app->getToken()->getReusableCSRFToken())) ?>" data-task-creation-url="url->href('TaskCreationController', 'show', array('project_id' => $project['id'])) ?>" > diff --git a/app/Template/column/index.php b/app/Template/column/index.php index eb6432496..5e6f4687e 100644 --- a/app/Template/column/index.php +++ b/app/Template/column/index.php @@ -12,7 +12,7 @@ + data-save-position-url="url->href('ColumnController', 'move', array('project_id' => $project['id'], 'csrf_token' => $this->app->getToken()->getReusableCSRFToken())) ?>"> diff --git a/app/Template/swimlane/table.php b/app/Template/swimlane/table.php index f564424ce..edbf25cd4 100644 --- a/app/Template/swimlane/table.php +++ b/app/Template/swimlane/table.php @@ -1,6 +1,6 @@
+ data-save-position-url="url->href('SwimlaneController', 'move', array('project_id' => $project['id'], 'csrf_token' => $this->app->getToken()->getReusableCSRFToken())) ?>"> diff --git a/app/Template/task/details.php b/app/Template/task/details.php index f3c6d8964..f9b788a65 100644 --- a/app/Template/task/details.php +++ b/app/Template/task/details.php @@ -86,7 +86,7 @@ user->getId()): ?> - - url->link(t('Assign to me'), 'TaskModificationController', 'assignToMe', ['task_id' => $task['id'], 'project_id' => $task['project_id']]) ?> + - url->link(t('Assign to me'), 'TaskModificationController', 'assignToMe', ['task_id' => $task['id'], 'project_id' => $task['project_id'], 'csrf_token' => $this->app->getToken()->getReusableCSRFToken()]) ?> @@ -124,7 +124,7 @@ dt->datetime($task['date_started']) ?> - url->link(t('Start now'), 'TaskModificationController', 'start', ['task_id' => $task['id'], 'project_id' => $task['project_id']]) ?> + url->link(t('Start now'), 'TaskModificationController', 'start', ['task_id' => $task['id'], 'project_id' => $task['project_id'], 'csrf_token' => $this->app->getToken()->getReusableCSRFToken()]) ?>
  • diff --git a/app/Template/task/dropdown.php b/app/Template/task/dropdown.php index da2e7d7e1..d3d482767 100644 --- a/app/Template/task/dropdown.php +++ b/app/Template/task/dropdown.php @@ -6,12 +6,12 @@ projectRole->canUpdateTask($task)): ?> projectRole->canChangeAssignee($task) && array_key_exists('owner_id', $task) && $task['owner_id'] != $this->user->getId()): ?>
  • - url->icon('hand-o-right', t('Assign to me'), 'TaskModificationController', 'assignToMe', ['task_id' => $task['id'], 'project_id' => $task['project_id'], 'redirect' => isset($redirect) ? $redirect : '']) ?> + url->icon('hand-o-right', t('Assign to me'), 'TaskModificationController', 'assignToMe', ['task_id' => $task['id'], 'project_id' => $task['project_id'], 'csrf_token' => $this->app->getToken()->getReusableCSRFToken(), 'redirect' => isset($redirect) ? $redirect : '']) ?>
  • - url->icon('play', t('Set the start date automatically'), 'TaskModificationController', 'start', ['task_id' => $task['id'], 'project_id' => $task['project_id'], 'redirect' => isset($redirect) ? $redirect : '']) ?> + url->icon('play', t('Set the start date automatically'), 'TaskModificationController', 'start', ['task_id' => $task['id'], 'project_id' => $task['project_id'], 'csrf_token' => $this->app->getToken()->getReusableCSRFToken(), 'redirect' => isset($redirect) ? $redirect : '']) ?>
  • diff --git a/app/Template/task_comments/show.php b/app/Template/task_comments/show.php index 9010437ce..f815fbb03 100644 --- a/app/Template/task_comments/show.php +++ b/app/Template/task_comments/show.php @@ -4,7 +4,7 @@
    - url->icon('sort', t('Change sorting'), 'CommentController', 'toggleSorting', array('task_id' => $task['id'], 'project_id' => $task['project_id'])) ?> + url->icon('sort', t('Change sorting'), 'CommentController', 'toggleSorting', array('task_id' => $task['id'], 'project_id' => $task['project_id'], 'csrf_token' => $this->app->getToken()->getReusableCSRFToken())) ?> modal->medium('paper-plane', t('Send by email'), 'CommentMailController', 'create', array('task_id' => $task['id'], 'project_id' => $task['project_id'])) ?> diff --git a/app/Template/task_list/header.php b/app/Template/task_list/header.php index d91c6c6ed..4ff074ceb 100644 --- a/app/Template/task_list/header.php +++ b/app/Template/task_list/header.php @@ -30,9 +30,9 @@
    user->hasSubtaskListActivated()): ?> - url->icon('tasks', t('Hide subtasks'), 'TaskListController', 'show', array('project_id' => $project['id'], 'hide_subtasks' => 1)) ?> + url->icon('tasks', t('Hide subtasks'), 'TaskListController', 'show', array('project_id' => $project['id'], 'hide_subtasks' => 1, 'csrf_token' => $this->app->getToken()->getReusableCSRFToken())) ?> - url->icon('tasks', t('Show subtasks'), 'TaskListController', 'show', array('project_id' => $project['id'], 'show_subtasks' => 1)) ?> + url->icon('tasks', t('Show subtasks'), 'TaskListController', 'show', array('project_id' => $project['id'], 'show_subtasks' => 1, 'csrf_token' => $this->app->getToken()->getReusableCSRFToken())) ?> diff --git a/app/Template/task_move_position/show.php b/app/Template/task_move_position/show.php index dbbfdbd4d..a7c87e661 100644 --- a/app/Template/task_move_position/show.php +++ b/app/Template/task_move_position/show.php @@ -5,7 +5,7 @@
    app->component('task-move-position', array( - 'saveUrl' => $this->url->href('TaskMovePositionController', 'save', array('task_id' => $task['id'], 'project_id' => $task['project_id'])), + 'saveUrl' => $this->url->href('TaskMovePositionController', 'save', array('task_id' => $task['id'], 'project_id' => $task['project_id'], 'csrf_token' => $this->app->getToken()->getReusableCSRFToken())), 'board' => $board, 'task' => $task, 'swimlaneLabel' => t('Swimlane'), diff --git a/app/Template/web_notification/show.php b/app/Template/web_notification/show.php index d4f22ade4..72ebdb245 100644 --- a/app/Template/web_notification/show.php +++ b/app/Template/web_notification/show.php @@ -4,7 +4,7 @@
    • - modal->replaceIconLink('check-square-o', t('Mark all as read'), 'WebNotificationController', 'flush', array('user_id' => $user['id'])) ?> + modal->replaceIconLink('check-square-o', t('Mark all as read'), 'WebNotificationController', 'flush', array('user_id' => $user['id'], 'csrf_token' => $this->app->getToken()->getReusableCSRFToken())) ?>
    @@ -60,7 +60,7 @@
    dt->datetime($notification['date_creation']) ?> - modal->replaceIconLink('check', t('Mark as read'), 'WebNotificationController', 'remove', array('user_id' => $user['id'], 'notification_id' => $notification['id'])) ?> + modal->replaceIconLink('check', t('Mark as read'), 'WebNotificationController', 'remove', array('user_id' => $user['id'], 'notification_id' => $notification['id'], 'csrf_token' => $this->app->getToken()->getReusableCSRFToken())) ?>