From 8d70890dc0b7c24b25a344808ac4e63e6a5dd74e Mon Sep 17 00:00:00 2001 From: "lpsolit%gmail.com" <> Date: Mon, 2 Feb 2009 18:21:33 +0000 Subject: Bug 38862: [SECURITY] attachments should be at a different hostname - Patch by Byron Jones and Frédéric Buclin r=mkanat a=LpSolit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- attachment.cgi | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 95 insertions(+), 9 deletions(-) (limited to 'attachment.cgi') diff --git a/attachment.cgi b/attachment.cgi index 35829343f..f1753261d 100755 --- a/attachment.cgi +++ b/attachment.cgi @@ -27,6 +27,7 @@ # Greg Hendricks # Frédéric Buclin # Marc Schumann +# Byron Jones ################################################################################ # 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,13 +72,27 @@ 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; }; -if ($action eq "view") +# 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; -- cgit v1.2.3-24-g4f1b