101560 - processmail doesn't work with the P4DTI

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect, P2)

Description

[Gareth Rees]

BACKGROUND

The Perforce Defect Tracking Integration <http://www.ravenbrook.com/project/
p4dti/> integrates Bugzilla with the Perforce software configuration management 
system.  It replicates bugs between Bugzilla and Perforce so that users of 
Perforce can see and edit their bugs, and relate bugs to source changes.

When someone changes a bug in Perforce, the P4DTI updates the bug in Bugzilla.  
It then needs to send e-mail notifications, which it does by running the Bugzilla 
processmail script.

DEFECT DESCRIPTION

When the P4DTI runs the processmail script from Bugzilla 2.14, no e-mail is sent.  
Instead, the following error message appears in the standard error:

Insecure $ENV{BASH_ENV} while running with -T switch at ./processmail line 745 \
(#1)

    (F) You can't use system(), exec(), or a piped open in a setuid or
    setgid script if any of $ENV{PATH}, $ENV{IFS}, $ENV{CDPATH},
    $ENV{ENV} or $ENV{BASH_ENV} are derived from data supplied (or
    potentially supplied) by the user.  The script must set the path to a
    known value, using trustworthy data.  See perlsec.

REPRODUCING THIS BUG

You probably don't want to install the P4DTI in order to confirm this bug.  
Instead, run Bugzilla in an environment where some of the dangerous environment 
variable have values.  You'll see that processmail doesn't send e-mail.

DIAGNOSIS

The processmail script is invoking the shell in order to open a pipe to sendmail.  
It does so with taint mode turned on (-T option to perl) but without clearing the 
dangerous environment variables.

SOLUTION

Clear the dangerous environment variables, as recommended in the perlsec manpage, 
by running

      delete @ENV{qw(IFS CDPATH ENV BASH_ENV)};

before invoking the shell.

I added this line to my processmail script and the P4DTI was then able to send e-
mail notifications.

OTHER NOTES

Why doesn't this bug show up in normal operation?  Perhaps because Bugzilla 
always runs in an environment where the dangerous environment variables aren't 
set (e.g., in Apache under user "nobody").

This is related to bug 59349.

Comment 1

[Jacob Steenhagen]

Yes, in bug 53949 delete $ENV{path}; was added to globals.pl, but the other
environment variables were not.  These variables apparently do not exist when
apache runs the processmail script (as you pointed out) so they don't cause
bailing under "normal" circumstances.  I see no problem with adding these extra
deletions where PATH is currently deleted.

Comment 2

[Bradley Baetz (:bbaetz)]

I fixed this as part of bug 97469 - I got sick of having to unset BASH_ENV to
test processmail from the cmd line

Comment 3

[Bradley Baetz (:bbaetz)]

Checked in.

Comment 4

[Bradley Baetz (:bbaetz)]

oops - reopening. ddk pointed out that you wanted other env vars removed, too

Comment 5

[Gareth Rees]

Yes, BASH_ENV was the one that I observed, but it seems wise to clear them all to prevent problems in future.

Comment 6

[Bradley Baetz (:bbaetz)]

Attachment: patch

Comment 7

[Matthew Tuck [:CodeMachine]]

Questions:

How did you make sure this list was complete?
Why don't you remove PATH on the same line as everything else?

Comment 8

[Bradley Baetz (:bbaetz)]

> How did you make sure this list was complete?

See the error message.

> Why don't you remove PATH on the same line as everything else?

No particular reason, except that PATH can be modified, while the others are
just unwanted.

Comment 9

[Gervase Markham [:gerv]]

Why use two lines where one will do? Add PATH to the list. :-)

By the way, [e-mail] - that's the best bug report I've seen in ages :-)

Gerv

Comment 10

[Bradley Baetz (:bbaetz)]

Attachment: new patch

Comment 11

[Gervase Markham [:gerv]]

Comment on attachment 57445 [details] [diff] [review]
new patch

r=gerv.

Gerv

Comment 12

[Bradley Baetz (:bbaetz)]

Checked in

Comment 13

[Bradley Baetz (:bbaetz)]

Attachment: bah

perl 5.005 appears to have a bug, and warns on this:

Use of implicit split to @_ is deprecated 
(D deprecated) It makes a lot of work for the compiler when you clobber a
subroutine's argument list, so it's better if you assign the results of a
split() explicitly to an array (or list).

Bad internal code? Anyway, this fixes it.