When processmail is run using perl -T, as all web applications should, it bails out since subroutine NewProcessOneBug creates files using the bug number that was passed as an argument to the program. The attached patch verifies the correctness of the bug number (i.e., a decimal number) and untaints it if okay.

moving to real milestones...

Patch looks good to me, although I haven't tested it out. (Getting perl scripts to work in taint mode can sometimes be a real pain.) Adding security keyword.

The code in question will be removed as part of bug 71552. However, perl -cwT processmail still failed on me... until I added use lib "."; before it tried to load RelationSet.pm

Jake, did you add the use lib line in the other patch, or doed that still need to be done?

After adding the use lib line and adding a T to the #! line of processmail, upon modifying a bug I get the error: Insecure $ENV{PATH} while running with -T switch at ./processmail line 696. Line 696 is where it opens sendmail. So I'd say processmail still won't run in taint mode. Johan, did you make any further changes to processmail?

delete $::ENV{PATH}; will fix that. I do that all the time in my CGIs. Nothing you run from a CGI environment is safe with a PATH anyway, so every you run better be using explicit pathnames anyway, and not be depending on PATH.

deleting $::ENV{PATH} did in fact allow processmail to run in taint mode. What I did is add 'delete $::ENV{PATH};' to globals.pl and 'use lib ".";' to processmail (i tried adding that in globals.pl, too, but it didn't like it). I guess the remaining questions are, do we want to add this to the distro.... and to what extent? (Every script that would want to run in taint mode would need 'use lib ".";' added to it.)

isn't there a bug somewhere for getting all of Bugzilla to run in taint mode?

Not that I can find in my quick queries. Theoretically we could make this that bug. Jake--how does globals.pl make it's dislike known?

IIRC, when I put 'use lib ".";' in globals.pl, it just had no effect. I got the same error message from processmail as I did w/out it there. When I put it in processmail, it worked. I haven't been running processmail (or any of the other files, for that matter), in taint mode lately, but as I recall from my testing, 'use lib ".";' will have to be added to each file which you want to run in taint mode.

Preminary investigation of running buglist.cgi in Perl's taint mode shows that it is relatively easy since, as Dave says, "the -T on the perl flags will only affect our calls to system() and file pipes," and most of our tainted web form data only ever goes to the database, which has its own flag for running in taint mode. Setting that latter flag, however, is much harder, since it means all form data (such as every field on the query form) needs to be validated via a regular expression before it can be used in an SQL query. This is a good thing, but it is also a lot of work for those files that take a lot of input (buglist.cgi, process_bug.cgi).

$dbh->{Taint} = 1 actually shouldn't be too big a deal in processmail... processmail isn't dealing with web data, it's only dealing with a command-line and stuff it pulls from the database. Untaint the stuff passed in on the command-line and it'll probably solve it.

I guess the question now is, to what (if any) extent are we going to do this for 2.14? None, just the -T on the shebang line, or |$dbh->{Taint} = 1;|?

Given that 2.14 is supposedly the "security release" we *should* go all the way with the $db->{Taint} = 1... however, that's going to require a significant amount of work and testing to implement... in the interest of getting 2.14 done and out, maybe it should wait. On the other hand, processmail should be one of the easier files to do this to, comparatively (buglist and process_bug will be PITAs)

The patch I just attached still bails, but I thought it might be a good idea to let some other people look at it to make sure I'm going about this the right way...

PS, die_with_dignity() is only temporary so I can use confess to get the stack trace.

I finally recieved an e-mail with taint mode turned on for processmail. The patch I attached seems to work but I think it should spend some time on landfill to be certain. Also, before it is checked in, use Carp should be commented out again and the sub die_with_diginity() should be removed. use Carp is filling my error logs with things like (at least 4 similar messages every time a script is run): Subroutine confess redefined at /usr/lib/perl5/5.6.0/Exporter.pm line 57. Exporter::import('confess', 'croak', 'carp') called at globals.pl line 71 main::BEGIN() called at globals.pl line 71 require 0 called at globals.pl line 71 require globals.pl called at CGI.pl line 49 require CGI.pl called at /home/httpd/html/bugzilla/process_bug.cgi line 31

I have this installed on bugzilla.syndicomm.com. I put it there instead of landfill because we're going to need a production server to get any kind of testing out of email I think... First impressions say it works though.

I just had to yank this patch from syndicomm. :-) nothing like testing on a live server to bring out the bugs in the best code. Interestingly enough, the point it dies at is before it even calls processmail, in the INSERT trying to put the comment itself into the database if you try to add a comment to a bug. Posting Bug One moment please... INSERT INTO longdescs (bug_id, who, bug_when, thetext) VALUES (346, '1', now(), '''): You have an error in your SQL syntax near '''')' at line 1 at globals.pl line 205 main::SendSQL('INSERT INTO longdescs (bug_id, who, bug_when, thetext) VALUES (3...') called at /usr/local/bugzilla/post_bug.cgi line 252 Looks to me like SqlQuote($comment) somehow got turned into ''' (an unquoted single quote with single-quotes around it). Oh, I doctored SendSQL to use confess instead of die so I could find out where it was calling SendSQL from. Maybe we ought to do that anyway... how likely is SendSQL to ever die from an error it caused itself? ;)

I ran accross this one yesterday while trying to change params on my site. The problem is that SqlQuote() runs the value through detaint_string() [because an SqlQuoting the string makes it safe to send to the db] which basically does: $str =~ m/^(.*)$/; $str = $1; which works great on single line statements. But comments are often more the one line (well, on my site, they're often one liners, but I only have to explain myself to me :) and that expression didn't deal too well with that. Adding an 's' as a modifier to the expression was the solution.

patch is now running at http://landfill.tequilarista.org/bz59349/

OK, couple days on landfill and syndicomm and it looks like it works again. r= justdave Checked in.

Moving to Bugzilla product