Skip to content

Commit a565e76

Browse files
driusandriusan
authored
Revert "[MRI Violated scans] Have the minc file linked to brainbrowser (Redmine 10928) (aces#2219)" (aces#2794)
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.
1 parent 72f99d7 commit a565e76

4 files changed

Lines changed: 30 additions & 58 deletions

File tree

‎modules/brainbrowser/ajax/minc.php‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,14 +22,10 @@
2222

2323
$headers = array();
2424

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

27-
if (isset($_REQUEST['minc_location'])) {
28-
$minc_file = ($_REQUEST['minc_location']);
29-
} else {
30-
$minc_file = $DB->pselectOne($query, array('MincID' => $_REQUEST['minc_id']));
31-
$minc_file = getMincLocation() . $minc_file;
32-
}
3329

3430
$header = $_REQUEST['minc_headers'];
3531
$header_data = $_REQUEST['raw_data'];

‎modules/brainbrowser/js/brainbrowser.loris.js‎

Lines changed: 11 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -259,7 +259,8 @@ $(function() {
259259
viewer.volumes.forEach(function(volume) {
260260
volume.setWorldCoords(x, y, z);
261261
});
262-
} else {
262+
}
263+
else {
263264
viewer.volumes[vol_id].setWorldCoords(x, y, z);
264265
}
265266

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

782783
link = window.location.search;
783-
var minc_tag;
784784

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

787-
minc_ids = getQueryVariable("minc_location");
788-
minc_ids_arr = [minc_ids];
789-
minc_tag = "minc_location";
790791
} else {
791-
792-
minc_ids = getQueryVariable("minc_id");
793-
minc_tag = "minc_id";
794-
795-
if (minc_ids[0] === '[') {
796-
797-
// An array was passed. Get rid of the brackets and then split on ","
798-
minc_ids = minc_ids.substring(1, minc_ids.length - 1);
799-
minc_ids_arr = minc_ids.split(",");
800-
801-
} else {
802-
803-
// Only one passed
804-
minc_ids_arr = [minc_ids];
805-
}
792+
// Only one passed
793+
minc_ids_arr = [minc_ids];
806794
}
807795

808-
809-
810796
for (i = 0; i < minc_ids_arr.length; i += 1) {
811797

812798
minc_volumes.push({
813799
type: 'minc',
814-
header_url: loris.BaseURL + "/brainbrowser/ajax/minc.php?" + minc_tag + "=" + minc_ids_arr[i] + "&minc_headers=true",
815-
raw_data_url: loris.BaseURL + "/brainbrowser/ajax/minc.php?" + minc_tag + "=" + minc_ids_arr[i] + "&raw_data=true",
800+
header_url: loris.BaseURL + "/brainbrowser/ajax/minc.php?minc_id=" + minc_ids_arr[i] + "&minc_headers=true",
801+
raw_data_url: loris.BaseURL + "/brainbrowser/ajax/minc.php?minc_id=" + minc_ids_arr[i] + "&raw_data=true",
816802
template: {
817803
element_id: "volume-ui-template4d",
818804
viewer_insert_class: "volume-viewer-display"

‎modules/mri_violations/php/NDB_Menu_Filter_Form_mri_violations.class.inc‎

Lines changed: 16 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -63,16 +63,17 @@ class NDB_Menu_Filter_Form_mri_violations extends NDB_Menu_Filter_Form
6363
break;
6464
}
6565
// user input doesn't match DB, so we update the DB
66-
else {
66+
else{
6767
$setArray = array('Resolved'=>(string)$val,'ChangeDate'=>date("Y-m-d H:i:s"));
6868
$whereArray = array('hash'=>$hash);
6969
$DB->update('violations_resolved', $setArray, $whereArray);
7070
}
7171
}
7272
}
73-
} else {
74-
//if row is not found no resolve status was assigned,
75-
// if selection<>0, then insert new row.
73+
}
74+
//if row is not found no resolve status was assigned,
75+
// if selection<>0, then insert new row.
76+
else{
7677
// no need to insert to DB for Unresolved value.
7778
if($val=='unresolved'){
7879
continue;
@@ -137,24 +138,21 @@ class NDB_Menu_Filter_Form_mri_violations extends NDB_Menu_Filter_Form
137138
// set the class variables
138139
// create user object
139140
$user =& User::singleton();
140-
$DB =& Database::singleton();
141141

142142
$config =& NDB_Config::singleton();
143143
$useProjects = $config->getSetting("useProjects");
144144
if ($useProjects === "false") {
145145
$useProjects = false;
146-
} else {
146+
}
147+
else{
147148
$useProjects = true;
148149
}
149150

150-
$dir_path = $config->getSetting("imagePath");
151-
152151
$this->columns = array(
153152
'v.PatientName',
154153
'v.Site',
155154
'v.TimeRun',
156155
'v.MincFile',
157-
'v.MincFileViolated',
158156
'v.Series_Description as Series_Description_Or_Scan_Type',
159157
'v.Problem',
160158
'v.SeriesUID',
@@ -167,18 +165,12 @@ class NDB_Menu_Filter_Form_mri_violations extends NDB_Menu_Filter_Form
167165
array_splice($this->columns, 2, 0, 'v.Subproject');
168166
}
169167

170-
// Recreating the path (MincFileViolated field) to the minc file for the table mri_violations_log is more complicated
171-
// because of the 3 cases that can occur as we pull the data from the db.
172-
// 1. if the Mincfile starts with "assembly" then we need to add the directory path in front of it.
173-
// 2. if the word "assembly" is there but not at the beginning, then uses it as is, the path is correct.
174-
// 3. if it is not in the assembly dir, then it is in the trashbin, so we fix the path with that in mind.
175168
$this->query = " FROM (
176169
SELECT PatientName as PatientName,
177170
time_run as TimeRun,
178171
c.ProjectID as Project,
179172
s.SubprojectID as Subproject,
180173
minc_location as MincFile,
181-
CONCAT_WS(''," . $DB->quote($dir_path) . ",'trashbin/',SUBSTRING_INDEX(minc_location, '/', -2)) as MincFileViolated,
182174
series_description as Series_Description,
183175
'Could not identify scan type' as Problem,
184176
SeriesUID,
@@ -203,7 +195,6 @@ class NDB_Menu_Filter_Form_mri_violations extends NDB_Menu_Filter_Form
203195
c.ProjectID as Project,
204196
s.SubprojectID as Subproject,
205197
MincFile,
206-
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,
207198
mri_scan_type.Scan_type,
208199
'Protocol Violation',
209200
SeriesUID,
@@ -230,7 +221,6 @@ class NDB_Menu_Filter_Form_mri_violations extends NDB_Menu_Filter_Form
230221
c.ProjectID as Project,
231222
s.SubprojectID as Subproject,
232223
MincFile,
233-
CONCAT_WS(''," . $DB->quote($dir_path) . ",'trashbin/',SUBSTRING_INDEX(MincFile, '/', -2)) as MincFileViolated,
234224
null,
235225
Reason,
236226
SeriesUID,
@@ -338,7 +328,8 @@ class NDB_Menu_Filter_Form_mri_violations extends NDB_Menu_Filter_Form
338328
if(is_array($sites)){
339329
$sites = array('' => 'All') + $sites;
340330
}
341-
} else {
331+
}
332+
else {
342333
// allow only to view own site data
343334
$site =& Site::singleton($user->getData('CenterID'));
344335
if ($site->isStudySite()) {
@@ -404,17 +395,20 @@ class NDB_Menu_Filter_Form_mri_violations extends NDB_Menu_Filter_Form
404395
$this->form->form[$hash]['name'] = "resolvable[$hash]";
405396
}
406397
if ($key === 'Problem') {
407-
if ($val === "Could not identify scan type") {
398+
if($val === "Could not identify scan type"){
408399
$this->tpl_data['join_tbl'] = "mri_protocol_violated_scans";
409-
} elseif ($val === "Protocol Violation") {
400+
}
401+
elseif($val === "Protocol Violation"){
410402
$this->tpl_data['join_tbl'] = "mri_violations_log";
411-
} else {
403+
}
404+
else{
412405
$this->tpl_data['join_tbl'] = "MRICandidateErrors";
413406
}
414407
}
415408
if ($key === 'SeriesUID') {
416409
$this->tpl_data['items'][$x]['series'] = $val;
417-
} else {
410+
}
411+
else {
418412
$this->tpl_data['items'][$x][$i]['name'] = $key;
419413
$this->tpl_data['items'][$x][$i]['value'] = $val;
420414
}

‎modules/mri_violations/templates/menu_mri_violations.tpl‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -135,10 +135,6 @@
135135
{$items[item][piece].value}
136136
</a>
137137
</td>
138-
{elseif $items[item][piece].name eq 'MincFileViolated'}
139-
<td nowrap="nowrap" bgcolor="{$items[item][piece].bgcolor}">
140-
<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>
141-
</td>
142138
{elseif $items[item][piece].value eq 'Protocol Violation'}
143139
<td nowrap="nowrap" bgcolor="{$items[item][piece].bgcolor}">
144140
<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>

0 commit comments

Comments
 (0)