Fix #14016: delete_attachments_threshold is not checked

davidhicks · davidhicks · commit ceafe6f0c679 · 2012-06-02T21:15:26.000+10:00
Roland Becker (MantisBT developer) reported the following
security/access control bug:

In a default installation delete_attachments_threshold is set to
DEVELOPER but having access level &gt;= update_bug_threshold is enough to
delete attachments if form_security_validation is set to OFF.

MantisBT was not checking the access level of the user requesting
deletion of an attachment to an issue against
$g_delete_attachments_threshold.

The new access control logic for deleting an issue attachment is now:
1. Does the user have an access level of at least update_bug_threshold?
2. If the user is the owner of the file and
$g_allow_delete_own_attachments=OFF, does this user have an access level
of at least delete_attachments_threshold?
3. If the user is not the owner of the file, do they have an access
level of at least delete_attachments_threshold?

Also refer to issue #14015 for discussion on whether
update_bug_threshold should be part of the access control logic.

The relevant SOAP API call has also been updated.
diff --git a/api/soap/mc_issue_attachment_api.php b/api/soap/mc_issue_attachment_api.php
@@ -64,12 +64,27 @@ function mc_issue_attachment_add( $p_username, $p_password, $p_issue_id, $p_name
  */
 function mc_issue_attachment_delete( $p_username, $p_password, $p_issue_attachment_id ) {
 	$t_user_id = mci_check_login( $p_username, $p_password );
+
 	if( $t_user_id === false ) {
 		return mci_soap_fault_login_failed();
 	}
+
 	$t_bug_id = file_get_field( $p_issue_attachment_id, 'bug_id' );
+
+	# Check access against update_bug_threshold
 	if( !access_has_bug_level( config_get( 'update_bug_threshold' ), $t_bug_id, $t_user_id ) ) {
 		return mci_soap_fault_access_denied( $t_user_id );
 	}
+
+	$t_attachment_owner = file_get_field( $f_file_id, 'user_id' );
+	$t_current_user_is_attachment_owner = $t_attachment_owner == $t_user_id;
+	# Factor in allow_delete_own_attachments=ON|OFF
+	if ( !$t_current_user_is_attachment_owner || ( $t_current_user_is_attachment_owner && !config_get( 'allow_delete_own_attachments' ) ) ) {
+		# Check access against delete_attachments_threshold
+		if ( !access_has_bug_level( config_get( 'delete_attachments_threshold' ), $t_bug_id, $t_user_id ) ) {
+			return mci_soap_fault_access_denied( $t_user_id );
+		}
+	}
+
 	return file_delete( $p_issue_attachment_id, 'bug' );
 }
diff --git a/bug_file_delete.php b/bug_file_delete.php
@@ -44,6 +44,12 @@
 
 	access_ensure_bug_level( config_get( 'update_bug_threshold' ), $t_bug_id );
 
+	$t_attachment_owner = file_get_field( $f_file_id, 'user_id' );
+	$t_current_user_is_attachment_owner = $t_attachment_owner == auth_get_current_user_id();
+	if ( !$t_current_user_is_attachment_owner || ( $t_current_user_is_attachment_owner && !config_get( 'allow_delete_own_attachments' ) ) ) {
+		access_ensure_bug_level( config_get( 'delete_attachments_threshold'), $t_bug_id );
+	}
+
 	helper_ensure_confirmed( lang_get( 'delete_attachment_sure_msg' ), lang_get( 'delete_attachment_button' ) );
 
 	file_delete( $f_file_id, 'bug' );
