Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:96827
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain zend.com designates 104.47.33.108 as permitted sender)
To: "php-dev@coydogsoftware.net" <php-dev@coydogsoftware.net>
CC: Dmitry Stogov <dmitry@zend.com>, "rasmus@lerdorf.com"
	<rasmus@lerdorf.com>, "internals@lists.php.net" <internals@lists.php.net>,
	"Anatol Belski (ab@php.net)" <ab@php.net>, Zeev Suraski <zeev@zend.com>
Thread-Topic: [PATCH] opcache bug #69090, prepend user identifier to keys
Thread-Index: AQHSO+UMD4h3upkAiEqzeDgxtnu8Qg==
Date: Fri, 11 Nov 2016 07:31:03 +0000
Message-ID: <MWHPR02MB247797DCC52957382A6E120BBFBB0@MWHPR02MB2477.namprd02.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: zend.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: multipart/alternative;
	boundary="_000_MWHPR02MB247797DCC52957382A6E120BBFBB0MWHPR02MB2477namp_"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 11 Nov 2016 07:31:03.5674
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 32210298-c08b-4829-8097-6b12c025a892
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR02MB2478
Subject: Re: [PATCH] opcache bug #69090, prepend user identifier to keys
From: dmitry@zend.com (Dmitry Stogov)

--_000_MWHPR02MB247797DCC52957382A6E120BBFBB0MWHPR02MB2477namp_
Content-Type: text/plain; charset="koi8-r"
Content-Transfer-Encoding: quoted-printable

Hi,

On Nov 10, 2016 5:10 PM, php-dev@coydogsoftware.net wrote:
>
> Hello,
>
> Thank you for the response. Replies inline:
>
> On Thu, Nov 10, 2016 at 08:51:58AM +0000, Dmitry Stogov wrote:
> >
> > 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).

I didn't see the problem in real life, but it's clear, that serving of few =
chroot environments using the same cache may lead to duplicate keys. FPM wi=
th separate pools shouldn't be affected.

>
> 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)?

Serving different users by the same process opens a lot of ways to stole da=
ta (sessions, persistent caches, persistent conections, dirty memory, etc).
They should be served by different FPM pools or separate Apache processes.

>
> > 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.

Yeah, inode is not an exelent solution as well.

> I still strongly believe a user identifier is needed in addition to
> dev+inode due to the permissions bypass issue.

Opcache just wasn't designed to solve permission problems in shared hosting=
 evvironment.
"user id prefix", looks for me like a quick hack, that doesn't solve the wh=
ole problem anyway.

>
> > 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.

Do you talk about executing "unreadable" PHP scripts of different users?
I think, the proper way to fix this, whould executing access() check on eac=
h cached script access (this might be enabled/disabled through php.ini)

>
> > 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.

RINIT

>
> > 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.

Let me check this.

>. 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?

Let methink a bit more.

Thanks. Dmitry.

> --
> - php-dev@coydogsoftware.net

--_000_MWHPR02MB247797DCC52957382A6E120BBFBB0MWHPR02MB2477namp_--