Bug 1075578: [SECURITY] Improper filtering of CGI arguments

r=dkl,a=sgreen

11 files changed, 52 insertions, 50 deletions

diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm
index 69939a657..fa8845358 100644
--- a/Bugzilla/Attachment.pm
+++ b/Bugzilla/Attachment.pm
@@ -911,10 +911,12 @@ sub get_content_type {
     return 'text/plain' if ($cgi->param('ispatch') || $cgi->param('attach_text'));
 
     my $content_type;
-    if (!defined $cgi->param('contenttypemethod')) {
+    my $method = $cgi->param('contenttypemethod');
+
+    if (!defined $method) {
         ThrowUserError("missing_content_type_method");
     }
-    elsif ($cgi->param('contenttypemethod') eq 'autodetect') {
+    elsif ($method eq 'autodetect') {
         defined $cgi->upload('data') || ThrowUserError('file_not_specified');
         # The user asked us to auto-detect the content type, so use the type
         # specified in the HTTP request headers.
@@ -935,18 +937,17 @@ sub get_content_type {
             $content_type = 'image/png';
         }
     }
-    elsif ($cgi->param('contenttypemethod') eq 'list') {
+    elsif ($method eq 'list') {
         # The user selected a content type from the list, so use their
         # selection.
         $content_type = $cgi->param('contenttypeselection');
     }
-    elsif ($cgi->param('contenttypemethod') eq 'manual') {
+    elsif ($method eq 'manual') {
         # The user entered a content type manually, so use their entry.
         $content_type = $cgi->param('contenttypeentry');
     }
     else {
-        ThrowCodeError("illegal_content_type_method",
-                       { contenttypemethod => $cgi->param('contenttypemethod') });
+        ThrowCodeError("illegal_content_type_method", { contenttypemethod => $method });
     }
     return $content_type;
 }
diff --git a/Bugzilla/Chart.pm b/Bugzilla/Chart.pm
index dfbf32a51..8fd4706e4 100644
--- a/Bugzilla/Chart.pm
+++ b/Bugzilla/Chart.pm
@@ -110,10 +110,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 0078a4c16..d707d68c0 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -228,8 +228,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;
@@ -537,13 +538,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 faeb56176..a7eb98df5 100755
--- a/buglist.cgi
+++ b/buglist.cgi
@@ -1014,7 +1014,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 = new Bugzilla::Product({ name => $cgi->param('product') });
+        $one_product = new Bugzilla::Product({ name => $product_input[0] });
     }
 }
 # We only want the template to use it if the user can actually 
diff --git a/editflagtypes.cgi b/editflagtypes.cgi
index d75bebba2..d28188ad7 100755
--- a/editflagtypes.cgi
+++ b/editflagtypes.cgi
@@ -59,23 +59,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 a879aa770..ccd0bd432 100755
--- a/editgroups.cgi
+++ b/editgroups.cgi
@@ -242,7 +242,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(*)
@@ -266,7 +266,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 c0878b0da..6e1ecec81 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -168,7 +168,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').
@@ -183,9 +186,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 07796f9f6..dc8f7f422 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');
 
@@ -159,9 +153,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 897ab148e..402862734 100644
--- a/template/en/default/filterexceptions.pl
+++ b/template/en/default/filterexceptions.pl
@@ -186,7 +186,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 2567d4a7a..6cc15ccd8 100644
--- a/template/en/default/global/messages.html.tmpl
+++ b/template/en/default/global/messages.html.tmpl
@@ -941,7 +941,7 @@
 [% IF !message %]
   [% message = BLOCK %]
     You are using [% terms.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 messages.html.tmpl.<br>
     <br>
     If you are a [% terms.Bugzilla %] end-user seeing this message, please
diff --git a/token.cgi b/token.cgi
index 901094be4..a321974c3 100755
--- a/token.cgi
+++ b/token.cgi
@@ -382,7 +382,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.