Bug #2724: security: stat cache *very large* race condition if caching when follow_symlink disabled - Lighttpd - lighty labs

Bug #2724

security: stat cache very large race condition if caching when follow_symlink disabled

Added by gstrauss about 1 year ago. Updated 11 months ago.

Status:

Fixed

Priority:

Normal

Assignee:

Category:

core

Target version:

1.4.41

Start date:

2016-04-06

Due date:

% Done:

100%

Estimated time:

Missing in 1.5.x:

Description

security: stat cache very large race condition if caching when follow_symlink disabled

stat_cache.c documents that there is a race condition between stat()/lstat() and open(). This race condition becomes a very large race condition with the "simple" stat cache engine (enabled by default), which caches for one second.

While disabling caching when follow_symlink is disabled will be a performance hit, keeping caching enabled when follow_symlink is disabled is very insecure programming. Doing so creates a huge, easily exploitable race condition. The race condition is so large that it can be exploited with 100% success and without any failures showing up in the error logs.

The following patch disables using stat_cache cache hits when follow_symlink is disabled. One of the lines is for the "simple" stat_cache engine, and the other is for the FAM stat_cache.

diff --git a/src/stat_cache.c b/src/stat_cache.c
index fa9b7cb..7691902 100644
--- a/src/stat_cache.c
+++ b/src/stat_cache.c
@@ -422,7 +422,7 @@ handler_t stat_cache_get_entry(server *srv, connection *con, buffer *name, stat_

                if (buffer_is_equal(name, sce->name)) {
                        if (srv->srvconf.stat_cache_engine == STAT_CACHE_ENGINE_SIMPLE) {
-                               if (sce->stat_ts == srv->cur_ts) {
+                               if (sce->stat_ts == srv->cur_ts && con->conf.follow_symlink) {
                                        *ret_sce = sce;
                                        return HANDLER_GO_ON;
                                }
@@ -463,7 +463,7 @@ handler_t stat_cache_get_entry(server *srv, connection *con, buffer *name, stat_
                        /* check whether we got a collision */
                        if (buffer_is_equal(sc->dir_name, fam_dir->name)) {
                                /* test whether a found file cache entry is still ok */
-                               if ((NULL != sce) && (fam_dir->version == sce->dir_version)) {
+                               if ((NULL != sce) && (fam_dir->version == sce->dir_version) && con->conf.follow_symlink) {
                                        /* the stat()-cache entry is still ok */

                                        *ret_sce = sce;

Aside: leaving caching enabled when using FAM stat_cache engine and follow_symlink is disabled would not be as bad as doing so with "simple" stat_cache engine. The FAM stat_cache engine race condition is only as large as until the next set of FAM events are read and processed, whereas the "simple" stat_cache engine race condition is up to one second long. NOTE: as mentioned before, these stat_cache caching race conditions are both in addition to the race condition between stat()/lstat() and open(), which is not addressed by the above patch.

See also https://redmine.lighttpd.net/issues/2551 "mod_magnet lighty.stat is_symlink" where the symlink race condition is touched upon.

Associated revisions

Revision acd5e450 (diff)
Added by gstrauss 11 months ago

[security] disable stat_cache if !follow-symlink (fixes #2724)

disable stat_cache if server.follow-symlink = "disable"
if server.stat-cache-engine = "simple". Caching is still enabled
for server.stat-cache-engine = "fam" since the FAM notification is
almost immediate, however there is still a small race condition.

NOTE: server.follow-symlink = "disable" implementation still has
time-of-check versus time-of-use (ToC-ToU) race conditions and
its use is *not recommended* except to discourage symlinking.
It *does not* prevent symlinking by a determined attacker with
the ability to create files on the server.

server.stat-cache-engine = "disable" can also be used to discourage
symlinking, and also does not eliminate ToC-ToU race conditions.

While more modern systems might use openat() and other *at() routines
to eliminate the ToC-ToU race conditions, this is not currently
implemented in lighttpd. Besides, for systems needing such
protections against actors able to modify local files, it would be
better to set up multiple lighttpd servers running in separate user
contexts with filesystem permissions preventing access, rather than
giving a single lighttpd server running under a single lighttpd user
access to files across security boundaries, and trying to prevent
access by lighttpd user if a file is a symlink.

Note that there are performance implications to setting either of
server.follow-symlink = "disable"
server.stat-cache-engine = "disable"
since stat cache normally reduces filesystem overhead for
frequently-accessed files.

x-ref:
"security: stat cache *very large* race condition if caching when
follow_symlink disabled"
https://redmine.lighttpd.net/issues/2724

History

#1 Updated by gstrauss about 1 year ago

Status changed from New to Patch Pending

#2 Updated by gstrauss about 1 year ago

Category set to core

#3 Updated by stbuehler about 1 year ago

I'd say the FAM stat cache way should still work.

As an option to patching we could document it explicitly somehow that the cache should be disabled when disabling follow symlink.

#4 Updated by gstrauss about 1 year ago

Since there is already stat() then open() race condition which can be exacerbated with busy I/O, I am okay with leaving the FAM behavior as-is as long as the documentation is also updated. Using FAM extends the race condition to the processing of the current list of ready non-blocking events.

Since the "simple" stat_cache is enabled by default and has up to a 1-second (huge) race condition, I would prefer to disable stat_cache if "simple" and !con->conf.follow_symlink. Yes, the sample configs and documentation should also be updated to be more explicit about this behavior.

When the next release is being prepared, I'd like to commit the following, plus documentation updates that I'll prepare separately.

diff --git a/src/stat_cache.c b/src/stat_cache.c
index fa9b7cb..7691902 100644
--- a/src/stat_cache.c
+++ b/src/stat_cache.c
@@ -422,7 +422,7 @@ handler_t stat_cache_get_entry(server *srv, connection *con, buffer *name, stat_

                if (buffer_is_equal(name, sce->name)) {
                        if (srv->srvconf.stat_cache_engine == STAT_CACHE_ENGINE_SIMPLE) {
-                               if (sce->stat_ts == srv->cur_ts) {
+                               if (sce->stat_ts == srv->cur_ts && con->conf.follow_symlink) {
                                        *ret_sce = sce;
                                        return HANDLER_GO_ON;
                                }