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). 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? -- - php-...@coydogsoftware.net -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
