View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0013656 | mantisbt | security | public | 2011-12-08 16:48 | 2014-12-22 08:22 |
Reporter | atrol | Assigned To | rombert | ||
Priority | urgent | Severity | major | Reproducibility | always |
Status | closed | Resolution | fixed | ||
Product Version | 1.2.8 | ||||
Target Version | 1.2.9 | Fixed in Version | 1.2.9 | ||
Summary | 0013656: Reporters have read/write access to existing data of other users | ||||
Description | I noticed in history that user "a" was able to delete note 10379 in issue 0003932 One of our managers or administrators should check If a) is true and b) is false the problem might be caused by SOAP API. Is there any reason why we set $g_mc_readwrite_access_level_threshold = REPORTER; in file mc_config_defaults_inc.php and not DEVELOPER ? At first sight it seems that a reporter is able to clean up our tracker by using SOAP. | ||||
Tags | No tags attached. | ||||
Attached Files | |||||
related to | 0012328 | acknowledged | Normalise access checks between the web interface and the SOAP API | |
related to | 0015721 | closed | grangeway | Functionality to consider porting to master-2.0.x |
related to | 0010491 | closed | vboctor | mc_issue_add incorrect access level check |
related to | 0013736 | closed | rombert | mc_issue_get_id_from_summary incorrectly checks for permissions |
related to | 0013737 | closed | rombert | mci_issue_get_tags_for_bug_id incorrect access checks |
This was introduced when fixing 0010491 |
|
Good catch. I think that - for the time being - we should raise the threshold on mantisbt.org to DEVELOPER immediately. I will then review the usage of this setting in the SOAP API and ensure that we do not have any security leaks. |
|
Patch to correct access checks when deleting bugs SOAP-API-proper-access-checks-when-deleting-bugs.patch (822 bytes)
From 1af3e6dce4d595be0b13cf6e948ca635ff0aea16 Fri, 9 Dec 2011 00:50:58 +0200 From: Robert Munteanu <robert.munteanu@gmail.com> Date: Fri, 9 Dec 2011 00:49:17 +0200 Subject: [PATCH] SOAP API: proper access checks when deleting bugs Affects #13656 : Reporters have read/write access to existing data of other users diff --git a/api/soap/mc_issue_api.php b/api/soap/mc_issue_api.php index 1254039..2be49bb 100644 --- a/api/soap/mc_issue_api.php +++ b/api/soap/mc_issue_api.php @@ -943,6 +943,10 @@ if( !mci_has_readwrite_access( $t_user_id, $t_project_id ) ) { return mci_soap_fault_access_denied( $t_user_id ); } + + if ( !access_has_bug_level( config_get( 'delete_bug_threshold' ), $p_issue_id, $t_user_id ) ) { + return mci_soap_fault_access_denied( $t_user_id ); + } return bug_delete( $p_issue_id ); } |
|
Patch to correct access check when deleting bugnotes SOAP-API-proper-access-checks-when-deleting-bugnotes.patch (1,080 bytes)
From 76bb64383a8417f538433770ae6851103cac8f0d Fri, 9 Dec 2011 00:55:06 +0200 From: Robert Munteanu <robert.munteanu@gmail.com> Date: Fri, 9 Dec 2011 00:46:34 +0200 Subject: [PATCH] SOAP API: proper access checks when deleting bugnotes Affects #13656 : Reporters have read/write access to existing data of other users diff --git a/api/soap/mc_issue_api.php b/api/soap/mc_issue_api.php index 27ae499..1752ba1 100644 --- a/api/soap/mc_issue_api.php +++ b/api/soap/mc_issue_api.php @@ -1030,6 +1030,15 @@ if( !mci_has_readwrite_access( $t_user_id, $t_project_id ) ) { return mci_soap_fault_access_denied( $t_user_id ); } + + $t_reporter_id = bugnote_get_field( $p_issue_note_id, 'reporter_id' ); + + // mirrors check from bugnote_delete.php + if ( ( $t_user_id != $t_reporter_id ) || ( OFF == config_get( 'bugnote_allow_user_edit_delete' ) ) ) { + if ( !access_has_bugnote_level( config_get( 'delete_bugnote_threshold' ), $p_issue_note_id ) ) { + return mci_soap_fault_access_denied( $t_user_id ); + } + } return bugnote_delete( $p_issue_note_id ); } |
|
mc_issue_attachment_get and mc_project_attachment_get still need proper checks, but that requires extracting minimal verification API from file_download.php . Anyway, that's nowhere as severe as the other two issues. |
|
Reminder sent to: dhx, dregad, giallu, grangeway, jreese, siebrand, vboctor All: this is a pretty serious issue, as all MantisBT installations are vulnerable. I have attached patches for the deletion issues. By monday I should have a proper fix for the 'download unauthorized attachments' issue, as it is still an problem. I think we should cut a 1.2.9 release to fix this next week. And at the same time advise all users how to appy a hotfix by creating a mc_config_inc.php in api/soap/ and adding $g_mc_readwrite_access_level_threshold = DEVELOPER; to it. |
|
Can one of the admins have a look at the bug history table in the DB, and check if there are more occurences of such deletions ? If possible, I think it would be useful to restore deleted bugnotes from backup. If that is not possible or we don't have a backup, maybe Google cache can help. Regarding ~10379 (deleted note in 0003932), I have added the contents of the deleted note into a new one. |
|
I double-checked the access checks for attachment download and they are fine. The only fixes to apply are those attached to the bug. I'd like to wait pushing them until we are ready to make a release. I will supply fixes for both master and master-1.2.x . The question is, when can we make that release? |
|
I personally don't have any issues with pushing 1.2.9 out. The fixes I'm currently working on are not so urgent, and can wait for a future release, no problem. As for the when, I guess that mostly depends on John's availability. |
|
Anyone? Can at least someone with access to the bugtracker set $g_mc_readwrite_access_level_threshold = DEVELOPER as a hotfix until we upgrade the code? |
|
I've updated this install to set readwrite to developer. |
|
I'm then going to close this with the proposition of releasing 1.2.9 as soon as it is feasible. |
|
Reminder sent to: rombert You want to release 1.2.9 without applying your patches? |
|
(In reply to comment 0013656:0030807)
Thank you. Obviously not. But I'll apply the patches in a few hours. |
|
Reminder sent to: rombert You didn't push to master-1.2.x |
|
Apparently I didn't . I'll do that tonight. |
|
Third time's the charm, hopefully. https://github.com/mantisbt/mantisbt/commit/df7782a65e96aa1c9639a7625a658102134c7fe0 |
|
Apologies for missing this bug report until now. Firstly, thanks for pushing out some fixes. This is what has always concerned me about MantisBT -- inconsistent permission checks throughout the code base. The first random SOAP API call I checked is incorrect: mc_issue_get_id_from_summary. It does not take into account whether the issue is private and as a result will always reveal whether or not a private issue with a specified summary exists -- even if the user does not have permission to know about private issues. This is nitpicking because timing analysis of SOAP calls (which we make no attempt to protect against yet) could reveal this information too, so I'm not overly concerned about this one. The second randomly selected call I checked was also incorrect: mci_issue_get_tags_for_bug_id. This call also fails to check whether the issue is private and whether the user has permission to read the private issue. The access check should be changed to access_has_bug_level instead which correctly handles private issues, project specific permissions, etc. I don't want to pick on SOAP API because I could open up almost any other file in the MantisBT source tree and find similar problems of inconsistent permission checks. It's really something we need to spend some time sorting out throughout the code base, restructuring to remove these inconsistencies. |
|
David, thanks for the comments ; I've created two new bugs and linked them to this report ; I think that the large security risk was in the deletion problem, but I will fix the other two as well. I fully agree with your analysis of the current state of security checks - it's too hard because they are currently duplicated across pages and APIs - the SOAP API simply copies what the regular pages are doing. This is why I've earlier created bug 0012328, which aims to extract a higher-level API usable for all 'clients' which need access to information. |
|
This issue is now public to aid with a CVE request on the oss-security mailing list. The latest version (1.2.9) has already been released, including patches to solve this problem. |
|
A CVE identifier has been assigned to this issue: CVE-2012-1120 MantisBT 1.2.8 13656 |
|
Marking as 'acknowledged' not resolved/closed to track that change gets ported to master-2.0.x branch |
|