[Security] Amend earlier download fix #2876

johnsaigle · 2017-06-14T15:30:23Z

This pull request restores the broken functionality merged in #2868. This fixes the access control issue while allowing valid files to download.

See also: Redmine 12586.

johnsaigle · 2017-06-14T15:35:01Z

@driusan Please take a look at this fix when you can. Also, I strongly recommend that we include this function in the Utility class as we'll likely need this workaround in the future.

realpath() doesn't work for us as it requires that the web-user have write access in each directory in the hierarchy.

MounaSafiHarab · 2017-06-14T18:21:11Z

@johnsaigle

I tested this, and I now can see the images again in Imaging browser, and can download (I tested MINC downloads as this is the type of files I usually download).

johnsaigle · 2017-06-14T18:30:11Z

@MounaSafiHarab thanks for testing!

driusan · 2017-06-14T20:37:26Z

htdocs/mri/jiv/get_file.php

+    }
+    // resolve path parts (single dot, double dot and double delimiters)
+    $path      = str_replace(array( '/', '\\' ), DIRECTORY_SEPARATOR, $path);
+    $parts     = array_filter(explode(DIRECTORY_SEPARATOR, $path), 'strlen');


This is a pretty hard to read way to strip out the empty parts.

Can you just use a function defined inline which returns true if strlen > 0 to make it a little more obvious what the array_filter is trying to filter for, instead of basing it on an implicit assumption that 0 and false are equivalent?

driusan · 2017-06-14T20:38:28Z

htdocs/mri/jiv/get_file.php

+            continue;
+        }
+        if ('..' == $part ) {
+            array_pop($absolutes);


It might be a good idea to check the return value here and throw an exception if you're trying to pop off of an empty stack.

driusan · 2017-06-14T20:39:12Z

htdocs/mri/jiv/get_file.php

+    $path = implode(DIRECTORY_SEPARATOR, $absolutes);
+    // resolve any symlinks
+    if (file_exists($path) && linkinfo($path) > 0 ) {
+        $path = readlink($path);


is this necessary? What's wrong with resolving the path to a symbolic link?

driusan · 2017-06-14T20:41:07Z

htdocs/mri/jiv/get_file.php

@@ -19,6 +19,46 @@
 *  @link     https://github.com/aces/Loris-Trunk
 */

+/**
+ * This function is to replace PHP's extremely buggy realpath().
+ * Taken from https://gist.github.com/umidjons/6154548


What's the copyright on it? I don't see any copyright notice, which means it defaults to all rights reserved and we don't have the right to use it unless the author gives explicit written approval.

(Which makes most of my below comments a moot point..)

Gotcha... I've messaged the author and if he's feeling generous he'll hopefully license it properly. I'll look into another solution in the meantime.

driusan · 2017-06-14T20:43:01Z

htdocs/mri/jiv/get_file.php

+// Check if the resolved 'true' path starts with the prefix.
+// If not, user supplied a relative path which is forbidden
+if (substr($TruePath, 0, strlen($PathPrefix)) !== $PathPrefix) {
+    error_log("ERROR: User supplied relative path. Potentially malicious.");


You might want to include the IP address or username if you're going to provide a message like this, so it can be followed up with.

Cool, can do.

driusan · 2017-06-14T20:44:32Z

htdocs/mri/jiv/get_file.php

 switch($FileExt) {
 case 'mnc':
-    $FullPath         = $mincPath . '/' . $File;
+    $PathPrefix       = $mincPath;


Why are these all changed instead of just switching realpath to truepath and verifying that it fixes the security issues (and works..)?

Well I did verify it works and fixes the security issue (which has been confirmed by @MounaSafiHarab). You're right this is more of a cosmetic change but it also makes the file a little smaller and I think clearer.

driusan · 2017-06-14T20:45:02Z

Adding the blocked tag because of the copyright issue.

johnsaigle · 2017-06-15T15:42:01Z

@driusan I removed the offending code as well as the cleanup changes. This PR now only contains the minimum changes needed for security.

I've tested this on my VM and it allows files to download while also preventing the exploit.

Changes donne

johnsaigle · 2017-06-16T15:20:34Z

@driusan do you think you could give me an approving review now that the changes are done? 😂 Also, what are your thoughts on moving this to Utility?

johnsaigle · 2017-06-16T16:43:27Z

@MounaSafiHarab Would you mind re-testing this with the new changes?

MounaSafiHarab · 2017-06-16T19:36:04Z

@johnsaigle

works for me (tested minc images download in imaging browser)!
do I move it to the last column, or is it up to @driusan to approve it as a reviewer first?

johnsaigle added Category: Bug PR or issue that aims to report or fix a bug Release: Breaking changes PR that contains changes that might impact the code or accepted practices of active projects labels Jun 14, 2017

johnsaigle added this to the 17.0 milestone Jun 14, 2017

xlecours self-assigned this Jun 14, 2017

johnsaigle added the Passed manual tests PR has been successfully tested by at least one peer label Jun 14, 2017

johnsaigle mentioned this pull request Jun 14, 2017

[ImgBrowser] Add DICOM Download #2791

Closed

driusan previously requested changes Jun 14, 2017

View reviewed changes

driusan added the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Jun 14, 2017

johnsaigle added 2 commits June 15, 2017 11:37

Create resolve path function to eliminate ..

505284d

phpcs

3900314

johnsaigle added 2 commits June 15, 2017 11:44

pop not shift

881a08e

refactor for happy path. add error msgs. remove debug info.

ec41df9

johnsaigle removed the State: Blocked PR or issue awaiting an external event such as the merge or another PR to proceed label Jun 15, 2017

johnsaigle mentioned this pull request Jun 15, 2017

Update 17.1-dev with 17.0-dev #2878

Merged

johnsaigle removed the Passed manual tests PR has been successfully tested by at least one peer label Jun 16, 2017

MounaSafiHarab added the Passed manual tests PR has been successfully tested by at least one peer label Jun 16, 2017

driusan merged commit f460ad7 into aces:17.0-dev Jun 21, 2017

johnsaigle mentioned this pull request Jul 3, 2017

[Libraries] Move resolvePath function to the Utility class #2918

Merged

johnsaigle added the Category: Security PR or issue that aims to improve security label Jul 23, 2018

johnsaigle mentioned this pull request Aug 15, 2018

[Core, Media] Interface for safe file downloading #3881

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] Amend earlier download fix #2876

[Security] Amend earlier download fix #2876

johnsaigle commented Jun 14, 2017

johnsaigle commented Jun 14, 2017 •

edited

Loading

MounaSafiHarab commented Jun 14, 2017

johnsaigle commented Jun 14, 2017

driusan Jun 14, 2017

driusan Jun 14, 2017

driusan Jun 14, 2017

driusan Jun 14, 2017

johnsaigle Jun 14, 2017 •

edited

Loading

driusan Jun 14, 2017

johnsaigle Jun 14, 2017

driusan Jun 14, 2017

johnsaigle Jun 14, 2017

driusan commented Jun 14, 2017

johnsaigle commented Jun 15, 2017

johnsaigle commented Jun 16, 2017 •

edited

Loading

johnsaigle commented Jun 16, 2017

MounaSafiHarab commented Jun 16, 2017 •

edited

Loading

[Security] Amend earlier download fix #2876

[Security] Amend earlier download fix #2876

Conversation

johnsaigle commented Jun 14, 2017

johnsaigle commented Jun 14, 2017 • edited Loading

MounaSafiHarab commented Jun 14, 2017

johnsaigle commented Jun 14, 2017

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 14, 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

driusan commented Jun 14, 2017

johnsaigle commented Jun 15, 2017

johnsaigle commented Jun 16, 2017 • edited Loading

johnsaigle commented Jun 16, 2017

MounaSafiHarab commented Jun 16, 2017 • edited Loading

johnsaigle commented Jun 14, 2017 •

edited

Loading

johnsaigle Jun 14, 2017 •

edited

Loading

johnsaigle commented Jun 16, 2017 •

edited

Loading

MounaSafiHarab commented Jun 16, 2017 •

edited

Loading