summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorByron Jones <glob@mozilla.com>2011-08-04 22:33:28 +0200
committerFrédéric Buclin <LpSolit@gmail.com>2011-08-04 22:33:28 +0200
commit818ad5e10408f6b513ac276f575bceb082401142 (patch)
tree450f4742368ac5ff584821185d0ddbbd323d4dc8
parent10e5c4a1c297d0c7a22f866b9941ac71f70d0dd6 (diff)
downloadbugzilla-818ad5e10408f6b513ac276f575bceb082401142.tar.gz
bugzilla-818ad5e10408f6b513ac276f575bceb082401142.tar.xz
Bug 637981: (CVE-2011-2379) [SECURITY] "Raw Unified" patch diffs can cause XSS on this domain in IE 6-8 and Safari
r/a=LpSolit
-rw-r--r--Bugzilla/Attachment/PatchReader.pm2
-rwxr-xr-xattachment.cgi129
-rw-r--r--template/en/default/global/user-error.html.tmpl5
3 files changed, 106 insertions, 30 deletions
diff --git a/Bugzilla/Attachment/PatchReader.pm b/Bugzilla/Attachment/PatchReader.pm
index cfc7610f4..01a624a8f 100644
--- a/Bugzilla/Attachment/PatchReader.pm
+++ b/Bugzilla/Attachment/PatchReader.pm
@@ -37,6 +37,7 @@ sub process_diff {
$last_reader->sends_data_to(new PatchReader::DiffPrinter::raw());
# Actually print out the patch.
print $cgi->header(-type => 'text/plain',
+ -x_content_type_options => "nosniff",
-expires => '+3M');
disable_utf8();
$reader->iterate_string('Attachment ' . $attachment->id, $attachment->data);
@@ -118,6 +119,7 @@ sub process_interdiff {
$last_reader->sends_data_to(new PatchReader::DiffPrinter::raw());
# Actually print out the patch.
print $cgi->header(-type => 'text/plain',
+ -x_content_type_options => "nosniff",
-expires => '+3M');
disable_utf8();
}
diff --git a/attachment.cgi b/attachment.cgi
index f612815e2..5eba13611 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -74,10 +74,13 @@ local our $vars = {};
# Determine whether to use the action specified by the user or the default.
my $action = $cgi->param('action') || 'view';
+my $format = $cgi->param('format') || '';
# You must use the appropriate urlbase/sslbase param when doing anything
-# but viewing an attachment.
-if ($action ne 'view') {
+# but viewing an attachment, or a raw diff.
+if ($action ne 'view'
+ && (($action !~ /^(?:interdiff|diff)$/) || $format ne 'raw'))
+{
do_ssl_redirect_if_required();
if ($cgi->url_is_attachment_base) {
$cgi->redirect_to_urlbase;
@@ -167,11 +170,12 @@ sub validateID {
# non-natural, so use the original value from $cgi in our exception
# message here.
detaint_natural($attach_id)
- || ThrowUserError("invalid_attach_id", { attach_id => $cgi->param($param) });
+ || ThrowUserError("invalid_attach_id",
+ { attach_id => scalar $cgi->param($param) });
# Make sure the attachment exists in the database.
my $attachment = new Bugzilla::Attachment($attach_id)
- || ThrowUserError("invalid_attach_id", { attach_id => $attach_id });
+ || ThrowUserError("invalid_attach_id", { attach_id => $attach_id });
return $attachment if ($dont_validate_access || check_can_access($attachment));
}
@@ -187,7 +191,8 @@ sub check_can_access {
&& !$user->is_insider)
{
ThrowUserError('auth_failure', {action => 'access',
- object => 'attachment'});
+ object => 'attachment',
+ attach_id => $attachment->id});
}
return 1;
}
@@ -230,34 +235,51 @@ sub validateContext
return $context;
}
-################################################################################
-# Functions
-################################################################################
+# Gets the attachment object(s) generated by validateID, while ensuring
+# attachbase and token authentication is used when required.
+sub get_attachment {
+ my @field_names = @_ ? @_ : qw(id);
-# Display an attachment.
-sub view {
- my $attachment;
+ my %attachments;
if (use_attachbase()) {
- $attachment = validateID(undef, 1);
- my $path = 'attachment.cgi?id=' . $attachment->id;
- # The user is allowed to override the content type of the attachment.
- if (defined $cgi->param('content_type')) {
- $path .= '&content_type=' . url_quote($cgi->param('content_type'));
+ # Load each attachment, and ensure they are all from the same bug
+ my $bug_id = 0;
+ foreach my $field_name (@field_names) {
+ my $attachment = validateID($field_name, 1);
+ if (!$bug_id) {
+ $bug_id = $attachment->bug_id;
+ } elsif ($attachment->bug_id != $bug_id) {
+ ThrowUserError('attachment_bug_id_mismatch');
+ }
+ $attachments{$field_name} = $attachment;
}
+ my @args = map { $_ . '=' . $attachments{$_}->id } @field_names;
+ my $cgi_params = $cgi->canonicalise_query(@field_names, 't',
+ 'Bugzilla_login', 'Bugzilla_password');
+ push(@args, $cgi_params) if $cgi_params;
+ my $path = 'attachment.cgi?' . join('&', @args);
# Make sure the attachment is served from the correct server.
- my $bug_id = $attachment->bug_id;
if ($cgi->url_is_attachment_base($bug_id)) {
# No need to validate the token for public attachments. We cannot request
# credentials as we are on the alternate host.
- if (!attachmentIsPublic($attachment)) {
+ if (!all_attachments_are_public(\%attachments)) {
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))
- {
+ my ($userid, undef, $token_data) = Bugzilla::Token::GetTokenData($token);
+ my %token_data = unpack_token_data($token_data);
+ my $valid_token = 1;
+ foreach my $field_name (@field_names) {
+ my $token_id = $token_data{$field_name};
+ if (!$token_id
+ || !detaint_natural($token_id)
+ || $attachments{$field_name}->id != $token_id)
+ {
+ $valid_token = 0;
+ last;
+ }
+ }
+ unless ($userid && $valid_token) {
# Not a valid token.
print $cgi->redirect('-location' => correct_urlbase() . $path);
exit;
@@ -283,15 +305,17 @@ sub view {
# Replace %bugid% by the ID of the bug the attachment
# belongs to, if present.
$attachbase =~ s/\%bugid\%/$bug_id/;
- if (attachmentIsPublic($attachment)) {
+ if (all_attachments_are_public(\%attachments)) {
# 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);
+ foreach my $field_name (@field_names) {
+ check_can_access($attachments{$field_name});
+ }
# Create a token and redirect.
- my $token = url_quote(issue_session_token($attachment->id));
+ my $token = url_quote(issue_session_token(pack_token_data(\%attachments)));
print $cgi->redirect(-location => $attachbase . "$path&t=$token");
exit;
}
@@ -300,8 +324,48 @@ sub view {
do_ssl_redirect_if_required();
# No alternate host is used. Request credentials if required.
Bugzilla->login();
- $attachment = validateID();
+ foreach my $field_name (@field_names) {
+ $attachments{$field_name} = validateID($field_name);
+ }
+ }
+
+ return wantarray
+ ? map { $attachments{$_} } @field_names
+ : $attachments{$field_names[0]};
+}
+
+sub all_attachments_are_public {
+ my $attachments = shift;
+ foreach my $field_name (keys %$attachments) {
+ if (!attachmentIsPublic($attachments->{$field_name})) {
+ return 0;
+ }
+ }
+ return 1;
+}
+
+sub pack_token_data {
+ my $attachments = shift;
+ return join(' ', map { $_ . '=' . $attachments->{$_}->id } keys %$attachments);
+}
+
+sub unpack_token_data {
+ my @token_data = split(/ /, shift || '');
+ my %data;
+ foreach my $token (@token_data) {
+ my ($field_name, $attach_id) = split('=', $token);
+ $data{$field_name} = $attach_id;
}
+ return %data;
+}
+
+################################################################################
+# Functions
+################################################################################
+
+# Display an attachment.
+sub view {
+ my $attachment = get_attachment();
# At this point, Bugzilla->login has been called if it had to.
my $contenttype = $attachment->contenttype;
@@ -352,9 +416,14 @@ sub view {
sub interdiff {
# Retrieve and validate parameters
- my $old_attachment = validateID('oldid');
- my $new_attachment = validateID('newid');
my $format = validateFormat('html', 'raw');
+ my($old_attachment, $new_attachment);
+ if ($format eq 'raw') {
+ ($old_attachment, $new_attachment) = get_attachment('oldid', 'newid');
+ } else {
+ $old_attachment = validateID('oldid');
+ $new_attachment = validateID('newid');
+ }
my $context = validateContext();
Bugzilla::Attachment::PatchReader::process_interdiff(
@@ -363,8 +432,8 @@ sub interdiff {
sub diff {
# Retrieve and validate parameters
- my $attachment = validateID();
my $format = validateFormat('html', 'raw');
+ my $attachment = $format eq 'raw' ? get_attachment() : validateID();
my $context = validateContext();
# If it is not a patch, view normally.
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index 3e1b8748e..af2fc7b36 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -109,6 +109,11 @@
[% terms.Bug %] aliases cannot be longer than 20 characters.
Please choose a shorter alias.
+ [% ELSIF error == "attachment_bug_id_mismatch" %]
+ [% title = "Invalid Attachments" %]
+ You tried to perform an action on attachments from different [% terms.bugs %].
+ This operation requires all attachments to be from the same [% terms.bug %].
+
[% ELSIF error == "auth_cant_create_account" %]
[% title = "Can't create accounts" %]
This site is using an authentication scheme which does not permit