View Issue Details

IDProjectCategoryView StatusLast Update
0027039mantisbtsecuritypublic2020-09-25 14:53
Reporterpijama Assigned Todregad  
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version2.23.0 
Target Version2.24.3Fixed in Version2.24.3 
Summary0027039: CVE-2020-25781: Access to private bug note attachments
Description

Sorry for my English.

The problem.
User that has access to project's issue (VS_PUBLIC) , but has not access to private bug notes of this project issue, can download private bug note attachments directly (by file download url /file_download.php?file_id={FILE_ID}&type=bug).

The possible solution.
Need check access for private bug note attachment. Something similar like in 0026893.

Added patch as solution example (file_download.php[113-129]).

Steps To Reproduce

Create user1 who has public access to project and can download attachment of public issue/bug note
Create user2 who has any access to same project and can create private bug note with attachments.
By user1 try download private bug note attachment created by user2 using direct link (/file_download.php?file_id={FILE_ID}&type=bug).
User1 can do file id substitution in order to determine available file.

Tagspatch
Attached Files
file_download.php (7,471 bytes)   
<?php
# MantisBT - A PHP based bugtracking system

# MantisBT is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
# the Free Software Foundation, either version 2 of the License, or
# (at your option) any later version.
#
# MantisBT is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with MantisBT.  If not, see <http://www.gnu.org/licenses/>.

/**
 * Add file and redirect to the referring page
 *
 * @package MantisBT
 * @copyright Copyright 2000 - 2002  Kenzaburo Ito - kenito@300baud.org
 * @copyright Copyright 2002  MantisBT Team - mantisbt-dev@lists.sourceforge.net
 * @link http://www.mantisbt.org
 *
 * @uses core.php
 * @uses access_api.php
 * @uses authentication_api.php
 * @uses bug_api.php
 * @uses config_api.php
 * @uses constant_inc.php
 * @uses database_api.php
 * @uses file_api.php
 * @uses gpc_api.php
 * @uses http_api.php
 * @uses utility_api.php
 */

# Prevent output of HTML in the content if errors occur
define( 'DISABLE_INLINE_ERROR_REPORTING', true );

$g_bypass_headers = true; # suppress headers as we will send our own later
define( 'COMPRESSION_DISABLED', true );

require_once( 'core.php' );
require_api( 'access_api.php' );
require_api( 'authentication_api.php' );
require_api( 'bug_api.php' );
require_api( 'config_api.php' );
require_api( 'constant_inc.php' );
require_api( 'database_api.php' );
require_api( 'file_api.php' );
require_api( 'gpc_api.php' );
require_api( 'http_api.php' );
require_api( 'utility_api.php' );

auth_ensure_user_authenticated();

$f_show_inline = gpc_get_bool( 'show_inline', false );

# To prevent cross-domain inline hotlinking to attachments we require a CSRF
# token from the user to show any attachment inline within the browser.
# Without this security in place a malicious user could upload a HTML file
# attachment and direct a user to file_download.php?file_id=X&type=bug&show_inline=1
# and the malicious HTML content would be rendered in the user's browser,
# violating cross-domain security.
if( $f_show_inline ) {
	# Disable errors for form_security_validate as we need to send HTTP
	# headers prior to raising an error (the error handler
	# doesn't check that headers have been sent, it just
	# makes the assumption that they've been sent already).
	if( !@form_security_validate( 'file_show_inline' ) ) {
		http_all_headers();
		trigger_error( ERROR_FORM_TOKEN_INVALID, ERROR );
	}
}

$f_file_id = gpc_get_int( 'file_id' );
$f_type	= gpc_get_string( 'type' );

$c_file_id = (integer)$f_file_id;

# we handle the case where the file is attached to a bug
# or attached to a project as a project doc.
$t_query = '';
switch( $f_type ) {
	case 'bug':
		$t_query = 'SELECT * FROM {bug_file} WHERE id=' . db_param();
		break;
	case 'doc':
		$t_query = 'SELECT * FROM {project_file} WHERE id=' . db_param();
		break;
	default:
		access_denied();
}
$t_result = db_query( $t_query, array( $c_file_id ) );
$t_row = db_fetch_array( $t_result );
if( false === $t_row ) {
	# Attachment not found
	error_parameters( $c_file_id );
	trigger_error( ERROR_FILE_NOT_FOUND, ERROR );
}
extract( $t_row, EXTR_PREFIX_ALL, 'v' );

if( $f_type == 'bug' ) {
	$t_project_id = bug_get_field( $v_bug_id, 'project_id' );
} else {
	$t_project_id = $v_project_id;
}

# Check access rights
switch( $f_type ) {
	case 'bug':
		# This covers access checks for issue attachments
		if( !file_can_download_bug_attachments( $v_bug_id, (int)$v_user_id ) ) {
			access_denied();
		}

		# This covers access checks for issue note attachments
		$t_attachment_note_id = (int)$v_bugnote_id;

		if( $t_attachment_note_id !== 0 ) {
			if( bugnote_get_field( $t_attachment_note_id, 'view_state' ) != VS_PUBLIC ) {

				$t_attachments_view_threshold = config_get( 'download_attachments_threshold', null, $v_user_id, $t_project_id );
				if( !access_has_bugnote_level( $t_attachments_view_threshold, $t_attachment_note_id ) ) {
					access_denied();
				}
			}
		}
		break;
	case 'doc':
		# Check if project documentation feature is enabled.
		if( OFF == config_get( 'enable_project_documentation' ) ) {
			access_denied();
		}

		access_ensure_project_level( config_get( 'view_proj_doc_threshold' ), $v_project_id );
		break;
}

# throw away output buffer contents (and disable it) to protect download
while( @ob_end_clean() ) {
}

if( ini_get( 'zlib.output_compression' ) && function_exists( 'ini_set' ) ) {
	ini_set( 'zlib.output_compression', false );
}

http_security_headers();

# Make sure that IE can download the attachments under https.
header( 'Pragma: public' );

# To fix an IE bug which causes problems when downloading
# attached files via HTTPS, we disable the "Pragma: no-cache"
# command when IE is used over HTTPS.
global $g_allow_file_cache;
if( http_is_protocol_https() && is_browser_internet_explorer() ) {
	# Suppress "Pragma: no-cache" header.
} else {
	if( !isset( $g_allow_file_cache ) ) {
		header( 'Pragma: no-cache' );
	}
}
header( 'Expires: ' . gmdate( 'D, d M Y H:i:s \G\M\T', time() ) );

header( 'Last-Modified: ' . gmdate( 'D, d M Y H:i:s \G\M\T', $v_date_added ) );

$t_upload_method = config_get( 'file_upload_method' );
$t_filename = file_get_display_name( $v_filename );

# Content headers
$t_content_type = $v_file_type;

$t_content_type_override = file_get_content_type_override( $t_filename );
$t_file_info_type = false;

switch( $t_upload_method ) {
	case DISK:
		$t_local_disk_file = file_normalize_attachment_path( $v_diskfile, $t_project_id );
		if( file_exists( $t_local_disk_file ) ) {
			$t_file_info_type = file_get_mime_type( $t_local_disk_file );
		}
		break;
	case DATABASE:
		$t_file_info_type = file_get_mime_type_for_content( $v_content );
		break;
	default:
		trigger_error( ERROR_GENERIC, ERROR );

}

if( $t_file_info_type !== false ) {
	$t_content_type = $t_file_info_type;
}

if( $t_content_type_override ) {
	$t_content_type = $t_content_type_override;
}

# Decide what should open inline in the browser vs. download as attachment
# https://www.thoughtco.com/mime-types-by-content-type-3469108
$t_show_inline = $f_show_inline;
$t_mime_force_inline = array(
	'image/jpeg', 'image/gif', 'image/tiff', 'image/bmp', 'image/svg+xml', 'image/png',
	'application/pdf' );
$t_mime_force_attachment = array( 'application/x-shockwave-flash', 'text/html' );

# extract mime type from content type
$t_mime_type = explode( ';', $t_content_type, 2 );
$t_mime_type = $t_mime_type[0];

if( in_array( $t_mime_type, $t_mime_force_inline ) ) {
	$t_show_inline = true;
} else if( in_array( $t_mime_type, $t_mime_force_attachment ) ) {
	$t_show_inline = false;
}

http_content_disposition_header( $t_filename, $t_show_inline );

header( 'Content-Type: ' . $t_content_type );
header( 'Content-Length: ' . $v_filesize );

# Don't let Internet Explorer second-guess our content-type [1]
# Also disable Flash content-type sniffing [2]
# [1] http://blogs.msdn.com/b/ie/archive/2008/07/02/ie8-security-part-v-comprehensive-protection.aspx
# [2] http://50.56.33.56/blog/?p=242
header( 'X-Content-Type-Options: nosniff' );

# dump file content to the connection.
switch( $t_upload_method ) {
	case DISK:
		readfile( $t_local_disk_file );
		break;
	case DATABASE:
		echo $v_content;
}
file_download.php (7,471 bytes)   

Relationships

related to 0026893 closedvboctor APIs expose private attachments to users who has access to issue but not private notes 
related to 0009802 closedvboctor Support attachments associated with private notes 
has duplicate 0027262 closeddregad Private files can be downloaded by attacker 
related to 0027299 closeddregad Remove code duplication in File API 
related to 0026631 closedvboctor file_get_visible_attachments shows private files that should be invisible to the user 

Activities

dregad

dregad

2020-06-16 12:48

developer   ~0064105

Thanks for the detailed bug report , I confirm the bug.

I will check your proposed patch.

For next time, instead of uploading the whole modified file, could you please provide a unified diff, git patch or even better submit a pull request.

dregad

dregad

2020-09-19 10:56

developer   ~0064454

@pijama I'm planning to request a CVE for this issue, would you like to be credited for the finding, and if so please indicate how.

dregad

dregad

2020-09-19 11:31

developer   ~0064455

Proposed fix, test and review comments welcome.

27039.patch (14,936 bytes)   
From b83248d3cdc7b6cf7d7fbd2262c224186b79ab3d Mon Sep 17 00:00:00 2001
From: Damien Regad <dregad@mantisbt.org>
Date: Sat, 12 Sep 2020 22:04:18 +0200
Subject: [PATCH 1/5] New file_can_view_or_download() function

file_can_view_bug_attachments() and file_can_download_bug_attachments()
have nearly identical code, the only difference being the names of the
configs.

Adding a new internal File API function to avoid code duplication.

Issue #27299
---
 core/file_api.php | 60 ++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 51 insertions(+), 9 deletions(-)

diff --git a/core/file_api.php b/core/file_api.php
index a4bee8989..2517aaba2 100644
--- a/core/file_api.php
+++ b/core/file_api.php
@@ -206,30 +206,72 @@ function file_bug_has_attachments( $p_bug_id ) {
 	}
 }
 
+/**
+ * Check if the current user can view or download attachments.
+ *
+ * Generic call used by
+ * - {@see file_can_view_bug_attachments()}
+ * - {@see file_can_view_bugnote_attachments}
+ * - {@see file_can_download_bug_attachments()}
+ * - {@see file_can_download_bugnote_attachments}
+ *
+ * @param string   $p_action            'view' or 'download'
+ * @param int      $p_bug_id            A bug identifier
+ * @param int      $p_uploader_user_id  The user who uploaded the attachment
+ *
+ * @return bool
+ *
+ * @internal Should not be used outside of File API.
+ */
+function file_can_view_or_download( $p_action, $p_bug_id, $p_uploader_user_id ) {
+	switch( $p_action ) {
+		case 'view':
+			$t_threshold_global = 'view_attachments_threshold';
+			$t_threshold_own = 'allow_view_own_attachments';
+			break;
+		case 'download':
+			$t_threshold_global = 'download_attachments_threshold';
+			$t_threshold_own = 'allow_download_own_attachments';
+			break;
+		default:
+			trigger_error( ERROR_GENERIC, ERROR );
+	}
+
+	$t_project_id = bug_get_field( $p_bug_id, 'project_id' );
+	$t_access_global = config_get( $t_threshold_global,null, null, $t_project_id );
+
+	$t_can_access = access_has_bug_level( $t_access_global, $p_bug_id );
+	if( $t_can_access ) {
+		return true;
+	}
+
+	$t_uploaded_by_me = auth_get_current_user_id() == $p_uploader_user_id;
+	$t_view_own = config_get( $t_threshold_own, null, null, $t_project_id );
+	return $t_uploaded_by_me && $t_view_own;
+}
+
 /**
  * Check if the current user can view attachments for the specified bug.
+ *
  * @param integer $p_bug_id           A bug identifier.
  * @param integer $p_uploader_user_id A user identifier.
+ *
  * @return boolean
  */
 function file_can_view_bug_attachments( $p_bug_id, $p_uploader_user_id = null ) {
-	$t_uploaded_by_me = auth_get_current_user_id() === $p_uploader_user_id;
-	$t_can_view = access_has_bug_level( config_get( 'view_attachments_threshold' ), $p_bug_id );
-	$t_can_view = $t_can_view || ( $t_uploaded_by_me && config_get( 'allow_view_own_attachments' ) );
-	return $t_can_view;
+	return file_can_view_or_download( 'view', $p_bug_id, $p_uploader_user_id );
 }
 
 /**
  * Check if the current user can download attachments for the specified bug.
+ *
  * @param integer $p_bug_id           A bug identifier.
- * @param integer $p_uploader_user_id A user identifier.
+ * @param integer $p_uploader_user_id The user who uploaded the attachment.
+ *
  * @return boolean
  */
 function file_can_download_bug_attachments( $p_bug_id, $p_uploader_user_id = null ) {
-	$t_uploaded_by_me = auth_get_current_user_id() === $p_uploader_user_id;
-	$t_can_download = access_has_bug_level( config_get( 'download_attachments_threshold', null, null, bug_get_field( $p_bug_id, 'project_id' ) ), $p_bug_id );
-	$t_can_download = $t_can_download || ( $t_uploaded_by_me && config_get( 'allow_download_own_attachments', null, null, bug_get_field( $p_bug_id, 'project_id' ) ) );
-	return $t_can_download;
+	return file_can_view_or_download( 'download', $p_bug_id, $p_uploader_user_id );
 }
 
 /**
-- 
2.25.1


From 1a24845b321ac3a5c62bc11524aaab770adc89be Mon Sep 17 00:00:00 2001
From: Damien Regad <dregad@mantisbt.org>
Date: Sat, 12 Sep 2020 22:09:06 +0200
Subject: [PATCH 2/5] Functions to check view/download ability at bugnote level

2 new File API functions:
- file_can_view_bugnote_attachments()
- file_can_download_bugnote_attachments

Prerequisite to fix issue #27039
---
 core/file_api.php | 41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/core/file_api.php b/core/file_api.php
index 2517aaba2..63de3e487 100644
--- a/core/file_api.php
+++ b/core/file_api.php
@@ -218,12 +218,13 @@ function file_bug_has_attachments( $p_bug_id ) {
  * @param string   $p_action            'view' or 'download'
  * @param int      $p_bug_id            A bug identifier
  * @param int      $p_uploader_user_id  The user who uploaded the attachment
+ * @param int|null $p_bugnote_id        If specified, will check at bugnote level
  *
  * @return bool
  *
  * @internal Should not be used outside of File API.
  */
-function file_can_view_or_download( $p_action, $p_bug_id, $p_uploader_user_id ) {
+function file_can_view_or_download( $p_action, $p_bug_id, $p_uploader_user_id, $p_bugnote_id = null ) {
 	switch( $p_action ) {
 		case 'view':
 			$t_threshold_global = 'view_attachments_threshold';
@@ -240,7 +241,11 @@ function file_can_view_or_download( $p_action, $p_bug_id, $p_uploader_user_id )
 	$t_project_id = bug_get_field( $p_bug_id, 'project_id' );
 	$t_access_global = config_get( $t_threshold_global,null, null, $t_project_id );
 
-	$t_can_access = access_has_bug_level( $t_access_global, $p_bug_id );
+	if( $p_bugnote_id === null ) {
+		$t_can_access = access_has_bug_level( $t_access_global, $p_bug_id );
+	} else {
+		$t_can_access = access_has_bugnote_level( $t_access_global, $p_bugnote_id );
+	}
 	if( $t_can_access ) {
 		return true;
 	}
@@ -262,6 +267,22 @@ function file_can_view_bug_attachments( $p_bug_id, $p_uploader_user_id = null )
 	return file_can_view_or_download( 'view', $p_bug_id, $p_uploader_user_id );
 }
 
+/**
+ * Check if the current user can view attachments for the specified bug note.
+ *
+ * @param integer $p_bugnote_id       A bugnote identifier.
+ * @param integer $p_uploader_user_id The user who uploaded the attachment.
+ *
+ * @return boolean
+ */
+function file_can_view_bugnote_attachments( $p_bugnote_id, $p_uploader_user_id = null ) {
+	if( $p_bugnote_id == 0 ) {
+		return true;
+	}
+	$t_bug_id = bugnote_get_field( $p_bugnote_id, 'bug_id' );
+	return file_can_view_or_download( 'view', $t_bug_id, $p_uploader_user_id );
+}
+
 /**
  * Check if the current user can download attachments for the specified bug.
  *
@@ -274,6 +295,22 @@ function file_can_download_bug_attachments( $p_bug_id, $p_uploader_user_id = nul
 	return file_can_view_or_download( 'download', $p_bug_id, $p_uploader_user_id );
 }
 
+/**
+ * Check if the current user can download attachments for the specified bug note.
+ *
+ * @param integer $p_bugnote_id       A bugnote identifier.
+ * @param integer $p_uploader_user_id The user who uploaded the attachment.
+ *
+ * @return boolean
+ */
+function file_can_download_bugnote_attachments( $p_bugnote_id, $p_uploader_user_id = null ) {
+	if( $p_bugnote_id == 0 ) {
+		return true;
+	}
+	$t_bug_id = bugnote_get_field( $p_bugnote_id, 'bug_id' );
+	return file_can_view_or_download( 'download', $t_bug_id, $p_uploader_user_id, $p_bugnote_id );
+}
+
 /**
  * Check if the current user can delete attachments from the specified bug.
  * @param integer $p_bug_id           A bug identifier.
-- 
2.25.1


From 2a44141d82ec9aed9e14431b895ee181b9194fc4 Mon Sep 17 00:00:00 2001
From: Damien Regad <dregad@mantisbt.org>
Date: Sat, 12 Sep 2020 22:21:09 +0200
Subject: [PATCH 3/5] 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`.

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

Fixes #27039
---
 core/file_api.php | 30 ++++++++++--------------------
 file_download.php |  4 +++-
 2 files changed, 13 insertions(+), 21 deletions(-)

diff --git a/core/file_api.php b/core/file_api.php
index 63de3e487..245b30353 100644
--- a/core/file_api.php
+++ b/core/file_api.php
@@ -451,31 +451,21 @@ function file_get_visible_attachments( $p_bug_id ) {
 
 	$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'];
 
@@ -483,14 +473,14 @@ function file_get_visible_attachments( $p_bug_id ) {
 		$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 d423681b0..d7404548a 100644
--- a/file_download.php
+++ b/file_download.php
@@ -110,7 +110,9 @@ 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;
-- 
2.25.1


From 8d9f4346963b2dc5462fb27700f4c60ea4a80a1e Mon Sep 17 00:00:00 2001
From: Damien Regad <dregad@mantisbt.org>
Date: Sat, 12 Sep 2020 22:24:21 +0200
Subject: [PATCH 4/5] Improve PHPDoc for file_get_visible_attachments()

---
 core/file_api.php | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/core/file_api.php b/core/file_api.php
index 245b30353..67e4cecef 100644
--- a/core/file_api.php
+++ b/core/file_api.php
@@ -422,19 +422,21 @@ function file_normalize_attachment_path( $p_diskfile, $p_project_id ) {
 
 /**
  * Gets an array of attachments that are visible to the currently logged in user.
+ *
  * Each element of the array contains the following:
- * display_name - The attachment display name (i.e. file name dot extension)
- * size - The attachment size in bytes.
- * date_added - The date where the attachment was added.
- * can_download - true: logged in user has access to download the attachment, false: otherwise.
- * diskfile - The name of the file on disk.  Typically this is a hash without an extension.
- * download_url - The download URL for the attachment (only set if can_download is true).
- * exists - Applicable for DISK attachments.  true: file exists, otherwise false.
- * can_delete - The logged in user can delete the attachments.
- * preview - true: the attachment should be previewable, otherwise false.
- * type - Can be "image", "text" or empty for other types.
- * alt - The alternate text to be associated with the icon.
- * icon - array with icon information, contains 'url' and 'alt' elements.
+ * - display_name - The attachment display name (i.e. file name dot extension)
+ * - size - The attachment size in bytes.
+ * - date_added - The date where the attachment was added.
+ * - can_download - true: logged in user has access to download the attachment, false: otherwise.
+ * - diskfile - The name of the file on disk.  Typically this is a hash without an extension.
+ * - download_url - The download URL for the attachment (only set if can_download is true).
+ * - exists - Applicable for DISK attachments.  true: file exists, otherwise false.
+ * - can_delete - The logged in user can delete the attachments.
+ * - preview - true: the attachment should be previewable, otherwise false.
+ * - type - Can be "image", "text" or empty for other types.
+ * - alt - The alternate text to be associated with the icon.
+ * - icon - array with icon information, contains 'url' and 'alt' elements.
+ *
  * @param integer $p_bug_id A bug identifier.
  * @return array
  */
-- 
2.25.1


From 32f1a5f3f4c7e78e4655ca0faaa616d97170c1c9 Mon Sep 17 00:00:00 2001
From: Damien Regad <dregad@mantisbt.org>
Date: Sat, 12 Sep 2020 22:25:06 +0200
Subject: [PATCH 5/5] Fix PHPStorm undefined variable warnings

---
 file_download.php | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/file_download.php b/file_download.php
index d7404548a..9ed9b5f44 100644
--- a/file_download.php
+++ b/file_download.php
@@ -99,6 +99,18 @@ if( false === $t_row ) {
 	error_parameters( $c_file_id );
 	trigger_error( ERROR_FILE_NOT_FOUND, ERROR );
 }
+/**
+ * @var int    $v_bug_id
+ * @var int    $v_project_id
+ * @var string $v_diskfile
+ * @var string $v_filename
+ * @var int    $v_filesize
+ * @var string $v_file_type
+ * @var string $v_content
+ * @var int    $v_date_added
+ * @var int    $v_user_id
+ * @var int    $v_bugnote_id
+ */
 extract( $t_row, EXTR_PREFIX_ALL, 'v' );
 
 if( $f_type == 'bug' ) {
-- 
2.25.1

27039.patch (14,936 bytes)   
dregad

dregad

2020-09-19 11:32

developer   ~0064456

CVE Request 961621 sent

dregad

dregad

2020-09-20 07:10

developer   ~0064458

Last edited: 2020-09-20 07:20

CVE-2020-25781 assigned.

vboctor

vboctor

2020-09-20 23:13

manager   ~0064462

@dregad I'm missing where you are checking that if a user is trying to view/download an attachment that is associated with a private note, that they have access to such private note.

dregad

dregad

2020-09-21 12:23

developer   ~0064470

@vboctor the logic takes place in new File API functions file_can_view_bugnote_attachments() / file_can_download_bugnote_attachments().

For the scenario where a is not allowed to view bugnote attachments, file_get_visible_attachments() now excludes them (file_api.php line 461).

For a direct call to file_download.php to get bug attachments, if the latter returns false, user gets an access denied error (line 114), see commit Check ability to download attachments at bugnote level

Related Changesets

MantisBT: master-2.24 5595c90f

2020-09-12 12:09

dregad


Details Diff
Functions to check view/download ability at bugnote level

2 new File API functions:
- file_can_view_bugnote_attachments()
- file_can_download_bugnote_attachments

Prerequisite to fix issue 0027039
Affected Issues
0027039
mod - core/file_api.php Diff File

MantisBT: master-2.24 9de20c09

2020-09-12 12:21

dregad


Details Diff
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 0027039
Affected Issues
0027039
mod - core/file_api.php Diff File
mod - file_download.php Diff File