From f82f98cc78ab2c15e5b16acd27a3d0c57e7ba98e Mon Sep 17 00:00:00 2001 From: David Hicks Date: Sat, 2 Jun 2012 21:10:32 +1000 Subject: [PATCH] Fix #14016: delete_attachments_threshold is not checked 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 --- api/soap/mc_issue_attachment_api.php | 15 +++++++++++++++ bug_file_delete.php | 6 ++++++ 2 files changed, 21 insertions(+) diff --git a/api/soap/mc_issue_attachment_api.php b/api/soap/mc_issue_attachment_api.php index f42397e42f..5348596963 100644 --- 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 index 5ff5fa649f..c96a078748 100644 --- a/bug_file_delete.php +++ b/bug_file_delete.php @@ -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' );