From 79b572263ea0dfcc1638757057825c3e6a2ee38d Mon Sep 17 00:00:00 2001 From: "lpsolit%gmail.com" <> Date: Sun, 15 Oct 2006 04:04:55 +0000 Subject: 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 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Bugzilla/Attachment.pm | 76 +++++++++++++++++++++++++------------- Bugzilla/Attachment/PatchReader.pm | 36 +++++++++--------- Bugzilla/User.pm | 16 ++++++++ 3 files changed, 84 insertions(+), 44 deletions(-) (limited to 'Bugzilla') 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 # Myk Melez # Marc Schumann +# Frédéric Buclin use strict; @@ -475,7 +476,8 @@ sub _validate_data { =item C -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 -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 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 + +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 -- cgit v1.2.3-24-g4f1b