Bug 346086: [SECURITY] attachment.cgi lets you view descriptions of private attachments even when you are not in the insidergroup - Patch by Frédéric Buclin <LpSolit@gmail.com> r=myk a=justdave

8 files changed, 143 insertions, 121 deletions

diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm
index ab2562521..ebf8c8de8 100644
--- a/Bugzilla/Attachment.pm
+++ b/Bugzilla/Attachment.pm
@@ -20,6 +20,7 @@
 # Contributor(s): Terry Weissman <terry@mozilla.org>
 #                 Myk Melez <myk@mozilla.org>
 #                 Marc Schumann <wurblzap@gmail.com>
+#                 Frédéric Buclin <LpSolit@gmail.com>
 
 use strict;
 
@@ -475,7 +476,8 @@ sub _validate_data {
 
 =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.
@@ -486,10 +488,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;
 }
@@ -597,33 +611,41 @@ sub validate_content_type {
 
 =item C<validate_can_edit()>
 
-Description: validates if the user is allowed to edit the attachment.
+Description: validates if the user is allowed to view and edit the attachment.
              Only the submitter or someone with editbugs privs can edit it.
+             Only the submitter and users in the insider group can view
+             private attachments.
 
 Returns:     1 on success. Else an error is thrown.
 
 =cut
 
 sub validate_can_edit {
-    my ($class, $attach_id) = @_;
+    my $attachment = shift;
     my $dbh = Bugzilla->dbh;
     my $user = Bugzilla->user;
 
-    # People in editbugs can edit all attachments
-    return if $user->in_group('editbugs');
+    # Bug 97729 - the submitter can edit their attachments.
+    return if ($attachment->attacher->id == $user->id);
 
-    # Bug 97729 - the submitter can edit their attachments
-    my ($ref) = $dbh->selectrow_array('SELECT attach_id FROM attachments
-                                       WHERE attach_id = ? AND submitter_id = ?',
-                                       undef, ($attach_id, $user->id));
+    # Only users in the insider group can view private attachments.
+    if ($attachment->isprivate && !$user->is_insider) {
+        ThrowUserError('illegal_attachment_edit', {attach_id => $attachment->id});
+    }
 
-    $ref || ThrowUserError('illegal_attachment_edit', { attach_id => $attach_id });
+    # Users with editbugs privs can edit all attachments.
+    return if $user->in_group('editbugs');
+
+    # If we come here, then this attachment cannot be seen by the user.
+    ThrowUserError('illegal_attachment_edit', { attach_id => $attachment->id });
 }
 
 =item C<validate_obsolete($bug_id)>
 
 Description: validates if attachments the user wants to mark as obsolete
              really belong to the given bug and are not already obsolete.
+             Moreover, a user cannot mark an attachment as obsolete if
+             he cannot view it (due to restrictions on it).
 
 Params:      $bug_id - The bug ID obsolete attachments should belong to.
 
@@ -636,7 +658,8 @@ sub validate_obsolete {
     my $cgi = Bugzilla->cgi;
 
     # 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.
     my @obsolete_attachments;
     foreach my $attachid ($cgi->param('obsolete')) {
         my $vars = {};
@@ -650,6 +673,9 @@ sub validate_obsolete {
         # Make sure the attachment exists in the database.
         ThrowUserError('invalid_attach_id', $vars) unless $attachment;
 
+        # Check that the user can view and edit this attachment.
+        $attachment->validate_can_edit;
+
         $vars->{'description'} = $attachment->description;
 
         if ($attachment->bug_id != $bug_id) {
@@ -662,8 +688,6 @@ sub validate_obsolete {
           ThrowCodeError('attachment_already_obsolete', $vars);
         }
 
-        # Check that the user can modify this attachment.
-        $class->validate_can_edit($attachid);
         push(@obsolete_attachments, $attachment);
     }
     return @obsolete_attachments;
@@ -704,8 +728,10 @@ sub insert_attachment_for_bug {
     $class->validate_is_patch($throw_error) || return 0;
     $class->validate_description($throw_error) || return 0;
 
-    if (($attachurl =~ /^(http|https|ftp):\/\/\S+/)
-         && !(defined $cgi->upload('data'))) {
+    if (Bugzilla->params->{'allow_attach_url'}
+        && ($attachurl =~ /^(http|https|ftp):\/\/\S+/)
+        && !defined $cgi->upload('data'))
+    {
         $filename = '';
         $data = $attachurl;
         $isurl = 1;
@@ -729,6 +755,12 @@ sub insert_attachment_for_bug {
         $isurl = 0;
     }
 
+    # Check attachments the user tries to mark as obsolete.
+    my @obsolete_attachments;
+    if ($cgi->param('obsolete')) {
+        @obsolete_attachments = $class->validate_obsolete($bug->bug_id);
+    }
+
     # The order of these function calls is important, as Flag::validate
     # assumes User::match_field has ensured that the
     # values in the requestee fields are legitimate user email addresses.
@@ -801,12 +833,6 @@ sub insert_attachment_for_bug {
         close $fh;
     }
 
-    # Now handle flags.
-    my @obsolete_attachments;
-    if ($cgi->param('obsolete')) {
-        @obsolete_attachments = $class->validate_obsolete($bug->bug_id);
-    }
-
     # Make existing attachments obsolete.
     my $fieldid = get_field_id('attachments.isobsolete');
 
diff --git a/Bugzilla/Attachment/PatchReader.pm b/Bugzilla/Attachment/PatchReader.pm
index 8543d6e22..00623dcf2 100644
--- a/Bugzilla/Attachment/PatchReader.pm
+++ b/Bugzilla/Attachment/PatchReader.pm
@@ -20,6 +20,7 @@ use strict;
 package Bugzilla::Attachment::PatchReader;
 
 use Bugzilla::Error;
+use Bugzilla::Attachment;
 
 
 sub process_diff {
@@ -41,32 +42,28 @@ sub process_diff {
         $reader->iterate_string('Attachment ' . $attachment->id, $attachment->data);
     }
     else {
-        $vars->{'other_patches'} = [];
+        my @other_patches = ();
         if ($lc->{interdiffbin} && $lc->{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($attachment->bug_id)};
+            # 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.
-            my $attachment_list =
-                $dbh->selectall_arrayref('SELECT attach_id, description
-                                            FROM attachments
-                                           WHERE bug_id = ?
-                                             AND ispatch = 1
-                                        ORDER BY creation_ts DESC',
-                                          undef, $attachment->bug_id);
-
             my $select_next_patch = 0;
-            foreach (@$attachment_list) {
-                my ($other_id, $other_desc) = @$_;
-                if ($other_id == $attachment->id) {
+            foreach my $attach (@attachments) {
+                if ($attach->id == $attachment->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;
-                    }
+                    push(@other_patches, { 'id'       => $attach->id,
+                                           'desc'     => $attach->description,
+                                           'selected' => $select_next_patch });
+                    $select_next_patch = 0;
                 }
             }
         }
@@ -74,6 +71,7 @@ sub process_diff {
         $vars->{'bugid'} = $attachment->bug_id;
         $vars->{'attachid'} = $attachment->id;
         $vars->{'description'} = $attachment->description;
+        $vars->{'other_patches'} = \@other_patches;
 
         setup_template_patch_reader($last_reader, $format, $context, $vars);
         # Actually print out the patch.
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 19a72b9e7..02f17b85d 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -1348,6 +1348,17 @@ sub is_mover {
     return $self->{'is_mover'};
 }
 
+sub is_insider {
+    my $self = shift;
+
+    if (!defined $self->{'is_insider'}) {
+        my $insider_group = Bugzilla->params->{'insidergroup'};
+        $self->{'is_insider'} =
+            ($insider_group && $self->in_group($insider_group)) ? 1 : 0;
+    }
+    return $self->{'is_insider'};
+}
+
 sub get_userlist {
     my $self = shift;
 
@@ -1886,6 +1897,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
diff --git a/attachment.cgi b/attachment.cgi
index 70cad865c..431db444e 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -137,7 +137,8 @@ sub validateID
 {
     my $param = @_ ? $_[0] : 'id';
     my $dbh = Bugzilla->dbh;
-    
+    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.
     # Happens when calling plain attachment.cgi from the urlbar directly
@@ -158,8 +159,8 @@ sub validateID
      || ThrowUserError("invalid_attach_id", { attach_id => $cgi->param($param) });
   
     # Make sure the attachment exists in the database.
-    my ($bugid, $isprivate) = $dbh->selectrow_array(
-                                    "SELECT bug_id, isprivate 
+    my ($bugid, $isprivate, $submitter_id) = $dbh->selectrow_array(
+                                    "SELECT bug_id, isprivate, submitter_id
                                      FROM attachments 
                                      WHERE attach_id = ?",
                                      undef, $attach_id);
@@ -167,15 +168,13 @@ sub validateID
         unless $bugid;
 
     # Make sure the user is authorized to access this attachment's bug.
-
     ValidateBugID($bugid);
-    if ($isprivate && Bugzilla->params->{"insidergroup"}) {
-        Bugzilla->user->in_group(Bugzilla->params->{"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);
+    return ($attach_id, $bugid);
 }
 
 # Validates format of a diff/interdiff. Takes a list as an parameter, which
@@ -386,56 +385,27 @@ sub diff {
 
 # Display all attachments for a given bug in a series of IFRAMEs within one
 # HTML page.
-sub viewall
-{
+sub viewall {
     # Retrieve and validate parameters
     my $bugid = $cgi->param('bugid');
     ValidateBugID($bugid);
+    my $bug = new Bugzilla::Bug($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;
+    my $attachments = Bugzilla::Attachment->get_attachments_by_bug($bugid);
 
-    if ( Bugzilla->params->{"insidergroup"} 
-         && !Bugzilla->user->in_group(Bugzilla->params->{"insidergroup"}) )
-    {
-        $privacy = "AND isprivate < 1 ";
+    foreach my $a (@$attachments) {
+        $a->{'isviewable'} = isViewable($a->contenttype);
     }
-  my $attachments = $dbh->selectall_arrayref(
-           "SELECT attach_id AS attachid, " .
-            $dbh->sql_date_format('creation_ts', '%Y.%m.%d %H:%i') . " AS date,
-            mimetype AS contenttype, description, ispatch, isobsolete, isprivate, 
-            LENGTH(thedata) AS datasize
-            FROM attachments 
-            INNER JOIN attach_data
-            ON attach_id = id
-            WHERE bug_id = ? $privacy 
-            ORDER BY attach_id", {'Slice'=>{}}, $bugid);
-
-  foreach my $a (@{$attachments})
-  {
-    
-    $a->{'isviewable'} = isViewable($a->{'contenttype'});
-    $a->{'flags'} = Bugzilla::Flag::match({ 'attach_id' => $a->{'attachid'} });
-  }
 
-  # Retrieve the bug summary (for displaying on screen) and assignee.
-  my ($bugsummary, $assignee_id) = $dbh->selectrow_array(
-          "SELECT short_desc, assigned_to FROM bugs " .
-          "WHERE bug_id = ?", undef, $bugid);
+    # Define the variables and functions that will be passed to the UI template.
+    $vars->{'bug'} = $bug;
+    $vars->{'attachments'} = $attachments;
 
-  # Define the variables and functions that will be passed to the UI template.
-  $vars->{'bugid'} = $bugid;
-  $vars->{'attachments'} = $attachments;
-  $vars->{'bugassignee_id'} = $assignee_id;
-  $vars->{'bugsummary'} = $bugsummary;
-
-  print $cgi->header();
+    print $cgi->header();
 
-  # Generate and return the UI (HTML page) from the appropriate template.
-  $template->process("attachment/show-multiple.html.tmpl", $vars)
-    || ThrowTemplateError($template->error());
+    # Generate and return the UI (HTML page) from the appropriate template.
+    $template->process("attachment/show-multiple.html.tmpl", $vars)
+      || ThrowTemplateError($template->error());
 }
 
 # Display a form for entering a new attachment.
@@ -586,9 +556,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
@@ -629,19 +599,34 @@ sub edit {
 # Users cannot edit the content of the attachment itself.
 sub update
 {
-  my $userid = Bugzilla->user->id;
+    my $user = Bugzilla->user;
+    my $userid = $user->id;
+    my $dbh = Bugzilla->dbh;
 
     # Retrieve and validate parameters
     ValidateComment(scalar $cgi->param('comment'));
     my ($attach_id, $bugid) = validateID();
-    Bugzilla::Attachment->validate_can_edit($attach_id);
+    my $attachment = Bugzilla::Attachment->get($attach_id);
+    $attachment->validate_can_edit;
     validateCanChangeAttachment($attach_id);
     Bugzilla::Attachment->validate_description(THROW_ERROR);
     Bugzilla::Attachment->validate_is_patch(THROW_ERROR);
     Bugzilla::Attachment->validate_content_type(THROW_ERROR) unless $cgi->param('ispatch');
     validateIsObsolete();
     validatePrivate();
-    my $dbh = Bugzilla->dbh;
+
+    # 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 Flag::validate
     # assumes User::match_field has ensured that the values in the
@@ -688,7 +673,6 @@ sub update
   # to attachments so that we can delete pending requests if the user
   # is obsoleting this attachment without deleting any requests
   # the user submits at the same time.
-  my $attachment = Bugzilla::Attachment->get($attach_id);
   Bugzilla::Flag::process($bug, $attachment, $timestamp, $cgi);
 
   # Update the attachment record in the database.
@@ -799,10 +783,10 @@ sub delete_attachment {
 
     # Make sure the administrator is allowed to edit this attachment.
     my ($attach_id, $bug_id) = validateID();
-    Bugzilla::Attachment->validate_can_edit($attach_id);
+    my $attachment = Bugzilla::Attachment->get($attach_id);
+    $attachment->validate_can_edit;
     validateCanChangeAttachment($attach_id);
 
-    my $attachment = Bugzilla::Attachment->get($attach_id);
     $attachment->datasize || ThrowUserError('attachment_removed');
 
     # We don't want to let a malicious URL accidentally delete an attachment.
diff --git a/request.cgi b/request.cgi
index 4b2adb6b5..8d514347a 100755
--- a/request.cgi
+++ b/request.cgi
@@ -78,13 +78,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 (Bugzilla->params->{"insidergroup"} 
-        && !Bugzilla->user->in_group(Bugzilla->params->{"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.
@@ -105,7 +98,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
@@ -134,7 +127,13 @@ sub queue {
                  (bugs.assigned_to = $userid) " .
                  (Bugzilla->params->{'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)";
+    }
+
     # Limit query to pending requests.
     $query .= " AND flags.status = '?' " unless $status;
 
diff --git a/template/en/default/attachment/list.html.tmpl b/template/en/default/attachment/list.html.tmpl
index adb927e1a..a0445b16a 100644
--- a/template/en/default/attachment/list.html.tmpl
+++ b/template/en/default/attachment/list.html.tmpl
@@ -32,11 +32,10 @@
     [% END %]
     <th bgcolor="#cccccc" align="left">Actions</th>
   </tr>
-  [% canseeprivate = !Param("insidergroup") || user.in_group(Param("insidergroup")) %]
   [% count = 0 %]
   [% FOREACH attachment = attachments %]
     [% count = count + 1 %]
-    [% 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 name="a[% count %]" href="attachment.cgi?id=[% attachment.id %]">[% attachment.description FILTER html FILTER obsolete(attachment.isobsolete) %]</a>
diff --git a/template/en/default/attachment/show-multiple.html.tmpl b/template/en/default/attachment/show-multiple.html.tmpl
index ca2690c6e..ad0dfbafd 100644
--- a/template/en/default/attachment/show-multiple.html.tmpl
+++ b/template/en/default/attachment/show-multiple.html.tmpl
@@ -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">
@@ -76,20 +76,20 @@
       </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 %]" width="75%" height="350">
+    <iframe src="attachment.cgi?id=[% a.id %]" 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 %]">View the attachment on a separate page</a>.</b>
+      <a href="attachment.cgi?id=[% a.id %]">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 %]">Download the attachment instead</a>.
+      <a href="attachment.cgi?id=[% a.id %]">Download the attachment instead</a>.
     </b></p>
   [% END %]
   </div>
diff --git a/template/en/default/filterexceptions.pl b/template/en/default/filterexceptions.pl
index e80c758cd..d9a3e1913 100644
--- a/template/en/default/filterexceptions.pl
+++ b/template/en/default/filterexceptions.pl
@@ -436,7 +436,7 @@
 ],
 
 'attachment/show-multiple.html.tmpl' => [
-  'a.attachid', 
+  'a.id',
   'flag.status'
 ],