CMS MADE SIMPLE FORGE

CMS Made Simple Core

 

[#8964] excessive delay in backoffice caused by a bug in the code (and its solution)

avatar
Created By: bess (bess)
Date Submitted: Thu Feb 21 04:07:47 -0500 2013

Assigned To:
Version: 1,11,4
CMSMS Version: None
Severity: Minor
Resolution: Won't Fix
State: Closed
Summary:
excessive delay in backoffice caused by a bug in the code (and its solution)
Detailed Description:
the term bug may be excessive, however it is possible to easily correct :)

I have a website with more than 700MB image and distributed in 4000 files to 4
levels deep in the directory / upload / Gallery. after an upgrade from 1.9.4.3
to 1.11.4 I met excessive delays leading to regular timeouts on my Hoster in:
ImageManager, FileManager but not in Gallery

After some research in the code I found this:

It exists in the class /lib/ImageManager/Classes/ImageManager.php the function
getDirs() which, in essence, gives a list of existing directories and
subdirectories from a given path to the class that we will call here $PATH

In our case (4 or 5 levels deep, thousands of files) it takes time to explore
everything. This is normal in absolute ...

But there is another feature in this class: validRelativePath(), a function
whose role is to determine whether a relative path passed as parameter is
correct with respect to the $PATH of the class. I would have expected a code
like this:
 * check the existence of the directory
 * a regex control the absence of crappy path (../../../../config.php)
 * control to ensure that the relative path starts well in $ PATH

Nothing complex ... except that the code validRelativePath() uses getDirs() that
suddenly a listing TOTAL existing directories to see if it exists in the list
... Even better it performs this scan painful and returns true if the parameter
is set to '/' ... as saying that the scan is unnecessarily when you're in the
root path

/**
     * Check if the given path is part of the subdirectories
     * under the base_dir.
     * @param string $path the relative path to be checked
     * @return boolean true if the path exists, false otherwise
     */
    function validRelativePath($path)
    {
        $dirs = $this->getDirs();
        if($path == '/' || $path == '\/') return true; // stupid windoze.
        //check the path given in the url against the
        //list of paths in the system.
        for($i = 0; $i < count($dirs); $i++)
        {
            $key = key($dirs);
            //we found the path
            if($key == $path) {
              return true;
            }
       
            next($dirs);
        }
        return false;
    }

And lack of bowl .... validRelativePath() is used everywhere ... even in
ImageManager and FileManager


Now the solution : 

	function validRelativePath($path) 
	{
		return strpos(
			realpath($this->getBaseDir() . DIRECTORY_SEPARATOR . $path), 
			realpath($this->getBaseDir())
		) === 0;
	}

explanation :
http://stackoverflow.com/questions/7545468/regex-to-check-if-a-path-go-only-down

it's secure and instantly returns the result :)


History

Comments
avatar
Date: 2013-02-21 09:51
Posted By: Robert Campbell (calguy1000)

Since this functionality has not changed in a long time and this is the first
report of the issue, I have to say that the issue is minor.

Also, realpath is not a valid solution in this type of function due to the use
of symbolic links, or network filesystems on some hosting providers.

Since ImageManager is slated for removal, we will not fix this.
      
avatar
Date: 2013-02-21 12:25
Posted By: Robert Campbell (calguy1000)

well, realpath in this case is okay, because it's just used to compare paths. 
but it's still a minor issue.
never really complained about before.... and since ImageManager is an ugly beast
that has given us nothing but grief, and is subject to replacement... probably
won't be fixed.
      
avatar
Date: 2013-02-21 12:57
Posted By: bess (bess)

a little suggestion : if my code is okay for every case, why won't you include
it into the 1.11.5 release ?

if and only if my code is okay for every case of course ! after reading your
second message i may understand that it was okay (?)

i'm currently forced to make my own hack even if i can understand that it's a
"minor" bug for every else ;) but i'm like you : i hate changes in the source
code :D
      
avatar
Date: 2014-03-14 23:34
Posted By: Robert Campbell (calguy1000)

ImageManager is gone in 2.0
      
Updates

Updated: 2015-09-06 13:18
cmsms_version_id: 29887 => -1
state: Open => Closed

Updated: 2013-02-21 09:51
resolution_id: 5 => 8
severity_id: 2 => 3

Updated: 2013-02-21 04:23
description: the term bug may be excessive, however it is possible to easily correct :) I have a website with more than 700MB image and distributed in 4000 files to 4 levels deep in the directory / upload / Gallery. after an upgrade from 1.9.4.3 to 1.11.4 I met exc => the term bug may be excessive, however it is possible to easily correct :) I have a website with more than 700MB image and distributed in 4000 files to 4 levels deep in the directory / upload / Gallery. after an upgrade from 1.9.4.3 to 1.11.4 I met exc
resolution_id: => 5