View Issue Details

IDProjectCategoryView StatusLast Update
0011804mantisbtsecuritypublic2014-12-08 00:34
Reportermattmccutchen Assigned Todhx  
PrioritynormalSeveritymajorReproducibilityalways
Status closedResolutionfixed 
Product Versiongit trunk 
Target Version1.3.0-beta.1Fixed in Version1.3.0-beta.1 
Summary0011804: 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
  1. Navigate to a URL of the following form, where A is a bug you reported:
    http://path/to/bugs/bug_change_status_page.php?id=A&new_status=B
    This will give you a valid form security token for bug_update.php.
  2. Add any desired fields, such as description, to the form with a tool such as Firebug.
  3. Submit and enjoy.
TagsNo 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 ) );
bug-update-access-checks.patch (4,624 bytes)   

Relationships

related to 0015721 closedgrangeway Functionality to consider porting to master-2.0.x 
has duplicate 0010053 closeddhx Security holes in bug_update.php allow reporters to do changes that should not be allowed 
related to 0011967 closeddhx Problems with EVENT_UPDATE_BUG 
child of 0012097 closedatrol Tracking issue for the refactoring of bug_update.php 

Activities

mattmccutchen

mattmccutchen

2010-04-16 08:22

reporter   ~0025145

As a demonstration, I used my allow_reporter_reopen privilege to confirm this issue.

mattmccutchen

mattmccutchen

2010-04-16 08:37

reporter   ~0025146

The attached patch should fix the security problem, though it may break other things and may not be right in some of the details.

mattmccutchen

mattmccutchen

2010-05-22 06:21

reporter   ~0025588

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.

jreese

jreese

2010-05-23 12:08

reporter   ~0025591

Currently investigating...

dhx

dhx

2010-05-27 10:54

reporter   ~0025617

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.

dhx

dhx

2010-05-27 10:55

reporter   ~0025618

To be more precise, I'm removing $f_update_mode completely.

grangeway

grangeway

2013-04-05 17:57

reporter   ~0036490

Marking as 'acknowledged' not resolved/closed to track that change gets ported to master-2.0.x branch

Related Changesets

MantisBT: master 035a1302

2010-06-22 20:44

dhx


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