Skip to content

Commit

Permalink
Revert "[MRI Violated scans] Have the minc file linked to brainbrowse…
Browse files Browse the repository at this point in the history 
…r (Redmine 10928) (aces#2219)"

This reverts commit 1bf483f.

The addition of the parameter "minc_location" parameter added by this commit introduces
a severe security hole, where user input is getting passed directly to the PHP
passthru command.
  • Loading branch information
@driusan
driusan committed May 10, 2017
1 parent 72f99d7 commit 4fd5a69
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 58 deletions.
10 changes: 3 additions & 7 deletions modules/brainbrowser/ajax/minc.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,10 @@

$headers = array();

$query = "select File from files where FileID = :MincID";
$query = "select File from files where FileID = :MincID";
$minc_file = $DB->pselectOne($query, array('MincID' => $_REQUEST['minc_id']));
$minc_file = getMincLocation() . $minc_file;

if (isset($_REQUEST['minc_location'])) {
$minc_file = ($_REQUEST['minc_location']);
} else {
$minc_file = $DB->pselectOne($query, array('MincID' => $_REQUEST['minc_id']));
$minc_file = getMincLocation() . $minc_file;
}

$header = $_REQUEST['minc_headers'];
$header_data = $_REQUEST['raw_data'];
Expand Down
36 changes: 11 additions & 25 deletions modules/brainbrowser/js/brainbrowser.loris.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,8 @@ $(function() {
viewer.volumes.forEach(function(volume) {
volume.setWorldCoords(x, y, z);
});
} else {
}
else {
viewer.volumes[vol_id].setWorldCoords(x, y, z);
}

Expand Down Expand Up @@ -780,39 +781,24 @@ $(function() {
}); // Should cursors in all panels be synchronized?

link = window.location.search;
var minc_tag;

if (getQueryVariable("minc_location")) {
minc_ids = getQueryVariable("minc_id");
if (minc_ids[0] === '[') {
// An array was passed. Get rid of the brackets and then split on ","
minc_ids = minc_ids.substring(1, minc_ids.length - 1);
minc_ids_arr = minc_ids.split(",");

minc_ids = getQueryVariable("minc_location");
minc_ids_arr = [minc_ids];
minc_tag = "minc_location";
} else {

minc_ids = getQueryVariable("minc_id");
minc_tag = "minc_id";

if (minc_ids[0] === '[') {

// An array was passed. Get rid of the brackets and then split on ","
minc_ids = minc_ids.substring(1, minc_ids.length - 1);
minc_ids_arr = minc_ids.split(",");

} else {

// Only one passed
minc_ids_arr = [minc_ids];
}
// Only one passed
minc_ids_arr = [minc_ids];
}



for (i = 0; i < minc_ids_arr.length; i += 1) {

minc_volumes.push({
type: 'minc',
header_url: loris.BaseURL + "/brainbrowser/ajax/minc.php?" + minc_tag + "=" + minc_ids_arr[i] + "&minc_headers=true",
raw_data_url: loris.BaseURL + "/brainbrowser/ajax/minc.php?" + minc_tag + "=" + minc_ids_arr[i] + "&raw_data=true",
header_url: loris.BaseURL + "/brainbrowser/ajax/minc.php?minc_id=" + minc_ids_arr[i] + "&minc_headers=true",
raw_data_url: loris.BaseURL + "/brainbrowser/ajax/minc.php?minc_id=" + minc_ids_arr[i] + "&raw_data=true",
template: {
element_id: "volume-ui-template4d",
viewer_insert_class: "volume-viewer-display"
Expand Down
38 changes: 16 additions & 22 deletions modules/mri_violations/php/NDB_Menu_Filter_Form_mri_violations.class.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,17 @@ class NDB_Menu_Filter_Form_mri_violations extends NDB_Menu_Filter_Form
break;
}
// user input doesn't match DB, so we update the DB
else {
else{
$setArray = array('Resolved'=>(string)$val,'ChangeDate'=>date("Y-m-d H:i:s"));
$whereArray = array('hash'=>$hash);
$DB->update('violations_resolved', $setArray, $whereArray);
}
}
}
} else {
//if row is not found no resolve status was assigned,
// if selection<>0, then insert new row.
}
//if row is not found no resolve status was assigned,
// if selection<>0, then insert new row.
else{
// no need to insert to DB for Unresolved value.
if($val=='unresolved'){
continue;
Expand Down Expand Up @@ -137,24 +138,21 @@ class NDB_Menu_Filter_Form_mri_violations extends NDB_Menu_Filter_Form
// set the class variables
// create user object
$user =& User::singleton();
$DB =& Database::singleton();

$config =& NDB_Config::singleton();
$useProjects = $config->getSetting("useProjects");
if ($useProjects === "false") {
$useProjects = false;
} else {
}
else{
$useProjects = true;
}

$dir_path = $config->getSetting("imagePath");

$this->columns = array(
'v.PatientName',
'v.Site',
'v.TimeRun',
'v.MincFile',
'v.MincFileViolated',
'v.Series_Description as Series_Description_Or_Scan_Type',
'v.Problem',
'v.SeriesUID',
Expand All @@ -167,18 +165,12 @@ class NDB_Menu_Filter_Form_mri_violations extends NDB_Menu_Filter_Form
array_splice($this->columns, 2, 0, 'v.Subproject');
}

// Recreating the path (MincFileViolated field) to the minc file for the table mri_violations_log is more complicated
// because of the 3 cases that can occur as we pull the data from the db.
// 1. if the Mincfile starts with "assembly" then we need to add the directory path in front of it.
// 2. if the word "assembly" is there but not at the beginning, then uses it as is, the path is correct.
// 3. if it is not in the assembly dir, then it is in the trashbin, so we fix the path with that in mind.
$this->query = " FROM (
SELECT PatientName as PatientName,
time_run as TimeRun,
c.ProjectID as Project,
s.SubprojectID as Subproject,
minc_location as MincFile,
CONCAT_WS(''," . $DB->quote($dir_path) . ",'trashbin/',SUBSTRING_INDEX(minc_location, '/', -2)) as MincFileViolated,
series_description as Series_Description,
'Could not identify scan type' as Problem,
SeriesUID,
Expand All @@ -203,7 +195,6 @@ class NDB_Menu_Filter_Form_mri_violations extends NDB_Menu_Filter_Form
c.ProjectID as Project,
s.SubprojectID as Subproject,
MincFile,
IF(INSTR(`MincFile`, 'assembly'), IF(LEFT(`MincFile`, 8) = 'assembly',CONCAT_WS(''," . $DB->quote($dir_path) . ",`MincFile`),`MincFile`), CONCAT_WS(''," . $DB->quote($dir_path) . ",'trashbin/',SUBSTRING_INDEX(MincFile, '/', -2))) as MincFileViolated,
mri_scan_type.Scan_type,
'Protocol Violation',
SeriesUID,
Expand All @@ -230,7 +221,6 @@ class NDB_Menu_Filter_Form_mri_violations extends NDB_Menu_Filter_Form
c.ProjectID as Project,
s.SubprojectID as Subproject,
MincFile,
CONCAT_WS(''," . $DB->quote($dir_path) . ",'trashbin/',SUBSTRING_INDEX(MincFile, '/', -2)) as MincFileViolated,
null,
Reason,
SeriesUID,
Expand Down Expand Up @@ -338,7 +328,8 @@ class NDB_Menu_Filter_Form_mri_violations extends NDB_Menu_Filter_Form
if(is_array($sites)){
$sites = array('' => 'All') + $sites;
}
} else {
}
else {
// allow only to view own site data
$site =& Site::singleton($user->getData('CenterID'));
if ($site->isStudySite()) {
Expand Down Expand Up @@ -404,17 +395,20 @@ class NDB_Menu_Filter_Form_mri_violations extends NDB_Menu_Filter_Form
$this->form->form[$hash]['name'] = "resolvable[$hash]";
}
if ($key === 'Problem') {
if ($val === "Could not identify scan type") {
if($val === "Could not identify scan type"){
$this->tpl_data['join_tbl'] = "mri_protocol_violated_scans";
} elseif ($val === "Protocol Violation") {
}
elseif($val === "Protocol Violation"){
$this->tpl_data['join_tbl'] = "mri_violations_log";
} else {
}
else{
$this->tpl_data['join_tbl'] = "MRICandidateErrors";
}
}
if ($key === 'SeriesUID') {
$this->tpl_data['items'][$x]['series'] = $val;
} else {
}
else {
$this->tpl_data['items'][$x][$i]['name'] = $key;
$this->tpl_data['items'][$x][$i]['value'] = $val;
}
Expand Down
4 changes: 0 additions & 4 deletions modules/mri_violations/templates/menu_mri_violations.tpl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,6 @@
{$items[item][piece].value}
</a>
</td>
{elseif $items[item][piece].name eq 'MincFileViolated'}
<td nowrap="nowrap" bgcolor="{$items[item][piece].bgcolor}">
<a href="#noID" onclick="window.open('{$baseurl}/brainbrowser/?minc_location={$items[item][piece].value}', 'BrainBrowser Volume Viewer', 'location = 0,width = auto, height = auto, scrollbars=yes')">{$items[item][piece].value}</a>
</td>
{elseif $items[item][piece].value eq 'Protocol Violation'}
<td nowrap="nowrap" bgcolor="{$items[item][piece].bgcolor}">
<a href="#" class="mri_violations" id="mri_protocol_check_violations" data-PatientName="{$items[item].PatientName}" "{if $items[item].series}" data-SeriesUID="{$items[item].series}{/if}">{{$items[item][piece].value}}</a>
Expand Down

0 comments on commit 4fd5a69

Please sign in to comment.