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);
