CVE-2020-25781: Missing Authorization (CWE-862)
value: An issue was discovered in file_download.php in MantisBT before 2.24.3. Users without access to view private issue notes are able to download the (supposedly private) attachments linked to these notes by accessing the corresponding file download URL directly.
CWE description: The product does not perform an authorization check when an actor attempts to access a resource or perform an action.
CVSS v3 base score: 4.3
Published: 2020-09-30T21:15Z
Repository: https://github.com/mantisbt/mantisbt
Language: PHP
Files changed: core/file_api.php, file_download.php
Commit date: 2020-09-23 19:06:04 +0200
Commit message: Check ability to download attachments at bugnote level

This prevents users authorized to download attachments but not to view
private bugnotes, from accessing files attached to a private note via
`file_download.php?file_id={FILE_ID}&type=bug` (CVE-2020-25781).

Includes some minor code cleanup in file_get_visible_attachments():
- use a foreach loop
- reuse variables instead of derefenrcing array

Fixes #27039

Diff: diff --git a/core/file_api.php b/core/file_api.php
index 63de3e48782..245b3035317 100644
--- a/core/file_api.php
+++ b/core/file_api.php
@@ -449,50 +449,40 @@ function file_get_visible_attachments( $p_bug_id ) {
 
 	$t_attachments = array();
 
 	$t_preview_text_ext = config_get( 'preview_text_extensions' );
 	$t_preview_image_ext = config_get( 'preview_image_extensions' );
-	$t_attachments_view_threshold = config_get( 'view_attachments_threshold' );
 
 	$t_image_previewed = false;
-	for( $i = 0;$i < $t_attachments_count;$i++ ) {
-		$t_row = $t_attachment_rows[$i];
+	foreach( $t_attachment_rows as $t_row ) {
 		$t_user_id = (int)$t_row['user_id'];
+		$t_attachment_note_id = (int)$t_row['bugnote_id'];
 
-		# This covers access checks for issue attachments
-		if( !file_can_view_bug_attachments( $p_bug_id, $t_user_id ) ) {
+		if( !file_can_view_bug_attachments( $p_bug_id, $t_user_id )
+		|| !file_can_view_bugnote_attachments( $t_attachment_note_id, $t_user_id )
+		) {
 			continue;
 		}
 
-		# This covers access checks for issue note attachments
-		$t_attachment_note_id = (int)$t_row['bugnote_id'];
-		if( $t_attachment_note_id !== 0 ) {
-			if( bugnote_get_field( $t_attachment_note_id, 'view_state' ) != VS_PUBLIC ) {
-				if( !access_has_bugnote_level( $t_attachments_view_threshold, $t_attachment_note_id ) ) {
-					continue;
-				}
-			}
-		}
-
 		$t_id = (int)$t_row['id'];
 		$t_filename = $t_row['filename'];
-		$t_filesize = $t_row['filesize'];
+		$t_filesize = (int)$t_row['filesize'];
 		$t_diskfile = file_normalize_attachment_path( $t_row['diskfile'], bug_get_field( $p_bug_id, 'project_id' ) );
 		$t_date_added = $t_row['date_added'];
 
 		$t_attachment = array();
 		$t_attachment['id'] = $t_id;
 		$t_attachment['user_id'] = $t_user_id;
 		$t_attachment['display_name'] = file_get_display_name( $t_filename );
-		$t_attachment['size'] = (int)$t_filesize;
+		$t_attachment['size'] = $t_filesize;
 		$t_attachment['date_added'] = $t_date_added;
 		$t_attachment['diskfile'] = $t_diskfile;
 		$t_attachment['file_type'] = $t_row['file_type'];
-		$t_attachment['bugnote_id'] = (int)$t_row['bugnote_id'];
+		$t_attachment['bugnote_id'] = $t_attachment_note_id;
 
-		$t_attachment['can_download'] = file_can_download_bug_attachments( $p_bug_id, (int)$t_row['user_id'] );
-		$t_attachment['can_delete'] = file_can_delete_bug_attachments( $p_bug_id, (int)$t_row['user_id'] );
+		$t_attachment['can_download'] = file_can_download_bug_attachments( $p_bug_id, $t_user_id );
+		$t_attachment['can_delete'] = file_can_delete_bug_attachments( $p_bug_id, $t_user_id );
 
 		if( $t_attachment['can_download'] ) {
 			$t_attachment['download_url'] = 'file_download.php?file_id=' . $t_id . '&type=bug';
 		}
 
diff --git a/file_download.php b/file_download.php
index d423681b0fb..d7404548a22 100644
--- a/file_download.php
+++ b/file_download.php
@@ -108,11 +108,13 @@ if( $f_type == 'bug' ) {
 }
 
 # Check access rights
 switch( $f_type ) {
 	case 'bug':
-		if( !file_can_download_bug_attachments( $v_bug_id, (int)$v_user_id ) ) {
+		if( !file_can_download_bug_attachments( $v_bug_id, $v_user_id )
+		|| !file_can_download_bugnote_attachments( $v_bugnote_id, $v_user_id )
+		) {
 			access_denied();
 		}
 		break;
 	case 'doc':
 		# Check if project documentation feature is enabled.

Vulnerable code: 	$t_attachments_view_threshold = config_get( 'view_attachments_threshold' );
	for( $i = 0;$i < $t_attachments_count;$i++ ) {
		$t_row = $t_attachment_rows[$i];
		# This covers access checks for issue attachments
		if( !file_can_view_bug_attachments( $p_bug_id, $t_user_id ) ) {
		# This covers access checks for issue note attachments
		$t_attachment_note_id = (int)$t_row['bugnote_id'];
		if( $t_attachment_note_id !== 0 ) {
			if( bugnote_get_field( $t_attachment_note_id, 'view_state' ) != VS_PUBLIC ) {
				if( !access_has_bugnote_level( $t_attachments_view_threshold, $t_attachment_note_id ) ) {
					continue;
				}
			}
		}

		$t_filesize = $t_row['filesize'];
		$t_attachment['size'] = (int)$t_filesize;
		$t_attachment['bugnote_id'] = (int)$t_row['bugnote_id'];
		$t_attachment['can_download'] = file_can_download_bug_attachments( $p_bug_id, (int)$t_row['user_id'] );
		$t_attachment['can_delete'] = file_can_delete_bug_attachments( $p_bug_id, (int)$t_row['user_id'] );
		if( !file_can_download_bug_attachments( $v_bug_id, (int)$v_user_id ) ) {
Fixed code: 	foreach( $t_attachment_rows as $t_row ) {
		$t_attachment_note_id = (int)$t_row['bugnote_id'];
		if( !file_can_view_bug_attachments( $p_bug_id, $t_user_id )
		|| !file_can_view_bugnote_attachments( $t_attachment_note_id, $t_user_id )
		) {
		$t_filesize = (int)$t_row['filesize'];
		$t_attachment['size'] = $t_filesize;
		$t_attachment['bugnote_id'] = $t_attachment_note_id;
		$t_attachment['can_download'] = file_can_download_bug_attachments( $p_bug_id, $t_user_id );
		$t_attachment['can_delete'] = file_can_delete_bug_attachments( $p_bug_id, $t_user_id );
		if( !file_can_download_bug_attachments( $v_bug_id, $v_user_id )
		|| !file_can_download_bugnote_attachments( $v_bugnote_id, $v_user_id )
		) {