Project

General

Profile

Bug #2724

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

Added by gstrauss over 4 years ago. Updated almost 4 years ago.

Status:
Fixed
Priority:
Normal
Category:
core
Target version:
ASK QUESTIONS IN Forums:

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.

#1

Updated by gstrauss over 4 years ago

  • Status changed from New to Patch Pending
#2

Updated by gstrauss over 4 years ago

  • Category set to core
#3

Updated by stbuehler about 4 years 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 4 years 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;
                                }

#5

Updated by gstrauss about 4 years ago

  • Target version changed from 1.4.x to 1.4.40
#6

Updated by gstrauss about 4 years ago

  • Target version changed from 1.4.40 to 1.4.41
#7

Updated by gstrauss almost 4 years ago

  • Status changed from Patch Pending to Fixed
  • % Done changed from 0 to 100
#8

Updated by gstrauss almost 4 years ago

  • Private changed from Yes to No

Also available in: Atom