Bug #52312 [Com]: PHP safe_mode/open_basedir - lstat performance problem

From: Date: Sat, 05 Nov 2016 13:35:32 +0000
Subject: Bug #52312 [Com]: PHP safe_mode/open_basedir - lstat performance problem
References: 1  Groups: php.bugs 
Request: Send a blank email to php-bugs+get-205196@lists.php.net to get a copy of this message
Edit report at https://bugs.php.net/bug.php?id=52312&edit=1

 ID:                 52312
 Comment by:         spam2 at rhsoft dot net
 Reported by:        v dot damore at gmail dot com
 Summary:            PHP safe_mode/open_basedir - lstat performance
                     problem
 Status:             Analyzed
 Type:               Bug
 Package:            Safe Mode/open_basedir
 Operating System:   Linux
 PHP Version:        5.2.13
 Block user comment: N
 Private report:     N

 New Comment:

FRANKLY: why not check if "disable_functions" contains "link" and
"symlink" and in that case just use the realpath-cache as if there would not be a
open_basedir setting or even to avoid the overhead of check this add a
"realpath_cache_openbasedir" to config options where admins which disabled the link
functions can decide for themself?


Previous Comments:
------------------------------------------------------------------------
[2016-08-31 13:05:12] pierre dot renaudet at gmail dot com

We have this issue with Symfony(3.1.3), it's really poor performance with lot of stat on
file...

With the same project on Windows (IIS 7.5 - PHP 5.6.1
 With open_basedir (empty) it's take ~300-350ms for the index
 With open_basedir set (~9 path) it's take 3.5-10s for the index

I understand security problem, but why not just clear cache (before and/or after risky function)

things like that : 
--------------------
diff --git a/ext/standard/link.c b/ext/standard/link.c
index 62e7295..a844f46 100644
--- a/ext/standard/link.c
+++ b/ext/standard/link.c
@@ -158,6 +158,11 @@ PHP_FUNCTION(symlink)
 		RETURN_FALSE;
 	}
 
+	/* Reset realpath_cache when open_basedir is not null to avoid security issues */
+	if(PG(open_basedir)){
+		realpath_cache_clean();
+	}
+
 	/* For the source, an expanded path must be used (in ZTS an other thread could have changed the
CWD).
 	 * For the target the exact string given by the user must be used, relative or not, existing or
not.
 	 * The target is relative to the link itself, not to the CWD. */
@@ -206,6 +211,11 @@ PHP_FUNCTION(link)
 		RETURN_FALSE;
 	}
 
+	/* Reset realpath_cache when open_basedir is not null to avoid security issues */
+	if(PG(open_basedir)){
+		realpath_cache_clean();
+	}
+
 #ifndef ZTS
 	ret = link(topath, frompath);
 #else
 
diff --git a/ext/standard/link_win32.c b/ext/standard/link_win32.c
index 7d43162..e47e265 100644
--- a/ext/standard/link_win32.c
+++ b/ext/standard/link_win32.c
@@ -168,6 +168,12 @@ PHP_FUNCTION(symlink)
 		php_error_docref(NULL, E_WARNING, "UTF-16 conversion failed (error %d)",
GetLastError());
 		RETURN_FALSE;
 	}
+	
+	/* Reset realpath_cache when open_basedir is not null to avoid security issues */
+	if(PG(open_basedir)){
+		realpath_cache_clean();
+	}
+
 	/* For the source, an expanded path must be used (in ZTS an other thread could have changed the
CWD).
 	 * For the target the exact string given by the user must be used, relative or not, existing or
not.
 	 * The target is relative to the link itself, not to the CWD. */
@@ -223,6 +229,11 @@ PHP_FUNCTION(link)
 		RETURN_FALSE;
 	}
 
+	/* Reset realpath_cache when open_basedir is not null to avoid security issues */
+	if(PG(open_basedir)){
+		realpath_cache_clean();
+	}
+
 #ifndef ZTS
 	ret = CreateHardLinkA(topath, frompath, NULL);
 #else
 
diff --git a/main/main.c b/main/main.c
index bb98f27..7e5904e 100644
--- a/main/main.c
+++ b/main/main.c
@@ -2222,7 +2222,7 @@ int php_module_startup(sapi_module_struct *sf, zend_module_entry
*additional_mod
 
 	/* Disable realpath cache if an open_basedir is set */
 	if (PG(open_basedir) && *PG(open_basedir)) {
-		CWDG(realpath_cache_size_limit) = 0;
+		/* CWDG(realpath_cache_size_limit) = 0; */
 	}
 
 	/* initialize stream wrappers registry
--------------

------------------------------------------------------------------------
[2016-04-26 09:19:07] ianphp at binkmail dot com

This problem causes extreme loading on otherwise powerful servers when using PHP7/FastCGI/IIS, so
bad in fact that it's unusable for busy sites on an otherwise powerful server.

Unfortunately because this is a Windows server, the workarounds are not applicable.

I think that patch proposal is a good start to getting this fixed.

------------------------------------------------------------------------
[2013-12-16 16:39:10] pembo13 at gmail dot com

I seem to be suffering from this bug on an Apache/Linux + nfs setup. I'm using open_basedir and
the performance is so poor, that I can't even really stress test the server any more.

------------------------------------------------------------------------
[2013-05-29 21:45:36] phpdotnet at hostultra dot com

This bug is a real performance killer.

I propose this solution...
1. Modify symlink() php function so that if open_basedir or safe_mode is on, it disallows relative
symlinks with .. components, instead it creates absolute symlink.
This prevents attacker from exploiting CVE-2006-5178.

2. Allow the realpath cache to work even if open_basedir or safe_mode is on.

I did this with my own php code.

Before
---------
last pid: 30437;  load averages: 32.93, 24.67, 15.62
98 processes:  43 running, 46 sleeping, 9 lock
CPU:  2.7% user,  0.0% nice, 75.3% system,  0.5% interrupt, 21.5% idle

After
---------
last pid: 30582;  load averages:  2.06,  3.57, 10.58
68 processes:  6 running, 62 sleeping
CPU:  6.8% user,  0.0% nice,  1.7% system,  0.9% interrupt, 90.6% idle

------------------------------------------------------------------------
[2013-03-05 13:11:32] Terry at ellisons dot org dot uk

Rasmus, picking up our 2013-02-22 23:17 / 23:26 UTC conversation, I've thought about this some
further and gone through some test cases on the debugger.

Having walked through these call stacks, my view is that the PHP / Zend path scanning, file checking
and opening is a tangled mess.  A typical open goes through sometimes 7 seven wrapping layers each
of which can do compound path resolution.  The only reason that this doesn't end up slugging
performance is because the lowest level tsrm_realparth_r() uses a resolution cache and
short-circuits the actual I/O requests 95+% of the time. 

The CVE-2006-5178 advisory really to a vulerability in the open_basedir checks.  The root issue was
that a key check in php_check_specific_open_basedir() was using this cached path resolution; this
introduced the vulnerability because the caching enabled a race condition.  The fix was to turn of
ALL realpath resolution caching.  Yes this addressed the vulnerability, but at the price of killing
performance in the typical usecase where open_basedir might be used. This was unnecessary overkill.

If you think about it realpath caching itself doesn't introduce any vulnerability.  The issue
is that the open itself -- or in this case the preceding php_check_specific_open_basedir() check
must disable any caching for that one check alone.  Consider an example there the base dir is /a/b/c
and some path needs to be checked by php_check_specific_open_basedir().  There's not
vulnerability introduced by any previous resolution being used.  This will result in one of two
scenarios:

  *  The resolved path is not of the form /a/b/c.... in which case the error is thrown
  *  The resolved path is of the form /a/b/c... but the actual path might contain raced symlinks, so
it must be scanned (by tsrm_realparth_r) to ensure that no links are extant immediately prior to the
open.  There is also a sound case for curtailing this scan on a root-owned directory, but this of
second order.

Implementing this is a small code change.
  
 (i)  First drop the open_basedir predicated clearing of CWDG(realpath_cache_size_limit) = 0 in  
main/main.c:php_module_startup().  

 (ii) introduce a per-call mechanism for disabling the cache.  This could be done by adding a flag
to realpath, but the two variants (ZTS and none-ZTS) and the wide use of the wrapper VCWD_REALPATH()
macro might complicate this.  An alternative would be to add another flag to the PG() structure,
checking this in tsrm_realparth_r() and setting it in php_check_specific_open_basedir() around the
VCWD_REALPATH(path_tmp, resolved_name) call.

Reactions / Thoughts?  Is it worth me proposing a patch?

------------------------------------------------------------------------


The remainder of the comments for this report are too long. To view
the rest of the comments, please view the bug report online at

    https://bugs.php.net/bug.php?id=52312


--
Edit this bug report at https://bugs.php.net/bug.php?id=52312&edit=1


Thread (64 messages)

« previous php.bugs (#205196) next »