php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Sec Bug #66171 ps_files_open: Block symlinks properly, prevent opening other users' sessions
Submitted: 2013-11-25 20:43 UTC Modified: 2014-04-14 12:28 UTC
From: jann+php at thejh dot net Assigned:
Status: Closed Package: Session related
PHP Version: master-Git-2013-11-25 (Git) OS: Linux
Private report: No CVE-ID:
 [2013-11-25 20:43 UTC] jann+php at thejh dot net
Description:
------------
My php version: git commit a37ff1fa4bb149dc81fc812d03cdf7685c499403

This patch fixes two problems. First problem: php does an fstat() on a file descriptor to check whether the file it just opened was accessed through a symlink. That's never going to work because the fd points to the file, not to the symlink. My fix: Remove the check, use O_NOFOLLOW instead.

Second problem: When the session.save_path is a directory that everyone can write into (like on Debian), even if it's not possible to find the IDs of existing sessions, a local attacker can just create a new session file with malicious session data, chmod it to 666 and access any webapp hosted on the system with the session ID he chose. The webapp then opens the session file and treats it as if it had created it. My fix: fstat() the session, check the uid that created the file. If it's neither the result of getuid() nor uid 0, ignore the existing file.

(uid 0 because someone might be crazy enough to put session.save_path on a filesystem that doesn't support uids, which would probably make the uid default to 0)


Patches

Add a Patch

Pull Requests

Pull requests:

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2013-12-13 19:05 UTC] jann+php at thejh dot net
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.
 [2014-02-03 14:54 UTC] jann+php at thejh dot net
OK guys, you have 14 days to reply or this goes to full-disclosure and bugtraq.
 [2014-02-03 15:18 UTC] jann+php at thejh dot net
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.
 [2014-02-03 15:49 UTC] pajoye@php.net
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.
 [2014-02-03 18:04 UTC] pajoye@php.net
-Status: Open +Status: Feedback
 [2014-02-03 20:18 UTC] jann+php at thejh dot net
-Status: Feedback +Status: Open
 [2014-02-03 20:18 UTC] jann+php at thejh dot net
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);
 [2014-03-04 20:57 UTC] jann+php at thejh dot net
Mailed this to bugtraq.
 [2014-04-14 02:34 UTC] stas@php.net
Removing private flag since it's on http://seclists.org/bugtraq/2014/Mar/23
 [2014-04-14 03:36 UTC] stas@php.net
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.
 [2014-04-14 12:28 UTC] jann+php at thejh dot net
@stas Looks good to me.
 [2014-04-14 17:54 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=40a9316dff6e043b534844b2ab167318250be277
Log: Fix bug #66171: better handling of symlinks
 [2014-04-14 17:54 UTC] stas@php.net
-Status: Open +Status: Closed
 [2014-04-14 17:54 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=7f43aeb1675994ce0b00a1113fb69ea468f9f884
Log: Fix bug #66171: better handling of symlinks
 [2014-04-14 17:54 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=41569b10290e31affb7d8a4e5623b051790fab05
Log: Fix bug #66171: better handling of symlinks
 [2014-04-14 17:54 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=fb96a62b7d5697ea78be0eba20785ecc07bc9bce
Log: Fix bug #66171: better handling of symlinks
 [2014-04-15 12:04 UTC] ab@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=41569b10290e31affb7d8a4e5623b051790fab05
Log: Fix bug #66171: better handling of symlinks
 [2014-04-15 12:04 UTC] ab@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=7f43aeb1675994ce0b00a1113fb69ea468f9f884
Log: Fix bug #66171: better handling of symlinks
 [2014-04-15 12:04 UTC] ab@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=40a9316dff6e043b534844b2ab167318250be277
Log: Fix bug #66171: better handling of symlinks
 [2014-04-15 13:05 UTC] ab@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=fb96a62b7d5697ea78be0eba20785ecc07bc9bce
Log: Fix bug #66171: better handling of symlinks
 [2014-04-15 13:05 UTC] ab@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=41569b10290e31affb7d8a4e5623b051790fab05
Log: Fix bug #66171: better handling of symlinks
 [2014-04-15 13:05 UTC] ab@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=7f43aeb1675994ce0b00a1113fb69ea468f9f884
Log: Fix bug #66171: better handling of symlinks
 [2014-04-15 13:05 UTC] ab@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=40a9316dff6e043b534844b2ab167318250be277
Log: Fix bug #66171: better handling of symlinks
 [2014-05-01 14:59 UTC] tyrael@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=41569b10290e31affb7d8a4e5623b051790fab05
Log: Fix bug #66171: better handling of symlinks
 [2014-05-01 14:59 UTC] tyrael@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=7f43aeb1675994ce0b00a1113fb69ea468f9f884
Log: Fix bug #66171: better handling of symlinks
 [2014-05-01 14:59 UTC] tyrael@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=40a9316dff6e043b534844b2ab167318250be277
Log: Fix bug #66171: better handling of symlinks
 [2014-05-11 03:50 UTC] post at michael-neubert dot de
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
 [2014-08-23 14:16 UTC] info at ihead dot ru
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;
 [2014-10-07 23:15 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src-security.git;a=commit;h=40a9316dff6e043b534844b2ab167318250be277
Log: Fix bug #66171: better handling of symlinks
 [2014-10-07 23:26 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src-security.git;a=commit;h=40a9316dff6e043b534844b2ab167318250be277
Log: Fix bug #66171: better handling of symlinks
 [2015-02-17 18:37 UTC] niels at omines dot nl
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.
 [2016-07-20 11:40 UTC] davey@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=fb96a62b7d5697ea78be0eba20785ecc07bc9bce
Log: Fix bug #66171: better handling of symlinks
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Tue Aug 29 15:01:52 2017 UTC