21253 - remove single-parameter system() calls from Bugzilla

Status: RESOLVED FIXED

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

Description

[Dan Mosedale (:dmosedale, :dmose)]

The processmail script is called using system().  Almost every call of
processmail puts unchecked values from the CGI forms in the command-line that's
given to the shell.  This means that people can execute arbitrary commands by
embedding shell meta-characters (eg `cat /etc/passwd | mail
[e-mail]`) in CGI form variables.

Ways to fix this include either making processmail take this information from
stdin, or carefully stripping shell metachars out of the command line, or giving
up on system() and doing the fork/exec directly.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Changing to mozilla.org only until it's fixed.

Comment 2

[Terry Weissman]

Actually, I think the only unchecked value ever fed to processmail is
$::FORM{'who'}, and it would be easy to check that one too.  Am I missing
something?

Comment 3

[Terry Weissman]

Anyway, another good idea is to call the multi-parameter version of system(),
which in perl will *not* invoke a shell.  If I remember correctly.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Yes, replacing all single-arg calls to system() with multi-arg calls would be an
excellent fix for this.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

another test

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

erk, wrong bug; sorry kids

Comment 7

[Terry Weissman]

[e-mail] is the new owner of Bugzilla and Bonsai.  (For details,
see my posting in netscape.public.mozilla.webtools,
news://news.mozilla.org/38F5D90D.F40E8C1A%40geocast.com .)

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Opening up the permissions so that tara can see this.

Comment 9

[Tara Hernandez]

Attachment: Patch submitted by Ed Korthof

Comment 10

[Tara Hernandez]

Attachment: This is the REAL patch submitted by Ed Korthof

Comment 11

[Tara Hernandez]

reassigning this to cyeh as he ended up handling the whole thing--Chris can you 
work with dmose to make sure this is all handled?

Comment 12

[Chris Yeh]

i applied this patch. it's part of the 2.10 bugzilla release. we still need to 
patch up the other sections and undergo a complete security review.

Comment 13

[Stephan Niemz]

Adding default QA contact to all open Webtools/Bugzilla bugs lacking one.
Sorry for the spam.

Comment 14

[Dave Miller [:justdave]]

moving to real milestones...

Comment 15

[Dave Miller [:justdave]]

Just re-found this.  We thought this was taken care of a long time ago.  I just 
reviewed all the system calls (cd bugzilla; fgrep "system\(.*\s.*\)" *) and there 
are very few left using single-parameter system() calls, and all of those are 
using values that are self-generated and trustworthy.

Comment 16

[Matthew Tuck [:CodeMachine]]

We should remove all instances of one param system even if we think the content
is trusted, just to be safe.

Before this is marked FIXED, we should have a tinderbox check that makes sure
one param system is used nowhere in the code to prevent this problem being
reintroduced.

Comment 17

[Dave Miller [:justdave]]

Some of the remaining one-param system() calls are done that way because they 
have output redirection.  You lose the ability to do that as part of the system 
call by making it multi-parameter (because a shell is not invoked).  However, you 
can redirect output from perl before making the call, like so:

open SAVEOUT, ">&STDOUT";               # stash the original output stream
open STDOUT, ">/path/to/output.file";   # redirect to file
select STDOUT; $| = 1;                  # disable buffering
system("system","call","goes","here");  # make your system call
open STDOUT, ">&SAVEOUT";               # redirect back to original stream

if you want to redirect STDERR as well, do like this:

open SAVEOUT, ">&STDOUT";
open SAVEERR, ">&STDERR";
open STDOUT, ">/path/to/output.file";
open STDERR, ">&STDOUT"
select STDERR; $| = 1;
select STDOUT; $| = 1;
system("whatever","with","params");
open STDERR, ">&SAVEERR";
open STDOUT, ">&SAVEOUT";

Comment 18

[Matthew Tuck [:CodeMachine]]

Sorry, I forgot to mention that.  We should create a subroutine to do all that.

Comment 19

[Dave Miller [:justdave]]

The tinderbox scripts (Scientists and Rooting, so far, the other when Hixie 
applies the patch to his) now test for single parameter system calls and create a 
fatal compilation error when any are found.

This means the tree is now red again until this bug is fixed again.

Comment 20

[Dave Miller [:justdave]]

Attachment: fixes for this bug for checksetup.pl (more coming for other files)

Comment 21

[Dave Miller [:justdave]]

OK, here's the build log with the checks in to make it fail on single-param 
system calls:

           Build Log (Full) 
   Rooting Bugzilla perl 5.006 darwin on 06/02 23:28 

Build Error Log

tinderbox: tree: Bugzilla
tinderbox: builddate: 991549712
tinderbox: status: busted
tinderbox: build: Rooting Bugzilla perl 5.006 darwin
tinderbox: errorparser: unix
tinderbox: buildfamily: unix
tinderbox: END

Running cvs checkout
Bug.pm has OK perl syntax
buglist.cgi has OK perl syntax
Running /usr/bin/perl  -cw -I/Users/dave/tinderbox/mozilla/webtools/bugzilla/ -I/
Users/dave/tinderbox/ -Msafesystem collectstats.pl; Results:
Compilation of collectstats.pl failed.
Errors: 
Not enough arguments for safesystem::system at
        /Users/dave/tinderbox/mozilla/webtools/bugzilla/collectstats.pl line 135, 
near ""rm -f data/duplicates/dupes$today*")" (#1)
    
    (F) The function requires more arguments than you specified.
    
/Users/dave/tinderbox/mozilla/webtools/bugzilla/collectstats.pl had compilation
        errors (#2)
    
    (F) The final summary message when a perl -c fails.
    
Uncaught exception from user code:
        /Users/dave/tinderbox/mozilla/webtools/bugzilla/collectstats.pl had 
compilation errors.
createaccount.cgi has OK perl syntax
createattachment.cgi has OK perl syntax
defparams.pl has OK perl syntax
describecomponents.cgi has OK perl syntax
describekeywords.cgi has OK perl syntax
doeditparams.cgi has OK perl syntax
doeditvotes.cgi has OK perl syntax
duplicates.cgi has OK perl syntax
editcomponents.cgi has OK perl syntax
editgroups.cgi has OK perl syntax
editkeywords.cgi has OK perl syntax
editmilestones.cgi has OK perl syntax
editparams.cgi has OK perl syntax
editproducts.cgi has OK perl syntax
editusers.cgi has OK perl syntax
editversions.cgi has OK perl syntax
enter_bug.cgi has OK perl syntax
importxml.pl has OK perl syntax
Running /usr/bin/perl  -cw -I/Users/dave/tinderbox/mozilla/webtools/bugzilla/ -I/
Users/dave/tinderbox/ -Msafesystem globals.pl; Results:
Compilation of globals.pl failed.
Errors: 
Not enough arguments for safesystem::system at
        /Users/dave/tinderbox/mozilla/webtools/bugzilla/globals.pl line 112, near 
""./syncshadowdb &")" (#1)
    
    (F) The function requires more arguments than you specified.
    
/Users/dave/tinderbox/mozilla/webtools/bugzilla/globals.pl had compilation
        errors (#2)
    
    (F) The final summary message when a perl -c fails.
    
Uncaught exception from user code:
        /Users/dave/tinderbox/mozilla/webtools/bugzilla/globals.pl had 
compilation errors.
long_list.cgi has OK perl syntax
Running /usr/bin/perl  -cw -I/Users/dave/tinderbox/mozilla/webtools/bugzilla/ -I/
Users/dave/tinderbox/ -Msafesystem move.pl; Results:
Compilation of move.pl failed.
Errors: 
Uncaught exception from user code:
        Uncaught exception from user code:
        Uncaught exception from user code:
        Not enough arguments for safesystem::system at /Users/dave/tinderbox/
mozilla/webtools/bugzilla//globals.pl line 112, near ""./syncshadowdb &")"
Compilation failed in require at /Users/dave/tinderbox/mozilla/webtools/bugzilla/
/Bug.pm line 30.
        require Bug.pm called at /Users/dave/tinderbox/mozilla/webtools/bugzilla/
move.pl line 26
        main::BEGIN() called at /Users/dave/tinderbox/mozilla/webtools/bugzilla//
globals.pl line 0
        require 0 called at /Users/dave/tinderbox/mozilla/webtools/bugzilla//
globals.pl line 0
Compilation failed in require at /Users/dave/tinderbox/mozilla/webtools/bugzilla/
/Bug.pm line 30.
        main::BEGIN() called at /Users/dave/tinderbox/mozilla/webtools/bugzilla//
Bug.pm line 26
        require 0 called at /Users/dave/tinderbox/mozilla/webtools/bugzilla//
Bug.pm line 26
BEGIN failed--compilation aborted at /Users/dave/tinderbox/mozilla/webtools/
bugzilla/move.pl line 26.
contrib/mysqld-watcher.pl has OK perl syntax
new_comment.cgi has OK perl syntax
post_bug.cgi has OK perl syntax
Running /usr/bin/perl  -cw -I/Users/dave/tinderbox/mozilla/webtools/bugzilla/ -I/
Users/dave/tinderbox/ -Msafesystem process_bug.cgi; Results:
Compilation of process_bug.cgi failed.
Errors: 
Not enough arguments for safesystem::system at
        /Users/dave/tinderbox/mozilla/webtools/bugzilla/process_bug.cgi line 
1007, near "@ARGLIST;" (#1)
    
    (F) The function requires more arguments than you specified.
    
/Users/dave/tinderbox/mozilla/webtools/bugzilla/process_bug.cgi had compilation
        errors (#2)
    
    (F) The final summary message when a perl -c fails.
    
Uncaught exception from user code:
        /Users/dave/tinderbox/mozilla/webtools/bugzilla/process_bug.cgi had 
compilation errors.
processmail has OK perl syntax
query.cgi has OK perl syntax
RelationSet.pm has OK perl syntax
relogin.cgi has OK perl syntax
reports.cgi has OK perl syntax
sanitycheck.cgi has OK perl syntax
show_activity.cgi has OK perl syntax
show_bug.cgi has OK perl syntax
showattachment.cgi has OK perl syntax
showdependencygraph.cgi has OK perl syntax
showdependencytree.cgi has OK perl syntax
showvotes.cgi has OK perl syntax
Running /usr/bin/perl  -cw -I/Users/dave/tinderbox/mozilla/webtools/bugzilla/ -I/
Users/dave/tinderbox/ -Msafesystem syncshadowdb; Results:
Compilation of syncshadowdb failed.
Errors: 
Not enough arguments for safesystem::system at
        /Users/dave/tinderbox/mozilla/webtools/bugzilla/syncshadowdb line 162, 
near ""mysqldump -l -e $db_name $tablelist > $tempfile")" (#1)
    
    (F) The function requires more arguments than you specified.
    
/Users/dave/tinderbox/mozilla/webtools/bugzilla/syncshadowdb had compilation
        errors (#2)
    
    (F) The final summary message when a perl -c fails.
    
Uncaught exception from user code:
        /Users/dave/tinderbox/mozilla/webtools/bugzilla/syncshadowdb had 
compilation errors.
userprefs.cgi has OK perl syntax
whineatnews.pl has OK perl syntax
Running /usr/bin/perl  -cw -I/Users/dave/tinderbox/mozilla/webtools/bugzilla/ -I/
Users/dave/tinderbox/ -Msafesystem xml.cgi; Results:
Compilation of xml.cgi failed.
Errors: 
Uncaught exception from user code:
        Uncaught exception from user code:
        Uncaught exception from user code:
        Not enough arguments for safesystem::system at /Users/dave/tinderbox/
mozilla/webtools/bugzilla//globals.pl line 112, near ""./syncshadowdb &")"
Compilation failed in require at /Users/dave/tinderbox/mozilla/webtools/bugzilla/
/Bug.pm line 30.
        require Bug.pm called at /Users/dave/tinderbox/mozilla/webtools/bugzilla/
xml.cgi line 26
        main::BEGIN() called at /Users/dave/tinderbox/mozilla/webtools/bugzilla//
globals.pl line 0
        require 0 called at /Users/dave/tinderbox/mozilla/webtools/bugzilla//
globals.pl line 0
Compilation failed in require at /Users/dave/tinderbox/mozilla/webtools/bugzilla/
/Bug.pm line 30.
        main::BEGIN() called at /Users/dave/tinderbox/mozilla/webtools/bugzilla//
Bug.pm line 26
        require 0 called at /Users/dave/tinderbox/mozilla/webtools/bugzilla//
Bug.pm line 26
BEGIN failed--compilation aborted at /Users/dave/tinderbox/mozilla/webtools/
bugzilla/xml.cgi line 26.
changepassword.cgi has OK perl syntax
bug_form.pl has OK perl syntax
CGI.pl has OK perl syntax
Running /usr/bin/perl  -cw -I/Users/dave/tinderbox/mozilla/webtools/bugzilla/ -I/
Users/dave/tinderbox/ -Msafesystem checksetup.pl; Results:
Compilation of checksetup.pl failed.
Errors: 
Not enough arguments for safesystem::system at
        /Users/dave/tinderbox/mozilla/webtools/bugzilla/checksetup.pl line 1230, 
near ""stty echo")" (#1)
    
    (F) The function requires more arguments than you specified.
    
Not enough arguments for safesystem::system at
        /Users/dave/tinderbox/mozilla/webtools/bugzilla/checksetup.pl line 1310, 
near ""stty -echo")" (#1)
Not enough arguments for safesystem::system at
        /Users/dave/tinderbox/mozilla/webtools/bugzilla/checksetup.pl line 1331, 
near ""stty echo")" (#1)

/Users/dave/tinderbox/mozilla/webtools/bugzilla/checksetup.pl had compilation
        errors (#2)
    
    (F) The final summary message when a perl -c fails.
    
Uncaught exception from user code:
        /Users/dave/tinderbox/mozilla/webtools/bugzilla/checksetup.pl had 
compilation errors.
colchange.cgi has OK perl syntax


No More Errors

Comment 22

[Dave Miller [:justdave]]

The highlights for those that don't want to read through the whole thing: :)

Not enough arguments for safesystem::system at
        /Users/dave/tinderbox/mozilla/webtools/bugzilla/collectstats.pl line 135, 
near ""rm -f data/duplicates/dupes$today*")" (#1)

Not enough arguments for safesystem::system at
        /Users/dave/tinderbox/mozilla/webtools/bugzilla/globals.pl line 112, near 
""./syncshadowdb &")" (#1)

Not enough arguments for safesystem::system at
        /Users/dave/tinderbox/mozilla/webtools/bugzilla/process_bug.cgi line 
1007, near "@ARGLIST;" (#1)

Not enough arguments for safesystem::system at
        /Users/dave/tinderbox/mozilla/webtools/bugzilla/syncshadowdb line 162, 
near ""mysqldump -l -e $db_name $tablelist > $tempfile")" (#1)

Not enough arguments for safesystem::system at
        /Users/dave/tinderbox/mozilla/webtools/bugzilla/checksetup.pl line 1230, 
near ""stty echo")" (#1)

So we have 5 files with 1-param system calls in them.  The patch I attached a 
couple minutes ago fixes them in checksetup.pl.

Comment 23

[Dave Miller [:justdave]]

In globals.pl, we have this:

sub SyncAnyPendingShadowChanges {
    if ($shadowchanges) {
        system("./syncshadowdb &");  
        $shadowchanges = 0;
    }
}       

I don't think the & there even works.  PutFooter calls this sub.  You ever notice 
when Bugzilla is busy that when you display a page, the browser stays connected 
for a long time after the footer displays?  I think this routine is not 
backgrounding like it was intended, so it's tying up browser time waiting for it 
to finish.  And since it's still attached to the CGI process I wonder if it gets 
killed when the user hits their stop button or loads another page before it 
finishes....

Comment 24

[Dave Miller [:justdave]]

Attachment: Patch for syncshadowdb

Comment 25

[Dave Miller [:justdave]]

I was having a hard time coming up with a "safe" way to make an output 
redirection sub that we could put in globals.pl that didn't require a lot of 
hokey typeglob stuff in order to either return the original filehandle to the 
caller or save it somewhere else.  So I opted to just put the redirection code 
around the system call directly in the patch for syncshadowdb.

Comment 26

[Dave Miller [:justdave]]

Attachment: Patch for process_bug.cgi

Comment 27

[Dave Miller [:justdave]]

Attachment: Patch for collectstats.pl

Comment 28

[Dave Miller [:justdave]]

And that's everything but the attempt to call syncshadowdb in the background in 
globals.pl.  I'm not sure what to do about that one.  fork and exec would be the 
obvious thing, except that I don't think fork() is guaranteed to work on all Perl 
variants...

Comment 29

[Dave Miller [:justdave]]

ok, did some reading up on it...  fork() works as long as the system is POSIX 
compliant.  Which I suppose rules out Windows....  anyone know if Windows claims 
to be POSIX for these purposes?

Comment 30

[Dave Miller [:justdave]]

Attachment: Patch for globals.pl

Comment 31

[Gervase Markham [:gerv]]

fork() is "experimental" on Windows:
http://aspn.activestate.com/ASPN/Reference/Products/ActivePerl/lib/Pod/perlfork.
html

Gerv

Comment 32

[Dave Miller [:justdave]]

Attachment: Patch v2 for collectstats.pl

Comment 33

[Dave Miller [:justdave]]

New patch for collectstats.pl, based on feedback in IRC, eliminate the system 
call altogether because there's a Perl built-in that does the same task.

Comment 34

[Dave Miller [:justdave]]

Attachment: Patch v2 for syncshadowdb

Comment 35

[Dave Miller [:justdave]]

In the new patch for syncshadowdb, I just realized that $tablelist was a list of 
parameters with spaces between them, generated from @tables.  Since we're using 
the list-based system() call now, we can just pass @tables itself without joining 
it first.

Comment 36

[Dave Miller [:justdave]]

guess I should own this since I wrote the most recent patches.

Comment 37

[Dave Miller [:justdave]]

Attachment: Most recent versions of all of the above patches rolled together.

Comment 38

[Tara Hernandez]

r=tara

Comment 39

[Dave Miller [:justdave]]

checked in.

Comment 40

[Tara Hernandez]

i'm a piss poor reviewer.  this line:

exec("./syncshadowdb",[]) or die "Unable to exec syncshadowdb: $!";

is causing an issue as i think dave thought it would.  hit the url on landfill
and see the fun little error message at the bottom.

Comment 41

[Dave Miller [:justdave]]

I don't see an error but I'm guessing it has something to do with "can't pass 
ARRAY(0xXXXXXXX) as a parameter" or something to that effect. :-)  Need to review 
syncshadowdb, the way to get this past the test is probably just to pass "--" as 
a paramater.

Comment 42

[Jacob Steenhagen]

What it's doing is printing "Usage: syncshadowdb [-v] [-syncall]" at the bottom
of the page.  I'd guess that making syncshodowdb accept some form of an empty
param (like --) would be the solution.

Comment 43

[Dave Miller [:justdave]]

Attachment: New patch to fix calling conventions for syncshadowdb

Comment 44

[Dave Miller [:justdave]]

revised patch r=tara in irc after testing on landfill.  Checked in.

Comment 45

[Dave Miller [:justdave]]

Moving to Bugzilla product