Bug 1075578: [SECURITY] Improper filtering of CGI arguments

r=dkl,a=sgreen

diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm
index 3f6b5d9230997b0d609b4b6f9fe10bcf8da15c1f..2fa9dfdbb8c54c0b480e67c2078489836f82b8d9 100644 (file)
--- a/Bugzilla/Attachment.pm
+++ b/Bugzilla/Attachment.pm
@@ -988,10 +988,12 @@ sub get_content_type {
     return 'text/plain' if ($cgi->param('ispatch') || $cgi->param('attachurl'));
 
     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.
@@ -1012,18 +1014,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 dfbf32a51d1cff01e590536e291a1d05f3e9174d..8fd4706e453f3e4217ae4c02ec9b39d21284ab1b 100644 (file)
--- 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 2706cde24668ee530fcd2d741e7208350c1ee8a7..a6bda6a3575e964c6ebe424d1810334ee0793fc1 100755 (executable)
--- 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;
@@ -547,13 +548,14 @@ sub insert {
 
     # Get the filehandle of the attachment.
     my $data_fh = $cgi->upload('data');
+    my $attach_url = $cgi->param('attachurl');
 
     my $attachment = Bugzilla::Attachment->create(
         {bug           => $bug,
          creation_ts   => $timestamp,
-         data          => scalar $cgi->param('attachurl') || $data_fh,
+         data          => $attach_url || $data_fh,
          description   => scalar $cgi->param('description'),
-         filename      => $cgi->param('attachurl') ? '' : scalar $cgi->upload('data'),
+         filename      => $attach_url ? '' : $data_fh,
          ispatch       => scalar $cgi->param('ispatch'),
          isprivate     => scalar $cgi->param('isprivate'),
          isurl         => scalar $cgi->param('attachurl'),

diff --git a/buglist.cgi b/buglist.cgi
index 931259e27bfde63ab7b7204da1ca9a8efa0214d2..7c223ea2d60318e997083205c031deba1fbbc182 100755 (executable)
--- a/buglist.cgi
+++ b/buglist.cgi
@@ -1083,7 +1083,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/editgroups.cgi b/editgroups.cgi
index a879aa7702e67d49f1a5264ec951d61c3f616ea1..ccd0bd43298b634a5717a6259443e7ce03d49367 100755 (executable)
--- 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 7a070a756382dda1d6827eccdb23cef9a409885f..86ffe8c6b76754e7c47c3c1b40fa0c6ef04ef7e1 100755 (executable)
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -186,7 +186,10 @@ if (defined $cgi->param('version')) {
 # after the bug is filed.
 
 # Add an attachment if requested.
-if (defined($cgi->upload('data')) || $cgi->param('attachurl')) {
+my $data_fh = $cgi->upload('data');
+my $attach_url = $cgi->param('attachurl');
+
+if ($data_fh || $attach_url) {
     $cgi->param('isprivate', $cgi->param('comment_is_private'));
 
     # Must be called before create() as it may alter $cgi->param('ispatch').
@@ -201,12 +204,12 @@ if (defined($cgi->upload('data')) || $cgi->param('attachurl')) {
         $attachment = Bugzilla::Attachment->create(
             {bug           => $bug,
              creation_ts   => $timestamp,
-             data          => scalar $cgi->param('attachurl') || $cgi->upload('data'),
+             data          => $attach_url || $data_fh,
              description   => scalar $cgi->param('description'),
-             filename      => $cgi->param('attachurl') ? '' : scalar $cgi->upload('data'),
+             filename      => $attach_url ? '' : $data_fh,
              ispatch       => scalar $cgi->param('ispatch'),
              isprivate     => scalar $cgi->param('isprivate'),
-             isurl         => scalar $cgi->param('attachurl'),
+             isurl         => $attach_url,
              mimetype      => $content_type,
              store_in_file => scalar $cgi->param('bigfile'),
             });

diff --git a/query.cgi b/query.cgi
index e10a698a3bc22e8ebeecdfedd083d0bbc1cec265..405c7b70c1765e4636465f9f7ec33678617f2787 100755 (executable)
--- a/query.cgi
+++ b/query.cgi
@@ -269,8 +269,8 @@ for (my $chart = 0; $cgi->param("field$chart-0-0"); $chart++) {
             if (!defined($value)) {
                 $value = '';
             }
-            push(@cols, { field => $cgi->param("field$chart-$row-$col"),
-                          type => $cgi->param("type$chart-$row-$col") || 'noop',
+            push(@cols, { field => scalar $cgi->param("field$chart-$row-$col"),
+                          type => scalar $cgi->param("type$chart-$row-$col") || 'noop',
                           value => $value });
         }
         push(@rows, \@cols);

diff --git a/relogin.cgi b/relogin.cgi
index d31c8099327ba8d5320caa652a7ace6b5a04937b..29dd4731ad452546986e74561bf52588019fbb9c 100755 (executable)
--- 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, $[, 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, $[, 200);
+
     # Go ahead and send out the message now
     my $message;
     my $mail_template = Bugzilla->template_inner($target_user->settings->{'lang'}->{'value'});

diff --git a/template/en/default/filterexceptions.pl b/template/en/default/filterexceptions.pl
index e04e9de147800c347f424ce0bf991e3721360d06..6928e79f8372519cae6698fb18dfa737c79d4c76 100644 (file)
--- a/template/en/default/filterexceptions.pl
+++ b/template/en/default/filterexceptions.pl
@@ -197,7 +197,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 52502a1d75cdfbf392655e14e7147d0325955265..9fd92a0b7c1dca5b0b4707ec1631c59f2daa8076 100644 (file)
--- a/template/en/default/global/messages.html.tmpl
+++ b/template/en/default/global/messages.html.tmpl
@@ -846,7 +846,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 65b0df8bf3c3f48dd03784a61972447a56c3c58f..a1f0f7098f302ef9caeb4704baf5d9fee29f2321 100755 (executable)
--- a/token.cgi
+++ b/token.cgi
@@ -377,7 +377,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.