Fix XSS in adm_config_report.php's action parameter

Yelin and Zhangdongsheng from VenusTech http://www.venustech.com.cn/ reported a vulnerability in the Configuration Report page, allowing an attacker to inject arbitrary code through a crafted 'action' parameter. Define a new set of constants (MANAGE_CONFIG_ACTION_*) replacing the hardcoded strings used in adm_config_report.php and adm_config_set.php. Sanitize the 'action' parameter to ensure it is only set to one of the allowed values Fixes #22537
mantisbt · Mar 20, 2017 · 15e52e8 · 15e52e8
1 parent 5efd115
commit 15e52e8
Show file tree

Hide file tree

Showing 3 changed files with 20 additions and 6 deletions.
diff --git a/adm_config_report.php b/adm_config_report.php
@@ -218,7 +218,17 @@ function check_config_value( $p_config ) {
 $t_edit_option          = gpc_get_string( 'config_option', $t_filter_config_value == META_FILTER_NONE ? '' : $t_filter_config_value );
 $t_edit_type            = gpc_get_string( 'type', CONFIG_TYPE_DEFAULT );
 $t_edit_value           = gpc_get_string( 'value', '' );
-$t_edit_action          = gpc_get_string( 'action', 'action_create' );
+
+$f_edit_action          = gpc_get_string( 'action', MANAGE_CONFIG_ACTION_CREATE );
+# Ensure we exclusively use one of the defined, valid actions (XSS protection)
+$t_valid_actions = array(
+	MANAGE_CONFIG_ACTION_CREATE,
+	MANAGE_CONFIG_ACTION_CLONE,
+	MANAGE_CONFIG_ACTION_EDIT
+);
+$t_edit_action = in_array( $f_edit_action, $t_valid_actions )
+	? $f_edit_action
+	: MANAGE_CONFIG_ACTION_CREATE;
 
 # Apply filters
 
@@ -443,7 +453,7 @@ function check_config_value( $p_config ) {
 					'config_option' => $v_config_id,
 					'type'          => $v_type,
 					'value'         => $v_value,
-					'action'        => 'action_edit',
+					'action'        => MANAGE_CONFIG_ACTION_EDIT,
 				),
 				OFF );
 			echo '</div>';
@@ -459,7 +469,7 @@ function check_config_value( $p_config ) {
 					'config_option' => $v_config_id,
 					'type'          => $v_type,
 					'value'         => $v_value,
-					'action'        => 'action_clone',
+					'action'        => MANAGE_CONFIG_ACTION_CLONE,
 				),
 				OFF );
 			echo '</div>';
@@ -514,7 +524,7 @@ function check_config_value( $p_config ) {
 		<div class="widget-header widget-header-small">
 		<h4 class="widget-title lighter">
 			<i class="ace-icon fa fa-sliders"></i>
-			<?php echo lang_get( 'set_configuration_option_' . $t_edit_action ) ?>
+			<?php echo lang_get( 'set_configuration_option_action_' . $t_edit_action ) ?>
 			</h4>
 		</div>
 
@@ -605,7 +615,7 @@ function check_config_value( $p_config ) {
 		<div class="widget-toolbox padding-4 clearfix">
 			<input type="hidden" name="action" value="<?php echo $t_edit_action; ?>" />
 			<input type="submit" name="config_set" class="btn btn-primary btn-white btn-round"
-				value="<?php echo lang_get( 'set_configuration_option_' . $t_edit_action ) ?>"/>
+				value="<?php echo lang_get( 'set_configuration_option_action_' . $t_edit_action ) ?>"/>
 		</div>
 	</div>
 	</div>

diff --git a/adm_config_set.php b/adm_config_set.php
@@ -134,7 +134,7 @@
 	}
 }
 
-if( 'action_edit' === $f_edit_action ){
+if( MANAGE_CONFIG_ACTION_EDIT === $f_edit_action ){
 	# EDIT action doesn't keep original if key values are different.
 	if ( $f_original_config_option !== $f_config_option
 			|| $f_original_user_id !== $f_user_id

diff --git a/core/constant_inc.php b/core/constant_inc.php
@@ -646,3 +646,7 @@
 # types, 2^31 is a safe limit to be used for all.
 define( 'DB_MAX_INT', 2147483647 );
 
+# Configuration management actions (adm_config_report.php)
+define( 'MANAGE_CONFIG_ACTION_CREATE', 'create' );
+define( 'MANAGE_CONFIG_ACTION_CLONE', 'clone' );
+define( 'MANAGE_CONFIG_ACTION_EDIT', 'edit' );