View Issue Details

IDProjectCategoryView StatusLast Update
0034432mantisbtsecuritypublic2024-05-12 15:39
Reporterunboundeduniverse Assigned Todregad  
PrioritynormalSeveritymajorReproducibilityalways
Status closedResolutionfixed 
Product Version2.26.1 
Target Version2.26.2Fixed in Version2.26.2 
Summary0034432: CVE-2024-34081: Unsanitised custom field names printed
Description

Improper escaping of a custom field's name allows an attacker to inject HTML and, if CSP settings permit, achieve execution of arbitrary JavaScript when:

  • resolving or closing issues (bug_change_status_page.php) belonging to a project linking said custom field
  • viewing issues (view_all_bug_page.php) when the custom field is displayed as a column
  • printing issues (print_all_bug_page.php) when the custom field is displayed as a column

https://github.com/mantisbt/mantisbt/security/advisories/GHSA-wgx7-jp56-65mq

Steps To Reproduce

Create a custom field with some html in its name.
Create an issue with this custom field.
View the issue (view.php).
In the status list, choose resolved or closed and click on the "Change Status To:" button.
The html in the custom field name is rendered (bug_change_status_page.php)

Additional Information

To fix:

bug_change_status_page.php
line 306:

<?php echo lang_get_defaulted( $t_def['name'] ) ?>

change to:

<?php echo string_attribute( lang_get_defaulted( $t_def['name'] ) ) ?>

similar to 0027304

TagsNo tags attached.

Relationships

related to 0027304 closeddregad CVE-2020-25830: HTML Injection in bug_actiongroup_page.php 
related to 0034441 closeddregad Excel error when opening exported issues with custom field with special characters 
related to 0034442 resolveddregad Wrong display of some column titles on "View Issues" page 

Activities

atrol

atrol

2024-05-04 13:49

developer   ~0068884

Last edited: 2024-05-04 14:33

I started some tests with such kind of custom fields.
"View Issues" and "Print Reports" pages are also affected.

dregad

dregad

2024-05-06 06:20

developer   ~0068896

@unboundeduniverse Thanks for the report.

  • Please confirm that your Github username is https://github.com/unboundeduniverse and if not tell us what it is.
  • Would you like to be credited for the finding ? If so, please tell us how.

@atrol
Where do you stand on this issue ? Is your testing completed ? Are you working on a patch or should I do it ?

For now I have opened Advisory GHSA-wgx7-jp56-65mq and requested a CVE ID.

atrol

atrol

2024-05-06 11:12

developer   ~0068904

Last edited: 2024-05-06 11:17

@dregad my testing is completed.

I had also a short look at the code, and found that we don't use only string_attribute for custom field names, but also string_display line.
This should be harmonized to use always string_attribute as we don't want valid html tags to be interpreted.
e. g. with standard settings of $g_html_valid_tags_single_line, <i>custom field name will be italic on "View Issue Details" and some other pages.
We can treat this as a separate issue, or fix it as part of this issue.

Please create a temporary private fork for the security advisory you created.
After that, I could start working on a patch and create a PR.

If you want to have a very quick fix to get out 2.26.2 ASAP in combination with other security related fixes, it would be better if you start working on it.

dregad

dregad

2024-05-06 12:17

developer   ~0068905

OK I will create the private fork.

I also don't mind patching the security issue so we can release a hotfix ASAP if you have no time. I was just asking first, to avoid both of us making the same changes simultaneously.

Just making sure I have not missed anything you may have found in your tests, here is what I have identified:

we don't use only string_attribute for custom field names, but also string_display line.
This should be harmonized to use always string_attribute as we don't want valid html tags to be interpreted.

Right.

$ git grep "string_display_line.*def.*name"
bug_report_page.php:655:                                        <?php echo string_display_line( lang_get_defaulted( $t_def['name'] ) ) ?>
bug_report_page.php:657:                        <?php } else { echo string_display_line( lang_get_defaulted( $t_def['name'] ) ); } ?>
bug_update_page.php:718:                echo '<span>', string_display_line( lang_get_defaulted( $t_def['name'] ) ), '</span>';
bug_view_inc.php:653:           echo '<th class="bug-custom-field category">', string_display_line( lang_get_defaulted( $t_def['name'] ) ), '</th>';
core/filter_form_api.php:2085:                          $t_field_name = string_display_line( lang_get_defaulted( column_get_custom_field_name( $t_sort ) ) );
core/filter_form_api.php:2119:                  $t_field_name = string_display_line( lang_get_defaulted( column_get_custom_field_name( $t_column ) ) );
core/filter_form_api.php:2831:                          $t_header = $get_field_header( 'custom_field_' . $t_cfdef['id'] . '_filter', string_display_line( lang_get_defaulted( $t_cfdef['name'] ) ) );
print_all_bug_page_word.php:396:                <?php echo string_display_line( sprintf( lang_get( 'label' ), lang_get_defaulted( $t_def['name'] ) ) ) ?>

Any others ?

atrol

atrol

2024-05-06 12:44

developer   ~0068906

Concerning

view_all_bug_page.php & print_all_bug_page.php

Wouldn't it be better to fix it on a lower level where the values are finally printed?
I thought about a change in print_link
Something like attached i34432.patch

i34432.patch (1,990 bytes)   
From b7abf7a490019cbf6767eb79145a5fa2a4c1a138 Mon Sep 17 00:00:00 2001
From: Roland Becker <roland@atrol.de>
Date: Mon, 6 May 2024 17:36:50 +0200
Subject: [PATCH] Sanitize custom field names before printing

Fixes #34432
---
 bug_change_status_page.php | 2 +-
 core/print_api.php         | 9 +++++----
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/bug_change_status_page.php b/bug_change_status_page.php
index 427850389..a771f2d6c 100644
--- a/bug_change_status_page.php
+++ b/bug_change_status_page.php
@@ -303,7 +303,7 @@ layout_page_begin();
 	<tr>
 		<th class="category">
 			<?php if( $t_require && $t_has_write_access ) {?><span class="required">*</span><?php } ?>
-			<?php echo lang_get_defaulted( $t_def['name'] ) ?>
+			<?php echo string_attribute( lang_get_defaulted( $t_def['name'] ) ) ?>
 		</th>
 		<td>
 <?php
diff --git a/core/print_api.php b/core/print_api.php
index c52bc3bf8..d7ec8947b 100644
--- a/core/print_api.php
+++ b/core/print_api.php
@@ -1492,17 +1492,18 @@ function print_bracket_link_prepared( $p_link ) {
  * @return void
  */
 function print_link( $p_link, $p_url_text, $p_new_window = false, $p_class = '' ) {
+	$t_url_text = string_attribute( $p_url_text );
 	if( is_blank( $p_link ) ) {
-		echo $p_url_text;
+		echo $t_url_text;
 	} else {
 		$t_link = htmlspecialchars( $p_link );
 		if( $p_new_window === true ) {
-			echo '<a class="new-window ' . $p_class . '" href="' . $t_link . '" target="_blank">' . $p_url_text . '</a>';
+			echo '<a class="new-window ' . $p_class . '" href="' . $t_link . '" target="_blank">' . $t_url_text . '</a>';
 		} else {
 			if( $p_class !== '' ) {
-				echo '<a class="' . $p_class . '" href="' . $t_link . '">' . $p_url_text . '</a>';
+				echo '<a class="' . $p_class . '" href="' . $t_link . '">' . $t_url_text . '</a>';
 			} else {
-				echo '<a href="' . $t_link . '">' . $p_url_text . '</a>';
+				echo '<a href="' . $t_link . '">' . $t_url_text . '</a>';
 			}
 		}
 	}
-- 
2.39.1.windows.1

i34432.patch (1,990 bytes)   
dregad

dregad

2024-05-06 13:00

developer   ~0068907

Wouldn't it be better to fix it on a lower level where the values are finally printed?

You're probably right.

I was concerned about those usages where print_link() is already given an escaped string, in which case we would get double encoding, e.g. &amp;lt;script&amp;gt; instead of &lt;script&gt; for <script>.

But there are only 11 occurrences of print_link() calls so it should be quick to find and fix them if needed.

atrol

atrol

2024-05-06 13:17

developer   ~0068908

I also don't mind patching the security issue so we can release a hotfix ASAP if you have no time.

Would be good if you could patch.
I am on travel starting tomorrow very early in the morning.
If needed, I could review / test not earlier than Wednesday evening.

unboundeduniverse

unboundeduniverse

2024-05-06 14:57

reporter   ~0068909

Yes that is my Github username.
No need for credit.

dregad

dregad

2024-05-06 17:50

developer   ~0068910

Thanks for the feedback @unboundeduniverse. I have added you to the draft Advisory, so you can access the private repository with the fix.

unboundeduniverse

unboundeduniverse

2024-05-07 10:33

reporter   ~0068917

Just to add a bit of info: if the custom field is set to be required on update then it will appear for all status options when changing status, not just resolved and closed. This doesn't change the fix - it looks ok.

unboundeduniverse

unboundeduniverse

2024-05-10 07:15

reporter   ~0068923

They also should be sanitised in the excel export.
Name a custom field as Custom <Field 1
Do an excel export which includes it in the columns.
Excel can'tĀ open the file because of errors.

excel_api.php
line 102:
return '<Cell><Data ss:Type="String">' . $p_column_title . '</Data></Cell>';

change it to:
return excel_prepare_string( $p_column_title );

dregad

dregad

2024-05-10 08:39

developer   ~0068924

Last edited: 2024-05-12 07:00

Right, good point. I didn't bother testing Excel export as I was focused on HTML injection which is not applicable in this context.
Will fix.

Follow-up in 0034441

Related Changesets

MantisBT: master-2.26 447a521a

2024-05-06 13:04

dregad


Details Diff
Proper escaping of Custom Field name for display

Fixes XSS vulnerability on
- bug_change_status_page.php (resolving and closing issues)
- view_all_bug_page.php & print_all_bug_page.php (when the custom field
is selected as a column for display/print)

Fixes 0034432, CVE-2024-34081
Affected Issues
0034432, 0034442
mod - bug_change_status_page.php Diff File
mod - core/print_api.php Diff File