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

@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.

@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.

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:

• bug_change_status_page.php
• view_all_bug_page.php & print_all_bug_page.php

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 ?

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

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

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. <script> instead of <script> for <script>.

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

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.

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

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

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.

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

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