Closed Bug 154008 Opened 21 years ago Closed 21 years ago

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

Categories

(Bugzilla :: Creating/Changing Bugs, defect)

2.16
x86
Linux
defect
Not set
minor

Tracking

()

RESOLVED FIXED
Bugzilla 2.16

People

(Reporter: erikba, Assigned: myk)

References

Details

Attachments

(4 files, 2 obsolete files)

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
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.
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.
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.
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.
Keywords: patch
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.
Attached patch [Issue 1] updated system() call (obsolete) — Splinter Review
Update to the third parameter: it is just the email address now.  Otherwise,
processmail will send out a second confirmation email.
Attachment #88994 - Attachment is obsolete: true
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.
Shouldn't it be '../processmail' - its in a parent directory.
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)
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.
-> 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.
Target Milestone: --- → Bugzilla 2.16
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 :-)
Attachment #88995 - Flags: review+
Comment on attachment 89104 [details] [diff] [review]
[Issue 1] updated system() call

2xr= justdave
Attachment #89104 - Flags: review+
Comment on attachment 88997 [details] [diff] [review]
[Issue 4] Bugzilla_email_append stop processing after error

r= justdave
Attachment #88997 - Flags: review+
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.
Attachment #88996 - Flags: review-
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 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.
Attachment #91464 - Flags: review+
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 on attachment 91464 [details] [diff] [review]
2.14.3/2.16 system() patch

r=justdave
Attachment #91464 - Flags: review+
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 :-)
Attachment #91464 - Flags: review+ → review-
This is on our list?  *sigh* There goes the dream of a short release cycle.
> "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 on attachment 89104 [details] [diff] [review]
[Issue 1] updated system() call

obsoleted by attachment 91464 [details] [diff] [review]
Attachment #89104 - Attachment is obsolete: true
Attachment #89104 - Flags: review+
Comment on attachment 91464 [details] [diff] [review]
2.14.3/2.16 system() patch

r= justdave with $Sender changed to $SenderShort
Attachment #91464 - Flags: review- → review+
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.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
*** Bug 139735 has been marked as a duplicate of this bug. ***
Noone actually tested this....

See bug 160631
No longer blocks: bz-email-in
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.