Bug 1232785 (CVE-2015-8509)

[SECURITY] Buglists in CSV format can be parsed as valid javascript in some browsers

RESOLVED FIXED in Bugzilla 4.2

Status

()

Bugzilla
Attachments & Requests
--
major
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: x, Assigned: dylan)

Tracking

(Blocks: 1 bug, {sec-high})

2.17.1
Bugzilla 4.2
sec-high
Dependency tree / graph
Bug Flags:
approval5.0 +
approval4.4 +
approval4.2 +
sec-bounty +

Details

(Whiteboard: ES 6 `-strings + no MIME restrictions on script src + no support for x-content-options: nosniff)

Attachments

(4 attachments, 3 obsolete attachments)

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,
Created attachment 8698593 [details]
poc.html
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: netfuzzer@yandex.com
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
Just to clarify, my personna login info are:
Log in: netfuzzer@yandex.com
Password: 1111111111

Comment 4

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

Comment 5

2 years ago
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?
(Assignee)

Comment 6

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

Comment 8

2 years ago
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.
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

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

Comment 12

2 years ago
Created attachment 8698717 [details]
csv that is also javascript

Comment 13

2 years ago
I can now reproduce the issue. The CSV format for buglists has been added in 2.17.1, see bug 177430.
Assignee: general → attach-and-request
Severity: normal → major
Status: UNCONFIRMED → NEW
Component: Bugzilla-General → Attachments & Requests
Ever confirmed: true
Summary: Bugzilla Buglist Information Disclosure Vulnerability. → [SECURITY] Attachments are able to read remote buglists containing restricted bugs
Target Milestone: --- → Bugzilla 4.4
Version: unspecified → 2.17.1

Comment 14

2 years ago
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?
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

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

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

2 years ago
(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?
(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.
(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
scripts from <script> tags are loaded in no-cors mode, so CORS is irrelevant...

Can we not include security sensitive bugs in CSV exports?
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.
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.
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

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

Comment 30

2 years ago
Another option is manipulating the output to disallow ES6 quoting seems to work as well.
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
(Assignee)

Comment 32

2 years ago
No, but that might work as well. I meant physically remove the ` from bug summaries.
(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.
(Assignee)

Updated

2 years ago
Assignee: attach-and-request → dylan
Status: NEW → ASSIGNED
Created attachment 8699452 [details] [diff] [review]
Patch v.1

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

Gerv
Assignee: dylan → gerv
Attachment #8699452 - Flags: review?(dylan)

Comment 35

2 years ago
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).
Attachment #8699452 - Flags: review?(dylan) → review-
(Assignee)

Comment 36

2 years ago
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?
Created attachment 8699454 [details] [diff] [review]
Patch v.1

With semicolon.

Gerv
Attachment #8699452 - Attachment is obsolete: true
Attachment #8699454 - Flags: review?(dylan)
A string of random undeclared comma-separated identifiers is valid JS?

<sigh>

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

gerv
(Assignee)

Comment 39

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

Comment 40

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

Comment 41

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

Comment 42

2 years ago
Created attachment 8699460 [details]
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
(Assignee)

Comment 43

2 years ago
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.
Attachment #8699454 - Flags: review?(dylan) → review-
:-( OK, other than the trailing comma on line 1, I'm now out of ideas for breaking the JS-ness of the file.

Gerv
(Assignee)

Comment 45

2 years ago
(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?
I wonder whether that would lead to more of a compatibility issue than a trailing comma on the first line...

Gerv
(Assignee)

Comment 47

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

Comment 48

2 years ago
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!
(Assignee)

Comment 49

2 years ago
about 211 users on BMO use ;
(Assignee)

Comment 50

2 years ago
We can always add a final column named ☃ that is empty for all other rows.
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.
(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
(Assignee)

Comment 53

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

Comment 54

2 years ago
Note: Safari is also vulnerable.
(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

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

Comment 57

2 years ago
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,
Assignee: gerv → dylan
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.
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
@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.
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
(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
(Assignee)

Comment 63

2 years ago
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?
(Assignee)

Comment 64

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

Comment 65

2 years ago
Is my fear outlined in comment 63 and comment 64 feasible?
Flags: needinfo?(ehsan)
(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.
Flags: needinfo?(ehsan)
(Assignee)

Comment 67

2 years ago
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.
(In reply to Mario Gomes from comment #11)
> Great! Could this be eligible for a bounty?

Please mail security@mozilla.org 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.
Blocks: 835424
Depends on: 471020
Flags: sec-bounty?
Keywords: sec-high
Whiteboard: ES 6 `-strings + no MIME restrictions on script src + no support for x-content-options: nosniff
(Assignee)

Comment 69

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

Comment 70

2 years ago
Created attachment 8699589 [details] [diff] [review]
1232785_2.patch

replace \ with &bsol; for logged in users. This means it will only break for about 20 people on BMO and hopefully small numbers on other sites.
Attachment #8699454 - Attachment is obsolete: true
Attachment #8699589 - Flags: review?(dkl)

Comment 71

2 years ago
(In reply to Dylan William Hardison [:dylan] from comment #70)
> replace \ with &bsol; 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 &bsol;, not converted back to \. That's painful. C:\Foo\Bar\file.ext would be displayed as C:&bsol;Foo&bsol;file.ext. Yay!
(Assignee)

Comment 72

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

Comment 73

2 years ago
I could also use http://www.fileformat.info/info/unicode/char/ff3c/index.htm
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?
Attachment #8699589 - Flags: review?(dkl) → review+
(Assignee)

Comment 75

2 years ago
Created attachment 8699686 [details] [diff] [review]
1232785_3.patch

use unicode Fullwidth Reverse Solidus instead of ASCII Reverse Solidus
Attachment #8699589 - Attachment is obsolete: true
Attachment #8699686 - Flags: review?(dkl)
(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?
(Assignee)

Comment 77

2 years ago
Chrome just refuses to execute text/csv as js.
(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
(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 on attachment 8699686 [details] [diff] [review]
1232785_3.patch

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

r=dkl
Attachment #8699686 - Flags: review?(dkl) → review+
(Assignee)

Comment 81

2 years ago
The patch cleanly applies and works in 4.4 and 5.0
Flags: approval5.0?
Flags: approval4.4?
(Assignee)

Updated

2 years ago
Summary: [SECURITY] Attachments are able to read remote buglists containing restricted bugs → [SECURITY] Buglists in CSV format can be parsed as valid javascript in some browsers

Updated

2 years ago
Flags: approval5.0?
Flags: approval5.0+
Flags: approval4.4?
Flags: approval4.4+
(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
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

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

Comment 85

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

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

Comment 89

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

Updated

2 years ago
Blocks: 1229728
dveditz, can we get a CVE for this please?

dkl
Flags: needinfo?(dveditz)

Updated

2 years ago
Blocks: 1234237
dveditz on pto. abillings is getting us a CVE
Flags: needinfo?(dveditz)
Alias: CVE-2015-8509
Any estimate date for this going public?

Updated

2 years ago
No longer depends on: 471020
Flags: approval4.2?
Target Milestone: Bugzilla 4.4 → Bugzilla 4.2
(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.
Flags: approval4.2? → approval4.2+
(Assignee)

Comment 95

2 years ago
To ssh://gitolite3@git.mozilla.org/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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED

Updated

2 years ago
Group: bugzilla-security
Flags: sec-bounty? → sec-bounty+
A bit late here, but had anyone considered checking referrer headers as a mitigation?
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.
(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
> "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.
:dkl, :dylan: views?

Gerv
Flags: needinfo?(dylan)
Flags: needinfo?(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
Flags: needinfo?(dkl)
(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
(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.
(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
(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
(Assignee)

Comment 106

a year ago
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.
Flags: needinfo?(dylan)
You need to log in before you can comment on or make changes to this bug.