Hey, any comment on this? Are there any issues with this patch? Are you planning to ship it?

It's been over two weeks, you're pretty slow.

OK guys, you have 14 days to reply or this goes to full-disclosure and bugtraq.

I can't see my patch here – can you see it? I'd just resubmit it, but this bugtracker doesn't let me add patches because the bug is marked as private.

hi,

I missed that # on the list back then, not sure about the other. Sorry for the delay.

The patch is not attached to this bug, please use a .txt extension, it should work.

Clicking "Add a Patch" just gives me an error message "The bug #66171 is not available to public", so I'll just paste it in here directly:

diff --git a/ext/session/mod_files.c b/ext/session/mod_files.c
index 004d9d4..7a430ef 100644
--- a/ext/session/mod_files.c
+++ b/ext/session/mod_files.c
@@ -135,22 +135,22 @@ static void ps_files_open(ps_files *data, const char *key TSRMLS_DC)
 
                data->lastkey = estrdup(key);
 
+               /* O_NOFOLLOW to prevent us from following evil symlinks */
+#ifdef O_NOFOLLOW
+               data->fd = VCWD_OPEN_MODE(buf, O_CREAT | O_RDWR | O_BINARY | O_NOFOLLOW, data->filemode);
+#else
                data->fd = VCWD_OPEN_MODE(buf, O_CREAT | O_RDWR | O_BINARY, data->filemode);
+#endif
 
                if (data->fd != -1) {
 #ifndef PHP_WIN32
-                       /* check to make sure that the opened file is not a symlink, linking to data outside of allowable dirs */
-                       if (PG(open_basedir)) {
-                               struct stat sbuf;
-
-                               if (fstat(data->fd, &sbuf)) {
-                                       close(data->fd);
-                                       return;
-                               }
-                               if (S_ISLNK(sbuf.st_mode) && php_check_open_basedir(buf TSRMLS_CC)) {
-                                       close(data->fd);
-                                       return;
-                               }
+                       /* check that this session file was created by us or root – we
+                          don't want to end up accepting the sessions of another webapp */
+                       struct stat sbuf;
+                       if (fstat(data->fd, &sbuf) || (sbuf.st_uid != 0 && sbuf.st_uid != getuid())) {
+                               close(data->fd);
+                               data->fd = -1;
+                               return;
                        }
 #endif
                        flock(data->fd, LOCK_EX);

Mailed this to bugtraq.

Removing private flag since it's on http://seclists.org/bugtraq/2014/Mar/23

Jann, could you check out the patch at https://github.com/php/php-src/pull/643 ? I've modified it a bit to cover (to some extent) the case where O_NOFOLLOW is not available.

@stas Looks good to me.

Dear PHP Team,

this Patch will affect a lot of websites, that use a file/session file handling, where the webserver has access only through the group permissions of the file and is not the owner.

When I upgraded to PHP 5.4.28 some websites broke with:

"PHP Warning: Unknown: Failed to write session data (files). Please verify that the current setting of session.save_path is correct (/xxxxxxxxxxxx) in Unknown on line 0"

First this error message is wrong (please say the true reason: file owner !== access user).

On the other side the new behaviour is not wise at all. If a sysadmin wants, that a session file shall be readable/writable by the webserver through group permissions and not owner permissions PHP should not restrict that.

Only my few thoughts on that topic…

Best wishes
Michael

Please remove this check! It is bad idea! Many sites are running under www-user! It breaks them with "Warning : Unknown: Failed to write session data (files). Please verify that the current setting of session.save_path is correct"

+                       /* check that this session file was created by us or root в.. we
+                          don't want to end up accepting the sessions of another webapp */
+                       if (fstat(data->fd, &sbuf) || (sbuf.st_uid != 0 && sbuf.st_uid != getuid() && sbuf.st_uid != geteuid())) {
+                               close(data->fd);
+                               data->fd = -1;
+                               return;

This rather ridiculous excuse for a bugfix breaks tons of existing sites, with no visible hint on why, as it prohibits storing session data on NFS shares with user mapping. See for example http://stackoverflow.com/questions/23695548/php-cannot-read-sessions-from-nfs-share/28568551#28568551

We have just spent 4 hours with 3 senior developers trying to isolate why our sessions suddenly broke completely after upgrading from 5.5.10 to 5.6.5, until I narrowed it down to the fact that our entire development environment is run from central NFS storage... including the session storage.

While I certainly understand the *idea* behind the fix it's executed in the most lacklustre way possible. It should either be configurable, or only be fatal when on the local filesystem. And in any way, produce a warning somewhere about what's happening so us developers don't waste 12 hours locating the root cause.

Also, as this is a breaking change it should never have been released in a minor version. I would've found it much sooner if it was correctly listed as breaking in a major version.