Hi,
Hello,
Thank you for the response. Replies inline:
I see the problem(s) and I took a look into the patch.
Can you confirm that you see the permissions bypass problem? I've seen
the chroot filename collision problem acknowledged in the bugtracker and
in old php-internals posts, but I've seen nobody from the PHP Project
explicitly acknowledge the permissions bypass vulnerability. If my
meaning isn't clear I can provide proof of concept off-list. The
permissions bypass affects both apache2handler (even with mod_ruid2) and
FPM (even with user pools).
I didn't see the problem in real life, but it's clear, that serving of few chroot environments using the same cache may lead to duplicate keys. FPM with separate pools shouldn't be affected.
They're separate problems both stemming from the simple filename design
of the cache keys.From the first look, I don't like the proposed solution.
It makes things a bit better, but can't solve shared-hosting
configuration problems.I'll be the first to agree it's not perfect; it's a band-aid from
someone with no prior familiarity with the codebase. I was just
surprised a trivially exploitable security hole like this was unpatched
for 2+ years and thought I would take a stab at a quick solution. Can
you elaborate on what shared-hosting problems it doesn't solve (aside
from chroot name collisions)?
Serving different users by the same process opens a lot of ways to stole data (sessions, persistent caches, persistent conections, dirty memory, etc).
They should be served by different FPM pools or separate Apache processes.
It doesn't solve even the simple chroot file resolution problem in
general (one user ma have few chroot environments with conflicting
file names).Agreed. Putting device+inode in the key would properly fix the chroot
scenario, but wouldn't the inode be readable if the parent directory is
readable? This could result in unexpected behaviour; ie a script
belonging to user alice, readable only by alice, can be run by user bob
if the parent directory is readable.Plus there's the performance considerations of
stat(); I know better
than to put thestat()call in the hot function. Apparently APC used a
stat struct passed from the SAPI? But how did this work with
included/required scripts which the SAPI wasn't aware of; were they all
cached under the parent script's key? I've skimmed that code in APC but
didn't have time for proper analysis. I admit ignorance here and thus I
preferred to leave dev+inode keying to the experts.
Yeah, inode is not an exelent solution as well.
I still strongly believe a user identifier is needed in addition to
dev+inode due to the permissions bypass issue.
Opcache just wasn't designed to solve permission problems in shared hosting evvironment.
"user id prefix", looks for me like a quick hack, that doesn't solve the whole problem anyway.
I'm not sure, if it's possible to make chroot on Windows, so why we
need to add windows user names?Frankly I don't know if any Windows configurations are vulnerable. I
have no experience with the Windows SAPI's. I didn't want to break the
Windows build, and wanted to keep the functionality analagous between
Windows and other platforms rather than leaving a possibly exploitable
design on Windows.But again I should stress that chroot filename collisions are not the
only bad behavior here. They're not the bug I'm most concerned with.
Do you talk about executing "unreadable" PHP scripts of different users?
I think, the proper way to fix this, whould executing access() check on each cached script access (this might be enabled/disabled through php.ini)
The patch introduces syscall in the hot function (this may be
optimized).Agreed. That isn't ideal. But the geteuid() call shouldn't be done
during opcache initialization when the SHM object is initialized,
because EUID might change afterwards. I didn't want to get EUID too
early so I erred on the side of caution, getting it at the last possible
moment. This is slower but safer because it prevents trivial cross-user
cache access from PHP userland. I'm open to suggestion if there's a more
"local" initialization function outside of key generation, which is
guaranteed to run after EUID changes in both FPM user pool, and
mod_ruid2/mod_php.
RINIT
I'm open for discussion and may change my mind. I'll also try to find
a better solution. Any suggestions are welcome.Frankly I think the better solution for FPM, would be to avoid doing
OPCache SHM object initialization in the FPM master before user pools
have forked and set EUID. That would fix both the permissions bypass and
probably key collisions in chroots.
Let me check this.
. mod_fcgid didn't have these problems
because PHP parent processes were started independently for each vhost.I'm not familiar enough with FPM code (yet) to be more specific, and I
don't like the fact that this doesn't doesn't address permissions bypass
with mod_php/mod_ruid2 which is a popular configuration which people
think is safe for shared hosting.Ideally I'd like to see OPCache keys fixed with a user identifier and
dev+inode, and FPM fixed as described above :).Thank you again for taking time to comment. I look forward to your
thoughts. Shall I send proof of concept for the permissions bypass,
off-list?
Let methink a bit more.
Thanks. Dmitry.
--
Dmitry,
Thank you for taking the time to answer my questions. Time allowing,
I'll be taking a closer look at the code this weekend. I do have a
couple of quick comments, see below:
Can you confirm that you see the permissions bypass problem? I've seen
the chroot filename collision problem acknowledged in the bugtracker and
in old php-internals posts, but I've seen nobody from the PHP Project
explicitly acknowledge the permissions bypass vulnerability. If my
meaning isn't clear I can provide proof of concept off-list. The
permissions bypass affects both apache2handler (even with mod_ruid2) and
FPM (even with user pools).I didn't see the problem in real life, but it's clear, that serving of
few chroot environments using the same cache may lead to duplicate
keys. FPM with separate pools shouldn't be affected.
FPM with separate user pools under a single master is affected by the
permissions bypass issue. To avoid the issue, separate user pools isn't
sufficient; you would need separate FPM master daemons.
Many users read about the ability to run separate pools with separate
users under a single master and think this provides adequate user
separation:
http://php.net/manual/en/install.fpm.configuration.php
But when OPCache is enabled, the user pools under a single master all
share a common cache, with disastrous results if a single user is
compromised.
Unfortunately this is the way the popular shared hosting control panels
have started implementing FPM: with a single master.
Was "single master, multiple pools with separate users" not intended for
a shared hosting environment? If not, what is the point of the 'user'
and 'chroot' directives? Were shared hosts using FPM always expected to
use separate FPM master daemons? If so, the documentation might be more
explicit IMHO (I'd be willing to put in some work here if you feel a
documentation fix is what's needed).
But again I should stress that chroot filename collisions are not the
only bad behavior here. They're not the bug I'm most concerned with.Do you talk about executing "unreadable" PHP scripts of different
users? I think, the proper way to fix this, whould executing access()
check on each cached script access (this might be enabled/disabled
through php.ini)
Yes! This is exactly my concern. And you're absolutely right, a check of
access() at script compile time is a better solution than my patch. I
think it should be the default behavior though.
I'm starting to think I should have opened a separate bug for the
permissions bypass issue and the chroot filename collision; in the
bug tracker users were already lumping these issues together but they're
really separate concerns.
Off-list I'm going to send you a proof of concept script which
demonstrates the problem with a typical FPM/OPCache deployment in a
shared environment.
-php-dev at coydogsoftware dot net