Skip to content

bugzilla
bugzilla
Commit 8d70890d authored by lpsolit%gmail.com's avatar lpsolit%gmail.com
Browse files
Options

Bug 38862: [SECURITY] attachments should be at a different hostname - Patch by…

Bug 38862: [SECURITY] attachments should be at a different hostname - Patch by Byron Jones <bugzilla@glob.com.au> and FrÃ:copyright:dÃ:copyright:ric Buclin <LpSolit@gmail.com> r=mkanat a=LpSolit
parent b23648ca
master dev master-old release-5.0-stable version-5.2 release-5.0.6-etersoft release-5.0.5-etersoft release-5.0.4-etersoft
Show whitespace changes
Inline Side-by-side
Showing with 161 additions and 39 deletions
Bugzilla/CGI.pm
View file @ 8d70890d
......@@ -71,6 +71,18 @@ sub new {
# Send appropriate charset
$self->charset(Bugzilla->params->{'utf8'} ? 'UTF-8' : '');
# Redirect to urlbase/sslbase if we are not viewing an attachment.
if (use_attachbase() && i_am_cgi()) {
my $cgi_file = $self->url('-path_info' => 0, '-query' => 0, '-relative' => 1);
$cgi_file =~ s/\?$//;
my $urlbase = Bugzilla->params->{'urlbase'};
my $sslbase = Bugzilla->params->{'sslbase'};
my $path_regexp = $sslbase ? qr/^(\Q$urlbase\E|\Q$sslbase\E)/ : qr/^\Q$urlbase\E/;
if ($cgi_file ne 'attachment.cgi' && $self->self_url !~ /$path_regexp/) {
$self->redirect_to_urlbase;
}
}
# Check for errors
# All of the Bugzilla code wants to do this, so do it here instead of
# in each script
......@@ -351,6 +363,14 @@ sub require_https {
exit;
}
# Redirect to the urlbase version of the current URL.
sub redirect_to_urlbase {
my $self = shift;
my $path = $self->url('-path_info' => 1, '-query' => 1, '-relative' => 1);
print $self->redirect('-location' => correct_urlbase() . $path);
exit;
}
1;
__END__
......@@ -421,6 +441,10 @@ If the client is using XMLRPC, it will not retain the QUERY_STRING since XMLRPC
It takes an optional argument which will be used as the base URL. If $baseurl
is not provided, the current URL is used.
=item C<redirect_to_urlbase>
Redirects from the current URL to one prefixed by the urlbase parameter.
=back
=head1 SEE ALSO
......
Bugzilla/Config/Attachment.pm
View file @ 8d70890d
......@@ -41,6 +41,13 @@ sub get_param_list {
my $class = shift;
my @param_list = (
{
name => 'attachment_base',
type => 't',
default => '',
checker => \&check_urlbase
},
{
name => 'allow_attachment_deletion',
type => 'b',
default => 0
......
Bugzilla/Util.pm
View file @ 8d70890d
......@@ -36,7 +36,7 @@ use base qw(Exporter);
html_quote url_quote xml_quote
css_class_quote html_light_quote url_decode
i_am_cgi get_netaddr correct_urlbase
lsearch ssl_require_redirect
lsearch ssl_require_redirect use_attachbase
diff_arrays diff_strings
trim wrap_hard wrap_comment find_wrap_point
format_time format_time_decimal validate_date
......@@ -294,6 +294,13 @@ sub correct_urlbase {
return Bugzilla->params->{'urlbase'};
}
sub use_attachbase {
my $attachbase = Bugzilla->params->{'attachment_base'};
return ($attachbase ne ''
&& $attachbase ne Bugzilla->params->{'urlbase'}
&& $attachbase ne Bugzilla->params->{'sslbase'}) ? 1 : 0;
}
sub lsearch {
my ($list,$item) = (@_);
my $count = 0;
......@@ -803,6 +810,11 @@ cookies) to only some addresses.
Returns either the C<sslbase> or C<urlbase> parameter, depending on the
current setting for the C<ssl> parameter.
=item C<use_attachbase()>
Returns true if an alternate host is used to display attachments; false
otherwise.
=back
=head2 Searching
......
attachment.cgi
View file @ 8d70890d
......@@ -27,6 +27,7 @@
# Greg Hendricks <ghendricks@novell.com>
# Frédéric Buclin <LpSolit@gmail.com>
# Marc Schumann <wurblzap@gmail.com>
# Byron Jones <bugzilla@glob.com.au>
################################################################################
# Script Initialization
......@@ -51,8 +52,6 @@ use Bugzilla::Attachment::PatchReader;
use Bugzilla::Token;
use Bugzilla::Keyword;
Bugzilla->login();
# For most scripts we don't make $cgi and $template global variables. But
# when preparing Bugzilla for mod_perl, this script used these
# variables in so many subroutines that it was easier to just
......@@ -73,12 +72,26 @@ local our $vars = {};
# Determine whether to use the action specified by the user or the default.
my $action = $cgi->param('action') || 'view';
# You must use the appropriate urlbase/sslbase param when doing anything
# but viewing an attachment.
if ($action ne 'view') {
my $urlbase = Bugzilla->params->{'urlbase'};
my $sslbase = Bugzilla->params->{'sslbase'};
my $path_regexp = $sslbase ? qr/^(\Q$urlbase\E|\Q$sslbase\E)/ : qr/^\Q$urlbase\E/;
if (use_attachbase() && $cgi->self_url !~ /$path_regexp/) {
$cgi->redirect_to_urlbase;
}
Bugzilla->login();
}
# Determine if PatchReader is installed
eval {
require PatchReader;
$vars->{'patchviewerinstalled'} = 1;
};
# When viewing an attachment, do not request credentials if we are on
# the alternate host. Let view() decide when to call Bugzilla->login.
if ($action eq "view")
{
view();
......@@ -131,7 +144,8 @@ exit;
# Validates an attachment ID. Optionally takes a parameter of a form
# variable name that contains the ID to be validated. If not specified,
# uses 'id'.
#
# If the second parameter is true, the attachment ID will be validated,
# however the current user's access to the attachment will not be checked.
# Will throw an error if 1) attachment ID is not a valid number,
# 2) attachment does not exist, or 3) user isn't allowed to access the
# attachment.
......@@ -139,8 +153,8 @@ exit;
# Returns an attachment object.
sub validateID {
my $param = @_ ? $_[0] : 'id';
my $user = Bugzilla->user;
my($param, $dont_validate_access) = @_;
$param ||= 'id';
# 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.
......@@ -164,6 +178,14 @@ sub validateID {
my $attachment = new Bugzilla::Attachment($attach_id)
|| ThrowUserError("invalid_attach_id", { attach_id => $attach_id });
return $attachment if ($dont_validate_access || check_can_access($attachment));
}
# Make sure the current user has access to the specified attachment.
sub check_can_access {
my $attachment = shift;
my $user = Bugzilla->user;
# Make sure the user is authorized to access this attachment's bug.
Bugzilla::Bug->check($attachment->bug_id);
if ($attachment->isprivate && $user->id != $attachment->attacher->id
......@@ -172,7 +194,19 @@ sub validateID {
ThrowUserError('auth_failure', {action => 'access',
object => 'attachment'});
}
return $attachment;
return 1;
}
# Determines if the attachment is public -- that is, if users who are
# not logged in have access to the attachment
sub attachmentIsPublic {
my $attachment = shift;
return 0 if Bugzilla->params->{'requirelogin'};
return 0 if $attachment->isprivate;
my $anon_user = new Bugzilla::User;
return $anon_user->can_see_bug($attachment->bug_id);
}
# Validates format of a diff/interdiff. Takes a list as an parameter, which
......@@ -223,8 +257,60 @@ sub validateCanChangeBug
# Display an attachment.
sub view {
# Retrieve and validate parameters
my $attachment = validateID();
my $attachment;
if (use_attachbase()) {
$attachment = validateID(undef, 1);
# Replace %bugid% by the ID of the bug the attachment belongs to, if present.
my $attachbase = Bugzilla->params->{'attachment_base'};
my $bug_id = $attachment->bug_id;
$attachbase =~ s/%bugid%/$bug_id/;
my $path = 'attachment.cgi?id=' . $attachment->id;
# Make sure the attachment is served from the correct server.
if ($cgi->self_url !~ /^\Q$attachbase\E/) {
# We couldn't call Bugzilla->login earlier as we first had to make sure
# we were not going to request credentials on the alternate host.
Bugzilla->login();
if (attachmentIsPublic($attachment)) {
# No need for a token; redirect to attachment base.
print $cgi->redirect(-location => $attachbase . $path);
exit;
} else {
# Make sure the user can view the attachment.
check_can_access($attachment);
# Create a token and redirect.
my $token = url_quote(issue_session_token($attachment->id));
print $cgi->redirect(-location => $attachbase . "$path&t=$token");
exit;
}
} else {
# No need to validate the token for public attachments. We cannot request
# credentials as we are on the alternate host.
if (!attachmentIsPublic($attachment)) {
my $token = $cgi->param('t');
my ($userid, undef, $token_attach_id) = Bugzilla::Token::GetTokenData($token);
unless ($userid
&& detaint_natural($token_attach_id)
&& ($token_attach_id == $attachment->id))
{
# Not a valid token.
print $cgi->redirect('-location' => correct_urlbase() . $path);
exit;
}
# Change current user without creating cookies.
Bugzilla->set_user(new Bugzilla::User($userid));
# Tokens are single use only, delete it.
delete_token($token);
}
}
} else {
# No alternate host is used. Request credentials if required.
Bugzilla->login();
$attachment = validateID();
}
# At this point, Bugzilla->login has been called if it had to.
my $contenttype = $attachment->contenttype;
my $filename = $attachment->filename;
......
template/en/default/admin/params/attachment.html.tmpl
View file @ 8d70890d
......@@ -24,6 +24,24 @@
%]
[% param_descs = {
attachment_base => "It is possible for a malicious attachment to steal your " _
"cookies or access other attachments to perform an attack " _
"on the user.<p>" _
"If you would like additional security on attachments " _
"to avoid this, set this parameter to an alternate URL " _
"for your $terms.Bugzilla that is not the same as " _
"<tt>urlbase</tt> or <tt>sslbase</tt>. That is, a different " _
"domain name that resolves to this exact same $terms.Bugzilla " _
"installation.<p>" _
"For added security, you can insert <tt>%bugid%</tt> into " _
"the URL, which will be replaced with the ID of the current " _
"$terms.bug that the attachment is on, when you access " _
"an attachment. This will limit attachments to accessing " _
"only other attachments on the same ${terms.bug}. " _
"Remember, though, that all those possible domain names " _
"(such as <tt>1234.your.domain.com</tt>) must point to " _
"this same $terms.Bugzilla instance."
allow_attachment_deletion => "If this option is on, administrators will be able to delete " _
"the content of attachments.",
......
template/en/default/attachment/edit.html.tmpl
View file @ 8d70890d
......@@ -39,6 +39,9 @@
doc_section = "attachments.html"
%]
[%# No need to display the Diff button and iframe if the attachment is not a patch. %]
[% patchviewerinstalled = (patchviewerinstalled && attachment.ispatch) %]
<script type="text/javascript">
<!--
var prev_mode = 'raw';
......@@ -47,37 +50,7 @@
var has_viewed_as_diff = 0;
function editAsComment()
{
// Get the content of the document as a string.
var viewFrame = document.getElementById('viewFrame');
var aSerializer = new XMLSerializer();
var contentDocument = viewFrame.contentDocument;
var theContent = aSerializer.serializeToString(contentDocument);
// If this is a plaintext document, remove cruft that Mozilla adds
// because it treats it as an HTML document with a big PRE section.
// http://bugzilla.mozilla.org/show_bug.cgi?id=86012
var contentType = '[% attachment.contenttype FILTER js %]';
if ( contentType == 'text/plain' )
{
theContent = theContent.replace( /^<html><head\/?><body><pre>/i , "" );
theContent = theContent.replace( /<\/pre><\/body><\/html>$/i , "" );
theContent = theContent.replace( /&lt;/gi , "<" );
theContent = theContent.replace( /&gt;/gi , ">" );
theContent = theContent.replace( /&amp;/gi , "&" );
}
// Add mail-style quote indicators (>) to the beginning of each line.
// ".*\n" matches lines that end with a newline, while ".+" matches
// the rare situation in which the last line of a file does not end
// with a newline.
theContent = theContent.replace( /(.*\n|.+)/g , ">$1" );
switchToMode('edit');
// Copy the contents of the diff into the textarea
var editFrame = document.getElementById('editFrame');
editFrame.value = theContent + "\n\n";
has_edited = 1;
}
function undoEditAsComment()
......@@ -306,6 +279,8 @@
minrows = 10
cols = 80
wrap = 'soft'
defaultcontent = (attachment.contenttype.match('^text\/')) ?
attachment.data.replace('(.*\n|.+)', '>$1') : undef
%]
<iframe id="viewFrame" src="attachment.cgi?id=[% attachment.id %]" style="height: 400px; width: 100%;">
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
......
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