Edit report at https://bugs.php.net/bug.php?id=69090&edit=1
ID: 69090
Patch added by: dmitry@php.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
Assigned To: dmitry
Block user comment: N
Private report: N
New Comment:
The following patch has been added/updated:
Patch Name: bug69090.diff
Revision: 1479222667
URL: https://bugs.php.net/patch-display.php?bug=69090&patch=bug69090.diff&revision=1479222667
Previous Comments:
------------------------------------------------------------------------
[2016-11-15 11:36:18] dmitry@php.net
I've attached a patch to enable optional file permission validation.
https://bugs.php.net/patch-display.php?bug_id=69090&patch=validate_permission.diff&revision=latest
With opcache.validate_permission=1 php.ini directive, PHP is going to revalidate readability of
cached files using access() syscall. This directive is going to be disabled by default and should be
enabled by shared hosting providers. Additional checks lead to ~5% slowdown on Wordpress.
The proposed manipulations with keys (e.g. including user name or root directory into key)
won't work out of the box, because in some cases opcache doesn't use keys constructed by
accel_make_persistent_key(), but uses full-real-name instead.
I didn't solve the chroot keys collision problem yet.
------------------------------------------------------------------------
[2016-11-15 11:21:20] dmitry@php.net
The following patch has been added/updated:
Patch Name: validate_permission.diff
Revision: 1479208880
URL: https://bugs.php.net/patch-display.php?bug=69090&patch=validate_permission.diff&revision=1479208880
------------------------------------------------------------------------
[2016-11-10 14:59:48] sjon at hortensius dot net
I'd prefer something like https://github.com/SjonHortensius/php-src/commit/21b596086843f02ff0e4d937a17c80e758b27365
(untested wip) instead
It's untested; but shows the general idea. After a successful chroot (either from fpm or
userspace); the path is stored in a global which is prepended to the cache keys.
------------------------------------------------------------------------
[2016-11-10 14:25:01] php-dev at coydogsoftware dot net
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.
------------------------------------------------------------------------
[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 :)
------------------------------------------------------------------------
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