Gitweb:
https://git.exim.org/exim.git/commitdiff/186e99bafcf8dbc53f9a25ea26998cab9b091a62
Commit: 186e99bafcf8dbc53f9a25ea26998cab9b091a62
Parent: 6552729ba7975985cbcb938cf4ecf7b54e395763
Author: Heiko Schlittermann (HS12-RIPE) <hs@???>
AuthorDate: Sun Mar 14 12:16:57 2021 +0100
Committer: Heiko Schlittermann (HS12-RIPE) <hs@???>
CommitDate: Thu May 27 21:30:41 2021 +0200
CVE-2020-28008: Assorted attacks in Exim's spool directory
We patch dbfn_open() by introducing two functions priv_drop_temp() and
priv_restore() (inspired by OpenSSH's functions temporarily_use_uid()
and restore_uid()), which temporarily drop and restore root privileges
thanks to seteuid(). This goes against Exim's developers' wishes ("Exim
(the project) doesn't trust seteuid to work reliably") but, to the best
of our knowledge, seteuid() works everywhere and is the only way to
securely fix dbfn_open().
(cherry picked from commit 18da59151dbafa89be61c63580bdb295db36e374)
(cherry picked from commit b05dc3573f4cd476482374b0ac0393153d344338)
---
doc/doc-txt/ChangeLog | 3 ++
src/src/dbfn.c | 116 ++++++++++++++++++++++++++++----------------------
2 files changed, 68 insertions(+), 51 deletions(-)
diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog
index 313dcbf..4debef8 100644
--- a/doc/doc-txt/ChangeLog
+++ b/doc/doc-txt/ChangeLog
@@ -296,6 +296,9 @@ PP/11 Fix security issue in BDAT state confusion.
HS/03 Die on "/../" in msglog file names
+QS/01 Creation of (database) files in $spool_dir: only uid=0 or the euid of
+ the Exim runtime user are allowed to create files.
+
Exim version 4.94
-----------------
diff --git a/src/src/dbfn.c b/src/src/dbfn.c
index 0f56ad5..b66d460 100644
--- a/src/src/dbfn.c
+++ b/src/src/dbfn.c
@@ -65,6 +65,66 @@ log_write(0, LOG_MAIN, "Berkeley DB error: %s", msg);
+static enum {
+ PRIV_DROPPING, PRIV_DROPPED,
+ PRIV_RESTORING, PRIV_RESTORED
+} priv_state = PRIV_RESTORED;
+
+static uid_t priv_euid;
+static gid_t priv_egid;
+static gid_t priv_groups[EXIM_GROUPLIST_SIZE + 1];
+static int priv_ngroups;
+
+/* Inspired by OpenSSH's temporarily_use_uid(). Thanks! */
+
+static void
+priv_drop_temp(const uid_t temp_uid, const gid_t temp_gid)
+{
+if (priv_state != PRIV_RESTORED) _exit(EXIT_FAILURE);
+priv_state = PRIV_DROPPING;
+
+priv_euid = geteuid();
+if (priv_euid == root_uid)
+ {
+ priv_egid = getegid();
+ priv_ngroups = getgroups(nelem(priv_groups), priv_groups);
+ if (priv_ngroups < 0) _exit(EXIT_FAILURE);
+
+ if (priv_ngroups > 0 && setgroups(1, &temp_gid) != 0) _exit(EXIT_FAILURE);
+ if (setegid(temp_gid) != 0) _exit(EXIT_FAILURE);
+ if (seteuid(temp_uid) != 0) _exit(EXIT_FAILURE);
+
+ if (geteuid() != temp_uid) _exit(EXIT_FAILURE);
+ if (getegid() != temp_gid) _exit(EXIT_FAILURE);
+ }
+
+priv_state = PRIV_DROPPED;
+}
+
+/* Inspired by OpenSSH's restore_uid(). Thanks! */
+
+static void
+priv_restore(void)
+{
+if (priv_state != PRIV_DROPPED) _exit(EXIT_FAILURE);
+priv_state = PRIV_RESTORING;
+
+if (priv_euid == root_uid)
+ {
+ if (seteuid(priv_euid) != 0) _exit(EXIT_FAILURE);
+ if (setegid(priv_egid) != 0) _exit(EXIT_FAILURE);
+ if (priv_ngroups > 0 && setgroups(priv_ngroups, priv_groups) != 0) _exit(EXIT_FAILURE);
+
+ if (geteuid() != priv_euid) _exit(EXIT_FAILURE);
+ if (getegid() != priv_egid) _exit(EXIT_FAILURE);
+ }
+
+priv_state = PRIV_RESTORED;
+}
+
+
+
+
/*************************************************
* Open and lock a database file *
*************************************************/
@@ -96,7 +156,6 @@ dbfn_open(uschar *name, int flags, open_db *dbblock, BOOL lof, BOOL panic)
{
int rc, save_errno;
BOOL read_only = flags == O_RDONLY;
-BOOL created = FALSE;
flock_t lock_data;
uschar dirname[PATHLEN], filename[PATHLEN];
@@ -118,12 +177,13 @@ exists, there is no error. */
snprintf(CS dirname, sizeof(dirname), "%s/db", spool_directory);
snprintf(CS filename, sizeof(filename), "%s/%s.lockfile", dirname, name);
+priv_drop_temp(exim_uid, exim_gid);
if ((dbblock->lockfd = Uopen(filename, O_RDWR, EXIMDB_LOCKFILE_MODE)) < 0)
{
- created = TRUE;
(void)directory_make(spool_directory, US"db", EXIMDB_DIRECTORY_MODE, panic);
dbblock->lockfd = Uopen(filename, O_RDWR|O_CREAT, EXIMDB_LOCKFILE_MODE);
}
+priv_restore();
if (dbblock->lockfd < 0)
{
@@ -172,63 +232,17 @@ it easy to pin this down, there are now debug statements on either side of the
open call. */
snprintf(CS filename, sizeof(filename), "%s/%s", dirname, name);
-EXIM_DBOPEN(filename, dirname, flags, EXIMDB_MODE, &(dbblock->dbptr));
+priv_drop_temp(exim_uid, exim_gid);
+EXIM_DBOPEN(filename, dirname, flags, EXIMDB_MODE, &(dbblock->dbptr));
if (!dbblock->dbptr && errno == ENOENT && flags == O_RDWR)
{
DEBUG(D_hints_lookup)
debug_printf_indent("%s appears not to exist: trying to create\n", filename);
- created = TRUE;
EXIM_DBOPEN(filename, dirname, flags|O_CREAT, EXIMDB_MODE, &(dbblock->dbptr));
}
-
save_errno = errno;
-
-/* If we are running as root and this is the first access to the database, its
-files will be owned by root. We want them to be owned by exim. We detect this
-situation by noting above when we had to create the lock file or the database
-itself. Because the different dbm libraries use different extensions for their
-files, I don't know of any easier way of arranging this than scanning the
-directory for files with the appropriate base name. At least this deals with
-the lock file at the same time. Also, the directory will typically have only
-half a dozen files, so the scan will be quick.
-
-This code is placed here, before the test for successful opening, because there
-was a case when a file was created, but the DBM library still returned NULL
-because of some problem. It also sorts out the lock file if that was created
-but creation of the database file failed. */
-
-if (created && geteuid() == root_uid)
- {
- DIR * dd;
- uschar path[PATHLEN];
- uschar *lastname;
- int namelen = Ustrlen(name);
-
- Ustrcpy(path, filename);
- lastname = Ustrrchr(path, '/') + 1;
- *lastname = 0;
-
- if ((dd = exim_opendir(path)))
- for (struct dirent *ent; ent = readdir(dd); )
- if (Ustrncmp(ent->d_name, name, namelen) == 0)
- {
- struct stat statbuf;
- /* Filenames from readdir() are trusted,
- so use a taint-nonchecking copy */
- strcpy(CS lastname, CCS ent->d_name);
- if (Ustat(path, &statbuf) >= 0 && statbuf.st_uid != exim_uid)
- {
- DEBUG(D_hints_lookup)
- debug_printf_indent("ensuring %s is owned by exim\n", path);
- if (exim_chown(path, exim_uid, exim_gid))
- DEBUG(D_hints_lookup)
- debug_printf_indent("failed setting %s to owned by exim\n", path);
- }
- }
-
- closedir(dd);
- }
+priv_restore();
/* If the open has failed, return NULL, leaving errno set. If lof is TRUE,
log the event - also for debugging - but debug only if the file just doesn't
