From 15e52e84c389afe8b03ed3cdb59b6549257ed197 Mon Sep 17 00:00:00 2001
From: Damien Regad <dregad@mantisbt.org>
Date: Fri, 17 Mar 2017 15:09:09 +0100
Subject: [PATCH] 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
---
 adm_config_report.php | 20 +++++++++++++++-----
 adm_config_set.php    |  2 +-
 core/constant_inc.php |  4 ++++
 3 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/adm_config_report.php b/adm_config_report.php
index 8c0f017dbe..37e8189e47 100644
--- 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
index 23c894770e..c9e41e97a7 100644
--- 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
index 4a33a82e3f..4ad7209850 100644
--- 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' );