637981 - (CVE-2011-2379) [SECURITY] "Raw Unified" patch diffs can cause XSS on this domain in IE 6-8 and Safari

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Neal Poole]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13
Build Identifier: 

A valid patch file, uploaded as an attachment to a Bugzilla installation (and marked as a patch), can be served up raw from the Bugzilla installation's main domain with Content-type text/plain. In IE 6 through 8, that Content-type triggers HTML sniffing behavior: as such, a malicious patch containing HTML will be parsed when viewed in those browsers, allowing for the execution of JavaScript in the context of the Bugzilla installation.

Reproducible: Always

Steps to Reproduce:
1. Create a patch file containing HTML
2. Upload it as an attachment to an existing bug, making sure to mark it as a patch
3. Under Attachments, find your malicious patch and click on "Diff" (it's right next to "Details")
4. Up toward the top of the page, click on "Raw Unified". Notice the address bar does not switch domains, as it normally does when serving up user content.
5. If you're in IE, you should see the attachment being parsed as HTML.

Comment 1

[Neal Poole]

Attachment: An example malicious patch

Since this bug is marked as a security issue, this won't work unless you're logged in to this Bugzilla in IE.

Comment 2

[Reed Loden [:reed]]

Fixed by bug 453425 on 4.0+, no?

Comment 3

[Max Kanat-Alexander]

Oh actually, that "Raw Unified" serves from this domain is a valid security issue.

bug 453425 does prevent this from happening on IE 8 and above, but we still need to fix this bug, to protect IE 6 & 7 users.

Nice catch, Neal, thank you! :-)

Comment 4

[Max Kanat-Alexander]

I'm guessing this affects us back to around 2.18, although when we write the actual Sec. Adv., we should be more specific.

Comment 5

[Frédéric Buclin]

(In reply to comment #3)
> Oh actually, that "Raw Unified" serves from this domain is a valid security
> issue.

It's a semi-valid security issue. We explicitly display the attachment as text/plain. It's this stupid IE "feature" which converts it into text/html.

Comment 6

[Neal Poole]

(In reply to comment #5) 
> It's a semi-valid security issue. We explicitly display the attachment as
> text/plain. It's this stupid IE "feature" which converts it into text/html.

And Safari ;-)

By adding /test.html to the end of the URL path (after /attachment.cgi, before ?), Safari will perform content sniffing and parse the HTML.

Here's the modified URL for the attachment on this bug: https://bugzilla.mozilla.org/attachment.cgi/test.html?id=516130&action=diff&context=patch&collapsed=&headers=1&format=raw

Comment 7

[:glob ✱]

Attachment: patch v1

forces attachbase usage when viewing raw unified diffs and interdiffs.

Comment 8

[Reed Loden [:reed]]

(In reply to comment #6)
> (In reply to comment #5) 
> > It's a semi-valid security issue. We explicitly display the attachment as
> > text/plain. It's this stupid IE "feature" which converts it into text/html.
> 
> And Safari ;-)
> 
> By adding /test.html to the end of the URL path (after /attachment.cgi, before
> ?), Safari will perform content sniffing and parse the HTML.

All versions of Safari, or what exactly?

http://blog.8-p.info/2011/01/10/nosniff.html makes it seem like text/plain sniffing only affects versions prior to Mac OS X 10.4.4.

Does 'X-Content-Type-Options: nosniff' get sent for the raw unified patch diff page? If not, we need to send it there, as well.

Comment 9

[:glob ✱]

> Does 'X-Content-Type-Options: nosniff' get sent for the raw unified patch diff
> page? If not, we need to send it there, as well.

ah, it doesn't.  i'll update the patch.

Comment 10

[:glob ✱]

Attachment: patch v2

Comment 11

[Neal Poole]

(In reply to comment #8)
> All versions of Safari, or what exactly?
> 
> http://blog.8-p.info/2011/01/10/nosniff.html makes it seem like text/plain
> sniffing only affects versions prior to Mac OS X 10.4.4.

Sorry. The Windows version of Safari exhibited the behavior. I just verified that Safari on OS X 10.6.6 does not sniff for HTML.

Comment 12

[Neal Poole]

For anyone curious, the latest version of Safari for Windows (5.0.3 (7533.19.4)) exhibits this content sniffing behavior.

Comment 15

[Max Kanat-Alexander]

It'd be nice to see this get fixed for 4.0.2.

Comment 16

[Frédéric Buclin]

(In reply to comment #15)
> It'd be nice to see this get fixed for 4.0.2.

Yes, I will review and test it carefully once I have some time, which means late next week probably.

Comment 17

[Frédéric Buclin]

Comment on attachment 516277 [details] [diff] [review]
patch v2

>=== modified file 'attachment.cgi'

>+if (
>+    $action ne 'view' &&
>+    !(($action eq 'diff' || $action eq 'interdiff') &&
>+    $cgi->param('format') eq 'raw')
>+) {

Per our Bugzilla guideline, we never leave |if(| alone on its own line. Also, the big NOT condition is maybe not trivial to parse. What about this?

  if ($action ne 'view'
      && ($action !~ /^(?:inter)?diff$/) || $cgi->param('format') ne 'raw'))
  {

This looks simpler to me.


>+        foreach my $name (@parameters) {
>+            if (defined $cgi->param($name)) {
>+                $path .= "&$name=" . url_quote($cgi->param($name));
>+            }
>         }

IMO, we should use $cgi->url to get arguments passed with the URL, or even better: $cgi->canonicalise_query so that we can remove credentials if they are passed to the script. Unrelated arguments do not hurt, so that's fine if we pass them when redirecting.



>+sub view {
>+    my $attachment = getAttachment(undef, 'content_type');

With what I said above, the 2nd argument would be useless; same with other calls to getAttachment() (which we should rename to get_attachment()).


Otherwise, I'm fine with the logic of your patch.

Comment 18

[:glob ✱]

Attachment: patch v3

Comment 19

[:glob ✱]

Attachment: patch v4

i forgot to strip user credentials in the previous patch.

Comment 20

[Frédéric Buclin]

Comment on attachment 538843 [details] [diff] [review]
patch v4

>=== modified file 'attachment.cgi'

>+sub get_attachment
>+{

Nit: move { on the previous line.


>+    my $attachID = shift;

It should be renamed to $field_name as it doesn't get an integer, but a string.


>+        my $path = 'attachment.cgi?id=' . $attachment->id . '&'

This should be $field_name= instead of id=, and $field_name should fall back to 'id' if undefined.


>+            . $cgi->canonicalise_query('id', 'Bugzilla_login', 'Bugzilla_password');

You must also skip 't' (token), else reload a private attachment triggers an endless loop.


>         $attachment = validateID();

You must also pass $field_name here.


> sub interdiff {

>+    my $old_attachment = $format eq 'raw'
>+        ? get_attachment('oldid') : validateID('oldid');

Nit: this can be on a single line.


> sub diff {

>+    my $attachment = $format eq 'raw' 
>+        ? get_attachment() : validateID();

Same here.


This summaries what I said on IRC. Maybe I said something more on IRC and forgot to report it here. But glob already has all the information. :)

Comment 21

[:glob ✱]

Attachment: patch v5

here we go.

Comment 22

[Frédéric Buclin]

Comment on attachment 538912 [details] [diff] [review]
patch v5

>=== modified file 'attachment.cgi'

> my $action = $cgi->param('action') || 'view';

>+if ($action ne 'view'
>+    && (($action !~ /^(?:interdiff|diff)$/) || $cgi->param('format') ne 'raw'))

$cgi->param('format') is not always defined, and so we get tons of
"Use of uninitialized value in string ne at attachment.cgi line 80."

To fix the problem, please add:

 my $format = $cgi->param('format') || '';

right below "my $action", and use it in the if() condition.


>+        $path =~ s/&$//g;

Nit: /g is useless as this can happen only once.


Note that your patch makes interdiff() trigger bug 631846, but I suppose we can live with it.


r=LpSolit for 4.1.3 with the fix above (about 'format'). I haven't tested it against 4.0 and 3.6 yet (the patch applies cleanly, though).

Comment 23

[Frédéric Buclin]

Comment on attachment 538912 [details] [diff] [review]
patch v5

Hum, there is one testcase which fails with your patch:

If the interdiff is made against a private patch and a public patch in a public bug, and newid points to the private patch and oldid to the public patch, then an error is thrown when you click "Raw Unified":

 "Sorry, you are not authorized to access this attachment."

This is because get_attachment('oldid') redirects us to the alternate host, and when validateID('newid') is called, Bugzilla is unable to check that the user is in the insidergroup to let him view the private attachment.

I suppose switching both lines will fix the problem, without introducing new regressions.

Comment 24

[Frédéric Buclin]

For 3.6.6 and above, please add attach_id => $attachment->id to ThrowUserError('auth_failure') in check_can_access() to get the exact attachment ID when something goes wrong. Else it's a pain to understand what's wrong. Do not add this to 3.4.12; this version doesn't support the attachment ID in the error message.

Comment 25

[:glob ✱]

Attachment: patch v6

this revision changes get_attachment() to support getting multiple attachments via a single redirect.

Comment 26

[Frédéric Buclin]

Comment on attachment 539452 [details] [diff] [review]
patch v6

>=== modified file 'attachment.cgi'

>+if ($action ne 'view'
>+    && (($action !~ /^(?:interdiff|diff)$/) || $cgi->param('format') ne 'raw'))

As said in comment 22, $cgi->param('format') is not always defined. Please use the fix I proposed in comment 22.


>+# Gets the attachment object(s) generated by validateID, while ensuring
>+# attachbase and token authentication is used when required

Nit: add a period at the end of the sentence.


>+sub get_attachment {
>+    my @field_names = @_ ? @_ : 'id';

Nit: qw(id) instead of 'id'.


>+    foreach my $field_name (@field_names) {
>+        $attachments{$field_name} = undef;
>+    }

This code is useless. We will populate the hash once attachments are validated.


>+        # Load each attachment, and ensure they are all from the same bug
>+        my $bug_id = 0;
>+        foreach my $field_name (keys %attachments) {
>+            my $attachment = validateID($field_name, 1);
>+            if (!$bug_id) {
>+                $bug_id = $attachment->bug_id;
>+            } elsif ($attachment->bug_id != $bug_id) {
>+                $cgi->redirect_to_urlbase;
>+            }
>+            $attachments{$field_name} = $attachment;
>+        }

Redirecting to urlbase if bug IDs do not match doesn't make sense. What do we expect after the redirect? An error must be thrown instead. I would prefer something like this:

 $attachments{$_} = validateID($_, 1) foreach @field_names; 
 if (scalar(@field_names) > 1
     && $attachments{oldid}->bug_id != $attachments{newid}->id)
 {
     ThrowUserError('attachment_bug_id_mismatch');
 }

Add 'attachment_bug_id_mismatch' to global/user-error.html.tmpl if there is no equivalent yet.



>+        foreach my $field_name (sort keys %attachments) {
>+            $path .= $field_name . '=' . $attachments{$field_name}->id . '&';
>+        }

No need to sort the list. Also, please use @field_names instead of |keys %attachments|. Also, to avoid the trailing &, this should be written as:

 my @args = map { $_ . '=' . $attachments{$_}->id } @field_names;
 $path .= join('&', @args);


>+        $path =~ s/&$//g;

This is now useless with my proposal above.


>+            if (all_attachments_are_public(%attachments)) {

Pass stuff by reference, not by value, to avoid data being copied again and again.


>+                my $token = url_quote(issue_session_token(pack_token_data(%attachments)));

Same here.


>+        foreach my $field_name (keys %attachments) {

use @field_names.


>+    if (wantarray) {
>+        my @result;
>+        foreach my $field_name (@field_names) {
>+            push @result, $attachments{$field_name};
>+        }
>+        return @result;
>+    } else {
>+        return $attachments{$field_names[0]};
>+    }

This is too complex. Simply write:

  return values %attachments;


>+sub pack_token_data {
>+    my %attachments = @_;
>+    my @token_data;
>+    foreach my $field_name (keys %attachments) {
>+        push @token_data, $field_name;
>+        push @token_data, $attachments{$field_name}->id;
>+    }
>+    return join(' ', @token_data);
>+}

Simpler to write:

  return join(' ', map { $_ . '=' . $attachments{$_}->id } keys %$attachments);

(note that I prefer the foo=bar syntax)


>+sub unpack_token_data {
>+    my @token_data = split(/ /, shift);
>+    my %attachments;
>+    while (@token_data) {
>+        my $field_name = shift @token_data;
>+        my $attach_id = shift @token_data;
>+        $attachments{$field_name} = $attach_id;
>+    }
>+    return %attachments;
>+}

Do not name the hash %attachments; that's confusing (and in fact doesn't contain any attachment object). Also, it would be simpler to write:

  my @token_data = split(/ /, shift);
  my %data;
  foreach my $token (@token_data) {
      my ($field_name, $attach_id) = split('=', $token);
      $data{$field_name} = $attach_id;
  }


I haven't tested this patch.

Comment 27

[:glob ✱]

(In reply to comment #26)

> As said in comment 22, $cgi->param('format') is not always defined. Please
> use the fix I proposed in comment 22.

oops, sorry about that.
 
> Redirecting to urlbase if bug IDs do not match doesn't make sense. What do
> we expect after the redirect? An error must be thrown instead. I would
> prefer something like this:

i took this approach to mirror the existing error management which happens when an invalid token is provided.
i will add an error message.

> >+    if (wantarray) {
> >+        my @result;
> >+        foreach my $field_name (@field_names) {
> >+            push @result, $attachments{$field_name};
> >+        }
> >+        return @result;
> >+    } else {
> >+        return $attachments{$field_names[0]};
> >+    }
> 
> This is too complex. Simply write:
>   return values %attachments;

that won't work as the values need to be returned in @field_names order, and we need wantarray otherwise we'll return 1 to most calls.

i'll use:

    return wantarray
        ? map { $attachments{$_} } @field_names
        : $attachments{$field_names[0]};

Comment 28

[:glob ✱]

Attachment: patch v7

Comment 29

[Frédéric Buclin]

(In reply to comment #27)
> i took this approach to mirror the existing error management which happens
> when an invalid token is provided.

Redirecting to urlbase when you get an invalid token makes sense, because this will generate a new and valid token. But when both attachments belong to different bugs, redirecting to urlbase won't make them belong to the same bug. :)

Comment 30

[Max Kanat-Alexander]

Wait. Why are we supporting interdiffing attachments that are on different bugs?

Comment 31

[Frédéric Buclin]

(In reply to comment #30)
> Wait. Why are we supporting interdiffing attachments that are on different
> bugs?

We are not supporting that. We fail to catch this case, currently.

Comment 32

[Max Kanat-Alexander]

(In reply to comment #31)
> We are not supporting that. We fail to catch this case, currently.

 Ahh, okay. So we're unsupporting it, as it were? :-) I'm all for that.

Comment 33

[Frédéric Buclin]

Comment on attachment 540400 [details] [diff] [review]
patch v7

>=== modified file 'attachment.cgi'

>+        push @args, $cgi->canonicalise_query(@field_names, 't', 'Bugzilla_login', 'Bugzilla_password');

Only push if we have something to push, else the URL ends with a trailing &. For instance, write:

  my $cgi_params = $cgi->canonicalise_query(@field_names, 't', 'Bugzilla_login', 'Bugzilla_password');
  push(@args, $cgi_params) if $cgi_params;


>+            if (!all_attachments_are_public(\%attachments)) {
>+                my ($userid, undef, $token_data) = Bugzilla::Token::GetTokenData($token);
>+                my %token_data = unpack_token_data($token_data);

If no token is passed, unpack_token_data() complains that $token_data is undefined:

  "attachment.cgi: Use of uninitialized value in split at /var/www/html/bugzilla/attachment.cgi line 350."

That's the reason of my r-.


>+sub pack_token_data {
>+    my $attachments = shift;
>+    my $r =  join(' ', map { $_ . '=' . $attachments->{$_}->id } keys %$attachments);
>+}

Please explicitly return $r.



>=== modified file 'template/en/default/global/user-error.html.tmpl'

>+  [% ELSIF error == "attachment_bug_id_mismatch" %]
>+    [% title = "Invalid Attachments" %]
>+    You tried to perform an action on attachments from different [% terms.bugs %].
>+    This operations requires all attachments to be from the same [% terms.bug %].

"operation" is singular, not plural.


Your patch works fine in all my testcases. Fix the few problems above, and we are good to go.

Comment 34

[:glob ✱]

Attachment: patch v8

Comment 35

[:glob ✱]

Attachment: patch for 3.6.x and above, v9

Comment 36

[Frédéric Buclin]

Comment on attachment 544232 [details] [diff] [review]
patch for 3.6.x and above, v9

Works fine now. Thanks! :) r=LpSolit

Comment 37

[Frédéric Buclin]

Patch v9 works fine with 4.0.2 and 3.6.6 too. A backport is needed for 3.4.12, though.

Comment 38

[:glob ✱]

Attachment: patch for 3.4, v1

Comment 39

[Frédéric Buclin]

Comment on attachment 544413 [details] [diff] [review]
patch for 3.4, v1

>=== modified file 'attachment.cgi'

> # You must use the appropriate urlbase/sslbase param when doing anything
> # but viewing an attachment.

Please update this comment to also mention raw unified, as you did for trunk.


>+if ($action ne 'view'
>+    && (($action !~  /^(?:interdiff|diff)$/) || $format ne 'raw'))

Nit: useless extra whitespace after !~


>+      || ThrowUserError("invalid_attach_id",
>+                          { attach_id => scalar $cgi->param($param) });

Please align { vertically with "invalid_...".


>         ThrowUserError('auth_failure', {action => 'access',
>+                                        object => 'attachment',
>+                                        attach_id => $attachment->id});

As I said in comment 24, Bugzilla 3.4 doesn't take attach_id as argument. Please revert this change.


Otherwise your patch looks good and works fine. r=LpSolit with these comments addressed in an updated patch. You can carry forward my r+.

Comment 40

[:glob ✱]

Attachment: patch for 3.4, v2

updated 3.4 patch, carrying forward r+

Comment 41

[Frédéric Buclin]

Comment on attachment 547328 [details] [diff] [review]
patch for 3.4, v2

>=== modified file 'userprefs.cgi'

This file has nothing to do here!

Comment 42

[:glob ✱]

Attachment: patch for 3.4, v3

oops; removing userprefs.cgi from the patch.  carrying forward r+

Comment 43

[Daniel Veditz [:dveditz]]

Calling this one CVE-2011-2379

Comment 44

[Frédéric Buclin]

(In reply to comment #11)
> Sorry. The Windows version of Safari exhibited the behavior.

I cannot reproduce this behavior using Safari 5.1 on Windows 7.

Comment 45

[Frédéric Buclin]

(In reply to comment #4)
> I'm guessing this affects us back to around 2.18

Actually, showattachment.cgi was implemented in 1999 in Bugzilla 2.4. I'm setting the version field to "2.10" as we have nothing older.

Comment 46

[Neal Poole]

(In reply to comment #44)
> (In reply to comment #11)
> > Sorry. The Windows version of Safari exhibited the behavior.
> 
> I cannot reproduce this behavior using Safari 5.1 on Windows 7.

Apple fixed the issue in Safari 5.1, which they released recently. See CVE-2010-1420 or http://support.apple.com/kb/HT4808.

Comment 47

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified attachment.cgi
modified Bugzilla/Attachment/PatchReader.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 7890.


Committing to: bzr+ssh://[e-mail]/bugzilla/4.0/
modified attachment.cgi
modified Bugzilla/Attachment/PatchReader.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 7636.


Committing to: bzr+ssh://[e-mail]/bugzilla/3.6/
modified attachment.cgi
modified Bugzilla/Attachment/PatchReader.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 7253.


Committing to: bzr+ssh://[e-mail]/bugzilla/3.4/
modified attachment.cgi
modified Bugzilla/Attachment/PatchReader.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 6807.

Comment 48

[Max Kanat-Alexander]

Security advisory sent, unlocking this bug.

Comment 49

[Frédéric Buclin]

(In reply to Neal Poole from comment #6)
> By adding /test.html to the end of the URL path (after /attachment.cgi,
> before ?), Safari will perform content sniffing and parse the HTML.

Note that this won't be a problem on Safari anymore once bug 243764 is fixed. /test.html will be trimmed.

Comment 50

[Mario Gomes]

Comment on attachment 516130 [details] [diff] [review]
An example malicious patch

Review of attachment 516130 [details] [diff] [review]:
-----------------------------------------------------------------

"'><img src=x onerror=alert(3);>