Edit report at https://bugs.php.net/bug.php?id=69090&edit=1
ID: 69090
Comment by: php-dev at coydogsoftware dot net
Reported by: simon at ikanobori dot jp
Summary: add prefix/xor to cache keys/check permissions or
separate caches
Status: Analyzed
Type: Feature/Change Request
Package: opcache
Operating System: linux/debian
PHP Version: 5.6.5
Block user comment: N
Private report: N
New Comment:
Simon, that's correct; my patch doesn't fix same-user, multiple chroot scenarios. Search
the comments for "I agree with Rasmus" for my thoughts on that. I was only attempting to
fix the cross-user permissions bypass, which I personally consider the more serious issue. If I knew
the appropriate place to do the stat() call I would have added device+inode, but I've only had
limited time for this and it's my first exposure to PHP internals so I kept the scope small.
Maybe if I get more free time, and reach consensus with the maintainers, I'll have a more
complete fix later.
Previous Comments:
------------------------------------------------------------------------
[2016-11-10 08:40:29] simon at ikanobori dot jp
Original reporter here, I sitll read the updates to this.
Thanks php-dev for the patch, it seems to work if just the euid is included but correct me if I am
wrong for yes the euid gets set after PHP-FPM forks a new pool under a specific user but just
running separate sites chrooted under the same user (say, www-data) would still cause the same
issues as described in my original report.
Now, that's obviously an unwise way to run any PHP shared hosting environment but it might be
something we want to change as well. I'll take a look at the patch as well but maybe we can do
something closer to inodes to be used in the cache key instead of effective user id.
I'll try to replicate with your patch applied over the weekend :)
------------------------------------------------------------------------
[2016-11-08 13:46:30] php-dev at coydogsoftware dot net
I've just tested suexec/mod_fcgid: This configuration seems to be unaffected because separate
vhosts' php-cgi processes don't share a common parent PHP process. mod_fcgid plays a
similar role to the FPM master, but because it's not PHP and doesn't initialize the
opcache, there's no single SHM object being shared. Each vhost gets its own opcache SHM object
if I understand correctly.
Thus mod_fcgid with suexec provides better compartmentization than FPM since it has no single parent
PHP process from which all others are descended.
To summarize,
- apache/mod_ruid2/mod_php is vulnerable.
- php-fpm is vulnerable (probably regardless of webserver).
- apache/suexec/mod_fcgid/php-cgi is not vulnerable.
Perhaps php-fpm needs a long hard look at the order in which it initializes pools vs extensions?
------------------------------------------------------------------------
[2016-11-08 07:06:10] php-dev at coydogsoftware dot net
kmark937, I've emailed you directly with a PoC exploit script. I'd love to hear about your
results when you try it.
dol+list, one nitpick with your earlier comments; according to OPCache comments any changes to the
key scheme need to be prepended, not appended, due to the way the key delimiters are parsed (I
wonder what happens when script filenames contain ':'?).
To any PHP project people still reading, would it be possible to reclassify this as a bug report
instead of feature request? This was opened as a feature request at the suggestion of the original
bug reporter in #67481, who withdrew their bug report when they decided that the vulnerable behavior
was desirable after they found a workaround by enabling use_cwd. use_cwd does not fix this issue in
most cases with common CMS applications because the use_cwd logic is only applied to scripts
invoked/included via relative paths, which is less common in real-world web server/CMS environments.
So far I've avoided opening yet another duplicate bug tracker item for this behavior, but it
absolutely needs to be treated as a bug and not a feature request since OPCache documentation does
not warn about it and php-internals discussions dating back to the Optimizer+ days indicate that the
maintainers do indeed intend this feature to be usable in multi-user environments.
jpauli, I do sincerely appreciate that you took the time to comment on this as a PHP project member.
Did you have a chance to review my response? Do you see that multiple FPM pools with separate users
to *not* have the "obvious" separate SHM behavior which you seem to think they do? Would
you care to try my exploit script? I'm trying to give the PHP project every chance to address
the issue before I publish the exploit script, but they've literally had years to address this
and show little interest in fixing it. There's nothing novel about the exploit btw; it does
nothing different from the PoC configs already given in this request and in bug #67481.
------------------------------------------------------------------------
[2016-11-08 01:58:51] kmark937 at gmail dot com
php-dev,
Thanks for providing further insight into this issue. I'd like to take you up on your offer of
a PoC for replicating under mod_php + open_basedir. FPM works as well but I'm particularly
interested in the mod_php case.
Thanks!
------------------------------------------------------------------------
[2016-11-07 15:13:13] php-dev at coydogsoftware dot net
dol+list, thank you for your comments. Your observations are consistent with my own experiences,
with the exception that I haven't had a chance to test exploitation under CloudLinux/CageFS. I
haven't bothered testing with cPanel's "Jail Apache" virtfs feature either, due
to unrelated reliability problems with that feature. I'm trying to avoid too much discussion of
such platform-specific mitigations because they're out of scope for a PHP bug report, and they
shouldn't even be necessary. I feel strongly that this is a PHP bug which needs to be fixed in
PHP.
I agree that device+inode should ideally be added to the key scheme, but they alone are
insufficient. They are not necessarily private information, and can be read if permissions of the
parent directory allow. This breaks user expectations; if wp-config.php is unreadable for a user,
that user should not be able to execute that script, period. This is why I think EUID should be part
of the key. I don't like the thought of opcache playing permissions referee and I think it
suggests a fundamental design flaw with the way SHM is being used, but working within the existing
SHM design I think EUID is the best way forward. Perhaps a more radical change would be better, but
I'm not familiar enough with prior art in PHP opcode caching to know what it is.
Using device+inode is also less straightforward than using EUID, otherwise I'm sure the
maintainers would have already done it: it seems inappropriate to stat() for this info during key
generation for performance reasons. APC apparently got a stat struct passed from the SAPI so the
stat() was already done outside of APC code (does this imply APC cached scripts per compilation unit
and not per file, since the web server wouldn't know which files are included? I haven't
reviewed APC code in enough detail yet to confirm).
For my patch I use EUID simply because there's no undue performance hit and it fixes the
cross-user permissions bypass in both "out of the box" and common control panel
environments. It's probably not perfect but surely we can agree it's better than the
existing code.
For anyone using FPM who wants to mitigate before PHP fixes the vulnerability, 2 years ago Mattias
Geniar confirmed that separate FPM master daemons is the way to go, and provided example configs.
It's cumbersome and inefficient and shouldn't be necessary: https://ma.ttias.be/a-better-way-to-run-php-fpm/
------------------------------------------------------------------------
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=69090
--
Edit this bug report at https://bugs.php.net/bug.php?id=69090&edit=1