Bug 1075578: [SECURITY] Improper filtering of CGI arguments

r=dkl,a=sgreen

11 files changed, 46 insertions, 45 deletions

diff --git a/Bugzilla/Chart.pm b/Bugzilla/Chart.pm
index c8cd41b52..3c69006aa 100644
--- a/Bugzilla/Chart.pm
+++ b/Bugzilla/Chart.pm
@@ -96,10 +96,9 @@ sub init {
     if ($self->{'datefrom'} && $self->{'dateto'} && 
         $self->{'datefrom'} > $self->{'dateto'}) 
     {
-          ThrowUserError("misarranged_dates", 
-                                         {'datefrom' => $cgi->param('datefrom'),
-                                          'dateto' => $cgi->param('dateto')});
-    }    
+          ThrowUserError('misarranged_dates', { 'datefrom' => scalar $cgi->param('datefrom'),
+                                                'dateto' => scalar $cgi->param('dateto') });
+    }
 }
 
 # Alter Chart so that the selected series are added to it.
diff --git a/attachment.cgi b/attachment.cgi
index f2eca5694..3d0ac2bb6 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -196,8 +196,9 @@ sub validateContext
 {
   my $context = $cgi->param('context') || "patch";
   if ($context ne "file" && $context ne "patch") {
-    detaint_natural($context)
-      || ThrowUserError("invalid_context", { context => $cgi->param('context') });
+      my $orig_context = $context;
+      detaint_natural($context)
+        || ThrowUserError("invalid_context", { context => $orig_context });
   }
 
   return $context;
@@ -515,13 +516,14 @@ sub insert {
 
     # Get the filehandle of the attachment.
     my $data_fh = $cgi->upload('data');
+    my $attach_text = $cgi->param('attach_text');
 
     my $attachment = Bugzilla::Attachment->create(
         {bug           => $bug,
          creation_ts   => $timestamp,
-         data          => scalar $cgi->param('attach_text') || $data_fh,
+         data          => $attach_text || $data_fh,
          description   => scalar $cgi->param('description'),
-         filename      => $cgi->param('attach_text') ? "file_$bugid.txt" : scalar $cgi->upload('data'),
+         filename      => $attach_text ? "file_$bugid.txt" : $data_fh,
          ispatch       => scalar $cgi->param('ispatch'),
          isprivate     => scalar $cgi->param('isprivate'),
          mimetype      => $content_type,
diff --git a/buglist.cgi b/buglist.cgi
index 5e84b340b..daee34c9b 100755
--- a/buglist.cgi
+++ b/buglist.cgi
@@ -945,7 +945,7 @@ if (scalar(@products) == 1) {
 # This is used in the "Zarroo Boogs" case.
 elsif (my @product_input = $cgi->param('product')) {
     if (scalar(@product_input) == 1 and $product_input[0] ne '') {
-        $one_product = Bugzilla::Product->new({ name => $cgi->param('product'), cache => 1 });
+        $one_product = Bugzilla::Product->new({ name => $product_input[0], cache => 1 });
     }
 }
 # We only want the template to use it if the user can actually 
diff --git a/editflagtypes.cgi b/editflagtypes.cgi
index 06144fc5c..d848d250a 100755
--- a/editflagtypes.cgi
+++ b/editflagtypes.cgi
@@ -41,23 +41,24 @@ my @products = @{$vars->{products}};
 
 my $action = $cgi->param('action') || 'list';
 my $token  = $cgi->param('token');
-my $product = $cgi->param('product');
-my $component = $cgi->param('component');
+my $prod_name = $cgi->param('product');
+my $comp_name = $cgi->param('component');
 my $flag_id = $cgi->param('id');
 
-if ($product) {
+my ($product, $component);
+
+if ($prod_name) {
     # Make sure the user is allowed to view this product name.
     # Users with global editcomponents privs can see all product names.
-    ($product) = grep { lc($_->name) eq lc($product) } @products;
-    $product || ThrowUserError('product_access_denied', { name => $cgi->param('product') });
+    ($product) = grep { lc($_->name) eq lc($prod_name) } @products;
+    $product || ThrowUserError('product_access_denied', { name => $prod_name });
 }
 
-if ($component) {
-    ($product && $product->id)
-      || ThrowUserError('flag_type_component_without_product');
-    ($component) = grep { lc($_->name) eq lc($component) } @{$product->components};
+if ($comp_name) {
+    $product || ThrowUserError('flag_type_component_without_product');
+    ($component) = grep { lc($_->name) eq lc($comp_name) } @{$product->components};
     $component || ThrowUserError('product_unknown_component', { product => $product->name,
-                                                                comp => $cgi->param('component') });
+                                                                comp => $comp_name });
 }
 
 # If 'categoryAction' is set, it has priority over 'action'.
diff --git a/editgroups.cgi b/editgroups.cgi
index 9c33a0ee3..287ac1114 100755
--- a/editgroups.cgi
+++ b/editgroups.cgi
@@ -224,7 +224,7 @@ if ($action eq 'new') {
 
 if ($action eq 'del') {
     # Check that an existing group ID is given
-    my $group = Bugzilla::Group->check({ id => $cgi->param('group') });
+    my $group = Bugzilla::Group->check({ id => scalar $cgi->param('group') });
     $group->check_remove({ test_only => 1 });
     $vars->{'shared_queries'} =
         $dbh->selectrow_array('SELECT COUNT(*)
@@ -248,7 +248,7 @@ if ($action eq 'del') {
 if ($action eq 'delete') {
     check_token_data($token, 'delete_group');
     # Check that an existing group ID is given
-    my $group = Bugzilla::Group->check({ id => $cgi->param('group') });
+    my $group = Bugzilla::Group->check({ id => scalar $cgi->param('group') });
     $vars->{'name'} = $group->name;
     $group->remove_from_db({
         remove_from_users => scalar $cgi->param('removeusers'),
diff --git a/post_bug.cgi b/post_bug.cgi
index f73ca6b29..9da8faec1 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -152,7 +152,10 @@ if (defined $cgi->param('version')) {
 # after the bug is filed.
 
 # Add an attachment if requested.
-if (defined($cgi->upload('data')) || $cgi->param('attach_text')) {
+my $data_fh = $cgi->upload('data');
+my $attach_text = $cgi->param('attach_text');
+
+if ($data_fh || $attach_text) {
     $cgi->param('isprivate', $cgi->param('comment_is_private'));
 
     # Must be called before create() as it may alter $cgi->param('ispatch').
@@ -167,9 +170,9 @@ if (defined($cgi->upload('data')) || $cgi->param('attach_text')) {
         $attachment = Bugzilla::Attachment->create(
             {bug           => $bug,
              creation_ts   => $timestamp,
-             data          => scalar $cgi->param('attach_text') || $cgi->upload('data'),
+             data          => $attach_text || $data_fh,
              description   => scalar $cgi->param('description'),
-             filename      => $cgi->param('attach_text') ? "file_$id.txt" : scalar $cgi->upload('data'),
+             filename      => $attach_text ? "file_$id.txt" : $data_fh,
              ispatch       => scalar $cgi->param('ispatch'),
              isprivate     => scalar $cgi->param('isprivate'),
              mimetype      => $content_type,
diff --git a/relogin.cgi b/relogin.cgi
index fd1f58ea0..6f0c970f1 100755
--- a/relogin.cgi
+++ b/relogin.cgi
@@ -87,19 +87,21 @@ elsif ($action eq 'begin-sudo') {
     {
         $credentials_provided = 1;
     }
-    
+
     # Next, log in the user
     my $user = Bugzilla->login(LOGIN_REQUIRED);
-    
+
+    my $target_login = $cgi->param('target_login');
+    my $reason = $cgi->param('reason') || '';
+
     # At this point, the user is logged in.  However, if they used a method
     # where they could have provided a username/password (i.e. CGI), but they 
     # did not provide a username/password, then throw an error.
     if ($user->authorizer->can_login && !$credentials_provided) {
         ThrowUserError('sudo_password_required',
-                       { target_login => $cgi->param('target_login'),
-                               reason => $cgi->param('reason')});
+                       { target_login => $target_login, reason => $reason });
     }
-    
+
     # The user must be in the 'bz_sudoers' group
     unless ($user->in_group('bz_sudoers')) {
         ThrowUserError('auth_failure', {  group => 'bz_sudoers',
@@ -123,30 +125,22 @@ elsif ($action eq 'begin-sudo') {
             && ($token_data eq 'sudo_prepared'))
     {
         ThrowUserError('sudo_preparation_required', 
-                       { target_login => scalar $cgi->param('target_login'),
-                               reason => scalar $cgi->param('reason')});
+                       { target_login => $target_login, reason => $reason });
     }
     delete_token($cgi->param('token'));
 
     # Get & verify the target user (the user who we will be impersonating)
-    my $target_user = 
-        new Bugzilla::User({ name => $cgi->param('target_login') });
+    my $target_user = new Bugzilla::User({ name => $target_login });
     unless (defined($target_user)
             && $target_user->id
             && $user->can_see_user($target_user))
     {
-        ThrowUserError('user_match_failed',
-                       { 'name' => $cgi->param('target_login') }
-        );
+        ThrowUserError('user_match_failed', { name => $target_login });
     }
     if ($target_user->in_group('bz_sudo_protect')) {
         ThrowUserError('sudo_protected', { login => $target_user->login });
     }
 
-    # If we have a reason passed in, keep it under 200 characters
-    my $reason = $cgi->param('reason') || '';
-    $reason = substr($reason, 0, 200);
-    
     # Calculate the session expiry time (T + 6 hours)
     my $time_string = time2str('%a, %d-%b-%Y %T %Z', time + MAX_SUDO_TOKEN_AGE, 'GMT');
 
@@ -166,9 +160,12 @@ elsif ($action eq 'begin-sudo') {
 
     # For the present, change the values of Bugzilla::user & Bugzilla::sudoer
     Bugzilla->sudo_request($target_user, $user);
-    
+
     # NOTE: If you want to log the start of an sudo session, do it here.
 
+    # If we have a reason passed in, keep it under 200 characters
+    $reason = substr($reason, 0, 200);
+
     # Go ahead and send out the message now
     my $message;
     my $mail_template = Bugzilla->template_inner($target_user->setting('lang'));
diff --git a/template/en/default/filterexceptions.pl b/template/en/default/filterexceptions.pl
index 93c4a42a9..3c5bfc217 100644
--- a/template/en/default/filterexceptions.pl
+++ b/template/en/default/filterexceptions.pl
@@ -167,7 +167,6 @@
 ],
 
 'global/messages.html.tmpl' => [
-  'message_tag', 
   'series.frequency * 2',
 ],
 
diff --git a/template/en/default/global/messages.html.tmpl b/template/en/default/global/messages.html.tmpl
index f47a1d6ec..3a8aa1ada 100644
--- a/template/en/default/global/messages.html.tmpl
+++ b/template/en/default/global/messages.html.tmpl
@@ -943,7 +943,7 @@
 [% IF !message %]
   [% message = BLOCK %]
     You are using Bugzilla's messaging functions incorrectly. You
-    passed in the string '[% message_tag %]'. The correct use is to pass
+    passed in the string '[% message_tag FILTER html %]'. The correct use is to pass
     in a tag, and define that tag in the file <kbd>messages.html.tmpl</kbd>.<br>
     <br>
     If you are a [% terms.Bugzilla %] end-user seeing this message, please
diff --git a/token.cgi b/token.cgi
index 145aa5225..830ecfccb 100755
--- a/token.cgi
+++ b/token.cgi
@@ -313,7 +313,7 @@ sub confirm_create_account {
 
     my $otheruser = Bugzilla::User->create({
         login_name => $login_name, 
-        realname   => $cgi->param('realname'), 
+        realname   => scalar $cgi->param('realname'),
         cryptpassword => $password});
 
     # Now delete this token.
diff --git a/userprefs.cgi b/userprefs.cgi
index ad5fb7d19..1f5f625f7 100755
--- a/userprefs.cgi
+++ b/userprefs.cgi
@@ -544,7 +544,7 @@ sub SaveApiKey {
     if ($cgi->param('new_key')) {
         $vars->{new_key} = Bugzilla::User::APIKey->create({
             user_id     => $user->id,
-            description => $cgi->param('new_description'),
+            description => scalar $cgi->param('new_description'),
         });
 
         # As a security precaution, we always sent out an e-mail when