[PATCH] opcache bug #69090, prepend user identifier to keys

Hello Dmitry,

A quick question after considering your comments and doing a bit more
homework on extension lifetime:

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.

Looks like OPCache currently does not implement a request_startup_func for
per-request initialization. I'm assuming this was avoided for
performance reasons and due to no perceived need until now for
per-request initialization. This seems like it would be the proper point
in extension lifetime to call geteuid() and save results in the
zend_accel_globals struct to use during key construction. Correct? Any
objection to my implementing this?

I'm also thinking the stat() for device+inode should be done in opcache
code at file compilation time; correct?

Would this be a step in the right direction? Apologies if these are
basic questions; I'm new to PHP extensions but strongly motivated to fix
OPCache keys quickly. I may have time for a second pass at this over the
weekend.

-php-dev at coydogsoftware dot net

Hi,

-----Original Message-----
From: php-dev@coydogsoftware.net [mailto:php-dev@coydogsoftware.net]
Sent: Thursday, November 10, 2016 3:10 PM
To: Dmitry Stogov dmitry@zend.com
Cc: internals@lists.php.net; rasmus@lerdorf.com; Zeev Suraski
zeev@zend.com; Anatol Belski (ab@php.net) ab@php.net
Subject: Re: [PATCH] opcache bug #69090, prepend user identifier to keys

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).

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

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
the stat() 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.

I still strongly believe a user identifier is needed in addition to
dev+inode due to the permissions bypass issue.

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.

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.

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

While inodes would indeed separate the cached physical entries, the data
would be saved into same shared memory segment, currently at least with MMAP
and SHM. I saw some hostings there, offering the file cache only part,
probably for this reason. So chroot or suphp are unlikely enforce the real
jail for Opcache.

Talking about the Windows side, I can link to the ticket I've investigated
some time ago https://bugs.php.net/bug.php?id=72645#1469569537 . The model
there is in many points different. Under IIS, the current implementation
allows separating caches using impersonation, different app pools, ACLs and
user accounts. This approach is certainly complicated, but gives also more
configuration flexibility. For Apache, running as a service, the config
possibilities would be limited, but still it should be possible to have a
separation per site/user for FCGI.

I was thinking about combiding these approaches. Maybe it might make sense
to use separate memory per site/user/etc. This could be achieved by
extending the SHM model to respect a configurable key. The key could be
generated by ftok() automatically or preconfigured by a shared hoster, or
both, whatsoever.

Pro for separating SHM

• SHM key can be made unique per INI, even if done automatically with ftok -
  it's a onetime effort
• no clashes can happen per se
• works for any SAPI

A downside I could think about - higher memory usage caused to hosting. But
that would happen, when inodes are used, too. For multisite approaches, like
it is possible fe with Drupal and others, another smarter configuration
would be needed anyway, to exclude the config files, etc. But having a
separation of memory pools looks to me like a clean and much simpler way to
exclude any thinkable implications to cache mistakes.

Regards

Anatol