The default bug view has changed. See this FAQ.
Bug 637981 (CVE-2011-2379)

[SECURITY] "Raw Unified" patch diffs can cause XSS on this domain in IE 6-8 and Safari

RESOLVED FIXED in Bugzilla 3.4

Status

()

Bugzilla
Attachments & Requests
--
major
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: Neal Poole, Assigned: glob)

Tracking

(Blocks: 1 bug)

2.10
Bugzilla 3.4
Dependency tree / graph
Bug Flags:
approval +
blocking4.2 +
approval4.0 +
blocking4.0.2 +
blocking4.0.1 -
approval3.6 +
blocking3.6.6 +
approval3.4 +
blocking3.4.12 +
sec-bounty +

Details

(URL)

Attachments

(3 attachments, 10 obsolete attachments)

172 bytes, patch
Details | Diff | Splinter Review
9.91 KB, patch
Frédéric Buclin
: review+
Details | Diff | Splinter Review
9.71 KB, patch
glob
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 years ago
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.
Fixed by bug 453425 on 4.0+, no?

Comment 3

6 years ago
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! :-)
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

6 years ago
Summary: Patch Diffs can cause XSS in IE 6-8 → "Raw Unified" patch diffs can cause XSS on this domain in IE 6-8
Target Milestone: --- → Bugzilla 3.4

Comment 4

6 years ago
I'm guessing this affects us back to around 2.18, although when we write the actual Sec. Adv., we should be more specific.
Flags: blocking4.0.1+
Flags: blocking3.6.5+
Flags: blocking3.4.11+
Version: unspecified → 2.18

Comment 5

6 years ago
(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.
(Reporter)

Comment 6

6 years ago
(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
Summary: "Raw Unified" patch diffs can cause XSS on this domain in IE 6-8 → "Raw Unified" patch diffs can cause XSS on this domain in IE 6-8 and Safari
(Assignee)

Comment 7

6 years ago
Created attachment 516240 [details] [diff] [review]
patch v1

forces attachbase usage when viewing raw unified diffs and interdiffs.
Assignee: attach-and-request → glob
Status: NEW → ASSIGNED
Attachment #516240 - Flags: review?

Updated

6 years ago
Attachment #516240 - Flags: review? → review?(LpSolit)
(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.
(Assignee)

Comment 9

6 years ago
> 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.
(Assignee)

Updated

6 years ago
Attachment #516240 - Flags: review?(LpSolit)
(Assignee)

Comment 10

6 years ago
Created attachment 516277 [details] [diff] [review]
patch v2
Attachment #516240 - Attachment is obsolete: true
Attachment #516277 - Flags: review?(LpSolit)
(Reporter)

Comment 11

6 years ago
(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.
(Reporter)

Comment 12

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

Updated

6 years ago
Flags: blocking4.2+
Flags: blocking4.0.2+
Flags: blocking4.0.1-
Flags: blocking4.0.1+

Comment 15

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

Comment 16

6 years ago
(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.

Updated

6 years ago
Blocks: 660528

Comment 17

6 years ago
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.
Attachment #516277 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 18

6 years ago
Created attachment 538828 [details] [diff] [review]
patch v3
Attachment #516277 - Attachment is obsolete: true
Attachment #538828 - Flags: review?(LpSolit)
(Assignee)

Comment 19

6 years ago
Created attachment 538843 [details] [diff] [review]
patch v4

i forgot to strip user credentials in the previous patch.
Attachment #538828 - Attachment is obsolete: true
Attachment #538828 - Flags: review?(LpSolit)
Attachment #538843 - Flags: review?(LpSolit)

Comment 20

6 years ago
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. :)
Attachment #538843 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 21

6 years ago
Created attachment 538912 [details] [diff] [review]
patch v5

here we go.
Attachment #538843 - Attachment is obsolete: true
Attachment #538912 - Flags: review?(LpSolit)

Comment 22

6 years ago
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).
Attachment #538912 - Flags: review?(LpSolit) → review+

Comment 23

6 years ago
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.
Attachment #538912 - Flags: review+ → review-

Comment 24

6 years ago
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.
Flags: blocking3.6.6+
Flags: blocking3.6.5+
Flags: blocking3.4.12+
Flags: blocking3.4.11+
(Assignee)

Comment 25

6 years ago
Created attachment 539452 [details] [diff] [review]
patch v6

this revision changes get_attachment() to support getting multiple attachments via a single redirect.
Attachment #538912 - Attachment is obsolete: true
Attachment #539452 - Flags: review?(LpSolit)

Comment 26

6 years ago
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.
Attachment #539452 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 27

6 years ago
(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]};
(Assignee)

Comment 28

6 years ago
Created attachment 540400 [details] [diff] [review]
patch v7
Attachment #539452 - Attachment is obsolete: true
Attachment #540400 - Flags: review?(LpSolit)

Comment 29

6 years ago
(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

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

Comment 31

6 years ago
(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

6 years ago
(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

6 years ago
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.
Attachment #540400 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 34

6 years ago
Created attachment 544121 [details] [diff] [review]
patch v8
Attachment #540400 - Attachment is obsolete: true
Attachment #544121 - Flags: review?(LpSolit)
(Assignee)

Comment 35

6 years ago
Created attachment 544232 [details] [diff] [review]
patch for 3.6.x and above, v9
Attachment #544121 - Attachment is obsolete: true
Attachment #544121 - Flags: review?(LpSolit)
Attachment #544232 - Flags: review?(LpSolit)

Comment 36

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

Works fine now. Thanks! :) r=LpSolit
Attachment #544232 - Flags: review?(LpSolit) → review+

Updated

6 years ago
Flags: approval?
Summary: "Raw Unified" patch diffs can cause XSS on this domain in IE 6-8 and Safari → [SECURITY] "Raw Unified" patch diffs can cause XSS on this domain in IE 6-8 and Safari

Comment 37

6 years ago
Patch v9 works fine with 4.0.2 and 3.6.6 too. A backport is needed for 3.4.12, though.
Flags: approval4.0?
Flags: approval3.6?

Updated

6 years ago
Attachment #544232 - Attachment description: patch v9 → patch for 3.6.x and above, v9
(Assignee)

Comment 38

6 years ago
Created attachment 544413 [details] [diff] [review]
patch for 3.4, v1
Attachment #544413 - Flags: review?(LpSolit)

Comment 39

6 years ago
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+.
Attachment #544413 - Flags: review?(LpSolit) → review+

Updated

6 years ago
Flags: approval3.4?
(Assignee)

Comment 40

6 years ago
Created attachment 547328 [details] [diff] [review]
patch for 3.4, v2

updated 3.4 patch, carrying forward r+
Attachment #544413 - Attachment is obsolete: true
Attachment #547328 - Flags: review+

Comment 41

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

>=== modified file 'userprefs.cgi'

This file has nothing to do here!
Attachment #547328 - Flags: review+ → review-
(Assignee)

Comment 42

6 years ago
Created attachment 547623 [details] [diff] [review]
patch for 3.4, v3

oops; removing userprefs.cgi from the patch.  carrying forward r+
Attachment #547328 - Attachment is obsolete: true
Attachment #547623 - Flags: review+
Calling this one CVE-2011-2379
Alias: CVE-2011-2379

Comment 44

6 years ago
(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

6 years ago
(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.
Version: 2.18 → 2.10
(Reporter)

Comment 46

6 years ago
(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.

Updated

6 years ago
Flags: approval?
Flags: approval4.0?
Flags: approval4.0+
Flags: approval3.6?
Flags: approval3.6+
Flags: approval3.4?
Flags: approval3.4+
Flags: approval+

Comment 47

6 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 48

6 years ago
Security advisory sent, unlocking this bug.
Group: bugzilla-security

Comment 49

5 years ago
(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 on attachment 516130 [details] [diff] [review]
An example malicious patch

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

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

Updated

4 years ago
Blocks: 835424
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.