Skip to content

bugzilla
bugzilla
Commit 79b57226 authored by lpsolit%gmail.com's avatar lpsolit%gmail.com
Browse files
Options

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

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
parent b0ddda44
Hide whitespace changes
Inline Side-by-side
Showing with 143 additions and 121 deletions
Bugzilla/Attachment.pm
View file @ 79b57226
......@@ -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');
......
Bugzilla/Attachment/PatchReader.pm
View file @ 79b57226
......@@ -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.
......
Bugzilla/User.pm
View file @ 79b57226
......@@ -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
......
attachment.cgi
View file @ 79b57226
......@@ -137,7 +137,8 @@ sub validateID
{
my $param = @_ ? $_[0] : 'id';
my $dbh = Bugzilla->dbh;
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.
# Happens when calling plain attachment.cgi from the urlbar directly
......@@ -158,8 +159,8 @@ sub validateID
|| ThrowUserError("invalid_attach_id", { attach_id => $cgi->param($param) });
# Make sure the attachment exists in the database.
my ($bugid, $isprivate) = $dbh->selectrow_array(
"SELECT bug_id, isprivate
my ($bugid, $isprivate, $submitter_id) = $dbh->selectrow_array(
"SELECT bug_id, isprivate, submitter_id
FROM attachments
WHERE attach_id = ?",
undef, $attach_id);
......@@ -167,15 +168,13 @@ sub validateID
unless $bugid;
# Make sure the user is authorized to access this attachment's bug.
ValidateBugID($bugid);
if ($isprivate && Bugzilla->params->{"insidergroup"}) {
Bugzilla->user->in_group(Bugzilla->params->{"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);
return ($attach_id, $bugid);
}
# Validates format of a diff/interdiff. Takes a list as an parameter, which
......@@ -386,56 +385,27 @@ sub diff {
# Display all attachments for a given bug in a series of IFRAMEs within one
# HTML page.
sub viewall
{
sub viewall {
# Retrieve and validate parameters
my $bugid = $cgi->param('bugid');
ValidateBugID($bugid);
my $bug = new Bugzilla::Bug($bugid);
# 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 $attachments = Bugzilla::Attachment->get_attachments_by_bug($bugid);
if ( Bugzilla->params->{"insidergroup"}
&& !Bugzilla->user->in_group(Bugzilla->params->{"insidergroup"}) )
{
$privacy = "AND isprivate < 1 ";
foreach my $a (@$attachments) {
$a->{'isviewable'} = isViewable($a->contenttype);
}
my $attachments = $dbh->selectall_arrayref(
"SELECT attach_id AS attachid, " .
$dbh->sql_date_format('creation_ts', '%Y.%m.%d %H:%i') . " AS date,
mimetype AS contenttype, description, ispatch, isobsolete, isprivate,
LENGTH(thedata) AS datasize
FROM attachments
INNER JOIN attach_data
ON attach_id = id
WHERE bug_id = ? $privacy
ORDER BY attach_id", {'Slice'=>{}}, $bugid);
foreach my $a (@{$attachments})
{
$a->{'isviewable'} = isViewable($a->{'contenttype'});
$a->{'flags'} = Bugzilla::Flag::match({ 'attach_id' => $a->{'attachid'} });
}
# Retrieve the bug summary (for displaying on screen) and assignee.
my ($bugsummary, $assignee_id) = $dbh->selectrow_array(
"SELECT short_desc, assigned_to FROM bugs " .
"WHERE bug_id = ?", undef, $bugid);
# Define the variables and functions that will be passed to the UI template.
$vars->{'bug'} = $bug;
$vars->{'attachments'} = $attachments;
# Define the variables and functions that will be passed to the UI template.
$vars->{'bugid'} = $bugid;
$vars->{'attachments'} = $attachments;
$vars->{'bugassignee_id'} = $assignee_id;
$vars->{'bugsummary'} = $bugsummary;
print $cgi->header();
print $cgi->header();
# Generate and return the UI (HTML page) from the appropriate template.
$template->process("attachment/show-multiple.html.tmpl", $vars)
|| ThrowTemplateError($template->error());
# Generate and return the UI (HTML page) from the appropriate template.
$template->process("attachment/show-multiple.html.tmpl", $vars)
|| ThrowTemplateError($template->error());
}
# Display a form for entering a new attachment.
......@@ -586,9 +556,9 @@ 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.
my $bugattachments =
$dbh->selectcol_arrayref('SELECT attach_id FROM attachments
WHERE bug_id = ? ORDER BY attach_id',
undef, $attachment->bug_id);
Bugzilla::Attachment->get_attachments_by_bug($attachment->bug_id);
# We only want attachment IDs.
@$bugattachments = map { $_->id } @$bugattachments;
my ($bugsummary, $product_id, $component_id) =
$dbh->selectrow_array('SELECT short_desc, product_id, component_id
......@@ -629,19 +599,34 @@ sub edit {
# Users cannot edit the content of the attachment itself.
sub update
{
my $userid = Bugzilla->user->id;
my $user = Bugzilla->user;
my $userid = $user->id;
my $dbh = Bugzilla->dbh;
# Retrieve and validate parameters
ValidateComment(scalar $cgi->param('comment'));
my ($attach_id, $bugid) = validateID();
Bugzilla::Attachment->validate_can_edit($attach_id);
my $attachment = Bugzilla::Attachment->get($attach_id);
$attachment->validate_can_edit;
validateCanChangeAttachment($attach_id);
Bugzilla::Attachment->validate_description(THROW_ERROR);
Bugzilla::Attachment->validate_is_patch(THROW_ERROR);
Bugzilla::Attachment->validate_content_type(THROW_ERROR) unless $cgi->param('ispatch');
validateIsObsolete();
validatePrivate();
my $dbh = Bugzilla->dbh;
# 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 Flag::validate
# assumes User::match_field has ensured that the values in the
......@@ -688,7 +673,6 @@ sub update
# to attachments so that we can delete pending requests if the user
# is obsoleting this attachment without deleting any requests
# the user submits at the same time.
my $attachment = Bugzilla::Attachment->get($attach_id);
Bugzilla::Flag::process($bug, $attachment, $timestamp, $cgi);
# Update the attachment record in the database.
......@@ -799,10 +783,10 @@ sub delete_attachment {
# Make sure the administrator is allowed to edit this attachment.
my ($attach_id, $bug_id) = validateID();
Bugzilla::Attachment->validate_can_edit($attach_id);
my $attachment = Bugzilla::Attachment->get($attach_id);
$attachment->validate_can_edit;
validateCanChangeAttachment($attach_id);
my $attachment = Bugzilla::Attachment->get($attach_id);
$attachment->datasize || ThrowUserError('attachment_removed');
# We don't want to let a malicious URL accidentally delete an attachment.
......
request.cgi
View file @ 79b57226
......@@ -78,13 +78,6 @@ sub queue {
my $status = validateStatus($cgi->param('status'));
my $form_group = validateGroup($cgi->param('group'));
my $attach_join_clause = "flags.attach_id = attachments.attach_id";
if (Bugzilla->params->{"insidergroup"}
&& !Bugzilla->user->in_group(Bugzilla->params->{"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.
......@@ -105,7 +98,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
......@@ -134,7 +127,13 @@ sub queue {
(bugs.assigned_to = $userid) " .
(Bugzilla->params->{'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 = $userid)";
}
# Limit query to pending requests.
$query .= " AND flags.status = '?' " unless $status;
......
template/en/default/attachment/list.html.tmpl
View file @ 79b57226
......@@ -32,11 +32,10 @@
[% END %]
<th bgcolor="#cccccc" align="left">Actions</th>
</tr>
[% canseeprivate = !Param("insidergroup") || user.in_group(Param("insidergroup")) %]
[% count = 0 %]
[% FOREACH attachment = attachments %]
[% count = count + 1 %]
[% IF !attachment.isprivate || canseeprivate %]
[% IF !attachment.isprivate || user.is_insider || attachment.attacher.id == user.id %]
<tr [% "class=\"bz_private\"" IF attachment.isprivate %]>
<td valign="top">
<a name="a[% count %]" href="attachment.cgi?id=[% attachment.id %]">[% attachment.description FILTER html FILTER obsolete(attachment.isobsolete) %]</a>
......
template/en/default/attachment/show-multiple.html.tmpl
View file @ 79b57226
......@@ -41,7 +41,7 @@
<table class="attachment_info" cellspacing="0" cellpadding="4" border="1" width="75%">
<tr>
<td valign="top" bgcolor="#cccccc" colspan="6">
<big><b>Attachment #[% a.attachid %]</b></big>
<big><b>Attachment #[% a.id %]</b></big>
</td>
</tr>
<tr>
......@@ -57,7 +57,7 @@
[% END %]
</td>
<td valign="top">[% a.date FILTER time %]</td>
<td valign="top">[% a.attached FILTER time %]</td>
<td valign="top">[% a.datasize FILTER unitconvert %]</td>
<td valign="top">
......@@ -76,20 +76,20 @@
</td>
<td valign="top">
<a href="attachment.cgi?id=[% a.attachid %]&amp;action=edit">Details</a>
<a href="attachment.cgi?id=[% a.id %]&amp;action=edit">Details</a>
</td>
</tr>
</table>
[% IF a.isviewable %]
<iframe src="attachment.cgi?id=[% a.attachid %]" width="75%" height="350">
<iframe src="attachment.cgi?id=[% a.id %]" width="75%" height="350">
<b>You cannot view the attachment on this page because your browser does not support IFRAMEs.
<a href="attachment.cgi?id=[% a.attachid %]">View the attachment on a separate page</a>.</b>
<a href="attachment.cgi?id=[% a.id %]">View the attachment on a separate page</a>.</b>
</iframe>
[% ELSE %]
<p><b>
Attachment cannot be viewed because its MIME type is not text/*, image/*, or application/vnd.mozilla.*.
<a href="attachment.cgi?id=[% a.attachid %]">Download the attachment instead</a>.
<a href="attachment.cgi?id=[% a.id %]">Download the attachment instead</a>.
</b></p>
[% END %]
</div>
......
template/en/default/filterexceptions.pl
View file @ 79b57226
......@@ -436,7 +436,7 @@
],
'attachment/show-multiple.html.tmpl' => [
'a.attachid',
'a.id',
'flag.status'
],
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment