From a565e76f9c4fba9da39933f7f2de132fe6997f00 Mon Sep 17 00:00:00 2001 From: Dave MacFarlane Date: Wed, 10 May 2017 15:28:48 -0400 Subject: [PATCH] Revert "[MRI Violated scans] Have the minc file linked to brainbrowser (Redmine 10928) (#2219)" (#2794) This reverts commit 1bf483f69b5a547239d6e6933dee8f4242d54357. 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. --- modules/brainbrowser/ajax/minc.php | 10 ++--- modules/brainbrowser/js/brainbrowser.loris.js | 36 ++++++------------ ..._Menu_Filter_Form_mri_violations.class.inc | 38 ++++++++----------- .../templates/menu_mri_violations.tpl | 4 -- 4 files changed, 30 insertions(+), 58 deletions(-) diff --git a/modules/brainbrowser/ajax/minc.php b/modules/brainbrowser/ajax/minc.php index 990cfb54f10..990a1d151d5 100644 --- a/modules/brainbrowser/ajax/minc.php +++ b/modules/brainbrowser/ajax/minc.php @@ -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']; diff --git a/modules/brainbrowser/js/brainbrowser.loris.js b/modules/brainbrowser/js/brainbrowser.loris.js index 05a9b06566c..f7eaa92f444 100644 --- a/modules/brainbrowser/js/brainbrowser.loris.js +++ b/modules/brainbrowser/js/brainbrowser.loris.js @@ -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); } @@ -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" diff --git a/modules/mri_violations/php/NDB_Menu_Filter_Form_mri_violations.class.inc b/modules/mri_violations/php/NDB_Menu_Filter_Form_mri_violations.class.inc index 81d12ff04a9..fbd98d79fef 100644 --- a/modules/mri_violations/php/NDB_Menu_Filter_Form_mri_violations.class.inc +++ b/modules/mri_violations/php/NDB_Menu_Filter_Form_mri_violations.class.inc @@ -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; @@ -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', @@ -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, @@ -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, @@ -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, @@ -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()) { @@ -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; } diff --git a/modules/mri_violations/templates/menu_mri_violations.tpl b/modules/mri_violations/templates/menu_mri_violations.tpl index bcf696422b1..34fe46fece1 100644 --- a/modules/mri_violations/templates/menu_mri_violations.tpl +++ b/modules/mri_violations/templates/menu_mri_violations.tpl @@ -135,10 +135,6 @@ {$items[item][piece].value} - {elseif $items[item][piece].name eq 'MincFileViolated'} - - {$items[item][piece].value} - {elseif $items[item][piece].value eq 'Protocol Violation'} {{$items[item][piece].value}}