1232785 - (CVE-2015-8509) [SECURITY] Buglists in CSV format can be parsed as valid javascript in some browsers

Status: RESOLVED FIXED

(Bugzilla :: Attachments & Requests, defect)

Description

[Mario Gomes]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/47.0.2526.80 Safari/537.36

Steps to reproduce:

Hey,

I'll explain it in the next comment.

Cheers,

Comment 1

[Mario Gomes]

Attachment: poc.html

Comment 2

[Mario Gomes]

This PoC works only on Firefox.

Steps to reproduce(firefox only):
1. Go to https://bugzilla-dev.allizom.org and log in using my persona account
Log in: [e-mail]
Log in: 1111111111
2. Once you're logged to my bugzilla-dev testing account, visit https://bugzilla.mozilla.org/attachment.cgi?id=8698593 
3. Wait until page finishes loading.
4. See my fake security bug informations(status, title, and date).


This vulnerability occurs due to the content of a CSV isn't properly sanitized, allowing me to break CSV files syntax, and make it "parseable" as javascript then I can steal victim's bugs informations.

In order to make it I have to be able to change the title of the first and the last bug listed on victim's "My bugs" page(let's supposed i'm CCed to them, then i can easily exploit this vulnerability).

Cheers,
Mario

Comment 3

[Mario Gomes]

Just to clarify, my personna login info are:
Log in: [e-mail]
Password: 1111111111

Comment 4

[Frédéric Buclin]

(In reply to Mario Gomes"'><img src=x onerror=prompt();> from comment #2)
> In order to make it I have to be able to change the title of the first and
> the last bug listed

Change how? I tested against landfill, and nothing is displayed.

Comment 5

[Dylan Hardison [:dylan] (he/him)]

so firefox will interpret text/plain as js if it is the target of a src= tag? Does it even matter if the attachment is served by BMO or somewhere else?

Comment 6

[Dylan Hardison [:dylan] (he/him)]

This works from any page context, in firefox only as reported. Chrome reports this error:

"Refused to execute script from 'https://bugzilla-dev.allizom.org/buglist.cgi?bug_status=UNCONFIRMED&bug_sta…value0-0-0=UNCONFIRMED&value0-0-1=netfuzzer%40yandex.com&ctype=csv&human=1' because its MIME type ('text/csv') is not executable, and strict MIME type checking is enabled."

Comment 7

[Mario Gomes]

(In reply to Frédéric Buclin from comment #4)
> Change how? I tested against landfill, and nothing is displayed.

In order to reproduce on your own,  you have to add:  \"; data = `
to the title of the first bug listed, also change the title of the last bug to: `;

Then copy the URL in the "CSV"(on "My bugs page") to the "SRC" attribute on the "poc.html".

(In reply to Dylan William Hardison [:dylan] from comment #5)
> so firefox will interpret text/plain as js if it is the target of a src=
> tag? Does it even matter if the attachment is served by BMO or somewhere
> else?

Yep - pretty much that. In fact, on my tests Firefox parses whatever comes up regardlessly the "content-type".  

No, it doesn't matter. It'll work the same way. A good idea por a patching this would be required a security token to display buglist CSV.

Comment 8

[Dylan Hardison [:dylan] (he/him)]

I confirmed with :ehsan that there is no client side fix for this at the moment, I will proceed with Frédéric Buclin proposed solution.

Comment 9

[(no longer active)]

We need bug 471020, with that, we could send X-Content-Type-Options: nosniff and fix this on the client side.

At the lack of that, we should make what we send unparseable on the server side.

Comment 10

[Frédéric Buclin]

(In reply to :Ehsan Akhgari from comment #9)
> We need bug 471020, with that, we could send X-Content-Type-Options: nosniff
> and fix this on the client side.

Since when does Firefox do content type sniffing?? AFAIK, only IE behaves this way.

Comment 11

[Mario Gomes]

Great! Could this be eligible for a bounty? also, what would be appropriate sec-rate for this bug? 

Possible attack would vector be:
======================
To exploit it attacker would have to control the FIRST and LAST shown on the page "My bugs"(which can be done by CC him to some old bug, and a recently created bug), after that, he would have to convince the victim to visit a specially crafted page exploiting this vulnerability and stealing all listed bugs information(report date, title and status, reporter).
======================

Comment 12

[Dylan Hardison [:dylan] (he/him)]

Attachment: csv that is also javascript

Comment 13

[Frédéric Buclin]

I can now reproduce the issue. The CSV format for buglists has been added in 2.17.1, see bug 177430.

Comment 14

[Frédéric Buclin]

Actually, from what I can read, this is exactly a duplicate of bug 471020 and/or bug 1048535. See these comments:
  bug 471020 comment 69 - bz says the problem has nothing to do with the content type. If the remote file looks like a JS file, it's interpreted as a JS file.
  bug 471020 comment 41 - Jann already describes the problem reported here.
  bug 1048535 comment 46 - Jeff also confirms that the only way to fix this problem is to reject external files which do not have the correct content type.

CCing bz and jruderman for some helpful input: wait for bug 471020 to be fixed, i.e. Fx should only accept remote scripts with the correct content type?

Comment 15

[Gervase Markham [:gerv]]

How can we fix this in Bugzilla? The CSV output is valid CSV; I don't think it's broken in any way, is it? The problem is Firefox loading it cross-site, which we can't fix.

Gerv

Comment 16

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #15)
> How can we fix this in Bugzilla? The CSV output is valid CSV; I don't think
> it's broken in any way, is it?

The CSV file is not broken. But Firefox recognizes some JS paths in it due to some strings present in the bug summaries and so it thinks it's a JS script and executes it.

Comment 17

[Mario Gomes]

(In reply to Gervase Markham [:gerv] from comment #15)
> How can we fix this in Bugzilla? 

As I suggested on comment 7, you could require a CSRF Security-Token for displaying buglist.cgi. By doing so, there wouldnt be possible to see it directly.

Comment 18

[Frédéric Buclin]

(In reply to Mario Gomes from comment #17)
> As I suggested on comment 7, you could require a CSRF Security-Token for
> displaying buglist.cgi. By doing so, there wouldnt be possible to see it
> directly.

That's exactly why it's not a good idea. You cannot bookmark CSV buglists anymore, you cannot call them directly either from 3rd-party applications.

Comment 19

[Mario Gomes]

(In reply to Frédéric Buclin from comment #18)
> That's exactly why it's not a good idea. You cannot bookmark CSV buglists
> anymore, you cannot call them directly either from 3rd-party applications.

OOh - I see now. Thought CSV were only supposed to be seen through "My bugs" pages. 

Did some research and it turns out that CSV has a attribute to indicate the "separator char"
Example: https://pt.wikipedia.org/wiki/Comma-separated_values
============
sep=,
1997,Ford,E350,"ac, abs, moon",30100.00
1999,Chevy,"Venture ""Extended Edition""",,49000.00
1996,Jeep,Grand Cherokee,"MUST SELL!
air, moon roof, loaded",479699.00
============

This could be a good patch, add this "sep=," to the beginning of the file, by doing so it would still be parsed as a CSV, but wouldn't work as javascript anymore.

Comment 20

[Frédéric Buclin]

(In reply to Mario Gomes from comment #19)
> This could be a good patch, add this "sep=," to the beginning of the file,
> by doing so it would still be parsed as a CSV, but wouldn't work as
> javascript anymore.

Why wouldn't it be seen as a JS script anymore?

Comment 21

[Mario Gomes]

(In reply to Frédéric Buclin from comment #20)
> (In reply to Mario Gomes from comment #19) 
> Why wouldn't it be seen as a JS script anymore?

Because "sep=," will throw a grammar error forcing Firefox to get stucker on it and not parse the rest of the file.

Comment 22

[Gervase Markham [:gerv]]

(In reply to Mario Gomes from comment #19)
> This could be a good patch, add this "sep=," to the beginning of the file,
> by doing so it would still be parsed as a CSV, but wouldn't work as
> javascript anymore.

Trouble is, that isn't in the RFC: https://tools.ietf.org/html/rfc4180 . It's an Excel-ism. Many implementations will expect the headers on the first line. So adding this will break stuff. I don't see a way to mangle the format without also breaking something, as the header lines may well be being matched by software to know which column is which.

We can't use CORS either, AFAICS. :-|

Gerv

Comment 23

[(no longer active)]

scripts from <script> tags are loaded in no-cors mode, so CORS is irrelevant...

Can we not include security sensitive bugs in CSV exports?

Comment 24

[Mario Gomes]

thebigbangwolf(In reply to Gervase Markham [:gerv] from comment #22)
> Trouble is, that isn't in the RFC: https://tools.ietf.org/html/rfc4180 .
> It's an Excel-ism. Many implementations will expect the headers on the first
> line. So adding this will break stuff. I don't see a way to mangle the
> format without also breaking something, as the header lines may well be
> being matched by software to know which column is which.

Good point. There's another way though: you could add one a "," to the add of the first line for the file.
=====================
"Bug ID","Product","Component","Assignee","Status","Resolution","Summary","Changed",,
=====================
Adding double comma to the end of the first line will prevent it from being parsed as JS(as it would cause a javascript grammar error). It'll stop any javascript parser and will still being a valid CSV.

Comment 25

[Mario Gomes]

Good point. There's another way though: you could add double comma(",,") to the end of the first line of the file.
=====================
"Bug ID","Product","Component","Assignee","Status","Resolution","Summary","Changed",,
=====================
Adding double comma to the end of the first line will prevent it from being parsed as JS(as it would cause a javascript grammar error). It'll stop any javascript parser and will still being a valid CSV.

Comment 26

[Gervase Markham [:gerv]]

That is an idea we could consider. It could be that some implementations then find an empty unlabelled column, but this seems much less impactful than the other options considered. LpSolit: views?

Gerv

Comment 27

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #26)
> That is an idea we could consider. It could be that some implementations
> then find an empty unlabelled column, but this seems much less impactful
> than the other options considered. LpSolit: views?

RFC 4180 says 2 things about this:

- There maybe an optional header line appearing as the first line
  of the file with the same format as normal record lines.  This
  header will contain names corresponding to the fields in the file
  and *should contain the same number of fields as the records in
  the rest of the file*

- Within the header and each record, there may be one or more
  fields, separated by commas.  Each line should contain the same
  number of fields throughout the file.  Spaces are considered part
  of a field and should not be ignored.  *The last field in the
  record must not be followed by a comma.*

So ending the first line with a double comma would be in contradiction with both items.


re comment 23: excluding non-public bugs from the CSV file would defeat the usefulness of the file. This is especially true if the Bugzilla installation requires login to access it. In that case, no bugs would be public and the CSV file would be empty. But this would be the safest approach, and wouldn't break the file format as specified by RFC 4180.


I wonder if we couldn't use a redirect to get the CSV file. Browsers are fine with redirects, and curl and wget too AFAIK. And maybe Firefox wouldn't follow the redirection into <script>, I don't know.

Comment 28

[Mario Gomes]

(In reply to Frédéric Buclin from comment #27)
> I wonder if we couldn't use a redirect to get the CSV file. Browsers are
> fine with redirects, and curl and wget too AFAIK. And maybe Firefox wouldn't
> follow the redirection into <script>, I don't know.

Unfortunately, Firefox(and any other browser) will follow redirect to get the JS content.

Comment 29

[Gervase Markham [:gerv]]

(In reply to Frédéric Buclin from comment #27) 
> So ending the first line with a double comma would be in contradiction with
> both items.

I agree. However, in absence of an idea to do it which does fit the file format, this seems like the least impactful option.

Gerv

Comment 30

[Dylan Hardison [:dylan] (he/him)]

Another option is manipulating the output to disallow ES6 quoting seems to work as well.

Comment 31

[Gervase Markham [:gerv]]

As in, switch the CSV library to only quote values which need quoting? Yes, that's an excellent idea. That should surely stop the parser in its tracks somewhere on line 1.

Gerv

Comment 32

[Dylan Hardison [:dylan] (he/him)]

No, but that might work as well. I meant physically remove the ` from bug summaries.

Comment 33

[Mario Gomes]

(In reply to Dylan William Hardison [:dylan] from comment #32)
> No, but that might work as well. I meant physically remove the ` from bug
> summaries.

Removing the: "`" won't fix this as the main problem here is the quotes("), remember it's parsed as javascript. I use \" to finish the string then I set the variable "data" stealing victim's info.

So, the better way for patching it would be remove quotes(") from the CSV.

Only removing ` won't work 'cause I can still set "data" as an array instead of a string.

Comment 34

[Gervase Markham [:gerv]]

Attachment: Patch v.1

How's this? RFC is here: https://tools.ietf.org/html/rfc4180 .

Gerv

Comment 35

[Frédéric Buclin]

Comment on attachment 8699452 [details] [diff] [review]
Patch v.1

>+                if ($var =~ /[\r\n,"]/) {

You must include semicolons too, because they are also used as field separator (there is a user pref to control which field separator you want).

Comment 36

[Dylan Hardison [:dylan] (he/him)]

can the list of fields be controlled by the requester? if that's the case, if you pick fields that are single words, you can still make it valid JS I think?

Comment 37

[Gervase Markham [:gerv]]

Attachment: Patch v.1

With semicolon.

Gerv

Comment 38

[Gervase Markham [:gerv]]

A string of random undeclared comma-separated identifiers is valid JS?

<sigh>

Yes, I believe the requester has control of the field list.

gerv

Comment 39

[Dylan Hardison [:dylan] (he/him)]

The exploit author gets to set any variable they want before making the <script> request. But it appears "Bug ID" is always first.

Comment 40

[Dylan Hardison [:dylan] (he/him)]

However, if "Bug ID" is ever valid js syntax this would be a time bomb...

Comment 41

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #38)
> Yes, I believe the requester has control of the field list.

Yes, he has. You can pass &columnlist=... in the URL bar.

Comment 42

[Dylan Hardison [:dylan] (he/him)]

Attachment: non-human buglist csv

Also human=0 (or no human=):

https://bugzilla-dev.allizom.org/buglist.cgi?bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&email1=netfuzzer%40yandex.com&emailassigned_to1=1&emailreporter1=1&emailtype1=exact&field0-0-0=bug_status&field0-0-1=reporter&list_id=357&query_format=advanced&type0-0-0=notequals&type0-0-1=equals&value0-0-0=UNCONFIRMED&value0-0-1=netfuzzer%40yandex.com&ctype=csv

Comment 43

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8699454 [details] [diff] [review]
Patch v.1

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

with gerv's patch, the file in comment 42 would still be valid js.

Comment 44

[Gervase Markham [:gerv]]

:-( OK, other than the trailing comma on line 1, I'm now out of ideas for breaking the JS-ness of the file.

Gerv

Comment 45

[Dylan Hardison [:dylan] (he/him)]

(In reply to Gervase Markham [:gerv] from comment #44)
> :-( OK, other than the trailing comma on line 1, I'm now out of ideas for
> breaking the JS-ness of the file.
> 
> Gerv

What about a trailing comma on every line?

Comment 46

[Gervase Markham [:gerv]]

I wonder whether that would lead to more of a compatibility issue than a trailing comma on the first line...

Gerv

Comment 47

[Dylan Hardison [:dylan] (he/him)]

we have two options -- make it invalid js or remove the feature. I think double trailing commas is the least invasive way of making it invalid js.

Comment 48

[Dylan Hardison [:dylan] (he/him)]

to be clear, can the user's sepchar be changed outside of the user's control? if it is set to ; ... we will still have valid js!

Comment 49

[Dylan Hardison [:dylan] (he/him)]

about 211 users on BMO use ;

Comment 50

[Dylan Hardison [:dylan] (he/him)]

We can always add a final column named ☃ that is empty for all other rows.

Comment 51

[(no longer active)]

Guys, trying to make this not parse as JS is pointless.  Either we're going to end up missing something, or a future ES version is going to add something that will result in sadness.

We should either remove all non-public bugs from the CSV export, or remove CSV export altogether.  There is no safe way to expose the information we currently expose in a CSV format.

Comment 52

[Gervase Markham [:gerv]]

(In reply to :Ehsan Akhgari from comment #51)
> Guys, trying to make this not parse as JS is pointless.  Either we're going
> to end up missing something, or a future ES version is going to add
> something that will result in sadness.

Ehsan: "don't ever put any sensitive data on the web (even password-protected) in any format that could possibly be parsed as JS" is not a reasonable position for Mozilla's Firefox team to take. There's no way anyone can ever meet that security criteria apart from by avoiding text-based file formats entirely, which is an unreasonable restriction.

If X-Content-Type-Options: nosniff is really the only way to fix this, then we'd better get on and implement that. How do we get that bug prioritized?

Gerv

Comment 53

[Dylan Hardison [:dylan] (he/him)]

at least js doesn't support XML literals (like Scala does) or else this would be really bad.

Comment 54

[Dylan Hardison [:dylan] (he/him)]

Note: Safari is also vulnerable.

Comment 55

[(no longer active)]

(In reply to Gervase Markham [:gerv] from comment #52)
> (In reply to :Ehsan Akhgari from comment #51)
> > Guys, trying to make this not parse as JS is pointless.  Either we're going
> > to end up missing something, or a future ES version is going to add
> > something that will result in sadness.
> 
> Ehsan: "don't ever put any sensitive data on the web (even
> password-protected) in any format that could possibly be parsed as JS" is
> not a reasonable position for Mozilla's Firefox team to take.

I am only suggesting this as a fix to this specific issue, not the general problem you're referring to.  The discussion on the latter can happen on dev-platform.

> There's no way
> anyone can ever meet that security criteria apart from by avoiding
> text-based file formats entirely, which is an unreasonable restriction.
> 
> If X-Content-Type-Options: nosniff is really the only way to fix this, then
> we'd better get on and implement that. How do we get that bug prioritized?

See the discussion on bug 471020?  The problem is not one of prioritizing a bug, it's about there being no complete specification on what this header is supposed to do, and it behaving differently in IE, Chrome and the partial specs written for it.  So it's hard to know what needs to be implemented here.

I think we can make a reasonable case for adding support for nosniff for <script> loads, but we need to know what the exact behavior we want to support it.

But now we're getting really off topic here.  I think we should focus on fixing this bug without nosniff.

Comment 56

[Frédéric Buclin]

OK, I think we have only one solution left: by default, the CSV file should only include public bugs (make sure to take the requirelogin parameter into account!). If you want to see private bugs, you must pass a valid API key in the URL. The CSV file would then only contain bugs which are visible to the user identified by this API key, independently of your current login cookies, to prevent the attacker from using the victim's credential.

Comment 57

[Dylan Hardison [:dylan] (he/him)]

I'm working on this Roughly speaking the same as comment 56. Cookies will be ignored for buglist.cgi when producing a csv formatted report,

Comment 58

[Mario Gomes]

There's also another way that doesn't break anything and prevent the xss. 
Have a look:
============================
Bug ID,"Product","Component","Assignee","Status","Resolution","Summary","Changed"
============================

Removing the quotes from the first field won't break anything and will cause a javascript error.

It'll still work in most CSV parsers.

Comment 59

[Gervase Markham [:gerv]]

Mario: Dylan covered that in comment 42. Adding "&human=0" to the URL gives database field names, which contain no spaces. So, I am told, removing the quotes from the field won't help.

And, because in JS semicolons are optional, I don't think we can add linebreaks to the first line to break the parser either. :-|

Gerv

Comment 60

[Mario Gomes]

@Gerv: how about checking the "Accept" header? Most browser send "Accept: */*"(IE,Chrome,Firefox and Safari) to get JS content. You guys could check this header and block the attack.

Comment 61

[Gervase Markham [:gerv]]

IE sends */* for some top-level loads too, AIUI. Accept header parsing would probably be pretty fragile. And it's not a security header so there's no guarantee some browsers don't have ways to modify it for some requests.

Gerv

Comment 62

[Gervase Markham [:gerv]]

(In reply to Dylan William Hardison [:dylan] from comment #57)
> I'm working on this Roughly speaking the same as comment 56. Cookies will be
> ignored for buglist.cgi when producing a csv formatted report,

I think this will break old BzAPI, for anyone still using it. Will you still allow explicit username/password login on the URL?

Gerv

Comment 63

[Dylan Hardison [:dylan] (he/him)]

So in other news, doesn't this also mean that you can craft (using only public bugs) js that gets run under the security context of bugzilla.mozilla.org?

Comment 64

[Dylan Hardison [:dylan] (he/him)]

can said code poke at the local storage that splinter uses? If I put this <script src> on a site that people that review security bugs go to, ....? I want to be wrong about this.

Comment 65

[Dylan Hardison [:dylan] (he/him)]

Is my fear outlined in comment 63 and comment 64 feasible?

Comment 66

[(no longer active)]

(In reply to Dylan William Hardison [:dylan] from comment #65)
> Is my fear outlined in comment 63 and comment 64 feasible?

No.  Script runs in the context of the web page that embeds the <script> tag, not the context of where the script is loaded from.

Comment 67

[Dylan Hardison [:dylan] (he/him)]

I am incredibly happy that I am wrong, and I wish I had remembered the context a script gets executed in is the page it is on.

Comment 68

[Daniel Veditz [:dveditz]]

(In reply to Mario Gomes from comment #11)
> Great! Could this be eligible for a bounty?

Please mail [e-mail] for bounty requests, as always, to make sure the bounty folks see it. (In this case I happened to see it, but that's no guarantee for next time.)

Severity is somewhere between moderate and high, I'll go on the high side to start. Easy for attacker to arrange bugs with the right titles to be first and last: even w/out editbugs powers you can create your own bugs and sort your target query on some irrelevant column that you put low/high markers in your boundary bugs. You don't get full details of bugs, summary is the most damaging. Sometimes that's pretty revealing, sometimes it's useless.

Even though Mario points out that in the particular case of CSV he could get it to be parsed as an array, the new ES 6 ` string delimiter is going to create more problems in more sites. No one's going to be filtering on that and prior to this feature any multi-line quoting attempts would be a syntax error. Part of the problem here is that CSV quote escaping is different from JS escaping, but that could be true in other formats as well.

At one point we tried to use Content Security Policy as a way to raise the bar on JS MIME type sniffing: bug 662227. That wouldn't have been a solution here because the attacker would presumably not use CSP, but the fact that it was removed from the initial spec shows the depth of the compatibility worries MIME type enforcement raises.

I thought we had a bug (even if WONTFIXed) about making strict mime type checking for JS the default regardless of any nosniff header but I can't currently find it. Would probably break the web since text/plain has historically been very popular for script files (we should add telemetry).

Maybe we could restrict new ES 6 features to files that have a reasonable content type? The doesn't solve this Bugzilla problem but longer term could help the web.

Comment 69

[Dylan Hardison [:dylan] (he/him)]

I think we could get away with filtering out all punctuation characters (basically anything that is not /[\s\w]/ in a perl regex). For minimum breakage, we could do this only for logged in users. For BMO this would be a very small sample of users -- and it would probably not break anyone's workflow beyond repair.

Comment 70

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1232785_2.patch

replace \ with \ for logged in users. This means it will only break for about 20 people on BMO and hopefully small numbers on other sites.

Comment 71

[Frédéric Buclin]

(In reply to Dylan William Hardison [:dylan] from comment #70)
> replace \ with \ for logged in users.

I don't see why you do HTML-escaping with a plain text file. This doesn't make any sense to me. When you then read the CSV file, text would be displayed as \, not converted back to \. That's painful. C:\Foo\Bar\file.ext would be displayed as C:\Foo\file.ext. Yay!

Comment 72

[Dylan Hardison [:dylan] (he/him)]

it's painful but less painful than other options. I was going to use [BACKSLASH] but dveditz suggested the html entity for shortness.

Comment 73

[Dylan Hardison [:dylan] (he/him)]

I could also use http://www.fileformat.info/info/unicode/char/ff3c/index.htm

Comment 74

[David Lawrence [:dkl]]

Comment on attachment 8699589 [details] [diff] [review]
1232785_2.patch

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

r=dkl as this fixes this particular exploit case as demonstrated by the reporter. I also see LpSolit's point about seeming wrong to change values in a plain text format causing extra work for the client. Until Firefox (and others) are fixed to do the correct thing with regards to text/plain and not execute as javascript, can we guarantee that some other similar issue will arise from this type of exploit. Is there a list of other characters we should be escaping as well or will this be enough to catch most cases?

Comment 75

[Dylan Hardison [:dylan] (he/him)]

Attachment: 1232785_3.patch

use unicode Fullwidth Reverse Solidus instead of ASCII Reverse Solidus

Comment 76

[Tanvi Vyas[:tanvi]]

(In reply to Dylan William Hardison [:dylan] from comment #6)
> This works from any page context, in firefox only as reported. Chrome
> reports this error:
> 
> "Refused to execute script from
> 'https://bugzilla-dev.allizom.org/buglist.
> cgi?bug_status=UNCONFIRMED&bug_sta…value0-0-0=UNCONFIRMED&value0-0-
> 1=netfuzzer%40yandex.com&ctype=csv&human=1' because its MIME type
> ('text/csv') is not executable, and strict MIME type checking is enabled."

Since bugzilla isn't sending "X-Content-Type-Options: nosniff", how is this getting blocked on chrome?  Maybe in addition to bug 471020, we need something else?

Comment 77

[Dylan Hardison [:dylan] (he/him)]

Chrome just refuses to execute text/csv as js.

Comment 78

[Gervase Markham [:gerv]]

(In reply to Dylan William Hardison [:dylan] from comment #77)
> Chrome just refuses to execute text/csv as js.

If we can't have "X-Content-Type-Options: nosniff" in Firefox, can we just have this? :-)

Gerv

Comment 79

[(no longer active)]

(In reply to Gervase Markham [:gerv] from comment #78)
> (In reply to Dylan William Hardison [:dylan] from comment #77)
> > Chrome just refuses to execute text/csv as js.
> 
> If we can't have "X-Content-Type-Options: nosniff" in Firefox, can we just
> have this? :-)

We can try, but this is the kind of thing that runs the risk of breaking websites so it's not a good fix for a bugzilla vulnerability.  It also won't fix anything for Safari users.

Comment 80

[David Lawrence [:dkl]]

Comment on attachment 8699686 [details] [diff] [review]
1232785_3.patch

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

r=dkl

Comment 81

[Dylan Hardison [:dylan] (he/him)]

The patch cleanly applies and works in 4.4 and 5.0

Comment 82

[Gervase Markham [:gerv]]

(In reply to :Ehsan Akhgari from comment #79)
> We can try, but this is the kind of thing that runs the risk of breaking
> websites so it's not a good fix for a bugzilla vulnerability.

You think there are websites out there serving valid JavaScript specifically as text/csv?

Gerv

Comment 83

[Gervase Markham [:gerv]]

I think that this patch silently changes data, which could have any number of unknown or unpredictable effects. I still prefer the trailing-comma-on-first-line fix. But there you go.

Gerv

Comment 84

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #82)
> You think there are websites out there serving valid JavaScript specifically
> as text/csv?

I don't think that's the case, because 1) they are abusing what a CSV file is used for, and 2) Chrome refuses to use them in <script>, and so these websites would be broken for Chrome users.

Comment 85

[Dylan Hardison [:dylan] (he/him)]

(In reply to Gervase Markham [:gerv] from comment #83)
> I think that this patch silently changes data, which could have any number
> of unknown or unpredictable effects. I still prefer the
> trailing-comma-on-first-line fix. But there you go.
> 
> Gerv
It would have two be two trailing commas. It would also not work for the case of someone using semicolons.

Comment 86

[Gervase Markham [:gerv]]

> It would have two be two trailing commas. It would also not work for the
> case of someone using semicolons.

I think the user preference to allow people to have their comma-separated values files using commas or semicolons is a dumb preference and I would be very happy to see it eliminated entirely. :-)

Gerv

Comment 87

[Frédéric Buclin]

(In reply to Gervase Markham [:gerv] from comment #86)
> I think the user preference to allow people to have their comma-separated
> values files using commas or semicolons is a dumb preference and I would be
> very happy to see it eliminated entirely. :-)

Just because it makes this bug harder to fix? That's a weak reason. This user pref has been implemented in Bugzilla 2.20 because Excel uses either commas or semicolons to parse CSV files, depending on the localization of your Windows installation, see bug 257767 comment 12. For instance, English uses commas, but both French and German use semicolons.

Comment 88

[Gervase Markham [:gerv]]

(In reply to Frédéric Buclin from comment #87)
> Just because it makes this bug harder to fix? That's a weak reason. This
> user pref has been implemented in Bugzilla 2.20 because Excel uses either
> commas or semicolons to parse CSV files, depending on the localization of
> your Windows installation, see bug 257767 comment 12. For instance, English
> uses commas, but both French and German use semicolons.

LibreOffice lets you decide what separator to use when importing the file; has Excel not yet caught up with this useful facility, then?

The idea of shipping a spreadsheet which imports "Comma-Separated Values" files but doesn't support using commas to separate the values is a comedy move from Microsoft :-)

Gerv

Comment 89

[Dylan Hardison [:dylan] (he/him)]

No, it is an example of MS not being anglo-centric. Lots of languages use , as a decimal seperator. Numbers don't have to be quoted, so using a semicolon makes a lot of sense. On BMO more than 200 people use semicolons. I wonder if we should consider exporting ods and xls formats and remove csv from future releases.

Comment 90

[Gervase Markham [:gerv]]

(In reply to Dylan William Hardison [:dylan] from comment #89)
> No, it is an example of MS not being anglo-centric. Lots of languages use ,
> as a decimal seperator.

I know. It's the fact that it can't _import_ the comma-using version which is clownshoes. If it could, we could just always export with commas and there would be no need for the preference. (We don't put decimal separators into our numbers when we export, so no extra quoting.)

Gerv

Comment 91

[David Lawrence [:dkl]]

dveditz, can we get a CVE for this please?

dkl

Comment 92

[David Lawrence [:dkl]]

dveditz on pto. abillings is getting us a CVE

Comment 93

[Mario Gomes]

Any estimate date for this going public?

Comment 94

[David Lawrence [:dkl]]

(In reply to Mario Gomes from comment #93)
> Any estimate date for this going public?

Trying for pushing a release today. We will see how it goes.

Comment 95

[Dylan Hardison [:dylan] (he/him)]

To ssh://[e-mail]/bugzilla/bugzilla.git
   49b2e90..c899b36  4.2 -> 4.2
   5f7540c..ce87073  4.4 -> 4.4
   dc076ed..a118534  5.0 -> 5.0
   e69201a..34f8910  master -> master

Comment 96

[Boris Zbarsky [:bzbarsky]]

A bit late here, but had anyone considered checking referrer headers as a mitigation?

Comment 97

[Masatoshi Kimura [:emk]]

How about adding an extra comma regardless of the separator that the user specified?
"Bug ID";"Product";"Component";"Assignee";"Status";"Resolution";"Summary";"Changed";,
is still invalid as JS, a data in the last (dummy) field as CSV.

Comment 98

[Gervase Markham [:gerv]]

(In reply to Masatoshi Kimura [:emk] from comment #97)
> How about adding an extra comma regardless of the separator that the user
> specified?
> "Bug
> ID";"Product";"Component";"Assignee";"Status";"Resolution";"Summary";
> "Changed";,
> is still invalid as JS, a data in the last (dummy) field as CSV.

That would lead to an additional final field containing just the text ",", when the separator was colon. It's not ideal to add additional fields with actual content.

(In reply to Boris Zbarsky [:bz] (Vacation until Jan 4) from comment #96)
> A bit late here, but had anyone considered checking referrer headers as a
> mitigation?

CSV is used as a format by software interfacing with Bugzilla; it's not just people clicking the link on the buglist page. What would the check be? "Referrer has to be blank, or the Bugzilla itself"?

Gerv

Comment 99

[Boris Zbarsky [:bzbarsky]]

> "Referrer has to be blank, or the Bugzilla itself"?

Yes.  That wouldn't help if people have their browsers configured to not send referrer, of course, but it should help in the common case.

Comment 100

[Gervase Markham [:gerv]]

:dkl, :dylan: views?

Gerv

Comment 101

[David Lawrence [:dkl]]

(In reply to Gervase Markham [:gerv] from comment #100)
> :dkl, :dylan: views?
> 
> Gerv

I would rather never use Referer as any means of protection as it is freely alterable and interceptable. And not to mention some people use dashboards that access CSV directly without coming from the CSV link in buglist.cgi.

dkl

Comment 102

[Gervase Markham [:gerv]]

(In reply to David Lawrence [:dkl] from comment #101)
> I would rather never use Referer as any means of protection as it is freely
> alterable and interceptable. 

If I'm trying to conduct an XSS attack on you, what mechanisms can I use to get your browser to send a bogus Referer?

> And not to mention some people use dashboards
> that access CSV directly without coming from the CSV link in buglist.cgi.

How do they do that? Same Origin Policy should mean the dashboard can't access the data...

Gerv

Comment 103

[Mario Gomes]

(In reply to Gervase Markham [:gerv] from comment #102)
> If I'm trying to conduct an XSS attack on you, what mechanisms can I use to
> get your browser to send a bogus Referer?

Hey, I can answer that :) It's possible to send "blank referer" requests using data URLS on Firefox.

Comment 104

[David Lawrence [:dkl]]

(In reply to Gervase Markham [:gerv] from comment #102)
> > And not to mention some people use dashboards
> > that access CSV directly without coming from the CSV link in buglist.cgi.
> 
> How do they do that? Same Origin Policy should mean the dashboard can't
> access the data...

By accessing a query URL directly for public bugs only such as:

https://bugzilla.mozilla.org/buglist.cgi?bug_status=NEW&component=Administration&product=Bugzilla&ctype=csv

dkl

Comment 105

[Gervase Markham [:gerv]]

(In reply to David Lawrence [:dkl] from comment #104) 
> By accessing a query URL directly for public bugs only such as:
> 
> https://bugzilla.mozilla.org/buglist.
> cgi?bug_status=NEW&component=Administration&product=Bugzilla&ctype=csv

Sorry if I'm being dense, but surely an HTML dashboard hosted at a random domain can't XHR such a URL and read the result because of the same origin policy?

Gerv

Comment 106

[Dylan Hardison [:dylan] (he/him)]

Such dashboards can access that server-side, which has no restrictions. The fix we have in place is the best possible one given the universe we live in.