[rt-devel] Better handling of sendmail (Re: Bugfix for security patch on mod_perl)

[Alex Vandiver]

On Wed, 2012-05-30 at 05:46 +1000, paul.szabo at sydney.edu.au wrote:
> I feel that the invocation of sendmail in RT/Interface/Email.pm is
> "wrong":
>  - uses IPC::Open2 instead of plain open($mail,"| $path @args") though
>    it never attempts to read STDOUT, loses STDERR without Open3

Using either the two-argument form of open, or ``, makes the call
vulnerable to shell injection in @args -- which is _precisely_ the
vulnerability that this change is meant to protect against.
Additionally, STDOUT is intentionally not read from, for parity with the
>/dev/null in the previous version of the code.  Finally, STDERR is
connected to the parent process's STDERR, which is not lost, but rather
goes directly into the Apache error log in all suggested deployment
options.

In a future version, we intend to move to using the more robust
IPC::Run3, when we will likely start explicitly logging STDOUT and
STDERR errors using RT's logging infrastructure; this was not a valid
option as part of the security patches, as it would have added an
additional dependency in the 3.8 series.

>  - uses pipe which can have consequences for invoker, including losing
>    the exit status or output of subprocess

Using IPC::Open2, the child exit status is available in $?, precisely
the same as when using ``.  I am not aware of any failure modes
involving loss of output -- not that sendmail, exim, postfix, or qmail
has ever been reported to produce any to STDOUT, or that any version of
the code has required examining it.

> I propose a more standard, simpler, handling of the sendmail process,
> in a way which captures the exit status and all the STDERR or STDOUT
> output.

Using this patch makes you vulnerable to CVE-2011-4458, remote execution
of code.  Please read the history of the codepath in question, available
in git, for a complete exploration of why the techniques you are
proposing are either incorrect, unnecessary, or have security
implications.
 - Alex