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

3 files changed, 84 insertions, 44 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