]> git.ipfire.org Git - thirdparty/bugzilla.git/commitdiff
Bug 346086: [SECURITY] attachment.cgi lets you view descriptions of private attachmen...
authorlpsolit%gmail.com <>
Sun, 15 Oct 2006 04:07:19 +0000 (04:07 +0000)
committerlpsolit%gmail.com <>
Sun, 15 Oct 2006 04:07:19 +0000 (04:07 +0000)
Bugzilla/Attachment.pm
Bugzilla/User.pm
attachment.cgi
request.cgi
template/en/default/attachment/list.html.tmpl
template/en/default/attachment/show-multiple.html.tmpl
template/en/default/filterexceptions.pl

index 8b020392438c35c70baa2d29560653bbb0f9d4cf..6cd539c9c2ea44d674353dde3d03b1c2505af8d5 100644 (file)
@@ -396,7 +396,8 @@ sub _get_local_filename {
 
 =item C<get_attachments_by_bug($bug_id)>
 
-Description: retrieves and returns the attachments for the given bug.
+Description: retrieves and returns the attachments the currently logged in
+             user can view for the given bug.
 
 Params:     C<$bug_id> - integer - the ID of the bug for which
             to retrieve and return attachments.
@@ -409,10 +410,22 @@ Returns:    a reference to an array of attachment objects.
 
 sub get_attachments_by_bug {
     my ($class, $bug_id) = @_;
-    my $attach_ids = Bugzilla->dbh->selectcol_arrayref("SELECT attach_id
-                                                        FROM attachments
-                                                        WHERE bug_id = ?",
-                                                       undef, $bug_id);
+    my $user = Bugzilla->user;
+    my $dbh = Bugzilla->dbh;
+
+    # By default, private attachments are not accessible, unless the user
+    # is in the insider group or submitted the attachment.
+    my $and_restriction = '';
+    my @values = ($bug_id);
+
+    unless ($user->is_insider) {
+        $and_restriction = 'AND (isprivate = 0 OR submitter_id = ?)';
+        push(@values, $user->id);
+    }
+
+    my $attach_ids = $dbh->selectcol_arrayref("SELECT attach_id FROM attachments
+                                               WHERE bug_id = ? $and_restriction",
+                                               undef, @values);
     my $attachments = Bugzilla::Attachment->get_list($attach_ids);
     return $attachments;
 }
index ca882f3fc3d7705f2211991feab15dc277311287..5c9ed93670ed2ad3f67fbeae52fa517f00e4d071 100644 (file)
@@ -1268,6 +1268,17 @@ sub is_mover {
     return $self->{'is_mover'};
 }
 
+sub is_insider {
+    my $self = shift;
+
+    if (!defined $self->{'is_insider'}) {
+        my $insider_group = Param('insidergroup');
+        $self->{'is_insider'} =
+            ($insider_group && $self->in_group($insider_group)) ? 1 : 0;
+    }
+    return $self->{'is_insider'};
+}
+
 sub get_userlist {
     my $self = shift;
 
@@ -1776,6 +1787,11 @@ Returns true if the user is in the list of users allowed to move bugs
 to another database. Note that this method doesn't check whether bug
 moving is enabled.
 
+=item C<is_insider>
+
+Returns true if the user can access private comments and attachments,
+i.e. if the 'insidergroup' parameter is set and the user belongs to this group.
+
 =back
 
 =head1 CLASS FUNCTIONS
index 09cde402f2492c9cb2bf2953bb4ec146ce52ebfa..fe755e84310b6c25e921f3699bf619e18a731f47 100755 (executable)
@@ -130,6 +130,7 @@ exit;
 sub validateID
 {
     my $param = @_ ? $_[0] : 'id';
+    my $user = Bugzilla->user;
 
     # If we're not doing interdiffs, check if id wasn't specified and
     # prompt them with a page that allows them to choose an attachment.
@@ -151,18 +152,18 @@ sub validateID
      || ThrowUserError("invalid_attach_id", { attach_id => $cgi->param($param) });
   
     # Make sure the attachment exists in the database.
-    SendSQL("SELECT bug_id, isprivate FROM attachments WHERE attach_id = $attach_id");
+    SendSQL("SELECT bug_id, isprivate, submitter_id
+             FROM attachments WHERE attach_id = $attach_id");
     MoreSQLData()
       || ThrowUserError("invalid_attach_id", { attach_id => $attach_id });
 
     # Make sure the user is authorized to access this attachment's bug.
-    (my $bugid, my $isprivate) = FetchSQLData();
+    my ($bugid, $isprivate, $submitter_id) = FetchSQLData();
 
     ValidateBugID($bugid);
-    if ($isprivate && Param("insidergroup")) {
-        UserInGroup(Param("insidergroup"))
-          || ThrowUserError("auth_failure", {action => "access",
-                                             object => "attachment"});
+    if ($isprivate && $user->id != $submitter_id && !$user->is_insider) {
+        ThrowUserError("auth_failure", {action => "access",
+                                        object => "attachment"});
     }
 
     return ($attach_id,$bugid);
@@ -199,17 +200,23 @@ sub validateContext
 sub validateCanEdit
 {
     my ($attach_id) = (@_);
+    my $user = Bugzilla->user;
 
-    # People in editbugs can edit all attachments
-    return if UserInGroup("editbugs");
+    my $attachment = Bugzilla::Attachment->get($attach_id);
 
     # Bug 97729 - the submitter can edit their attachments
-    SendSQL("SELECT attach_id FROM attachments WHERE " .
-            "attach_id = $attach_id AND submitter_id = " . Bugzilla->user->id);
+    return if ($attachment->attacher->id == $user->id);
 
-    FetchSQLData()
-      || ThrowUserError("illegal_attachment_edit",
-                        { attach_id => $attach_id });
+    # Only people in the insider group can view private attachments.
+    if ($attachment->isprivate && !$user->is_insider) {
+        ThrowUserError('illegal_attachment_edit', {attach_id => $attachment->id});
+    }
+
+    # People in editbugs can edit all attachments
+    return if UserInGroup("editbugs");
+
+    # If we come here, then this attachment cannot be seen by the user.
+    ThrowUserError('illegal_attachment_edit', { attach_id => $attachment->id });
 }
 
 sub validateCanChangeAttachment 
@@ -393,7 +400,8 @@ sub validateObsolete
   my @obsolete_ids = ();
 
   # Make sure the attachment id is valid and the user has permissions to view
-  # the bug to which it is attached.
+  # the bug to which it is attached. Make sure also that the user can view
+  # the attachment itself.
   foreach my $attachid ($cgi->param('obsolete')) {
     my $vars = {};
     $vars->{'attach_id'} = $attachid;
@@ -410,6 +418,9 @@ sub validateObsolete
 
     my ($bugid, $isobsolete, $description) = FetchSQLData();
 
+    # Check that the user can modify this attachment
+    validateCanEdit($attachid);
+
     $vars->{'description'} = $description;
     
     if ($bugid != $cgi->param('bugid'))
@@ -424,8 +435,6 @@ sub validateObsolete
       ThrowCodeError("attachment_already_obsolete", $vars);
     }
 
-    # Check that the user can modify this attachment
-    validateCanEdit($attachid);
     push(@obsolete_ids, $attachid);
   }
 
@@ -759,28 +768,35 @@ sub diff
   }
   else
   {
-    $vars->{other_patches} = [];
+    my @other_patches = ();
     if ($::interdiffbin && $::diffpath) {
-      # Get list of attachments on this bug.
+      # Get the list of attachments that the user can view in this bug.
+      my @attachments = @{Bugzilla::Attachment->get_attachments_by_bug($bugid)};
+      # Extract patches only.
+      @attachments = grep {$_->ispatch == 1} @attachments;
+      # We want them sorted from newer to older.
+      @attachments = sort { $b->id <=> $a->id } @attachments;
+
       # Ignore the current patch, but select the one right before it
       # chronologically.
-      SendSQL("SELECT attach_id, description FROM attachments WHERE bug_id = $bugid AND ispatch = 1 ORDER BY creation_ts DESC");
       my $select_next_patch = 0;
-      while (my ($other_id, $other_desc) = FetchSQLData()) {
-        if ($other_id eq $attach_id) {
-          $select_next_patch = 1;
-        } else {
-          push @{$vars->{other_patches}}, { id => $other_id, desc => $other_desc, selected => $select_next_patch };
-          if ($select_next_patch) {
-            $select_next_patch = 0;
+      foreach my $attach (@attachments) {
+          if ($attach->id == $attach_id) {
+              $select_next_patch = 1;
+          }
+          else {
+              push(@other_patches, { 'id'       => $attach->id,
+                                     'desc'     => $attach->description,
+                                     'selected' => $select_next_patch });
+              $select_next_patch = 0;
           }
-        }
       }
     }
 
     $vars->{bugid} = $bugid;
     $vars->{attachid} = $attach_id;
     $vars->{description} = $description;
+    $vars->{other_patches} = \@other_patches;
     setup_template_patch_reader($last_reader, $format, $context);
     # Actually print out the patch
     $reader->iterate_string("Attachment $attach_id", $thedata);
@@ -795,37 +811,10 @@ sub viewall
     my $bugid = $cgi->param('bugid');
     ValidateBugID($bugid);
 
-    # Retrieve the attachments from the database and write them into an array
-    # of hashes where each hash represents one attachment.
-    my $privacy = "";
-    my $dbh = Bugzilla->dbh;
-
-    if (Param("insidergroup") && !(UserInGroup(Param("insidergroup")))) {
-        $privacy = "AND isprivate < 1 ";
+    my $attachments = Bugzilla::Attachment->get_attachments_by_bug($bugid);
+    foreach my $a (@$attachments) {
+        $a->{'isviewable'} = isViewable($a->contenttype);
     }
-    SendSQL("SELECT attach_id, " .
-            $dbh->sql_date_format('creation_ts', '%Y.%m.%d %H:%i') . ",
-            mimetype, description, ispatch, isobsolete, isprivate, 
-            LENGTH(thedata)
-            FROM attachments 
-            INNER JOIN attach_data
-            ON attach_id = id
-            WHERE bug_id = $bugid $privacy 
-            ORDER BY attach_id");
-  my @attachments; # the attachments array
-  while (MoreSQLData())
-  {
-    my %a; # the attachment hash
-    ($a{'attachid'}, $a{'date'}, $a{'contenttype'},
-     $a{'description'}, $a{'ispatch'}, $a{'isobsolete'}, $a{'isprivate'},
-     $a{'datasize'}) = FetchSQLData();
-    $a{'isviewable'} = isViewable($a{'contenttype'});
-    $a{'flags'} = Bugzilla::Flag::match({ 'attach_id' => $a{'attachid'},
-                                          'is_active' => 1 });
-
-    # Add the hash representing the attachment to the array of attachments.
-    push @attachments, \%a;
-  }
 
   # Retrieve the bug summary (for displaying on screen) and assignee.
   SendSQL("SELECT short_desc, assigned_to FROM bugs " .
@@ -834,7 +823,7 @@ sub viewall
 
   # Define the variables and functions that will be passed to the UI template.
   $vars->{'bugid'} = $bugid;
-  $vars->{'attachments'} = \@attachments;
+  $vars->{'attachments'} = $attachments;
   $vars->{'bugassignee_id'} = $assignee_id;
   $vars->{'bugsummary'} = $bugsummary;
   $vars->{'GetBugLink'} = \&GetBugLink;
@@ -922,8 +911,9 @@ sub insert
     validateIsPatch();
     validateDescription();
   
-    if (($attachurl =~ /^(http|https|ftp):\/\/\S+/) 
-         && !(defined $cgi->upload('data'))) {
+    if (Param('allow_attach_url')
+        && ($attachurl =~ /^(http|https|ftp):\/\/\S+/)
+        && !defined $cgi->upload('data')) {
         $filename = '';
         $data = $attachurl;
         $isurl = 1;
@@ -1125,9 +1115,9 @@ sub edit {
   # Retrieve a list of attachments for this bug as well as a summary of the bug
   # to use in a navigation bar across the top of the screen.
   my $bugattachments =
-      $dbh->selectcol_arrayref('SELECT attach_id FROM attachments
-                                WHERE bug_id = ? ORDER BY attach_id',
-                                undef, $attachment->bug_id);
+      Bugzilla::Attachment->get_attachments_by_bug($attachment->bug_id);
+  # We only want attachment IDs.
+  @$bugattachments = map { $_->id } @$bugattachments;
 
   my ($bugsummary, $product_id, $component_id) =
       $dbh->selectrow_array('SELECT short_desc, product_id, component_id
@@ -1171,7 +1161,8 @@ sub edit {
 sub update
 {
   my $dbh = Bugzilla->dbh;
-  my $userid = Bugzilla->user->id;
+  my $user = Bugzilla->user;
+  my $userid = $user->id;
 
     # Retrieve and validate parameters
     ValidateComment(scalar $cgi->param('comment'));
@@ -1184,6 +1175,19 @@ sub update
     validateIsObsolete();
     validatePrivate();
 
+    # If the submitter of the attachment is not in the insidergroup,
+    # be sure that he cannot overwrite the private bit.
+    # This check must be done before calling Bugzilla::Flag*::validate(),
+    # because they will look at the private bit when checking permissions.
+    # XXX - This is a ugly hack. Ideally, we shouldn't have to look at the
+    # old private bit twice (first here, and then below again), but this is
+    # the less risky change.
+    unless ($user->is_insider) {
+        my $oldisprivate = $dbh->selectrow_array('SELECT isprivate FROM attachments
+                                                  WHERE attach_id = ?', undef, $attach_id);
+        $cgi->param('isprivate', $oldisprivate);
+    }
+
     # The order of these function calls is important, as both Flag::validate
     # and FlagType::validate assume User::match_field has ensured that the
     # values in the requestee fields are legitimate user email addresses.
index 1faeb1793381c5662d0dfe79cf888d1fd6eda081..0be7174a16a8ffd69ce49325b72c9ac6557e5f36 100755 (executable)
@@ -73,11 +73,6 @@ sub queue {
     my $status = validateStatus($cgi->param('status'));
     my $form_group = validateGroup($cgi->param('group'));
 
-    my $attach_join_clause = "flags.attach_id = attachments.attach_id";
-    if (Param("insidergroup") && !UserInGroup(Param("insidergroup"))) {
-        $attach_join_clause .= " AND attachments.isprivate < 1";
-    }
-
     my $query = 
     # Select columns describing each flag, the bug/attachment on which
     # it has been set, who set it, and of whom they are requesting it.
@@ -98,7 +93,7 @@ sub queue {
     "
       FROM           flags 
            LEFT JOIN attachments
-                  ON ($attach_join_clause)
+                  ON flags.attach_id = attachments.attach_id
           INNER JOIN flagtypes
                   ON flags.type_id = flagtypes.id
           INNER JOIN profiles AS requesters
@@ -127,7 +122,13 @@ sub queue {
                  (bugs.assigned_to = $userid) " .
                  (Param('useqacontact') ? "OR
                  (bugs.qa_contact = $userid))" : ")");
-    
+
+    unless ($user->is_insider) {
+        $query .= " AND (attachments.attach_id IS NULL
+                         OR attachments.isprivate = 0
+                         OR attachments.submitter_id = $userid)";
+    }
+
     # Non-deleted flags only
     $query .= " AND flags.is_active = 1 ";
     
index 8734487a6e1aecf1931798a06da74913012adc6c..598fbb1b16567bc0ce414d5595928f80c5a5f723 100644 (file)
@@ -32,9 +32,8 @@
     [% END %]
     <th bgcolor="#cccccc" align="left">Actions</th>
   </tr>
-  [% canseeprivate = !Param("insidergroup") || UserInGroup(Param("insidergroup")) %]
   [% FOREACH attachment = attachments %]
-    [% IF !attachment.isprivate || canseeprivate %]
+    [% IF !attachment.isprivate || user.is_insider || attachment.attacher.id == user.id %]
       <tr [% "class=\"bz_private\"" IF attachment.isprivate %]>
         <td valign="top">
           <a href="attachment.cgi?id=[% attachment.id %]">[% attachment.description FILTER html FILTER obsolete(attachment.isobsolete) %]</a>
index e2c918dbb6154448e2a3598749286ea7b71eac1d..0fa07f37494022047380cbdff0721aa19893e9a4 100644 (file)
@@ -41,7 +41,7 @@
   <table class="attachment_info" cellspacing="0" cellpadding="4" border="1" width="75%">
     <tr>
       <td valign="top" bgcolor="#cccccc" colspan="6">
-        <big><b>Attachment #[% a.attachid %]</b></big>
+        <big><b>Attachment #[% a.id %]</b></big>
       </td>
     </tr>
     <tr>
@@ -57,7 +57,7 @@
         [% END %]
       </td>
 
-      <td valign="top">[% a.date FILTER time %]</td>
+      <td valign="top">[% a.attached FILTER time %]</td>
       <td valign="top">[% a.datasize FILTER unitconvert %]</td>
 
       <td valign="top">
       </td>
 
       <td valign="top">
-        <a href="attachment.cgi?id=[% a.attachid %]&amp;action=edit">Details</a>
+        <a href="attachment.cgi?id=[% a.id %]&amp;action=edit">Details</a>
       </td>
     </tr>
   </table>
 
   [% IF a.isviewable %]
-    <iframe src="attachment.cgi?id=[% a.attachid %]&amp;action=view" width="75%" height="350">
+    <iframe src="attachment.cgi?id=[% a.id %]&amp;action=view" width="75%" height="350">
       <b>You cannot view the attachment on this page because your browser does not support IFRAMEs.
-      <a href="attachment.cgi?id=[% a.attachid %]&amp;action=view">View the attachment on a separate page</a>.</b>
+      <a href="attachment.cgi?id=[% a.id %]&amp;action=view">View the attachment on a separate page</a>.</b>
     </iframe>
   [% ELSE %]
     <p><b>
       Attachment cannot be viewed because its MIME type is not text/*, image/*, or application/vnd.mozilla.*.
-      <a href="attachment.cgi?id=[% a.attachid %]&amp;action=view">Download the attachment instead</a>.
+      <a href="attachment.cgi?id=[% a.id %]&amp;action=view">Download the attachment instead</a>.
     </b></p>
   [% END %]
   </div>
index e2123ac110710c3f227f88b02d8325c7ebb7ba4e..0440c0a1e7f0e3659d99335b516fd377a7699c07 100644 (file)
 ],
 
 'attachment/show-multiple.html.tmpl' => [
-  'a.attachid', 
+  'a.id',
   'flag.status'
 ],