Bug#809318: bts: overrides user-specified value of sendmail

[Daniel Shahaf]

Osamu Aoki wrote on Tue, Mar 08, 2016 at 23:08:58 +0900:
> I generally do not like to allow "multi words shell commandline" accepted
> via command line argument into perl code for security concern.  I do
> understand the point that "bts" may be safe for such concern.  But why
> add such capability when -n option exists.
> 
> This is at best a "feature request wishlist bug".

Osamu-san,

There are two different issues being discussed in this thread.  One of
them is a wishlist item and one of them is not.

The root problem is that I ran «bts --sendmail=foo» and
/usr/sbin/sendmail was executed.  This can happen not only when the
argument contains spaces (as in my original example) but also when the
argument is an absolute path to an executable or a script that
accidentally lacks the +x permission: for example, «chmod -x
~/bin/mysendmail && bts --sendmail="$HOME/bin/mysendmail"» would
reproduce the bug.

As I said earlier, using the wrong sendmail binary can lead to an email
being sent using incorrect settings (e.g., using the wrong relayhost or
from address), which IMHO merits a greater-than-wishlist severity.

A separate issue is how the argument to the --sendmail option should be
handled.  There are several sensible options.¹  Currently, the code
validates the argument, splits it on whitespace, and passes it to
execve().  As you say, asking to add a mode whereby the argument would
be passed to system() instead would be a wishlist item.  However, I did
not request that; I simply asked to change bts(1)'s behaviour when the
validation fails.  I suggested that either the validation should not be
done in the first place (just call execve() and let it error out), or
a failed validation should result in a die() rather than in proceeding
with a different sendmail binary than specified by the user.

Perhaps the "consider interpreting the argument to the --sendmail option
using Text::ParseWords(3perl) or system(3)" discussion should be spun off
into a separate bug report?  After all, it is mostly orthogonal to the
"Change the behaviour upon validation failure" issue.

Cheers,

Daniel

¹ Such as: pass it verbatim to system(); pass it execve() as the first
argument; pass it to execve() as multiple arguments.