Skip to content

Commit

Permalink
Rework the bug action group api such that we can easily convert this …
Browse files Browse the repository at this point in the history
…to an object in the future, and to validate calls to require once.

This leads to a security issue identified by IBM's Appscan program, whereby calls to require_once are not validated.
Depending on webserver configuration, this is a file inclusion vulnerability.

There will be a follow up commit to config api - probably:
-		if( $g_project_override != null ) {
+		if( $g_project_override != null && $p_project == null ) {

At the moment, the action group API calls config_get with a project parameter to use. This is ignored, due to project_override being set - so we either need to:
a) change project override within the command list function
b) modifify config api to only use the project override *if* it is attempting to look up information on the default project.
  • Loading branch information
mantis committed Aug 29, 2011
1 parent 224b0f8 commit a908cc6
Show file tree
Hide file tree
Showing 8 changed files with 36 additions and 47 deletions.
2 changes: 1 addition & 1 deletion bug_actiongroup_add_note_inc.php
Expand Up @@ -32,7 +32,7 @@
* @uses utility_api.php
*/

if ( !defined( 'BUG_ACTIONGROUP_ADD_NOTE_INC_ALLOW' ) ) {
if ( !defined( 'BUG_ACTIONGROUP_INC_ALLOW' ) ) {
return;
}

Expand Down
2 changes: 1 addition & 1 deletion bug_actiongroup_attach_tags_inc.php
Expand Up @@ -30,7 +30,7 @@
* @uses tag_api.php
*/

if ( !defined( 'BUG_ACTIONGROUP_ATTACH_TAGS_INC_ALLOW' ) ) {
if ( !defined( 'BUG_ACTIONGROUP_INC_ALLOW' ) ) {
return;
}

Expand Down
3 changes: 1 addition & 2 deletions bug_actiongroup_ext.php
Expand Up @@ -57,12 +57,11 @@
$f_action = gpc_get_string( 'action' );
$f_bug_arr = gpc_get_int_array( 'bug_arr', array() );

$t_action_include_file = 'bug_actiongroup_' . $f_action . '_inc.php';
$t_form_name = 'bug_actiongroup_' . $f_action;

form_security_validate( $t_form_name );

require_once( dirname( __FILE__ ) . DIRECTORY_SEPARATOR . $t_action_include_file );
bug_group_action_init( $f_action );

# group bugs by project
$t_projects_bugs = array();
Expand Down
27 changes: 6 additions & 21 deletions bug_actiongroup_ext_page.php
Expand Up @@ -30,10 +30,10 @@
* @uses utility_api.php
*/

/**
* MantisBT Core API's
*/
require_once( 'core.php' );
if ( !defined( 'BUG_ACTIONGROUP_INC_ALLOW' ) ) {
return;
}

require_api( 'authentication_api.php' );
require_api( 'bug_group_action_api.php' );
require_api( 'form_api.php' );
Expand All @@ -42,27 +42,12 @@
require_api( 'string_api.php' );
require_api( 'utility_api.php' );

auth_ensure_user_authenticated();

$f_action = gpc_get_string( 'action' );
$f_bug_arr = gpc_get_int_array( 'bug_arr', array() );

# redirect to view issues if nothing is selected
if ( is_blank( $f_action ) || ( 0 == count( $f_bug_arr ) ) ) {
print_header_redirect( 'view_all_bug_page.php' );
}

# redirect to view issues page if action doesn't have ext_* prefix.
# This should only occur if this page is called directly.
$t_external_action_prefix = 'EXT_';
if ( strpos( $f_action, $t_external_action_prefix ) !== 0 ) {
print_header_redirect( 'view_all_bug_page.php' );
}

$t_external_action = utf8_strtolower( utf8_substr( $f_action, utf8_strlen( $t_external_action_prefix ) ) );
$t_form_fields_page = 'bug_actiongroup_' . $t_external_action . '_inc.php';
$t_form_name = 'bug_actiongroup_' . $t_external_action;

bug_group_action_init( $t_external_action );

bug_group_action_print_top();
?>

Expand Down
5 changes: 5 additions & 0 deletions bug_actiongroup_page.php
Expand Up @@ -73,6 +73,7 @@
# run through the issues to see if they are all from one project
$t_project_id = ALL_PROJECTS;
$t_multiple_projects = false;
$t_projects = array();

bug_cache_array_rows( $f_bug_arr );

Expand All @@ -83,11 +84,13 @@
$t_multiple_projects = true;
} else {
$t_project_id = $t_bug->project_id;
$t_projects[$t_project_id] = $t_project_id;
}
}
}
if ( $t_multiple_projects ) {
$t_project_id = ALL_PROJECTS;
$t_projects[ALL_PROJECTS] = ALL_PROJECTS;
}
# override the project if necessary
if( $t_project_id != helper_get_current_project() ) {
Expand All @@ -96,6 +99,8 @@
$g_project_override = $t_project_id;
}

define( 'BUG_ACTIONGROUP_INC_ALLOW', true );

$t_finished = false;
$t_bugnote = false;

Expand Down
2 changes: 1 addition & 1 deletion bug_actiongroup_update_product_build_inc.php
Expand Up @@ -27,7 +27,7 @@
* @uses lang_api.php
*/

if ( !defined( 'BUG_ACTIONGROUP_UPDATE_PRODUCT_BUILD_INC_ALLOW' ) ) {
if ( !defined( 'BUG_ACTIONGROUP_INC_ALLOW' ) ) {
return;
}

Expand Down
2 changes: 1 addition & 1 deletion bug_actiongroup_update_severity_inc.php
Expand Up @@ -28,7 +28,7 @@
* @uses print_api.php
*/

if ( !defined( 'BUG_ACTIONGROUP_UPDATE_SEVERITY_INC_ALLOW' ) ) {
if ( !defined( 'BUG_ACTIONGROUP_INC_ALLOW' ) ) {
return;
}

Expand Down
40 changes: 20 additions & 20 deletions core/bug_group_action_api.php
Expand Up @@ -42,6 +42,26 @@

require_css( 'status_config.php' );

/**
* Initialise bug action group api
*/
function bug_group_action_init( $p_action ) {
$t_valid_actions = bug_group_action_get_commands( current_user_get_accessible_projects() );
$t_action = strtoupper($p_action);

if( !isset( $t_valid_actions[$t_action] ) &&
!isset( $t_valid_actions['EXT_' . $t_action] )
) {
trigger_error( ERROR_GENERIC, ERROR );
}

$t_include_file = config_get_global( 'absolute_path' ) . 'bug_actiongroup_' . $p_action . '_inc.php';
if( !file_exists( $t_include_file ) ) {
trigger_error( ERROR_GENERIC, ERROR );
} else {
require_once( $t_include_file );
}
}

/**
* Print the top part for the bug action group page.
Expand Down Expand Up @@ -118,11 +138,6 @@ function bug_group_action_print_hidden_fields( $p_bug_ids_array ) {
* @param $p_action The custom action name without the "EXT_" prefix.
*/
function bug_group_action_print_action_fields( $p_action ) {
$t_include_definition = strtoupper( 'bug_actiongroup_' . $p_action . '_inc_allow' );
if( !defined( $t_include_definition ) ) {
define( $t_include_definition, true );
}
require_once( config_get_global( 'absolute_path' ) . 'bug_actiongroup_' . $p_action . '_inc.php' );
$t_function_name = 'action_' . $p_action . '_print_fields';
$t_function_name();
}
Expand All @@ -134,11 +149,6 @@ function bug_group_action_print_action_fields( $p_action ) {
* @param $p_action The custom action name without the "EXT_" prefix.
*/
function bug_group_action_print_title( $p_action ) {
$t_include_definition = strtoupper( 'bug_actiongroup_' . $p_action . '_inc_allow' );
if( !defined( $t_include_definition ) ) {
define( $t_include_definition, true );
}
require_once( config_get_global( 'absolute_path' ) . 'bug_actiongroup_' . $p_action . '_inc.php' );
$t_function_name = 'action_' . $p_action . '_print_title';
$t_function_name();
}
Expand All @@ -153,11 +163,6 @@ function bug_group_action_print_title( $p_action ) {
* @returns true|array true if action can be applied or array of ( bug_id => reason for failure to validate )
*/
function bug_group_action_validate( $p_action, $p_bug_id ) {
$t_include_definition = strtoupper( 'bug_actiongroup_' . $p_action . '_inc_allow' );
if( !defined( $t_include_definition ) ) {
define( $t_include_definition, true );
}
require_once( config_get_global( 'absolute_path' ) . 'bug_actiongroup_' . $p_action . '_inc.php' );
$t_function_name = 'action_' . $p_action . '_validate';
return $t_function_name( $p_bug_id );
}
Expand All @@ -172,11 +177,6 @@ function bug_group_action_validate( $p_action, $p_bug_id ) {
* @returns true|array Action can be applied., ( bug_id => reason for failure to process )
*/
function bug_group_action_process( $p_action, $p_bug_id ) {
$t_include_definition = strtoupper( 'bug_actiongroup_' . $p_action . '_inc_allow' );
if( !defined( $t_include_definition ) ) {
define( $t_include_definition, true );
}
require_once( config_get_global( 'absolute_path' ) . 'bug_actiongroup_' . $p_action . '_inc.php' );
$t_function_name = 'action_' . $p_action . '_process';
return $t_function_name( $p_bug_id );
}
Expand Down

0 comments on commit a908cc6

Please sign in to comment.