Closed Bug 59349 Opened 24 years ago Closed 23 years ago

processmail bails out when running in secure (taint) mode

Categories

(Bugzilla :: Bugzilla-General, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Bugzilla 2.14

People

(Reporter: jvromans, Assigned: jacob)

References

Details

(Whiteboard: security code)

Attachments

(5 files)

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.
Whiteboard: 2.14
Keywords: patch
moving to real milestones...
Whiteboard: 2.14
Target Milestone: --- → Bugzilla 2.14
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.
OS: Linux → All
Hardware: PC → All
Whiteboard: security
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
Depends on: 71552
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?
Keywords: patch
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?
Status: NEW → ASSIGNED
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).
Whiteboard: security → security code
$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...
Assignee: tara → jake
Status: ASSIGNED → NEW
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
Keywords: patch
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.
OK, couple days on landfill and syndicomm and it looks like it works again.

r= justdave

Checked in.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Moving to Bugzilla product
Component: Bugzilla → Bugzilla-General
Product: Webtools → Bugzilla
Version: other → unspecified
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: