Skip to content

Commit

Permalink
Fix #14016: delete_attachments_threshold is not checked
Browse files Browse the repository at this point in the history
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 >= 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.

Conflicts:
	bug_file_delete.php
  • Loading branch information
davidhicks committed Jun 2, 2012
1 parent 8208170 commit f82f98c
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 0 deletions.
15 changes: 15 additions & 0 deletions api/soap/mc_issue_attachment_api.php
Expand Up @@ -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' );
}
6 changes: 6 additions & 0 deletions bug_file_delete.php
Expand Up @@ -63,6 +63,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' );
Expand Down

0 comments on commit f82f98c

Please sign in to comment.