From: lpsolit%gmail.com <> Date: Sun, 15 Oct 2006 04:11:08 +0000 (+0000) Subject: Bug 346086: [SECURITY] attachment.cgi lets you view descriptions of private attachmen... X-Git-Tag: bugzilla-2.18.6~3 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=f834d5784b7d4fdbd345eb18644e500d336652d8;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 e7b3ffe86e..44f5f00f9b 100644 --- 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 046efbc671..28c351bff1 100644 --- 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 + +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 6b863d0ea4..6adadbfba2 100755 --- 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 d403a03f8f..c269de19df 100644 --- 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 55629e20e8..d0875cc3d5 100755 --- 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 d5741d6a2c..b113cdf403 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) %]