Last Comment Bug 637981 - (CVE-2011-2379) [SECURITY] "Raw Unified" patch diffs can cause XSS on this domain in IE 6-8 and Safari
(CVE-2011-2379)
: [SECURITY] "Raw Unified" patch diffs can cause XSS on this domain in IE 6-8 a...
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Attachments & Requests (show other bugs)
: 2.10
: All All
: -- major (vote)
: Bugzilla 3.4
Assigned To: Byron Jones ‹:glob› [PTO until 2016-10-10]
: default-qa
:
Mentors:
https://landfill.bugzilla.org/bugzill...
Depends on:
Blocks: 835424 660528
  Show dependency treegraph
 
Reported: 2011-03-01 19:18 PST by Neal Poole
Modified: 2016-01-19 16:53 PST (History)
5 users (show)
LpSolit: approval+
LpSolit: blocking4.2+
LpSolit: approval4.0+
LpSolit: blocking4.0.2+
LpSolit: blocking4.0.1-
LpSolit: approval3.6+
LpSolit: blocking3.6.6+
LpSolit: approval3.4+
LpSolit: blocking3.4.12+
rforbes: sec‑bounty+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
An example malicious patch (172 bytes, patch)
2011-03-01 19:19 PST, Neal Poole
no flags Details | Diff | Splinter Review
patch v1 (3.21 KB, patch)
2011-03-02 05:36 PST, Byron Jones ‹:glob› [PTO until 2016-10-10]
no flags Details | Diff | Splinter Review
patch v2 (4.12 KB, patch)
2011-03-02 09:07 PST, Byron Jones ‹:glob› [PTO until 2016-10-10]
LpSolit: review-
Details | Diff | Splinter Review
patch v3 (3.84 KB, patch)
2011-06-13 01:39 PDT, Byron Jones ‹:glob› [PTO until 2016-10-10]
no flags Details | Diff | Splinter Review
patch v4 (3.88 KB, patch)
2011-06-13 03:11 PDT, Byron Jones ‹:glob› [PTO until 2016-10-10]
LpSolit: review-
Details | Diff | Splinter Review
patch v5 (4.10 KB, patch)
2011-06-13 08:38 PDT, Byron Jones ‹:glob› [PTO until 2016-10-10]
LpSolit: review-
Details | Diff | Splinter Review
patch v6 (8.73 KB, patch)
2011-06-15 01:08 PDT, Byron Jones ‹:glob› [PTO until 2016-10-10]
LpSolit: review-
Details | Diff | Splinter Review
patch v7 (9.14 KB, patch)
2011-06-19 23:13 PDT, Byron Jones ‹:glob› [PTO until 2016-10-10]
LpSolit: review-
Details | Diff | Splinter Review
patch v8 (9.21 KB, patch)
2011-07-05 19:19 PDT, Byron Jones ‹:glob› [PTO until 2016-10-10]
no flags Details | Diff | Splinter Review
patch for 3.6.x and above, v9 (9.91 KB, patch)
2011-07-06 07:12 PDT, Byron Jones ‹:glob› [PTO until 2016-10-10]
LpSolit: review+
Details | Diff | Splinter Review
patch for 3.4, v1 (9.94 KB, patch)
2011-07-06 23:19 PDT, Byron Jones ‹:glob› [PTO until 2016-10-10]
LpSolit: review+
Details | Diff | Splinter Review
patch for 3.4, v2 (10.17 KB, patch)
2011-07-20 23:25 PDT, Byron Jones ‹:glob› [PTO until 2016-10-10]
LpSolit: review-
Details | Diff | Splinter Review
patch for 3.4, v3 (9.71 KB, patch)
2011-07-22 00:27 PDT, Byron Jones ‹:glob› [PTO until 2016-10-10]
glob: review+
Details | Diff | Splinter Review

Description Neal Poole 2011-03-01 19:18:02 PST
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 2011-03-01 19:19:08 PST
Created attachment 516130 [details] [diff] [review]
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] (use needinfo?) 2011-03-01 19:45:06 PST
Fixed by bug 453425 on 4.0+, no?
Comment 3 Max Kanat-Alexander 2011-03-01 20:08:53 PST
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 2011-03-01 20:10:13 PST
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 2011-03-02 04:14:41 PST
(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 2011-03-02 05:30:13 PST
(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 Byron Jones ‹:glob› [PTO until 2016-10-10] 2011-03-02 05:36:22 PST
Created attachment 516240 [details] [diff] [review]
patch v1

forces attachbase usage when viewing raw unified diffs and interdiffs.
Comment 8 Reed Loden [:reed] (use needinfo?) 2011-03-02 08:59:25 PST
(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 Byron Jones ‹:glob› [PTO until 2016-10-10] 2011-03-02 09:01:32 PST
> 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 Byron Jones ‹:glob› [PTO until 2016-10-10] 2011-03-02 09:07:36 PST
Created attachment 516277 [details] [diff] [review]
patch v2
Comment 11 Neal Poole 2011-03-02 09:22:05 PST
(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 2011-03-02 09:30:15 PST
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 2011-05-27 20:33:02 PDT
It'd be nice to see this get fixed for 4.0.2.
Comment 16 Frédéric Buclin 2011-05-29 05:49:05 PDT
(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 2011-06-06 16:45:53 PDT
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 Byron Jones ‹:glob› [PTO until 2016-10-10] 2011-06-13 01:39:04 PDT
Created attachment 538828 [details] [diff] [review]
patch v3
Comment 19 Byron Jones ‹:glob› [PTO until 2016-10-10] 2011-06-13 03:11:17 PDT
Created attachment 538843 [details] [diff] [review]
patch v4

i forgot to strip user credentials in the previous patch.
Comment 20 Frédéric Buclin 2011-06-13 08:31:41 PDT
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 Byron Jones ‹:glob› [PTO until 2016-10-10] 2011-06-13 08:38:34 PDT
Created attachment 538912 [details] [diff] [review]
patch v5

here we go.
Comment 22 Frédéric Buclin 2011-06-13 14:24:22 PDT
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 2011-06-13 14:51:14 PDT
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 2011-06-13 15:01:06 PDT
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 Byron Jones ‹:glob› [PTO until 2016-10-10] 2011-06-15 01:08:11 PDT
Created attachment 539452 [details] [diff] [review]
patch v6

this revision changes get_attachment() to support getting multiple attachments via a single redirect.
Comment 26 Frédéric Buclin 2011-06-18 08:27:52 PDT
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 Byron Jones ‹:glob› [PTO until 2016-10-10] 2011-06-19 22:32:31 PDT
(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 Byron Jones ‹:glob› [PTO until 2016-10-10] 2011-06-19 23:13:52 PDT
Created attachment 540400 [details] [diff] [review]
patch v7
Comment 29 Frédéric Buclin 2011-06-20 03:09:51 PDT
(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 2011-06-20 15:33:26 PDT
Wait. Why are we supporting interdiffing attachments that are on different bugs?
Comment 31 Frédéric Buclin 2011-06-20 15:41:46 PDT
(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 2011-06-20 15:42:59 PDT
(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 2011-07-05 14:06:19 PDT
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 Byron Jones ‹:glob› [PTO until 2016-10-10] 2011-07-05 19:19:13 PDT
Created attachment 544121 [details] [diff] [review]
patch v8
Comment 35 Byron Jones ‹:glob› [PTO until 2016-10-10] 2011-07-06 07:12:59 PDT
Created attachment 544232 [details] [diff] [review]
patch for 3.6.x and above, v9
Comment 36 Frédéric Buclin 2011-07-06 07:29:16 PDT
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 2011-07-06 11:40:37 PDT
Patch v9 works fine with 4.0.2 and 3.6.6 too. A backport is needed for 3.4.12, though.
Comment 38 Byron Jones ‹:glob› [PTO until 2016-10-10] 2011-07-06 23:19:27 PDT
Created attachment 544413 [details] [diff] [review]
patch for 3.4, v1
Comment 39 Frédéric Buclin 2011-07-20 08:40:16 PDT
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 Byron Jones ‹:glob› [PTO until 2016-10-10] 2011-07-20 23:25:53 PDT
Created attachment 547328 [details] [diff] [review]
patch for 3.4, v2

updated 3.4 patch, carrying forward r+
Comment 41 Frédéric Buclin 2011-07-21 02:51:05 PDT
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 Byron Jones ‹:glob› [PTO until 2016-10-10] 2011-07-22 00:27:21 PDT
Created attachment 547623 [details] [diff] [review]
patch for 3.4, v3

oops; removing userprefs.cgi from the patch.  carrying forward r+
Comment 43 Daniel Veditz [:dveditz] 2011-08-01 16:25:25 PDT
Calling this one CVE-2011-2379
Comment 44 Frédéric Buclin 2011-08-02 02:21:39 PDT
(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 2011-08-02 02:54:12 PDT
(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 2011-08-02 05:42:03 PDT
(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 2011-08-04 13:39:52 PDT
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/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://lpsolit%40gmail.com@bzr.mozilla.org/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://lpsolit%40gmail.com@bzr.mozilla.org/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://lpsolit%40gmail.com@bzr.mozilla.org/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 2011-08-05 17:33:26 PDT
Security advisory sent, unlocking this bug.
Comment 49 Frédéric Buclin 2012-06-16 06:46:07 PDT
(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 2012-07-01 07:38:38 PDT
Comment on attachment 516130 [details] [diff] [review]
An example malicious patch

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

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

Note You need to log in before you can comment on or make changes to this bug.