View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0011804 | mantisbt | security | public | 2010-04-16 08:20 | 2014-12-08 00:34 |
Reporter | mattmccutchen | Assigned To | dhx | ||
Priority | normal | Severity | major | Reproducibility | always |
Status | closed | Resolution | fixed | ||
Product Version | git trunk | ||||
Target Version | 1.3.0-beta.1 | Fixed in Version | 1.3.0-beta.1 | ||
Summary | 0011804: allow_reporter_reopen lets reporter make any update, not just reopen | ||||
Description | As previously pointed out in 0009828:0020626, when allow_reporter_reopen is on, reporters can make arbitrary changes to their own bugs, not just reopen them. This could lead to some havoc. bug_update.php needs to be reworked so that allow_reporter_reopen and allow_reporter_close only allow that status transition and not changing other fields. The proposed patch in 0009828 is not sufficient. | ||||
Steps To Reproduce |
| ||||
Tags | No tags attached. | ||||
Attached Files | bug-update-access-checks.patch (4,624 bytes)
bug_update.php: Rework access checking so that the privilege to reopen or close a bug cannot be abused to make arbitrary updates. While I believe the approach is basically right, this patch is likely to break some customized setups. diff --git a/bug_update.php b/bug_update.php index 1f02d5d..0880570 100644 --- a/bug_update.php +++ b/bug_update.php @@ -44,15 +44,8 @@ $g_project_override = $t_bug_data->project_id; } - if ( !( - ( access_has_bug_level( access_get_status_threshold( $f_new_status, bug_get_field( $f_bug_id, 'project_id' ) ), $f_bug_id ) ) || - ( access_has_bug_level( config_get( 'update_bug_threshold' ) , $f_bug_id ) ) || - ( ( bug_get_field( $f_bug_id, 'reporter_id' ) == auth_get_current_user_id() ) && - ( ( ON == config_get( 'allow_reporter_reopen' ) ) || - ( ON == config_get( 'allow_reporter_close' ) ) ) ) - ) ) { - access_denied(); - } + # The primary access check is now done below so it can be more closely coupled + # to the actual operation being performed. # extract current extended information $t_old_bug_status = $t_bug_data->status; @@ -149,26 +142,38 @@ continue; } + # It's OK to do this before the primary access check, since we did an + # access check specifically for this custom field. if ( !custom_field_set_value( $t_id, $f_bug_id, gpc_get_custom_field( "custom_field_$t_id", $t_def['type'], NULL ) ) ) { error_parameters( lang_get_defaulted( custom_field_get_field( $t_id, 'name' ) ) ); trigger_error( ERROR_CUSTOM_FIELD_INVALID_VALUE, ERROR ); } } - $t_notify = true; - $t_bug_note_set = false; - if ( ( $t_old_bug_status != $t_bug_data->status ) && ( FALSE == $f_update_mode ) ) { + # Each of the alternatives here does a primary access check. + if ( $f_update_mode ) { + if ( ! access_has_bug_level( config_get( 'update_bug_threshold' ) , $f_bug_id ) ) { + access_denied(); + } + + $t_bug_data->update( true ); + + # Add a bugnote if there is one + bugnote_add( $f_bug_id, $f_bugnote_text, $f_time_tracking, $f_private, 0, '', NULL, FALSE ); + } else { # handle status transitions that come from pages other than bug_*update_page.php # this does the minimum to act on the bug and sends a specific message if ( $t_bug_data->status >= $t_resolved && $t_bug_data->status < $t_closed && $t_old_bug_status < $t_resolved ) { + if ( ! access_has_bug_level( access_get_status_threshold( $t_resolved, bug_get_field( $f_bug_id, 'project_id' ) ), $f_bug_id ) ) { + access_denied(); + } + # bug_resolve updates the status, fixed_in_version, resolution, handler_id and bugnote and sends message bug_resolve( $f_bug_id, $t_bug_data->resolution, $t_bug_data->fixed_in_version, $f_bugnote_text, $t_bug_data->duplicate_id, $t_bug_data->handler_id, $f_private, $f_time_tracking ); - $t_notify = false; - $t_bug_note_set = true; if ( $f_close_now ) { bug_set_field( $f_bug_id, 'status', $t_closed ); @@ -180,22 +185,17 @@ $t_bug_data->status = bug_get_field( $f_bug_id, 'status' ); } else if ( $t_bug_data->status >= $t_closed && $t_old_bug_status < $t_closed ) { + access_ensure_can_close_bug( $f_bug_id ); + # bug_close updates the status and bugnote and sends message bug_close( $f_bug_id, $f_bugnote_text, $f_private, $f_time_tracking ); - $t_notify = false; - $t_bug_note_set = true; } else if ( $t_bug_data->status == config_get( 'bug_reopen_status' ) && $t_old_bug_status >= $t_resolved ) { + access_ensure_can_reopen_bug( $f_bug_id ); + bug_set_field( $f_bug_id, 'handler_id', $t_bug_data->handler_id ); # fix: update handler_id before calling bug_reopen # bug_reopen updates the status and bugnote and sends message bug_reopen( $f_bug_id, $f_bugnote_text, $f_time_tracking, $f_private ); - $t_notify = false; - $t_bug_note_set = true; - - // update bug data with fields that may be updated inside bug_resolve(), otherwise changes will be overwritten - // in bug_update() call below. - $t_bug_data->status = bug_get_field( $f_bug_id, 'status' ); - $t_bug_data->resolution = bug_get_field( $f_bug_id, 'resolution' ); } } @@ -205,14 +205,6 @@ $t_bug_data = $t_new_bug_data; } - # Add a bugnote if there is one - if ( false == $t_bug_note_set ) { - bugnote_add( $f_bug_id, $f_bugnote_text, $f_time_tracking, $f_private, 0, '', NULL, FALSE ); - } - - # Update the bug entry, notify if we haven't done so already - $t_bug_data->update( true, ( false == $t_notify ) ); - form_security_purge( 'bug_update' ); helper_call_custom_function( 'issue_update_notify', array( $f_bug_id ) ); | ||||
related to | 0015721 | closed | grangeway | Functionality to consider porting to master-2.0.x |
has duplicate | 0010053 | closed | dhx | Security holes in bug_update.php allow reporters to do changes that should not be allowed |
related to | 0011967 | closed | dhx | Problems with EVENT_UPDATE_BUG |
child of | 0012097 | closed | atrol | Tracking issue for the refactoring of bug_update.php |
As a demonstration, I used my allow_reporter_reopen privilege to confirm this issue. |
|
The attached patch should fix the security problem, though it may break other things and may not be right in some of the details. |
|
Developers, if you want to keep this issue embargoed to the extent that it is, please let me know today, otherwise I am going to post my omnibus patch publicly in 0009828 so that Renegade doesn't duplicate the work. |
|
Currently investigating... |
|
I'm just confirming that I'm half way through refactoring bug_update.php to fix this problem and plenty of other ones too. The code for this page is currently a big mess. Basically I'm going to change bug_update.php so that it will check the user's permissions to change any of the fields submitted. If the user has requested a field to be changed and they can do that action, it'll be processed. If they're trying to change a field they don't have permission to access they'll receive an error and no changes will be made. The bug_reopen/bug_resolve/bug_close functions will be called automatically when a change in status triggers the conditions for those actions to be performed. It will be possible to change other fields while reopening a bug, as long as the user has permission to do so. |
|
To be more precise, I'm removing $f_update_mode completely. |
|
Marking as 'acknowledged' not resolved/closed to track that change gets ported to master-2.0.x branch |
|
MantisBT: master 035a1302 2010-06-22 20:44 Details Diff |
Refactor bug_update to fix multiple bugs This is a large change to bug_update.php which refactors the code to make it clearer and easier to understand. More importantly this refactoring fixes a number of bugs including the prior ability for reporters (with allow_reporter_reopen enabled) to make any modifications to the bug in question even when they didn't have permission to make those changes. This refactoring brings about a structural change to the bug update process. Previously the update checked a field (or number of fields) and then committed changes to the database before moving on to the next field. Hence it was possible for some of the requested changes to be committed to the database before a validation error kicked in, preventing the remainder of updates from being committed. The new structure of bug_update prevents partial commits from occurring as all validation and access checks are done prior to ANY data being committed to the database. If all the validation checks pass then the user can be safe in knowing that all changes should be committed to the database. If any of the validation checks fail, none of the changes have been committed. One remaining problem with this approach is the race condition whereby the administrator updates access checks between the validation of a field and the attempted committal of a field to the database. As access checks are performed internally within bug_api (and elsewhere), these could actually fail during committal (and after the validation steps in bug_update) if the access levels have changed in the meantime. This is a problem with requires rewriting much of the MantisBT codebase - all for a problem that is unlikely to occur and which is of low severity. Email notifications also need to be sorted out correctly some time in the future as it is hard to determine what the expected course of action should be. This update tries sending an email in this order: resolved, closed, reopened, handler added, status changed, bug updated. Only one email is sent so if the handler and status of an issue are updated at once and a user has refused to receive handler notifications, they won't get any email. This is because we currently give higher priority to notifying users of the addition of a handler to an issue rather than a change of status. Issue 0012097: Refactor bug_update.php Fixes 0009828: Reopen issue access check is wrong Fixes 0010226: No email on 'update->assign' Fixes 0011396: difference between closed and resolved Fixes 0011804: allow_reporter_reopen lets reporter make any update |
Affected Issues 0009828, 0010226, 0011396, 0011782, 0011804, 0012097 |
|
mod - bug_update.php | Diff File |