Hey,
It's a bit quiet here lately :)
Has anyone had a chance to try, test and benchmark the realpath() patch I
send to the list?
Andi
Has anyone had a chance to try, test and benchmark the
realpath()patch I
send to the list?
I had a look the other day, but here is a more detailed look at the system
calls involved in a php5 request. I have stripped out most of the stuff
we can't do anything about. The script I am testing is
/var/home/rasmus/php5/o which contains:
<?php
include '/var/www/lerdorf.com/abc';
include '/var/www/lerdorf.com/abc';
include '/var/www/lerdorf.com/def';
include '/var/www/phpics.com/ghi';
?>
The system calls required to open and read the script:
getcwd("/var/home/rasmus/php5", 4096) = 22
lstat64("/var/home/rasmus/php5/o", {st_mode=S_IFREG|0644, st_size=156, ...}) = 0
setitimer(ITIMER_PROF, {it_interval={0, 0}, it_value={0, 0}}, NULL) = 0
rt_sigaction(SIGPROF, {0x4067a7a0, [PROF], SA_RESTORER|SA_RESTART, 0x406ea658}, {0x4067a7a0, [PROF], SA_RESTORER|SA_RESTART, 0x406ea658}, 8) = 0
rt_sigprocmask(SIG_UNBLOCK, [PROF], NULL, 8) = 0
read(3, "<?php\n\tinclude '/var/www/lerdorf"..., 8192) = 156
read(3, "", 4096) = 0
read(3, "", 8192) = 0
close(3) = 0
ok so far. Now the first include:
getcwd("/var/home/rasmus/php5", 4096) = 22
lstat64("/var", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
lstat64("/var/www", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
lstat64("/var/www/lerdorf.com", {st_mode=S_IFDIR|0777, st_size=16384, ...}) = 0
lstat64("/var/www/lerdorf.com/abc";, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0
open("/var/www/lerdorf.com/abc";, O_RDONLY) = 3
fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0
fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0
lseek(3, 0, SEEK_CUR) = 0
read(3, "abc\n", 8192) = 4
read(3, "", 8192) = 0
read(3, "", 8192) = 0
close(3) = 0
Since nothing is cached yet we see realpath() statting the entire tree
leading to the abc include file. There is an obvious double fstat on the
opened descriptor there that we should track down.
Then we get:
fstat64(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 1), ...}) = 0
old_mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x40018000
write(1, "abc\n", 4) = 4
To write out the results.
The second time we include the same file the stat cache kicks in and we
have:
getcwd("/var/home/rasmus/php5", 4096) = 22
time(NULL) = 1090679437
open("/var/www/lerdorf.com/abc";, O_RDONLY) = 3
fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0
fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0
lseek(3, 0, SEEK_CUR) = 0
read(3, "abc\n", 8192) = 4
read(3, "", 8192) = 0
read(3, "", 8192) = 0
close(3) = 0
write(1, "abc\n", 4) = 4
Not sure where that time() call came from here, and again we see the
double fstat. And since we have already started writing the output the
results are written out immediately in a single write.
Now the /var/www/lerdorf.com/def include:
getcwd("/var/home/rasmus/php5", 4096) = 22
lstat64("/var", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
lstat64("/var/www", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
lstat64("/var/www/lerdorf.com", {st_mode=S_IFDIR|0777, st_size=16384, ...}) = 0
lstat64("/var/www/lerdorf.com/def";, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0
time(NULL) = 1090679437
open("/var/www/lerdorf.com/def";, O_RDONLY) = 3
fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0
fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0
lseek(3, 0, SEEK_CUR) = 0
read(3, "def\n", 8192) = 4
read(3, "", 8192) = 0
read(3, "", 8192) = 0
close(3) = 0
write(1, "def\n", 4) = 4
We get what we expect here too. Since the stat cache only caches full
paths and we are now including a different file from the same location we
have to stat the whole tree again.
And the final include:
getcwd("/var/home/rasmus/php5", 4096) = 22
lstat64("/var", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
lstat64("/var/www", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
lstat64("/var/www/phpics.com", {st_mode=S_IFDIR|0755, st_size=8192, ...}) = 0
lstat64("/var/www/phpics.com/ghi";, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0
time(NULL) = 1090679437
open("/var/www/phpics.com/ghi";, O_RDONLY) = 3
fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0
fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0
lseek(3, 0, SEEK_CUR) = 0
read(3, "ghi\n", 8192) = 4
read(3, "", 8192) = 0
read(3, "", 8192) = 0
close(3) = 0
write(1, "ghi\n", 4) = 4
No surprises there either.
Shutting down (cli version):
close(1) = 0
close(0) = 0
setitimer(ITIMER_PROF, {it_interval={0, 0}, it_value={0, 0}}, NULL) = 0
setitimer(ITIMER_PROF, {it_interval={0, 0}, it_value={30, 0}}, NULL) = 0
rt_sigaction(SIGPROF, {0x4067a7a0, [PROF], SA_RESTORER|SA_RESTART, 0x406ea658}, {0x4067a7a0, [PROF], SA_RESTORER|SA_RESTART, 0x406ea658}, 8) = 0
rt_sigprocmask(SIG_UNBLOCK, [PROF], NULL, 8) = 0
setitimer(ITIMER_PROF, {it_interval={0, 0}, it_value={0, 0}}, NULL) = 0
munmap(0x40018000, 4096) = 0
exit_group(0) = ?
I'd love to see a realpath() replacement function which makes use of the
stat cache for partial paths as well. Chances are that on a busy ISP
server, for example, you will have thousands of scripts and include files
served up from the same base path and you are going to be statting your
way along that tree every time you hit a file that isn't in the cache.
And that double-fstat needs to go away. A very quick look for it seems to
show one checking if the opened file is a regular file in
fopen_wrappers.c. Ideally we would simply use the stat struct from our
own realpath() implementation and neither of these fstat() calls would be
needed. The goal should be to average at most 1 stat per file, whether it
be the main script or include files. For the main script, if we are
running under Apache, Apache has already done this stat and passed us the
stat struct
-Rasmus
Hi,
I'd love to see a
realpath()replacement function which makes use of the
stat cache for partial paths as well. Chances are that on a busy ISP
server, for example, you will have thousands of scripts and include files
served up from the same base path and you are going to be statting your
way along that tree every time you hit a file that isn't in the cache.
It is a very good idea to replace realpath() with an own function.
Realpath is not only slow, it is also "dangerous" on some systems like
linux where it also works on paths that are not really existing.
example: include "./foo/bar/index.php/../../../../../../etc/passwd";;
On the BSDs this should be invalid, but linux happily accepts it as
/etc/passwd. This is just one thing that would largely benefit from an
own realpath().
Stefan
Stefan Esser wrote:
Realpath is not only slow, it is also "dangerous" on some systems like
linux where it also works on paths that are not really existing.example: include "./foo/bar/index.php/../../../../../../etc/passwd";;
This is a completely legal unix path as .. in / is / again. And from a
security point of view I'd say the only thing you gain by disallowing
this is that the number of .. in the path have to be guessed (pretty
easily) by the attacker.
Or am I missing something here?
Not that I can see the benefit of a home-grown realpath() :-)
- Chris
Christian Schneider wrote:
example: include "./foo/bar/index.php/../../../../../../etc/passwd";;
This is a completely legal unix path as .. in / is / again. And from a
security point of view I'd say the only thing you gain by disallowing
this is that the number of .. in the path have to be guessed (pretty
easily) by the attacker.
This is no legal unix path, because index.php is a file and not a directory.
And to understand the security impact:
include "./foo/bar/template_".$userinput;
By setting $userinput to "validtemplate.php/../../../../etc/passwd"; you
get to the /etc/passwd file, altough the path is completely illegal.
Stefan
Stefan Esser wrote:
This is no legal unix path, because index.php is a file and not a
Oops, missed that part of the path, just looked at the .. :-)
And to understand the security impact:
include "./foo/bar/template_".$userinput;
... which I'd consider bad practice anyway but that's another story :-)
- Chris
Hi,
Thanks for the in-depth analysis.
I agree that we should nuke that extra fstat(). We need to track down where
it is (probably streams :). I don't think it's a good idea to cache the
stat() itself because usually you don't stat() the same file twice in the
same request, and caching those in between requests could lead to weird
bugglets such as Last-Modified: not changing even though you changed the
script. I think the realpath() is the main performance problem and easiest
to deal with as very rarely will the real path change so caching them can't
do too much damage.
The time() call is part of the patch to check if we should refresh the
realpath cache for this specific path. If it becomes a problem then we
might need to think of a less deterministic approach where we call time()
only every few requests. Another idea I had was to call time() once at the
beginning of the request and always use the same one for reference. I don't
see any major problem with this as far as realpath is concerned. It might
even be useful in other places where not the exact time is needed but the
execution start time is good enough.
I think it's going to be quite hard and not really worthwhile to add
support for partial realpath(). In this case I like the kiss approach. It
gives a great bang for the buck and is very simple.
By the way, if we use the VIRTUAL_DIR support that getcwd() is also saved.
Not sure if we want to convert (it might be a bit too dangerous) but it's
something to keep in the back of the mind.
Andi
At 08:06 AM 7/24/2004 -0700, Rasmus Lerdorf wrote:
Has anyone had a chance to try, test and benchmark the
realpath()patch I
send to the list?I had a look the other day, but here is a more detailed look at the system
calls involved in a php5 request. I have stripped out most of the stuff
we can't do anything about. The script I am testing is
/var/home/rasmus/php5/o which contains:
<?php
include '/var/www/lerdorf.com/abc';
include '/var/www/lerdorf.com/abc';
include '/var/www/lerdorf.com/def';
include '/var/www/phpics.com/ghi';
?>The system calls required to open and read the script:
getcwd("/var/home/rasmus/php5", 4096) = 22
lstat64("/var/home/rasmus/php5/o", {st_mode=S_IFREG|0644, st_size=156,
...}) = 0
setitimer(ITIMER_PROF, {it_interval={0, 0}, it_value={0, 0}}, NULL) = 0
rt_sigaction(SIGPROF, {0x4067a7a0, [PROF], SA_RESTORER|SA_RESTART,
0x406ea658}, {0x4067a7a0, [PROF], SA_RESTORER|SA_RESTART, 0x406ea658}, 8) = 0
rt_sigprocmask(SIG_UNBLOCK, [PROF], NULL, 8) = 0
read(3, "<?php\n\tinclude '/var/www/lerdorf"..., 8192) = 156
read(3, "", 4096) = 0
read(3, "", 8192) = 0
close(3) = 0ok so far. Now the first include:
getcwd("/var/home/rasmus/php5", 4096) = 22
lstat64("/var", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
lstat64("/var/www", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
lstat64("/var/www/lerdorf.com", {st_mode=S_IFDIR|0777, st_size=16384,
...}) = 0
lstat64("/var/www/lerdorf.com/abc";, {st_mode=S_IFREG|0644, st_size=4,
...}) = 0
open("/var/www/lerdorf.com/abc";, O_RDONLY) = 3
fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0
fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0
lseek(3, 0, SEEK_CUR) = 0
read(3, "abc\n", 8192) = 4
read(3, "", 8192) = 0
read(3, "", 8192) = 0
close(3) = 0Since nothing is cached yet we see
realpath()statting the entire tree
leading to the abc include file. There is an obvious double fstat on the
opened descriptor there that we should track down.Then we get:
fstat64(1, {st_mode=S_IFCHR|0620, st_rdev=makedev(136, 1), ...}) = 0
old_mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
- = 0x40018000
write(1, "abc\n", 4) = 4To write out the results.
The second time we include the same file the stat cache kicks in and we
have:getcwd("/var/home/rasmus/php5", 4096) = 22
time(NULL) = 1090679437
open("/var/www/lerdorf.com/abc";, O_RDONLY) = 3
fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0
fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0
lseek(3, 0, SEEK_CUR) = 0
read(3, "abc\n", 8192) = 4
read(3, "", 8192) = 0
read(3, "", 8192) = 0
close(3) = 0
write(1, "abc\n", 4) = 4Not sure where that
time()call came from here, and again we see the
double fstat. And since we have already started writing the output the
results are written out immediately in a single write.Now the /var/www/lerdorf.com/def include:
getcwd("/var/home/rasmus/php5", 4096) = 22
lstat64("/var", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
lstat64("/var/www", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
lstat64("/var/www/lerdorf.com", {st_mode=S_IFDIR|0777, st_size=16384,
...}) = 0
lstat64("/var/www/lerdorf.com/def";, {st_mode=S_IFREG|0644, st_size=4,
...}) = 0
time(NULL) = 1090679437
open("/var/www/lerdorf.com/def";, O_RDONLY) = 3
fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0
fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0
lseek(3, 0, SEEK_CUR) = 0
read(3, "def\n", 8192) = 4
read(3, "", 8192) = 0
read(3, "", 8192) = 0
close(3) = 0
write(1, "def\n", 4) = 4We get what we expect here too. Since the stat cache only caches full
paths and we are now including a different file from the same location we
have to stat the whole tree again.And the final include:
getcwd("/var/home/rasmus/php5", 4096) = 22
lstat64("/var", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
lstat64("/var/www", {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
lstat64("/var/www/phpics.com", {st_mode=S_IFDIR|0755, st_size=8192, ...}) = 0
lstat64("/var/www/phpics.com/ghi";, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0
time(NULL) = 1090679437
open("/var/www/phpics.com/ghi";, O_RDONLY) = 3
fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0
fstat64(3, {st_mode=S_IFREG|0644, st_size=4, ...}) = 0
lseek(3, 0, SEEK_CUR) = 0
read(3, "ghi\n", 8192) = 4
read(3, "", 8192) = 0
read(3, "", 8192) = 0
close(3) = 0
write(1, "ghi\n", 4) = 4No surprises there either.
Shutting down (cli version):
close(1) = 0
close(0) = 0
setitimer(ITIMER_PROF, {it_interval={0, 0}, it_value={0, 0}}, NULL) = 0
setitimer(ITIMER_PROF, {it_interval={0, 0}, it_value={30, 0}}, NULL) = 0
rt_sigaction(SIGPROF, {0x4067a7a0, [PROF], SA_RESTORER|SA_RESTART,
0x406ea658}, {0x4067a7a0, [PROF], SA_RESTORER|SA_RESTART, 0x406ea658}, 8) = 0
rt_sigprocmask(SIG_UNBLOCK, [PROF], NULL, 8) = 0
setitimer(ITIMER_PROF, {it_interval={0, 0}, it_value={0, 0}}, NULL) = 0
munmap(0x40018000, 4096) = 0
exit_group(0) = ?I'd love to see a
realpath()replacement function which makes use of the
stat cache for partial paths as well. Chances are that on a busy ISP
server, for example, you will have thousands of scripts and include files
served up from the same base path and you are going to be statting your
way along that tree every time you hit a file that isn't in the cache.And that double-fstat needs to go away. A very quick look for it seems to
show one checking if the opened file is a regular file in
fopen_wrappers.c. Ideally we would simply use the stat struct from our
ownrealpath()implementation and neither of thesefstat()calls would be
needed. The goal should be to average at most 1 stat per file, whether it
be the main script or include files. For the main script, if we are
running under Apache, Apache has already done this stat and passed us the
stat struct-Rasmus
I think it's going to be quite hard and not really worthwhile to add
support for partialrealpath(). In this case I like the kiss approach. It
gives a great bang for the buck and is very simple.
Well, there is another approach here. There are only 2 places where we
need the full paths. The first is for the included_files list to be able
to do the require_once/include_once check and the second is for the
open_basedir mechanism. There are a lot of people out there who do not
use open_basedir and I would guess most people looking for maximum
performance wouldn't be in a situation where they needed the open_basedir
feature. So the only real feature in question here is the *_once
checking and even there you don't necessarily need the full path although
without it you could potentially run into problems if you include the same
file through 2 different relative paths or more obscurely if you
deliberately change your cwd halfway through your script and include a
file of the same name through the same relative path but this is not
actually the same file. In both cases you are basically going out of your
way to trick the _once check. At Yahoo I have been running everything
with relative paths for well over a year now without any problems.
So, some approaches:
a. compile-time switch to disable full paths completely
b. ini switch to disable full paths completely unless open_basedir is enabled
c. ini switch to disable full paths from include/require but enable them
for include_once/require_once. That is, if you want *_once protection
you use include_once/require_once. If you don't, you just use
include/require and include/require would not populate the
included_files list.
-Rasmus
The
time()call is part of the patch to check if we should refresh the
realpath cache for this specific path. If it becomes a problem then we
might need to think of a less deterministic approach where we calltime()
only every few requests. Another idea I had was to calltime()once at the
beginning of the request and always use the same one for reference. I don't
see any major problem with this as far as realpath is concerned. It might
even be useful in other places where not the exact time is needed but the
execution start time is good enough.
I meant to mention this a while ago, but it slipped my mind. Most web
servers do that time() call for us at the beginning of the request because
they need it for logging purposes. I think the right approach here is to
add a SAPI call to expose this. For Apache-1.3 it is right in the
request_rec in the request_time field. Obviously the SAPI call would
simply do the time() call itself if it can't fetch it from somewhere
internally.
-Rasmus
I meant to mention this a while ago, but it slipped my mind. Most web
servers do thattime()call for us at the beginning of the request because
they need it for logging purposes. I think the right approach here is to
add a SAPI call to expose this. For Apache-1.3 it is right in the
request_rec in the request_time field. Obviously the SAPI call would
simply do thetime()call itself if it can't fetch it from somewhere
internally.
It sounds good enough, but I'd worry about people using this in
incompletely-thought-out benchmarking scripts.
A SAPI which supports the request time record would appear to take slightly
longer than a SAPI which does not (the time between processing the request
in the web server and passing off to PHP {plus compile, plus execute down to
that instructuion})
-Sara
I meant to mention this a while ago, but it slipped my mind. Most web
servers do thattime()call for us at the beginning of the request because
they need it for logging purposes. I think the right approach here is to
add a SAPI call to expose this. For Apache-1.3 it is right in the
request_rec in the request_time field. Obviously the SAPI call would
simply do thetime()call itself if it can't fetch it from somewhere
internally.It sounds good enough, but I'd worry about people using this in
incompletely-thought-out benchmarking scripts.
Adding a SAPI call doesn't mean exposure to scripts, so I don't really see
how this is relevant. Sure, someone could write a PHP extension to
benchmark different servers, but what are the chances of that? And if
they are advanced enough to write an extension, they are smart enough to
normalize the request start times.
-Rasmus
Adding a SAPI call doesn't mean exposure to scripts, so I don't really see
how this is relevant. Sure, someone could write a PHP extension to
benchmark different servers, but what are the chances of that? And if
they are advanced enough to write an extension, they are smart enough to
normalize the request start times.
Sorry, I misread... I thought you were talking about exposing this call to
userspace.
-Sara
At 08:59 AM 8/6/2004 -0700, Sara Golemon wrote:
I meant to mention this a while ago, but it slipped my mind. Most web
servers do thattime()call for us at the beginning of the request because
they need it for logging purposes. I think the right approach here is to
add a SAPI call to expose this. For Apache-1.3 it is right in the
request_rec in the request_time field. Obviously the SAPI call would
simply do thetime()call itself if it can't fetch it from somewhere
internally.It sounds good enough, but I'd worry about people using this in
incompletely-thought-out benchmarking scripts.A SAPI which supports the request time record would appear to take slightly
<note> Do <emphasis>NOT</emphasis> use this method for benchmarking between different webservers, inconsistent implementations could give unexpected results. </note>
longer than a SAPI which does not (the time between processing the request
in the web server and passing off to PHP {plus compile, plus execute down to
that instructuion})
If you call it get_request_start_time() it should make it clearer what it
means.
I guess we could put it into SAPI I don't see any disadvantage although in
any case, doing it one more time per-request wouldn't be that bad either.
I hope to have more time towards the end of next week and then I can
hopefully make this improvement.
Andi
If you call it get_request_start_time() it should make it clearer what it
means.
I guess we could put it into SAPI I don't see any disadvantage although in
any case, doing it one more time per-request wouldn't be that bad either.
I hope to have more time towards the end of next week and then I can
hopefully make this improvement.
Sure, it's a single syscall, but this entire effort is about removing
syscalls, so adding one that isn't necessary because it has already been
done during the request makes sense.
I am just cleaning up some other code where a sapi call for this would
make things a lot cleaner. Without the sapi call you end up needing to do
this:
#if HAVE_APACHE
#include "httpd.h"
#endif
...
#if HAVE_APACHE
t = ((request_rec *)SG(server_context))->request_time;
#else
t = time(0);
#endif
-Rasmus
If you call it get_request_start_time() it should make it clearer
what it
means.
I guess we could put it into SAPI I don't see any disadvantage
although in
any case, doing it one more time per-request wouldn't be that bad
either.
I hope to have more time towards the end of next week and then I can
hopefully make this improvement.Sure, it's a single syscall, but this entire effort is about removing
syscalls, so adding one that isn't necessary because it has already
been
done during the request makes sense.
I didn't imagine this would be so controversial. I'm all for this
added callout.
George