Bug 637981: (CVE-2011-2379) [SECURITY] "Raw Unified" patch diffs can cause XSS…

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();
     }

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.

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