View Issue Details

IDProjectCategoryView StatusLast Update
0013656mantisbtsecuritypublic2014-12-22 08:22
Reporteratrol Assigned Torombert  
PriorityurgentSeveritymajorReproducibilityalways
Status closedResolutionfixed 
Product Version1.2.8 
Target Version1.2.9Fixed in Version1.2.9 
Summary0013656: 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
a) Is the user "a" a reporter?
b) Is configured that reporters are allowed to delete notes?

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.

TagsNo tags attached.
Attached Files

Relationships

related to 0012328 acknowledged Normalise access checks between the web interface and the SOAP API 
related to 0015721 closedgrangeway Functionality to consider porting to master-2.0.x 
related to 0010491 closedvboctor mc_issue_add incorrect access level check 
related to 0013736 closedrombert mc_issue_get_id_from_summary incorrectly checks for permissions 
related to 0013737 closedrombert mci_issue_get_tags_for_bug_id incorrect access checks 

Activities

atrol

atrol

2011-12-08 16:49

developer   ~0030525

This was introduced when fixing 0010491

rombert

rombert

2011-12-08 17:25

reporter   ~0030527

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.

rombert

rombert

2011-12-08 17:51

reporter   ~0030529

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 );
 }
rombert

rombert

2011-12-08 17:55

reporter   ~0030530

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 );
 }
rombert

rombert

2011-12-08 18:25

reporter   ~0030531

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.

rombert

rombert

2011-12-09 04:51

reporter   ~0030536

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.

dregad

dregad

2011-12-09 08:17

developer   ~0030543

Last edited: 2011-12-09 08:27

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.

rombert

rombert

2011-12-10 08:29

reporter   ~0030545

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?

dregad

dregad

2011-12-10 20:34

developer   ~0030549

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.

rombert

rombert

2011-12-16 09:00

reporter   ~0030637

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?

jreese

jreese

2011-12-16 12:10

reporter   ~0030643

I've updated this install to set readwrite to developer.

rombert

rombert

2012-01-05 11:05

reporter   ~0030806

I'm then going to close this with the proposition of releasing 1.2.9 as soon as it is feasible.

atrol

atrol

2012-01-05 11:50

developer   ~0030807

Reminder sent to: rombert

You want to release 1.2.9 without applying your patches?

rombert

rombert

2012-01-05 15:19

reporter   ~0030808

(In reply to comment 0013656:0030807)

You want to release 1.2.9 without applying your patches?

Thank you. Obviously not. But I'll apply the patches in a few hours.

atrol

atrol

2012-01-06 02:39

developer   ~0030820

Reminder sent to: rombert

You didn't push to master-1.2.x

rombert

rombert

2012-01-06 02:43

reporter   ~0030821

Apparently I didn't . I'll do that tonight.

rombert

rombert

2012-01-06 03:54

reporter   ~0030827

Third time's the charm, hopefully.

https://github.com/mantisbt/mantisbt/commit/df7782a65e96aa1c9639a7625a658102134c7fe0
https://github.com/mantisbt/mantisbt/commit/9d3f5783e6e0a4faf4fae13c769c9bfd45bf063c

dhx

dhx

2012-01-06 22:24

reporter   ~0030834

Last edited: 2012-01-06 22:25

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.

rombert

rombert

2012-01-07 16:25

reporter   ~0030852

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.

dhx

dhx

2012-03-06 07:22

reporter   ~0031386

Last edited: 2012-03-06 11:01

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.

dhx

dhx

2012-03-06 17:34

reporter   ~0031394

A CVE identifier has been assigned to this issue:

CVE-2012-1120 MantisBT 1.2.8 13656
elete_bug_threshold/bugnote_allow_user_edit_delete access check bypass
via SOAP API

grangeway

grangeway

2013-04-05 17:57

reporter   ~0036308

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