From b05dc3573f4cd476482374b0ac0393153d344338 Mon Sep 17 00:00:00 2001 From: "Heiko Schlittermann (HS12-RIPE)" Date: Sun, 14 Mar 2021 12:16:57 +0100 Subject: [PATCH] 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) --- doc/doc-txt/ChangeLog | 3 ++ src/src/dbfn.c | 113 ++++++++++++++++++++++++------------------ test/stderr/0275 | 2 +- test/stderr/0278 | 2 +- test/stderr/0386 | 2 +- test/stderr/0388 | 2 +- test/stderr/0402 | 2 +- test/stderr/0403 | 2 +- test/stderr/0404 | 2 +- test/stderr/0408 | 2 +- test/stderr/0487 | 2 +- 11 files changed, 77 insertions(+), 57 deletions(-) diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog index 98c1b05e2..636fdf71e 100644 --- a/doc/doc-txt/ChangeLog +++ b/doc/doc-txt/ChangeLog @@ -207,6 +207,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 bbf20a1d5..dceb5f140 100644 --- a/src/src/dbfn.c +++ b/src/src/dbfn.c @@ -60,6 +60,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 * *************************************************/ @@ -91,7 +151,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[256], filename[256]; @@ -113,12 +172,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) { @@ -167,60 +227,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 *lastname = Ustrrchr(filename, '/') + 1; - int namelen = Ustrlen(name); - - *lastname = 0; - - if ((dd = exim_opendir(filename))) - 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(filename, &statbuf) >= 0 && statbuf.st_uid != exim_uid) - { - DEBUG(D_hints_lookup) - debug_printf_indent("ensuring %s is owned by exim\n", filename); - if (exim_chown(filename, exim_uid, exim_gid)) - DEBUG(D_hints_lookup) - debug_printf_indent("failed setting %s to owned by exim\n", filename); - } - } - - 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 diff --git a/test/stderr/0275 b/test/stderr/0275 index 35f1e4442..47c9c9e28 100644 --- a/test/stderr/0275 +++ b/test/stderr/0275 @@ -172,7 +172,7 @@ Delivery address list: EXIM_DBOPEN: file dir flags=O_RDONLY returned from EXIM_DBOPEN: (nil) ensuring TESTSUITE/spool/db/retry.lockfile is owned by exim - failed to open DB file TESTSUITE/spool/db/retry.lockfile: No such file or directory + failed to open DB file TESTSUITE/spool/db/retry: No such file or directory no retry data available >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Considering: userx@test.ex diff --git a/test/stderr/0278 b/test/stderr/0278 index 23b0ba9c6..c09920b94 100644 --- a/test/stderr/0278 +++ b/test/stderr/0278 @@ -131,7 +131,7 @@ Delivery address list: EXIM_DBOPEN: file dir flags=O_RDONLY returned from EXIM_DBOPEN: (nil) ensuring TESTSUITE/spool/db/retry.lockfile is owned by exim - failed to open DB file TESTSUITE/spool/db/retry.lockfile: No such file or directory + failed to open DB file TESTSUITE/spool/db/retry: No such file or directory no retry data available >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Considering: CALLER@test.ex diff --git a/test/stderr/0386 b/test/stderr/0386 index b136548b7..7bc56d9b7 100644 --- a/test/stderr/0386 +++ b/test/stderr/0386 @@ -273,7 +273,7 @@ Delivery address list: EXIM_DBOPEN: file dir flags=O_RDONLY returned from EXIM_DBOPEN: (nil) ensuring TESTSUITE/spool/db/retry.lockfile is owned by exim - failed to open DB file TESTSUITE/spool/db/retry.lockfile: No such file or directory + failed to open DB file TESTSUITE/spool/db/retry: No such file or directory no retry data available >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Considering: 2@b diff --git a/test/stderr/0388 b/test/stderr/0388 index 468eae32d..7819fe907 100644 --- a/test/stderr/0388 +++ b/test/stderr/0388 @@ -11,7 +11,7 @@ set_process_info: pppp delivering 10HmaX-0005vi-00 EXIM_DBOPEN: file dir flags=O_RDONLY returned from EXIM_DBOPEN: (nil) ensuring TESTSUITE/spool/db/retry.lockfile is owned by exim - failed to open DB file TESTSUITE/spool/db/retry.lockfile: No such file or directory + failed to open DB file TESTSUITE/spool/db/retry: No such file or directory no retry data available >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Considering: x@y diff --git a/test/stderr/0402 b/test/stderr/0402 index 1f6bf19c7..ec4b9ea72 100644 --- a/test/stderr/0402 +++ b/test/stderr/0402 @@ -214,7 +214,7 @@ Delivery address list: EXIM_DBOPEN: file dir flags=O_RDONLY returned from EXIM_DBOPEN: (nil) ensuring TESTSUITE/spool/db/retry.lockfile is owned by exim - failed to open DB file TESTSUITE/spool/db/retry.lockfile: No such file or directory + failed to open DB file TESTSUITE/spool/db/retry: No such file or directory no retry data available >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Considering: CALLER@test.ex diff --git a/test/stderr/0403 b/test/stderr/0403 index 0c22cf8f5..9d759694c 100644 --- a/test/stderr/0403 +++ b/test/stderr/0403 @@ -71,7 +71,7 @@ Delivery address list: EXIM_DBOPEN: file dir flags=O_RDONLY returned from EXIM_DBOPEN: (nil) ensuring TESTSUITE/spool/db/retry.lockfile is owned by exim - failed to open DB file TESTSUITE/spool/db/retry.lockfile: No such file or directory + failed to open DB file TESTSUITE/spool/db/retry: No such file or directory no retry data available >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Considering: userx@test.ex diff --git a/test/stderr/0404 b/test/stderr/0404 index b1a78c9fa..f0c1c355b 100644 --- a/test/stderr/0404 +++ b/test/stderr/0404 @@ -172,7 +172,7 @@ Delivery address list: EXIM_DBOPEN: file dir flags=O_RDONLY returned from EXIM_DBOPEN: (nil) ensuring TESTSUITE/spool/db/retry.lockfile is owned by exim - failed to open DB file TESTSUITE/spool/db/retry.lockfile: No such file or directory + failed to open DB file TESTSUITE/spool/db/retry: No such file or directory no retry data available >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Considering: userx@test.ex diff --git a/test/stderr/0408 b/test/stderr/0408 index 3fbd0f2a6..50b853183 100644 --- a/test/stderr/0408 +++ b/test/stderr/0408 @@ -71,7 +71,7 @@ Delivery address list: EXIM_DBOPEN: file dir flags=O_RDONLY returned from EXIM_DBOPEN: (nil) ensuring TESTSUITE/spool/db/retry.lockfile is owned by exim - failed to open DB file TESTSUITE/spool/db/retry.lockfile: No such file or directory + failed to open DB file TESTSUITE/spool/db/retry: No such file or directory no retry data available >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Considering: userx@test.ex diff --git a/test/stderr/0487 b/test/stderr/0487 index 83d243a54..579b1bd5b 100644 --- a/test/stderr/0487 +++ b/test/stderr/0487 @@ -99,7 +99,7 @@ Delivery address list: EXIM_DBOPEN: file dir flags=O_RDONLY returned from EXIM_DBOPEN: (nil) ensuring TESTSUITE/spool/db/retry.lockfile is owned by exim - failed to open DB file TESTSUITE/spool/db/retry.lockfile: No such file or directory + failed to open DB file TESTSUITE/spool/db/retry: No such file or directory no retry data available >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> Considering: userx@test.ex -- 2.30.2