Re: [exim-dev] Exim4 spool directory symlink local root esca…

Pàgina inicial
Delete this message
Reply to this message
Autor: Jeremy Harris
Data:  
A: exim-dev
Assumpte: Re: [exim-dev] Exim4 spool directory symlink local root escalation - does this apply to 4.87?
On 11/09/16 18:58, Heiko Schlittermann wrote:
> Hi,
>
> Andreas Metzler <eximusers@???> (So 11 Sep 2016 16:32:11 CEST):
>> Hello,
>>
>> was there a thread or a bug report about
>> http://www.halfdog.net/Security/2016/DebianEximSpoolLocalRoot/ ?
>>
>
> I'm working on that. Until now I couldn't reproduce it. Maybe a timing
> issue in my environment. As long as I can't reproduce it, I do not
> change anything in Exim.


I've not tried to repro it, but it feels feasible. If there are
non-Exim programs running as the exim UID.

The trouble is that Exim is very flexible and provides for calling
out to other programs, code not under our control - so the above is
quite possible and the attack surface is unbounded.

As I understand it, the fake journal (symlink) could be created any
time; the inconvenience of needing to match a message_id that will
eventually be used is minor (and, in the face of deferred messages,
even that is simply bypassed). So the window for attack is wide.


There's a minor complication in that the -J file is opened in two
places (as it happens, in a single routine: deliver_messsage()).

First any journal left around after a previous crash is opened
and read; second, one is created (or opened if already existing)
for (re-)writing by this delivery attempt.


- I suggest these two should not both be done; if the first
succeeds then hold it open.

- The second, still needed if the first did not find one, can
then newly use an O_EXCL flag so that it fails if the path
exists - closing the short hole between first & second
operations.

- Finally, the first open should be done with an O_NOFOLLOW
flag (at least, on platforms supporting that. Linux does;
apparently FreeBSD also). This closes the main hole.
(Where O_NOFOLLOW is not we could shorten the hole to a large
extent by a stat-check of the file right next to the open -
but it seems hard to totally close that way. Perhaps a stat
and an fstat both after open, comparing the ino? Is it
worth coding that?)


I've coded that (attached) but due to the sensitive nature,
both re. security and data-integrity I'm reluctant to commit
it without review by other developers.

Comments?
--
Jeremy
diff --git a/src/src/deliver.c b/src/src/deliver.c
index 48ef2ab..fe2fc6d 100644
--- a/src/src/deliver.c
+++ b/src/src/deliver.c
@@ -5513,26 +5513,15 @@ Otherwise it might be needed again. */
uschar * fname = spool_fname(US"input", message_subdir, id, US"-J");
FILE * jread;

-  if ((jread = Ufopen(fname, "rb")))
+  if (  (journal_fd = Uopen(fname, O_RDWR|O_APPEND
+#ifdef O_NOFOLLOW
+                    |O_NOFOLLOW
+#endif
+    , SPOOL_MODE)) >= 0
+     && lseek(journal_fd, 0, SEEK_SET) == 0
+     && (jread = fdopen(journal_fd, "rb"))
+     )
     {
-    struct stat st;
-
-    if (fstat(fileno(jread), &st))
-      {
-      log_write(0, LOG_MAIN|LOG_PANIC, "attempt to stat journal file failed: %s", strerror(errno));
-      return continue_closedown();   /* yields DELIVER_NOT_ATTEMPTED */
-      }
-    if (!S_ISREG(st.st_mode))
-      {
-      log_write(0, LOG_MAIN|LOG_PANIC, "journal file %s is not a regular file; possible symlink attack", fname);
-      return continue_closedown();   /* yields DELIVER_NOT_ATTEMPTED */
-      }
-    if (!(st.st_mode & S_IWUSR))
-      {
-      log_write(0, LOG_MAIN|LOG_PANIC, "journal file %s is not writable", fname);
-      return continue_closedown();   /* yields DELIVER_NOT_ATTEMPTED */
-      }
-
     while (Ufgets(big_buffer, big_buffer_size, jread))
       {
       int n = Ustrlen(big_buffer);