View Issue Details

This bug affects 1 person(s).
 6
IDProjectCategoryView StatusLast Update
09856Bug reportsData Entry (non public)public2015-12-15 13:15
Reporteruser40445Assigned ToDenisChenu  
PrioritynormalSeverityminor 
Status closedResolutionfixed 
Product Version2.06+ 
Fixed in Version2.06+ 
Summary09856: kcfinder not working with symlinked /upload
Description

KCFinder does not work correctly if /upload is a symlinked directory.
Uploading images does not work ("Unknown error").

Steps To Reproduce
  1. Make /upload a symlink to some other directory.
  2. Try to upload an image.
Additional Information

This needs to be fixed in third_party/kcfinder/core/class/uploader.php
$this->typeDir is compared against a path that is run through realpath, so $this->typeDir needs to be run through realpath as well. Changes are necessary in lines 226, 234, 245.

TagsNo tags attached.
Attached Files
limesurvey_kcfinder_symlink_realpath.diff (1,886 bytes)   
--- /tmp/uploader.php	2015-12-04 15:27:34.000000000 +0100
+++ third_party/kcfinder/core/class/uploader.php	2015-12-04 16:01:34.000000000 +0100
@@ -94,6 +94,11 @@
         return property_exists($this, $property) ? $this->$property : null;
     }
 
+    protected function getSymLink($path) {
+	//return is_link($path) ? readlink($path) : $path;
+	return realpath($path);
+    }
+
     public function __construct() {
 
         // crsf_session
@@ -223,7 +228,7 @@
             $this->config['uploadDir'] = strlen($this->config['uploadDir'])
                 ? path::normalize($this->config['uploadDir'])
                 : path::url2fullPath("/$path");
-            $this->typeDir = "{$this->config['uploadDir']}/{$this->type}";
+            $this->typeDir = $this->getSymLink("{$this->config['uploadDir']}/{$this->type}");
             $this->typeURL = "{$this->config['uploadURL']}/{$this->type}";
 
         // SITE ROOT
@@ -231,7 +236,7 @@
             $this->config['uploadDir'] = strlen($this->config['uploadDir'])
                 ? path::normalize($this->config['uploadDir'])
                 : path::normalize($_SERVER['DOCUMENT_ROOT']);
-            $this->typeDir = "{$this->config['uploadDir']}/{$this->type}";
+            $this->typeDir = $this->getSymLink("{$this->config['uploadDir']}/{$this->type}");
             $this->typeURL = "/{$this->type}";
 
         // ABSOLUTE & RELATIVE
@@ -242,7 +247,7 @@
             $this->config['uploadDir'] = strlen($this->config['uploadDir'])
                 ? path::normalize($this->config['uploadDir'])
                 : path::url2fullPath($this->config['uploadURL']);
-            $this->typeDir = "{$this->config['uploadDir']}/{$this->type}";
+            $this->typeDir = $this->getSymLink("{$this->config['uploadDir']}/{$this->type}");
             $this->typeURL = "{$this->config['uploadURL']}/{$this->type}";
         }

Bug heat6
Complete LimeSurvey version number (& build)150825
I will donate to the project if issue is resolvedNo
Browserany
Database type & versionMySQL
Server OS (if known)RHEL 7
Webserver software & version (if known)Apache 2
PHP Version5.4

Users monitoring this issue

There are no users monitoring this issue.

Activities

DenisChenu

DenisChenu

2015-09-08 18:41

developer   ~33079

We already update a lot KCfinder : maybe you can make a pull request on our github ?

user46119

2015-12-04 16:15

  ~33707

I have attached a quick&dirty implementation of pfeigl idea - it seems working. Please note this change require proper security review - because this modification the existing protection behaviour significantly (eg.: checkFilePath also use realpath) - and it could introduce security problems.
Perhaps a is_link/readlink based implementation only for the symlinked upload directory could be a better choice...

DenisChenu

DenisChenu

2015-12-04 16:25

developer   ~33708

Basically : it does a realpath to dir ?
Can you explain the security risk you found ? I think do a realpath is always a better idea for security.

Can you make a pull request to github ? More easy to merge after.

user46119

2015-12-04 16:42

  ~33709

Yes, it does a realpath to dir / path. The security risks of this change is unknown right now - I am not familiar with KCFinder internals and security mechanism - this is why I mentioned that it need a proper security review.

c_schmitz

c_schmitz

2015-12-04 17:20

administrator   ~33713

I don't see any security risk with this.
A proper PullRequest on Github would be nice.

DenisChenu

DenisChenu

2015-12-09 18:07

developer   ~33831

Have the solution in LS app

DenisChenu

DenisChenu

2015-12-09 18:15

developer   ~33832

Fix committed to master branch: http://bugs.limesurvey.org/plugin.php?page=Source/view&id=16447

DenisChenu

DenisChenu

2015-12-09 18:16

developer   ~33833

Put directly the realpath of uploaddir forkcfinder $_SESSION.

c_schmitz

c_schmitz

2015-12-15 13:15

administrator   ~33936

2.06+ Build 151215 released

Related Changesets

LimeSurvey: master cb707d61

2015-12-09 17:15:11

DenisChenu

Details Diff
Fixed issue 09856: kcfinder not working with symlinked /upload
Dev: best is surely to put the realpath in config
Affected Issues
09856
mod - application/helpers/admin/htmleditor_helper.php Diff File

Issue History

Date Modified Username Field Change
2015-08-31 11:52 user40445 New Issue
2015-09-08 18:41 DenisChenu Note Added: 33079
2015-09-11 10:54 c_schmitz Project Bug reports => Feature requests
2015-12-04 16:15 user46119 Note Added: 33707
2015-12-04 16:15 user46119 File Added: limesurvey_kcfinder_symlink_realpath.diff
2015-12-04 16:25 DenisChenu Note Added: 33708
2015-12-04 16:25 DenisChenu Project Feature requests => Bug reports
2015-12-04 16:27 DenisChenu Assigned To => DenisChenu
2015-12-04 16:27 DenisChenu Status new => assigned
2015-12-04 16:42 user46119 Note Added: 33709
2015-12-04 17:20 c_schmitz Note Added: 33713
2015-12-09 18:07 DenisChenu Note Added: 33831
2015-12-09 18:15 DenisChenu Changeset attached => LimeSurvey master cb707d61
2015-12-09 18:15 DenisChenu Note Added: 33832
2015-12-09 18:15 DenisChenu Resolution open => fixed
2015-12-09 18:16 DenisChenu Note Added: 33833
2015-12-09 18:16 DenisChenu Status assigned => resolved
2015-12-09 18:16 DenisChenu Fixed in Version => 2.06+
2015-12-15 13:15 c_schmitz Note Added: 33936
2015-12-15 13:15 c_schmitz Status resolved => closed