From: lpsolit%gmail.com <> Date: Sun, 15 Oct 2006 04:09:10 +0000 (+0000) Subject: Bug 346086: [SECURITY] attachment.cgi lets you view descriptions of private attachmen... X-Git-Tag: bugzilla-2.20.3~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e6b16a3913681501bc22e819def4df510c474e49;p=thirdparty%2Fbugzilla.git 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 r=myk a=justdave --- diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm index 4d223d6338..bc4039a171 100644 --- a/Bugzilla/Attachment.pm +++ b/Bugzilla/Attachment.pm @@ -75,7 +75,8 @@ sub query my $list = $dbh->selectall_arrayref("SELECT attach_id, " . $dbh->sql_date_format('creation_ts', '%Y.%m.%d %H:%i') . ", mimetype, description, ispatch, - isobsolete, isprivate, LENGTH(thedata) + isobsolete, isprivate, LENGTH(thedata), + submitter_id FROM attachments WHERE bug_id = ? ORDER BY attach_id", undef, $bugid); @@ -85,7 +86,7 @@ sub query my %a; ($a{'attachid'}, $a{'date'}, $a{'contenttype'}, $a{'description'}, $a{'ispatch'}, $a{'isobsolete'}, - $a{'isprivate'}, $a{'datasize'}) = @$row; + $a{'isprivate'}, $a{'datasize'}, $a{'submitter_id'}) = @$row; # Retrieve a list of flags for this attachment. $a{'flags'} = Bugzilla::Flag::match({ 'attach_id' => $a{'attachid'}, diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm index f28669bfe5..384d873121 100644 --- a/Bugzilla/User.pm +++ b/Bugzilla/User.pm @@ -1147,6 +1147,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; @@ -1603,6 +1614,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 + +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 64bec6e5ae..4863e56be9 100755 --- a/attachment.cgi +++ b/attachment.cgi @@ -128,6 +128,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. @@ -149,18 +150,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); @@ -197,17 +198,27 @@ sub validateContext sub validateCanEdit { my ($attach_id) = (@_); + my $dbh = Bugzilla->dbh; + my $user = Bugzilla->user; - # People in editbugs can edit all attachments - return if UserInGroup("editbugs"); + 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 = " . Bugzilla->user->id); + return if ($submitter_id == $user->id); - FetchSQLData() - || ThrowUserError("illegal_attachment_edit", - { attach_id => $attach_id }); + # Only people in the insider group can view private attachments. + if ($is_private && !$user->is_insider) { + 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 @@ -393,6 +404,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')) @@ -407,8 +421,6 @@ sub validateObsolete ThrowCodeError("attachment_already_obsolete", $vars); } - # Check that the user can modify this attachment - validateCanEdit($attachid); push(@obsolete_ids, $attachid); } @@ -708,6 +720,7 @@ sub diff { # Retrieve and validate parameters my ($attach_id) = validateID(); + my $user = Bugzilla->user; my $format = validateFormat('html', 'raw'); my $context = validateContext(); @@ -739,10 +752,17 @@ 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 ($user->is_insider) { + $and_isprivate = 'AND (isprivate = 0 OR submitter_id = ' . $user->id . ')'; + } + 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 $attach_id) { @@ -775,11 +795,14 @@ sub viewall # 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 $user = Bugzilla->user; - 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 ($user->is_insider) { + $privacy = 'AND (isprivate = 0 OR submitter_id = ' . $user->id . ')'; } SendSQL("SELECT attach_id, " . $dbh->sql_date_format('creation_ts', '%Y.%m.%d %H:%i') . ", @@ -1072,6 +1095,7 @@ sub edit { # Retrieve and validate parameters my ($attach_id) = validateID(); + my $user = Bugzilla->user; # Retrieve the attachment from the database. SendSQL("SELECT description, mimetype, filename, bug_id, ispatch, isobsolete, isprivate, LENGTH(thedata) @@ -1082,7 +1106,12 @@ 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 ($user->is_insider) { + $and_isprivate = 'AND (isprivate = 0 OR submitter_id = ' . $user->id . ')'; + } + 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"); @@ -1136,8 +1165,9 @@ sub edit # Users cannot edit the content of the attachment itself. sub update { - my $dbh = Bugzilla->dbh; - my $userid = Bugzilla->user->id; + my $dbh = Bugzilla->dbh; + my $user = Bugzilla->user; + my $userid = $user->id; # Retrieve and validate parameters ValidateComment(scalar $cgi->param('comment')); @@ -1150,6 +1180,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. diff --git a/request.cgi b/request.cgi index d91846488d..9d448cfb99 100755 --- a/request.cgi +++ b/request.cgi @@ -57,15 +57,11 @@ exit; sub queue { my $cgi = Bugzilla->cgi; my $dbh = Bugzilla->dbh; + my $user = Bugzilla->user; 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. @@ -86,7 +82,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 @@ -102,10 +98,10 @@ sub queue { LEFT JOIN bug_group_map AS bgmap ON bgmap.bug_id = bugs.bug_id AND bgmap.group_id NOT IN (" . - join(', ', (-1, values(%{Bugzilla->user->groups}))) . ") + join(', ', (-1, values(%{$user->groups}))) . ") LEFT JOIN cc AS ccmap ON ccmap.who = $::userid - AND ccmap.bug_id = bugs.bug_id + AND ccmap.bug_id = bugs.bug_id " . # Weed out bug the user does not have access to @@ -115,7 +111,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 = " . $user->id . ")"; + } + # 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 8f6bbadb1d..8ac3a25584 100644 --- a/template/en/default/attachment/list.html.tmpl +++ b/template/en/default/attachment/list.html.tmpl @@ -31,9 +31,8 @@ [% END %] Actions - [% canseeprivate = !Param("insidergroup") || UserInGroup(Param("insidergroup")) %] [% FOREACH attachment = attachments %] - [% IF !attachment.isprivate || canseeprivate %] + [% IF !attachment.isprivate || user.is_insider || user.id == attachment.submitter_id %] [% attachment.description FILTER html FILTER obsolete(attachment.isobsolete) %]