Req #69090 [Ana]: add prefix/xor to cache keys/check permissions or separate caches

From: Date: Tue, 15 Nov 2016 11:36:25 +0000
Subject: Req #69090 [Ana]: add prefix/xor to cache keys/check permissions or separate caches
References: 1  Groups: php.bugs 
Request: Send a blank email to php-bugs+get-205377@lists.php.net to get a copy of this message
Edit report at https://bugs.php.net/bug.php?id=69090&edit=1

 ID:                 69090
 Updated 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:

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.


Previous Comments:
------------------------------------------------------------------------
[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 :)

------------------------------------------------------------------------
[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?

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


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


Thread (38 messages)

« previous php.bugs (#205377) next »