Fix #13191: Prevent further XSS issues relating to PHP_SELF

davidhicks · davidhicks · commit e679a1c02978 · 2011-09-05T23:36:39.000+10:00
Silvia Alvarez (Debian package manager for MantisBT) has performed additional testing of patch d00745f and discovered that the XSS problems surrounding the use of $_SERVER['PHP_SELF'] have not been fully fixed. The form_action_self() function also used $_SERVER['PHP_SELF'] (retrieving the basename() component of the file name). Callees of this function did not escape this file name prior to printing it in the 'action' attribute of <form> elements. This patch swaps out PHP_SELF for SCRIPT_NAME (much safer as end users have no control over the value) and also applies escaping to the 'action' attribute of relevant <form> elements. Refer to Debian bug report #640297 [1] and dicussion on the mantisbt-dev mailing list for further details. Thank you Sils for the detailed analysis of the problem, detailed report, proposed solutions and extensive testing of patches for the latest round of vulnerabilities discovered. [1] http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=640297
diff --git a/billing_inc.php b/billing_inc.php
@@ -75,7 +75,7 @@
 	# CSRF protection not required here - form does not result in modifications
 ?>
 
-<form method="post" action="<?php echo form_action_self() ?>">
+<form method="post" action="<?php echo string_attribute( form_action_self() ) ?>">
 	<input type="hidden" name="id" value="<?php echo isset( $f_bug_id ) ? $f_bug_id : 0 ?>" />
 	<table border="0" class="width100" cellspacing="0">
 		<tr>
diff --git a/bugnote_stats_inc.php b/bugnote_stats_inc.php
@@ -65,7 +65,7 @@
 	# CSRF protection not required here - form does not result in modifications
 ?>
 
-<form method="post" action="<?php echo form_action_self() . '#bugnotestats' ?>">
+<form method="post" action="<?php echo string_attribute( form_action_self() . '#bugnotestats' ) ?>">
 	<input type="hidden" name="id" value="<?php echo $f_bug_id ?>" />
 	<table border=0 class="width100" cellspacing="0">
 		<tr>
diff --git a/core/authentication_api.php b/core/authentication_api.php
@@ -694,7 +694,7 @@ function auth_reauthenticate_page( $p_user_id, $p_username ) {
 	}
 	?>
 </p>
-<form name="reauth_form" method="post" action="<?php echo form_action_self();?>">
+<form name="reauth_form" method="post" action="<?php echo string_attribute( form_action_self() ) ?>">
 <?php
 	# CSRF protection not required here - user needs to enter password
 	# (confirmation step) before the form is accepted.
diff --git a/core/form_api.php b/core/form_api.php
@@ -35,7 +35,8 @@
  * @return string Form action value
  */
 function form_action_self() {
-	return basename($_SERVER['PHP_SELF']);
+	$t_self = trim( str_replace( "\0", '', $_SERVER['SCRIPT_NAME'] ) );
+	return basename( $t_self );
 }
 
 /**
diff --git a/core/helper_api.php b/core/helper_api.php
@@ -299,7 +299,7 @@ function helper_ensure_confirmed( $p_message, $p_button_label ) {
 	print_hr();
 	echo "\n$p_message\n";
 
-	echo '<form method="post" action="' . form_action_self() . "\">\n";
+	echo '<form method="post" action="' . string_attribute( form_action_self() ) . "\">\n";
 	# CSRF protection not required here - user needs to confirm action
 	# before the form is accepted.
 	print_hidden_inputs( gpc_strip_slashes( $_POST ) );
diff --git a/manage_config_email_page.php b/manage_config_email_page.php
@@ -286,7 +286,7 @@ function get_section_end_for_email() {
 			echo form_security_field( 'manage_config_revert' );
 			echo "<input name=\"revert\" type=\"hidden\" value=\"notify_flags,default_notify_flags\"></input>";
 			echo "<input name=\"project\" type=\"hidden\" value=\"$t_project\"></input>";
-			echo "<input name=\"return\" type=\"hidden\" value=\"" . form_action_self() ."\"></input>";
+			echo "<input name=\"return\" type=\"hidden\" value=\"" . string_attribute( form_action_self() ) ."\"></input>";
 			echo "<input type=\"submit\" class=\"button\" value=\"";
 			if ( ALL_PROJECTS == $t_project ) {
                 echo lang_get( 'revert_to_system' );
diff --git a/manage_config_work_threshold_page.php b/manage_config_work_threshold_page.php
@@ -326,7 +326,7 @@ function get_section_end() {
 		echo form_security_field( 'manage_config_revert' );
         echo "<input name=\"revert\" type=\"hidden\" value=\"" . implode( ',', $t_overrides ) . "\"></input>";
         echo "<input name=\"project\" type=\"hidden\" value=\"$t_project_id\"></input>";
-        echo "<input name=\"return\" type=\"hidden\" value=\"" . form_action_self() ."\"></input>";
+        echo "<input name=\"return\" type=\"hidden\" value=\"" . string_attribute( form_action_self() ) ."\"></input>";
         echo "<input type=\"submit\" class=\"button\" value=\"";
         if ( ALL_PROJECTS == $t_project_id ) {
             echo lang_get( 'revert_to_system' );
diff --git a/manage_config_workflow_page.php b/manage_config_workflow_page.php
@@ -477,7 +477,7 @@ function access_row() {
 			echo form_security_field( 'manage_config_revert' );
             echo "<input name=\"revert\" type=\"hidden\" value=\"" . implode( ',', $t_overrides ) . "\"></input>";
             echo "<input name=\"project\" type=\"hidden\" value=\"$t_project\"></input>";
-            echo "<input name=\"return\" type=\"hidden\" value=\"" . form_action_self() ."\"></input>";
+            echo "<input name=\"return\" type=\"hidden\" value=\"" . string_attribute( form_action_self() ) ."\"></input>";
             echo "<input type=\"submit\" class=\"button\" value=\"";
             if ( ALL_PROJECTS == $t_project ) {
                 echo lang_get( 'revert_to_system' );

Original file line number	Diff line number	Diff line change
`@@ -694,7 +694,7 @@ function auth_reauthenticate_page( $p_user_id, $p_username ) {`
`694`	`694`	`}`
`695`	`695`	`?>`
`696`	`696`	`</p>`
`697`		`-<form name="reauth_form" method="post" action="<?php echo form_action_self();?>">`
	`697`	`+<form name="reauth_form" method="post" action="<?php echo string_attribute( form_action_self() ) ?>">`
`698`	`698`	`<?php`
`699`	`699`	`# CSRF protection not required here - user needs to enter password`
`700`	`700`	`# (confirmation step) before the form is accepted.`
Original file line number	Diff line number	Diff line change
`@@ -35,7 +35,8 @@`
`35`	`35`	`* @return string Form action value`
`36`	`36`	`*/`
`37`	`37`	`function form_action_self() {`
`38`		`- return basename($_SERVER['PHP_SELF']);`
	`38`	`+ $t_self = trim( str_replace( "\0", '', $_SERVER['SCRIPT_NAME'] ) );`
	`39`	`+ return basename( $t_self );`
`39`	`40`	`}`
`40`	`41`
`41`	`42`	`/**`