Skip to content
Permalink
Browse files Browse the repository at this point in the history
5632,5644 removed array_multisort to fix module install issues, and a…
…dded csrftoken to fix postential CSRF attack
  • Loading branch information
gregrgay committed Mar 5, 2016
1 parent df8a546 commit bfc6c80
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 8 deletions.
30 changes: 23 additions & 7 deletions mods/_core/modules/install_modules.php
Expand Up @@ -18,6 +18,8 @@
require(AT_INCLUDE_PATH.'../mods/_core/modules/classes/ModuleListParser.class.php');
require_once(AT_INCLUDE_PATH.'../mods/_core/file_manager/filemanager.inc.php');
// delete all folders and files in $dir


function clear_dir($dir)
{
if ($dh = opendir($dir))
Expand Down Expand Up @@ -154,8 +156,15 @@ function clear_dir($dir)

if (!$msg->containsErrors())
{
header('Location: module_install_step_1.php?mod='.urlencode($module_folder).SEP.'new=1');
exit;
if($_POST['csrftoken'] != $_SESSION['token']){
$msg->addError('ACCESS_DENIED');
} else {

header('Location: module_install_step_1.php?mod='.urlencode($module_folder).SEP.'new=1');
exit;
}
//header('Location: module_install_step_1.php?mod='.urlencode($module_folder).SEP.'new=1');
//exit;
}
}

Expand All @@ -181,8 +190,13 @@ function clear_dir($dir)
$dir_name = str_replace(array('.','..'), '', $_POST['mod']);

if (isset($_POST['install_manually'])) {
header('Location: '.AT_BASE_HREF.'mods/_core/modules/module_install_step_2.php?mod='.urlencode($dir_name).SEP.'new=1'.SEP.'mod_in=1');
exit;
// Check for potential CSRF
if($_POST['csrftoken'] != $_SESSION['token']){
$msg->addError('ACCESS_DENIED');
} else {
header('Location: '.AT_BASE_HREF.'mods/_core/modules/module_install_step_2.php?mod='.urlencode($dir_name).SEP.'new=1'.SEP.'mod_in=1');
exit;
}
}

} else if (isset($_POST['install_manually'])) {
Expand Down Expand Up @@ -255,16 +269,18 @@ function validate_filename() {

// Add $module_list_array as the last parameter, to sort by the common key
// Sorts by original $module_list_array by reference, then returns true|false
$sort_by_version = array_multisort($version, SORT_DESC, $module_list_array);
//$sort_by_version = array_multisort($version, SORT_DESC, $module_list_array);

// Create menu for filter ATutor versions
function select_atversion(){
function select_atversion($v=0){
global $sort_versions;
$menu = '<form action="'.$_SERVER['PHP_SELF'].'" method="post">';
$menu.= '<select name="atversions">';
$menu.= '<option value="0">'._AT("all").'</option>';
foreach($sort_versions as $version){
if($version == VERSION){
if($version == $v){
$menu .= '<option value="'.$version.'" selected="selected">'.$version.'</option>';
}else if($version == VERSION){
$menu .= '<option value="'.$version.'" selected="selected">'.$version.'</option>';
}else{
$menu .= '<option value="'.$version.'" >'.$version.'</option>';
Expand Down
17 changes: 16 additions & 1 deletion themes/default/admin/modules/install_modules.tmpl.php
Expand Up @@ -8,6 +8,7 @@

<div class="row">
<input type="hidden" name="MAX_FILE_SIZE" value="52428800" />
<input type="hidden" name="csrftoken" value="<?php echo $_SESSION['token'];?>" />
<input type="file" name="modulefile" size="50" />
</div>

Expand All @@ -26,6 +27,7 @@
{
?>
<form action="<?php echo $_SERVER['PHP_SELF']; ?>" method="post" name="installform">
<input type="hidden" name="csrftoken" value="<?php echo $_SESSION['token'];?>" />
<table class="data" summary="">
<thead>
<tr>
Expand Down Expand Up @@ -82,10 +84,23 @@
<div class="row">
<?php echo _AT('old_module_notes'); ?>
</div>
<?php echo select_atversion(); ?>
<?php

if(isset($_POST['atversions'])){
$v = htmlspecialchars($_POST['atversions']);
$_SESSION['atversion'] = $_POST['atversions'];
} elseif(isset($_SESSION['atversion'] )){
$v = substr($_SESSION['atversion'], 0, 3);
} else {
$v = substr(VERSION, 0, 3);
}
echo select_atversion($v);

?>
</div>
</fieldset>
<form action="<?php echo $_SERVER['PHP_SELF']; ?>" method="post" name="form">

This comment has been minimized.

Copy link
@stevenseeley

stevenseeley Mar 6, 2016

Contributor

At this line is a XSS vulnerability. Its the "echo $_SERVER['PHP_SELF']" that is the vulnerable code. So, /ATutor/themes/default/admin/modules/install_modules.tmpl.php/">[JS CODE HERE] should work...

<input type="hidden" name="csrftoken" value="<?php echo $_SESSION['token'];?>" />

This comment has been minimized.

Copy link
@atutor

atutor Mar 6, 2016

Owner

Not seeing it. Can a session variable take an XSS?

<table class="data" summary="">
<thead>
<tr>
Expand Down

6 comments on commit bfc6c80

@atutor
Copy link
Owner

@atutor atutor commented on bfc6c80 Mar 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an annoying bug. I wonder why after all these years, others have not noted this. Any suggestion, besides wrapping any instance in htmlspecialchars().

@stevenseeley
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

globally redefine superglobals that use htmlspecialchars(). That way you wont have to patch each and every file.

@atutor
Copy link
Owner

@atutor atutor commented on bfc6c80 Mar 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my test cases using a simple php file the javascript ran via PHP_SELF, however in ATutor I'm not able to get it to work. I tested with install_modules.php, and a few other admin tools. It breaks at line 170, include/header.inc.php when attaching script to install_modules.php:
/"><script>alert("test")</script>
https://github.com/atutor/ATutor/blob/master/include/header.inc.php#L170

It seems $_SERVER['PHP_SELF'] is being interpreted as a filename, and since it is not in the list of valid pages ($_pages[$current_page]), it redirects to a non-existant page. The template file you pointed to in your example above (install_modules.tmpl.php) would never be accessed directly by a user.

Are you able to point out a specific case where the PHP_SELF XSS occurs, not using a template file?

@stevenseeley
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, header.php does that check and 302's the user, but there templates that do not do this. For example: ATutor/themes/simplified_desktop/social/basic_profile.tmpl.php/"><script>alert(1)</script>

In fact, we are writing a patch for you in class right now.

Also, why do you think the templates would not be directly accessed? is there a .htaccess installed by default? If it is accessed by a logged in administrator, sent from an unauthenicated user, then the admin access will work and cookies/session can be hijacked.

@atutor
Copy link
Owner

@atutor atutor commented on bfc6c80 Mar 8, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working on a patch. That's Awesome!

ATutor users would not normally access the templates directly, but if there was a CSRF going on, the hacker could point to a template. That I see now.

Looking foward to the patch.

@stevenseeley
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.