[mri_violations ]Bring back clickable images #2843

gluneau · 2017-05-26T20:50:15Z

Being able to review mri violations visually is essential to the review process.

AnyhowStep · 2017-05-29T18:48:52Z

It looks as safe as the dicom processing code but, just to be safe, maybe escapeshellarg() will be good to use on both the $dimension and $minc_file before they are used in passthru() or exec():

http://php.net/manual/en/function.escapeshellarg.php

jstirling91 · 2017-05-30T19:17:52Z

modules/mri_violations/js/columnFormatterUnresolved.js

compile using npm run compile make sure you npm folder is up-to-date

driusan

missing a case

driusan · 2017-05-31T17:13:55Z

modules/brainbrowser/ajax/minc.php

+        $query = "select MincFile from mri_violations_log where LogID = :LogID";
+    } elseif ($l == 3) {
+        $query = "select MincFile from MRICandidateErrors where ID = :LogID";
+    }


There should be an else here which aborts with a bad request error in an unhandled case, otherwise if a user provides bad input with url hacking it's still going to get down to the passthru case, which will result in spawning a completely unnecessary process.

There won't be any user input making it to the shell, so it couldn't be used as a remote code exploit, but it could probably be used as part of a denial of service attack.

(Unrelatedly, stylistically I think this code would be a little cleaner with a switch statement than an if/elseif chain, and the SQL keywords should be capitalized.)

gluneau · 2017-05-31T19:06:47Z

@driusan @jstirling91 Please take a look and confirm your comments were addressed.

driusan · 2017-06-02T18:12:27Z

modules/brainbrowser/ajax/minc.php

+    $minc_file = $DB->pselectOne(
+        $query,
+        array(
+         'MincID' => escapeshellcmd($_REQUEST['minc_id']),


I don't see what's gained by calling escapeshellcmd here. It's being passed to a prepared statement binding, not being passed to a shell.

driusan · 2017-06-02T18:15:45Z

modules/brainbrowser/ajax/minc.php

-$minc_file = $DB->pselectOne($query, array('MincID' => $_REQUEST['minc_id']));
-$minc_file = getMincLocation() . $minc_file;
+if (strpos(escapeshellcmd($_REQUEST['minc_id']), 'l') !== false) {
+    list($l, $id) = explode('l', escapeshellcmd($_REQUEST['minc_id']));


Why are these being shell escaped here? It's not being passed to a shell or used as a command. This should be done right before the passthru command, not everywhere that the minc_id is used.

If you really want to check if the user was trying to inject something and abort early, you could probably do something like if (escapeshellcmd(x) !== x) { print error and exit} (note: I haven't tried that, but it should work.)

There's also this warning from the escapeshellcmd documentation that should be taken into account:

Warning escapeshellcmd() should be used on the whole command string, and it still allows the attacker to pass arbitrary number of arguments. For escaping a single argument escapeshellarg() should be used instead. (http://php.net/manual/en/function.escapeshellcmd.php)

changes done

jstirling91

Moving to needs testing

johnsaigle

Looks good from a security perspective! Added a couple of refactoring notes.

johnsaigle · 2017-06-05T15:44:16Z

modules/brainbrowser/ajax/minc.php

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


I'm not sure that this will work as intended. If you look here, !== will always return true when compared with numeric values. I tried it on the command line with -1, 0, and 1 and this was the case.

As strpos always returns a number, this condition will always be true.

@johnsaigle, it's the small case letter "L" being checked here. So it's not a number. ;-)

But strpos still returns a number regardless of what you check, right? 😅

Note that the use of !== false is deliberate; strpos returns either the offset at which the needle string begins in the haystack string, or the boolean false if the needle isn't found. Since 0 is a valid offset and 0 is "falsey", we can't use simpler constructs like !strpos($a, 'are').

https://stackoverflow.com/questions/4366730/how-do-i-check-if-a-string-contains-a-specific-word-in-php

Oh I'm sorry about that. I thought strpos returned a -1 on failure, not false. Bizarre behaviour...

The PHP standard functions are relatively consistent (relative to the rest of PHP) in returning false if a needle isn't found.

johnsaigle · 2017-06-05T15:44:37Z

modules/brainbrowser/ajax/minc.php


-$header      = $_REQUEST['minc_headers'];
-$header_data = $_REQUEST['raw_data'];
+        if (strpos($minc_file, 'assembly') !== false) {


~~See comment above. I believe this will always return true.~~
Resolved above.

johnsaigle · 2017-06-05T15:54:25Z

modules/brainbrowser/ajax/minc.php

+            if (strpos($minc_file, 'assembly') === 0) {
+                $minc_file = getMincLocation() . $minc_file;
+            } else {
+                $minc_file = $minc_file;


This is tautological.

Maybe refactor this section like the following...

if (strpos($minc_file, 'assembly') === 0) { $minc_file = getMincLocation() . $minc_file; } else if (strpos($minc_file, 'assemby') < 0) { $minc_file = getMincLocation() . "trashbin/" . $file; }

...and remove line 48 also.

This refers to line 52.. sorry I think I messed up the formatting

I still think this should be changed but I won't block the PR just for this. Going to approve changes. 👍

based on johns feedback

jstirling91 · 2017-06-06T15:46:54Z

@gluneau @driusan is this still relevant with the the following pull request #2861

gluneau · 2017-06-06T17:32:43Z

Once this #2861 gets merged, I'll rebase this one.

driusan · 2017-06-28T18:07:03Z

@gluneau @jstirling91 This is still tagged as "Blocked". The only reason I can see is the above comment, and the PR that it references looks like it's been merged.

Does this still need to be rebased and can the tag be removed?

MounaSafiHarab · 2017-07-05T18:43:23Z

@gluneau

works well!
I get multiple PHP Warnings though (one example shown below, but I get many more for every violated scan), so up to the senior developer to decide if changes are needed or the pull request is good to go!

[Wed Jul 05 14:30:49.660829 2017] [:error] [pid 29712] [client 132.216.56.241:49758] PHP Warning: substr() expects parameter 1 to be string, array given in /var/www/loris/modules/brainbrowser/ajax/getMincName.php on line 23, referer: http://mouna-dev.loris.ca/brainbrowser/?minc_id=1l50

gluneau added this to the 17.1 milestone May 26, 2017

gluneau added the Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) label May 29, 2017

jstirling91 previously requested changes May 30, 2017

View reviewed changes

driusan requested changes May 31, 2017

View reviewed changes

driusan added the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label May 31, 2017

gluneau removed the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Jun 2, 2017

driusan previously requested changes Jun 2, 2017

View reviewed changes

driusan requested a review from johnsaigle June 5, 2017 13:12

jstirling91 previously approved these changes Jun 5, 2017

View reviewed changes

johnsaigle suggested changes Jun 5, 2017

View reviewed changes

johnsaigle reviewed Jun 5, 2017

View reviewed changes

johnsaigle assigned gluneau Jun 5, 2017

johnsaigle approved these changes Jun 5, 2017

View reviewed changes

johnsaigle unassigned gluneau Jun 5, 2017

jstirling91 added the Blocked PR awaiting the merge, resolution or rejection of an other Pull Request label Jun 7, 2017

gluneau force-pushed the mri_violations_view branch from 0eb7e21 to 9c3c013 Compare June 27, 2017 18:36

make mri_violations great again

072916e

gluneau force-pushed the mri_violations_view branch from d28dc24 to 072916e Compare July 4, 2017 13:23

gluneau assigned MounaSafiHarab Jul 4, 2017

kongtiaowang closed this Jul 4, 2017

kongtiaowang reopened this Jul 4, 2017

gluneau removed the Blocked PR awaiting the merge, resolution or rejection of an other Pull Request label Jul 4, 2017

MounaSafiHarab added the Passed Manual Tests PR has undergone proper testing by at least one peer label Jul 5, 2017

driusan merged commit cf8d4c6 into aces:17.1-dev Jul 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mri_violations ]Bring back clickable images #2843

[mri_violations ]Bring back clickable images #2843

gluneau commented May 26, 2017 •

edited

Loading

AnyhowStep commented May 29, 2017

jstirling91 May 30, 2017

driusan left a comment

driusan May 31, 2017

gluneau commented May 31, 2017

driusan Jun 2, 2017

driusan Jun 2, 2017

jstirling91 left a comment

johnsaigle left a comment

johnsaigle Jun 5, 2017 •

edited

Loading

gluneau Jun 5, 2017

johnsaigle Jun 5, 2017

gluneau Jun 5, 2017

johnsaigle Jun 5, 2017

driusan Jun 5, 2017

johnsaigle Jun 5, 2017 •

edited

Loading

johnsaigle Jun 5, 2017

johnsaigle Jun 5, 2017

johnsaigle Jun 5, 2017

jstirling91 commented Jun 6, 2017

gluneau commented Jun 6, 2017

driusan commented Jun 28, 2017 •

edited

Loading

MounaSafiHarab commented Jul 5, 2017 •

edited

Loading

[mri_violations ]Bring back clickable images #2843

[mri_violations ]Bring back clickable images #2843

Conversation

gluneau commented May 26, 2017 • edited Loading

AnyhowStep commented May 29, 2017

Choose a reason for hiding this comment

driusan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gluneau commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstirling91 left a comment

Choose a reason for hiding this comment

johnsaigle left a comment

Choose a reason for hiding this comment

johnsaigle Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnsaigle Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstirling91 commented Jun 6, 2017

gluneau commented Jun 6, 2017

driusan commented Jun 28, 2017 • edited Loading

MounaSafiHarab commented Jul 5, 2017 • edited Loading

gluneau commented May 26, 2017 •

edited

Loading

johnsaigle Jun 5, 2017 •

edited

Loading

johnsaigle Jun 5, 2017 •

edited

Loading

driusan commented Jun 28, 2017 •

edited

Loading

MounaSafiHarab commented Jul 5, 2017 •

edited

Loading