aboutsummaryrefslogtreecommitdiffstats
path: root/Bugzilla
diff options
context:
space:
mode:
authorlpsolit%gmail.com <>2006-10-15 04:04:55 +0000
committerlpsolit%gmail.com <>2006-10-15 04:04:55 +0000
commit79b572263ea0dfcc1638757057825c3e6a2ee38d (patch)
tree2d373b78667d1af5e6ba588f28143229dbb2da77 /Bugzilla
parentb0ddda44bee03e94f04368dd68e8c0784de4a945 (diff)
downloadbugs-79b572263ea0dfcc1638757057825c3e6a2ee38d.tar
bugs-79b572263ea0dfcc1638757057825c3e6a2ee38d.tar.gz
bugs-79b572263ea0dfcc1638757057825c3e6a2ee38d.tar.bz2
bugs-79b572263ea0dfcc1638757057825c3e6a2ee38d.tar.xz
bugs-79b572263ea0dfcc1638757057825c3e6a2ee38d.zip
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
Diffstat (limited to 'Bugzilla')
-rw-r--r--Bugzilla/Attachment.pm76
-rw-r--r--Bugzilla/Attachment/PatchReader.pm36
-rw-r--r--Bugzilla/User.pm16
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