154008 - Basic script maintenance: contrib email scripts (bug_email.pl & bugzilla_email_append.pl)

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Erik Anderson]

While trying to get the two email-based bug management systems (bug_email.pl 
and bugzilla_email_append.pl) up and working, I ran across a number of issues 
that I believe should be resolved within Bugzilla.

While I realize that these scripts are not officially supported, and that their 
basic structure and functioning are being questioned, the fact that they are 
present as part of Bugzilla means that there should be basic mantinance done to 
them from time to time.  In this spirit, I propose the following patches. 

This is _not_ a duplicate of bug 44393

Comment 1

[Erik Anderson]

Attachment: [Issue 1] inappropriate system() call

Both of these scripts should be running in taint mode.	This system call here
is a fairly major security hole that allows someone to create a fake message
and gain access to the machine.  I don't know whether the third parameter
should be in single-quotes as it is now.

Comment 2

[Erik Anderson]

Attachment: [Issue 2] Bugzilla assumes the current directory is the main directory

Having the current directory be $BUGZILLA_HOME/contrib for these two scripts
causes Bugzilla to attempt to create a new contrib/data directory, with new
versions of the parameters and other required files.

Comment 3

[Erik Anderson]

Attachment: [Issue 3] Attempt to remove uninitialized variable warnings

This patch is a fix-em-as-they-crash patch to resolve a number of places where
perl complains about undefined variables being used in the code.  The patch
could probably be cleaned up a bit by someone more knowledgable in perl.

Comment 4

[Erik Anderson]

Attachment: [Issue 4] Bugzilla_email_append stop processing after error

This patch simply forces the script to exit after an error occurs, something
that it does not appear to do on its own.  The error return code is for my
qmail system and signals an email-bouncing error; something more
sendmail-centric would probably be more appropriate here.

This patch is based on experience, not study.  If the script is meant to run to
the end with errors, then this patch may not be necessary.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

It's my understanding that code in contrib/ doesn't need review.  Of course,
without review it's good to get permission before modifying someone else's code,
but this script looks pretty abandoned, so that's probably not necessary in this
case.  cc:ing Dave for confirmaton.

Comment 6

[Erik Anderson]

Attachment: [Issue 1] updated system() call

Update to the third parameter: it is just the email address now.  Otherwise,
processmail will send out a second confirmation email.

Comment 7

[Erik Anderson]

If anyone's managed to get bug_email.pl working they've got a potentially huge 
exploit available simply by successfully submitting a bug report with a shell 
command in their full name (not verified).  I accidentally found the bad system
() call only after I setgid'ed the script as the web server (activating 
taint).  I'd like to propose that at least some of these patches get sent up 
to .16, if not too difficult, for security reasons if nothing else.

Comment 8

[Bradley Baetz (:bbaetz)]

Shouldn't it be '../processmail' - its in a parent directory.

Comment 9

[Erik Anderson]

Bugzilla assumes that the current directory is the main directory, not 
contrib.  If [Issue 2] is not applied, then there needs to be a 'chdir ".."' on 
the line above the system() line. (and perhaps a 'chdir "contrib"' afterwards)

Comment 10

[Dave Miller [:justdave]]

The security fixes should go in now, no questions.

I certainly won't turn away the others but getting bug_email officially
supported was one of our goals for 2.18 IIRC.

Comment 11

[Bradley Baetz (:bbaetz)]

-> 2.16 so that we don't lose the security patch (even though we don't support
contrib/*, its not a good idea to ship with a known hole...)

I've never used bug_email - someone who has should ideally look these over first.

Comment 12

[Dave Miller [:justdave]]

Comment on attachment 88995 [details] [diff] [review]
[Issue 2] Bugzilla assumes the current directory is the main directory

2xr= justdave

I believe it left the current directory in contrib before on purpose, but I
know we've changed the way params are dealt with in a way that'll break this if
it's left like that.  This patch adaquately deals with both situations I think.

Will review the others later if no one beats me to it, need to go to work :-)

Comment 13

[Dave Miller [:justdave]]

Comment on attachment 89104 [details] [diff] [review]
[Issue 1] updated system() call

2xr= justdave

Comment 14

[Dave Miller [:justdave]]

Comment on attachment 88997 [details] [diff] [review]
[Issue 4] Bugzilla_email_append stop processing after error

r= justdave

Comment 15

[Dave Miller [:justdave]]

Comment on attachment 88996 [details] [diff] [review]
[Issue 3] Attempt to remove uninitialized variable warnings

>--- bugzilla_email_append.pl.orig	Mon Jun 24 12:53:28 2002
>+++ bugzilla_email_append.pl	Mon Jun 24 15:28:29 2002

>@@ -1146,7 +1148,7 @@
> my @used_fields;
> 
> foreach my $f (@AllowedLabels) {
>-    if ((exists $Control{$f}) && ($Control{$f} !~ /^\s*$/ )) {
>+    if (defined($Control{$f}) && (exists $Control{$f}) && ($Control{$f} !~ /^\s*$/ )) {

Not sure this has the desired effect here...

"exists $Control{$f}" isn't going to cause a warning unless either $Control or
$f are undefined.  So $Control and $f are what we need to be checking for
defined-ness here, not $Control{$f}.  You have to see if $Control{$f} exists or
not first before you can tell if its value is defined anyway.

Comment 16

[Bradley Baetz (:bbaetz)]

Attachment: 2.14.3/2.16 system() patch

OK, I finally managed to get this working. bug_email sucks. A lot.

It attempts to do fuzzy matching on my email address for some reason, and so
decides that bbaetz@localhost is, in fact bbaetz+foo@localhost (Yes, I have
valid bz accounts for both)

Not to mention that it decides that:

bbaetz@localhost' ; /bin/mail bbaetz < '/etc/passwd

Is a valid address matching bbaetz+foo@localhost, too. At least if gives an SQL
error if I try to add an @ sign into the mail command.

The check to see if I was a valid user kept failing even with a valid address,
though, so I had to bypass that. Note that the input to that function was
'bbaetz+foo@localhost', NOT the mali command, so that would not have stopped
this.

Anyway, the attached patch WFM, and should be checked into all 3 trees. I
think.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 91464 [details] [diff] [review]
2.14.3/2.16 system() patch

>Index: bug_email.pl

>@@ -1233,8 +1233,14 @@

>-	# Cool, the mail was successfull
>-	system("cd .. ; ./processmail $id '$Sender'");
>+	# Cool, the mail was successful
>+    # chdir back to the main directory which has the processmail script
>+    # Oh, for a processmail module....
>+    use Cwd;
>+    my $old_cwd = getcwd();
>+    chdir("..");
>+    system("./processmail", $id, $Sender);
>+    chdir($old_cwd);

Nit: this needs an extra four spaces of indentation.

r=myk; it looks good and passes "perl -cw", but I haven't tested it.

Comment 18

[Erik Anderson]

This patch is functionally similar to previous patches in this bug report, one 
difference that should be pointed out is that $Sender (containing the entire 
From: line) is being sent to processmail, but $SenderShort (just the email 
address) is more appropriate here.  Otherwise processmail will send out a 
duplicate confirmation email.

Comment 19

[Dave Miller [:justdave]]

Comment on attachment 91464 [details] [diff] [review]
2.14.3/2.16 system() patch

r=justdave

Comment 20

[Dave Miller [:justdave]]

Comment on attachment 91464 [details] [diff] [review]
2.14.3/2.16 system() patch

oops, yeah, he's right.  SenderShort belongs in there (because it contains just
the email address without the comments).

You're right, bbaetz, it sucks, and needs a major rewrite, but that's on our
goal list for 2.18 :-)

Comment 21

[Myk Melez [:myk] [@mykmelez]]

This is on our list?  *sigh* There goes the dream of a short release cycle.

Comment 22

[Erik Anderson]

> "exists $Control{$f}" isn't going to cause a warning unless either $Control 
> or $f are undefined.  So $Control and $f are what we need to be checking for
> defined-ness here, not $Control{$f}.  You have to see if $Control{$f} exists 
> or not first before you can tell if its value is defined anyway.

Okay, I've just looked at this in more detail (finally some time!).  If an 
empty email is sent to this script, then $Control{'assigned_to'} and $Control
{'groupset'} (possibly others) are being given a value of 'undefined'.  The 
keys exist in the hash (exists succeeds), but the values themselves are 
empty.  This is the cause for the changes at both lines 943 and 1149.

I took a look at trying to prevent the undef values from getting in there in 
the first place, but in the end the original patch seems to be the simplest 
solution.

Should I add the additional exists tests as commented?

Comment 23

[Dave Miller [:justdave]]

Comment on attachment 89104 [details] [diff] [review]
[Issue 1] updated system() call

obsoleted by attachment 91464 [details] [diff] [review]

Comment 24

[Dave Miller [:justdave]]

Comment on attachment 91464 [details] [diff] [review]
2.14.3/2.16 system() patch

r= justdave with $Sender changed to $SenderShort

Comment 25

[Dave Miller [:justdave]]

All patches applied cleanly to all three branches.  bbaetz's system() patch was
applied with the $SenderShort in it.

HEAD:

Checking in bug_email.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/bug_email.pl,v  <--  bug_email.pl
new revision: 1.10; previous revision: 1.9
done
Checking in bugzilla_email_append.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/bugzilla_email_append.pl,v  <-- 
bugzilla_email_append.pl
new revision: 1.3; previous revision: 1.2
done

BUGZILLA-2_16-BRANCH:

Checking in bug_email.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/bug_email.pl,v  <--  bug_email.pl
new revision: 1.9.12.1; previous revision: 1.9
done
Checking in bugzilla_email_append.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/bugzilla_email_append.pl,v  <-- 
bugzilla_email_append.pl
new revision: 1.2.14.1; previous revision: 1.2
done

BUGZILLA-2_14_1-BRANCH:

Checking in bug_email.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/bug_email.pl,v  <--  bug_email.pl
new revision: 1.9.10.1; previous revision: 1.9
done
Checking in bugzilla_email_append.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/bugzilla_email_append.pl,v  <-- 
bugzilla_email_append.pl
new revision: 1.2.12.1; previous revision: 1.2
done


Issue #3 in the list in this bug was NOT dealt with, but I don't think it's
important for the 2.16 timeframe.  In an effort to close out all the milestone
bugs, I'm resolving this FIXED.  Please open new bugs for the remaining issues
listed here which weren't resolved by this checkin.

Comment 26

[Jussi Sirpoma]

*** Bug 139735 has been marked as a duplicate of this bug. ***

Comment 27

[Bradley Baetz (:bbaetz)]

Noone actually tested this....

See bug 160631