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

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: The new attached patch should completely fix both problems. https://bugs.php.net/patch-display.php?bug_id=69090&patch=bug69090.diff&revision=latest It modifies values of hash function XOR-ing them with a value constructed from the root inode number. This value calculated once per request, using stat("/") at request startup, if opcache.validate_root is enabled (disabled by default). Previous Comments: ------------------------------------------------------------------------ [2016-11-15 15:11:07] dmitry@php.net 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 ------------------------------------------------------------------------ [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. ------------------------------------------------------------------------ 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