On 2016-09-11 at 22:41 +0100, Jeremy Harris wrote:
> There's a minor complication in that the -J file is opened in two
> places (as it happens, in a single routine: deliver_messsage()).
Why is the journal ever being opened as root, instead of as the Exim
run-time user? That seems like a flaw, and a root-cause to be
addressed.
> - 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?)
IMO:
* Use `O_NOFOLLOW` where available
* Stop trying to open the journal as root
Platforms which don't support `O_NOFOLLOW` have bigger issues. It's a
good guard to use to protect, where available.
But if the journal, in the Exim run-time spool area, is only ever
_opened_ as the Exim user, then there is no vulnerability. The root
cause is that root is doing something in an area where it's unsafe for
root to be acting in such a manner, and if that's true, then there's a
process flow bug in Exim.
> 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?
> 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"))
> + )
% view $(fgrep -rl Ufopen .)
Some of those are for user-supplied files (`bounce_message_file`,
`${readfile...}`) but a few others are not.
Look also at parse.c:parse_forward_list() where there's an
EXIM_HAVE_OPENAT ifdef block for logic to defend against symlinks.
And `O_NOFOLLOW` is already being used in there.
The above is a decent mitigation for this incident, but perhaps the
OPENAT logic needs to be encapsulate into a `safe_fopen()` wrapper and
used in more places?
-Phil
