Bug 346086: [SECURITY] attachment.cgi lets you view descriptions of private attachmen...

diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm
index e7b3ffe86e52885f84d758aaf28f00944e133c14..44f5f00f9b13b7f5ceff6f3dc612153bb4a802f2 100644 (file)
--- a/Bugzilla/Attachment.pm
+++ b/Bugzilla/Attachment.pm
@@ -84,9 +84,8 @@ sub query
   my @attachments = ();
   while (&::MoreSQLData()) {
     my %a;
-    my $submitter_id;
     ($a{'attachid'}, $a{'date'}, $a{'contenttype'}, $a{'description'},
-     $a{'ispatch'}, $a{'isobsolete'}, $a{'isprivate'}, $submitter_id, 
+     $a{'ispatch'}, $a{'isobsolete'}, $a{'isprivate'}, $a{'submitter_id'},
      $a{'datasize'}) = &::FetchSQLData();
 
     # Retrieve a list of flags for this attachment.
@@ -97,7 +96,7 @@ sub query
     # ie the are the submitter, or they have canedit.
     # Also show the link if the user is not logged in - in that cae,
     # They'll be prompted later
-    $a{'canedit'} = ($::userid == 0 || (($submitter_id == $::userid ||
+    $a{'canedit'} = ($::userid == 0 || (($a{'submitter_id'} == $::userid ||
                      $in_editbugs) && $caneditproduct));
     push @attachments, \%a;
   }

diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 046efbc671a2883a1e2cee4911b01d0ffb70ec65..28c351bff116a9ce93e90df05b9a305d1b111ba4 100644 (file)
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -709,6 +709,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'};
+}
+
 1;
 
 __END__
@@ -867,6 +878,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 SEE ALSO

diff --git a/attachment.cgi b/attachment.cgi
index 6b863d0ea4d514e853712584ff13491580c351ce..6adadbfba2ad553eb36f4c065ba4b2b36baec740 100755 (executable)
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -140,7 +140,16 @@ elsif ($action eq "update")
   validateContentType() unless $::FORM{'ispatch'};
   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.
+  unless (UserIsInsider()) {
+      SendSQL("SELECT isprivate FROM attachments WHERE attach_id = $::FORM{'id'}");
+      $::FORM{'isprivate'} = FetchOneColumn();
+  }
+
   # 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.
@@ -186,15 +195,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.
-    ($bugid, my $isprivate) = FetchSQLData();
+    ($bugid, my $isprivate, my $submitter_id) = FetchSQLData();
     ValidateBugID($bugid);
-    if (($isprivate > 0 ) && Param("insidergroup") && 
-        !(UserInGroup(Param("insidergroup")))) {
+    if ($isprivate
+        && (!defined Bugzilla->user || Bugzilla->user->id != $submitter_id)
+        && !UserIsInsider())
+    {
         ThrowUserError("attachment_access_denied");
     }
 
@@ -237,16 +249,26 @@ sub validateCanEdit
     # before calling this sub
     return if $::userid == 0;
 
-    # People in editbugs can edit all attachments
-    return if UserInGroup("editbugs");
+    my $dbh = Bugzilla->dbh;
+
+    my ($is_private, $submitter_id) =
+        $dbh->selectrow_array('SELECT isprivate, submitter_id
+                               FROM attachments WHERE attach_id = ?',
+                               undef, $attach_id);
 
     # Bug 97729 - the submitter can edit their attachments
-    SendSQL("SELECT attach_id FROM attachments WHERE " .
-            "attach_id = $attach_id AND submitter_id = $::userid");
+    return if (defined Bugzilla->user && $submitter_id == Bugzilla->user->id);
 
-    FetchSQLData()
-      || ThrowUserError("illegal_attachment_edit",
-                        { attach_id => $attach_id });
+    # Only people in the insider group can view private attachments.
+    if ($is_private && !UserIsInsider()) {
+        ThrowUserError('illegal_attachment_edit', {attach_id => $attach_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 => $attach_id });
 }
 
 sub validateCanChangeAttachment 
@@ -420,6 +442,9 @@ sub validateObsolete
 
     my ($bugid, $isobsolete, $description) = FetchSQLData();
 
+    # Check that the user can modify this attachment
+    validateCanEdit($attachid);
+
     $vars->{'description'} = $description;
     
     if ($bugid != $::FORM{'bugid'})
@@ -433,9 +458,6 @@ sub validateObsolete
     {
       ThrowCodeError("attachment_already_obsolete", $vars);
     }
-
-    # Check that the user can modify this attachment
-    validateCanEdit($attachid);
   }
 }
 
@@ -725,10 +747,22 @@ sub diff
   {
     $vars->{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.
       # 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 $and_isprivate = '';
+      unless (UserIsInsider()) {
+          $and_isprivate = 'AND (isprivate = 0';
+          if (defined Bugzilla->user) {
+              $and_isprivate .= ' OR submitter_id = ' . Bugzilla->user->id;
+          }
+          $and_isprivate .= ')';
+      }
+
+      SendSQL("SELECT attach_id, description FROM attachments
+               WHERE bug_id = $bugid AND ispatch = 1 $and_isprivate
+               ORDER BY creation_ts DESC");
+
       my $select_next_patch = 0;
       while (my ($other_id, $other_desc) = FetchSQLData()) {
         if ($other_id eq $::FORM{'id'}) {
@@ -757,10 +791,18 @@ sub viewall
 
   # Retrieve the attachments from the database and write them into an array
   # of hashes where each hash represents one attachment.
-    my $privacy = "";
-    if (Param("insidergroup") && !(UserInGroup(Param("insidergroup")))) {
-        $privacy = "AND isprivate < 1 ";
+
+    # By default, private attachments are not accessible, unless the user
+    # is in the insider group or submitted the attachment.
+    my $privacy = '';
+    unless (UserIsInsider()) {
+        $privacy = 'AND (isprivate = 0';
+        if (defined Bugzilla->user) {
+            $privacy .= ' OR submitter_id = ' . Bugzilla->user->id;
+        }
+        $privacy .= ')';
     }
+
     SendSQL("SELECT attach_id, DATE_FORMAT(creation_ts, '%Y.%m.%d %H:%i'),
             mimetype, description, ispatch, isobsolete, isprivate, 
             LENGTH(thedata)
@@ -986,7 +1028,18 @@ 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.
-  SendSQL("SELECT attach_id FROM attachments WHERE bug_id = $bugid ORDER BY attach_id");
+  my $and_isprivate = '';
+  unless (UserIsInsider()) {
+      $and_isprivate = 'AND (isprivate = 0';
+      if (defined Bugzilla->user) {
+          $and_isprivate .= ' OR submitter_id = ' . Bugzilla->user->id;
+      }
+      $and_isprivate .= ')';
+  }
+
+  SendSQL("SELECT attach_id FROM attachments
+           WHERE bug_id = $bugid $and_isprivate ORDER BY attach_id");
+
   my @bugattachments;
   push(@bugattachments, FetchSQLData()) while (MoreSQLData());
   SendSQL("SELECT short_desc FROM bugs WHERE bug_id = $bugid");

diff --git a/globals.pl b/globals.pl
index d403a03f8fc9a89eb9ea4b4fbd2a67cd7eda7f38..c269de19df364d7b5dfb97dcbd069cfebba57b19 100644 (file)
--- a/globals.pl
+++ b/globals.pl
@@ -1248,6 +1248,10 @@ sub UserInGroup {
     return defined Bugzilla->user && defined Bugzilla->user->groups->{$_[0]};
 }
 
+sub UserIsInsider {
+    return defined Bugzilla->user && Bugzilla->user->is_insider;
+}
+
 sub UserCanBlessGroup {
     my ($groupname) = (@_);
     PushGlobalSQLState();

diff --git a/request.cgi b/request.cgi
index 55629e20e86c84b39db3b307d187d9f83f512280..d0875cc3d54d58deaf738df8f80379ad8944af15 100755 (executable)
--- a/request.cgi
+++ b/request.cgi
@@ -59,11 +59,6 @@ sub queue {
     
     validateStatus($cgi->param('status'));
     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
@@ -92,7 +87,7 @@ sub queue {
     # and user_group_map tables to help us weed out secure bugs to which
     # the user should not have access.
     " FROM      flags 
-                LEFT JOIN attachments ON ($attach_join_clause), 
+                LEFT JOIN attachments ON flags.attach_id = attachments.attach_id,
                 flagtypes, 
                 profiles AS requesters
                 LEFT JOIN profiles AS requestees 
@@ -115,7 +110,16 @@ sub queue {
       AND       flags.setter_id     = requesters.userid
       AND       flags.bug_id        = bugs.bug_id
     ";
-    
+
+    unless (UserIsInsider()) {
+        $query .= " AND (attachments.attach_id IS NULL
+                         OR attachments.isprivate = 0";
+        if (defined Bugzilla->user) {
+            $query .= " OR attachments.submitter_id = " . Bugzilla->user->id;
+        }
+        $query .= ")";
+    }
+
     # Non-deleted flags only
     $query .= " AND flags.is_active = 1 ";
     

diff --git a/template/en/default/attachment/list.html.tmpl b/template/en/default/attachment/list.html.tmpl
index d5741d6a2cd2a5be4c110999fa946b7b1f14080e..b113cdf4036d8a4a25ca92e1041d151d690832ac 100644 (file)
--- a/template/en/default/attachment/list.html.tmpl
+++ b/template/en/default/attachment/list.html.tmpl
@@ -31,9 +31,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 || user.id == attachment.submitter_id %]
     <tr [% "class=\"bz_private\"" IF attachment.isprivate %]>
       <td valign="top">
         <a href="attachment.cgi?id=[% attachment.attachid %]">[% attachment.description FILTER html FILTER obsolete(attachment.isobsolete) %]</a>