| Project: | ProjectPier |
| Version: | 0.8.5.0-Beta1 |
| Component: | Code |
| Category: | bug report |
| Priority: | critical |
| Assigned: | phpfreak |
| Status: | closed - fixed |
The beta version solved the problem with users that can se messages, task, files and milestones by URL manipulation. But apparently some security holes are still open.
Scenario:
1. A user (UserA) logs in with "http://server1/pp/index.php?c=task&a=view_list&id=3&active_project=2" looking at his task list (he has permission to see). I used this URL to get valid manipulation data for a user withou permission.
2. Another user (UserB) is logged in and view hes task list as "http://server1/pp/index.php?c=task&a=view_list&id=2&active_project=1" (he has permission to this).
3. The UserB then manipulate his URL
from: http://server1/pp/index.php?c=task&a=view_list&id=2&active_project=1
to: http://server1/pp/index.php?c=task&a=view_list&id=2&active_project=2
the hes "Open task lists" shows all the non-private task lists from the UserA. If the user tries to look into one of the lists then he is re-routed to the dashboard/overview with the message "You don't have permission" as expected. I see this as a critical bug.
4. If instead UserB changed hes URL
from: http://server1/pp/index.php?c=task&a=view_list&id=2&active_project=1
to: http://server1/pp/index.php?c=task&a=view_list&id=3&active_project=2
then is is re-routed to dashboard/overview as espected with the message "You don't have permission....".
I am new to PHP and also the ProjectPier SW architecture, but I will try to see if I can fix it and supply a patch.
Hi,
I looked into this. I had to figure out how to debug the application on the apache server (now using Eclipse/XAMPP/XDebug) since I had to understand the software architecture, at least to some extend.
What I did was similar to what have already been done for solving node437. I just added the missing "$this->canGoOn()" in the TaskController function view_list() (see below).
1. should I provide at patch for this?
or
2. should I just check it in (in svn, in the trunk)?
If checking it into svn should the status be changed in the issue and so on.
What about the code itself. The line just after the canGoOn() that I added, is "if (!$task_list->canView(logged_user())) {". I thought the intention with that statement was to catch the case where a user was not allowed to see the list (which is the case after URL manipulation)? ... but is uses a different way of getting the project ID and checking for isProjectUser() that the canGoOn() function which is why it does not catch it? Am I wrong about this?
During the debug'ing (to find out how the code is working) I wondered if all these permissions check scatered around in the controllers could be encabsulated in a security "layer". The Controller->execute() checks if the action is valid and if so, call the correct controller and action handler. So somewhere around here a security layer could be inserted I guesss (I am still very very new to the architecture), or should it be earlier, since the CompanyWebsite class has already set the project since it is called as part of the URL modification, and before the Controller->execute(). A least I imagine that maintenance and test would be easier if security is handled as a concern in it's own layer, and then the different controllers handle only the clean business code for that controlling concern. But again I just started to look at the code, so I might change my view when I know more about how it all is working.
function view_list() {
$task_list = ProjectTaskLists::findById(get_id());
if (!($task_list instanceof ProjectTaskList)) {
flash_error(lang('task list dnx'));
$this->redirectTo('task');
} // if
$this->canGoOn(); // JUST ADDED THIS LINE!!
if (!$task_list->canView(logged_user())) {
flash_error(lang('no access permissions'));
$this->redirectToReferer(get_url('task'));
} // if
tpl_assign('task_list', $task_list);
// Sidebar
tpl_assign('open_task_lists', active_project()->getOpenTaskLists());
tpl_assign('completed_task_lists', active_project()->getCompletedTaskLists());
$this->setSidebar(get_template_path('index_sidebar', 'task'));
} // view_list
br,
astradss
Hi,
I was not sure if a patch was needed so a created one with the change from my last comment.
/Martin
Thank you Martin for supplying the patch. My opinion would be that your patch is used for now (version 0.8.5), but a security layer be considered for a future revision. I'll give it a try and get it into the next beta of 0.8.5. Feel free to post more details about your thoughts (or even a patch) for a security layer. You might want to start a discussion about the security layer on the developer mailing list (see http://www.projectpier.org/node/397).
Accepted into 0.8.6