1259881 - CSV export vulnerable to formulae injection (again)

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Muhammed Gamal]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/49.0.2623.87 Safari/537.36

Steps to reproduce:

file a new bug with name "-2+3+cmd|' /C calc'!A0" and continue the needed information


Actual results:

when the bugs exported as a csv file , it the cmd code will run poping up the calculator


Expected results:

filtering the inputs

Comment 2

[Muhammed Gamal]

Are there any news ?

Comment 3

[Frédéric Buclin]

Not for now, no.

Comment 4

[Paresh]

hi

this bug #1259881 and #1054702 Is the same issue ! (both are using same payload and method ("-2+3+cmd|' /C calc'!A0")
 i tested #1259881 (this issue which is reported by muhammed gamal)  and UNABLE TO REPRODUCE  that using "-2+3+cmd|' /C calc'!A0" .


so i tried with %0A-2+3+cmd|' /C calc'!D2 and its working with new Line unicode charcter !
can you please take a look again ! more details i mentioned in my report bug #1263581


Thanks

Comment 5

[Muhammed Gamal]

Hello Paresh ,

in the report you mentioned the reporter didn't use my payload he used "=" to perform actions .

in my report , of course it works with that's why i report it !

here is a Picture to Proof My concept
http://i.imgur.com/PLPmsQN.png

Comment 6

[Paresh]

ok! 

i tried again , still unable to reproduce your issue! ( tried in exel )

Anyways
i hope mozilla/bugzilla team checks both issues well !


Regards !

Comment 7

[Muhammed Gamal]

As u can see it is working with me in excel too ( excel 2013 version )

i hope you the best too !

Comment 9

[Dave Miller [:justdave]]

I don't see how this is a Bugzilla issue.  This sounds like a bug in Excel to me.

Comment 10

[Paresh]

(In reply to Dave Miller [:justdave] ([e-mail]) from comment #9)
> I don't see how this is a Bugzilla issue.  This sounds like a bug in Excel
> to me.

Yup! but also bugzilla issue.
This issue isn’t specific to web applications or any particular file format – any situation where untrusted content ends up in a spreadsheet could be exploited.

For fix:
Sanitise user input. There would not be a problem in the first place if the application were sanitising user input. Characters such as ‘=’ should not be allowed in fields such as  report Title. If these characters are to be allowed, a recommended fix is to append a single quote (‘) to the list of formula triggers ( =, +, -) before saving it in the database. LibreOffice or Excel will ignore the single quote and just show the malicious formula as a string i.e. it won’t be interpreted as a formula.


btw ive found 2 year old report, which is fixed https://bugzilla.mozilla.org/show_bug.cgi?id=1054702 

and this report same as Bug 1054702

Comment 11

[Dave Miller [:justdave]]

OK, so this is a duplicate of bug 1054702 and it's already fixed then?

Comment 12

[Muhammed Gamal]

Nope , in bug 1054702 it's fixed and the reporter already got his bounty , i have just bypassed the fix

Comment 13

[Dave Miller [:justdave]]

I notice your screenshot has an "Enable macros" dialog in it.  Did you have to enable macros for this exploit to work?

Comment 14

[Paresh]

(In reply to Muhammed Gamal from comment #12)
> Nope , in bug 1054702 it's fixed and the reporter already got his bounty , i
> have just bypassed the fix

How you bypassed the fix ?
in both report (yours and  bug 1054702) using same method (=) .

it would be nice if you provide Video Proof of concept.
because i tried it and (=) doesnt execute. 

Thanks

Comment 15

[Muhammed Gamal]

i don't know where is "=" in "-2+3+cmd|' /C calc'!A0" !!!

Comment 16

[Paresh]

(In reply to Muhammed Gamal from comment #15)
> i don't know where is "=" in "-2+3+cmd|' /C calc'!A0" !!!

Thats the point, how you executing this "-2+3+cmd|' /C calc'!A0"   Without (=) ? 
Thats why i am asking for video proof of concept.

Comment 17

[Muhammed Gamal]

This is a Unlisted POC Video
https://youtu.be/2OuQ8Awkq88

Comment 18

[Paresh]

(In reply to Muhammed Gamal from comment #17)
> This is a Unlisted POC Video
> https://youtu.be/2OuQ8Awkq88

Thanks,
next time you test anything, test on https://landfill.bugzilla.org/bugzilla-5.0-branch/ (test server)

Comment 19

[Muhammed Gamal]

OK , i knew after i tested it , and i removed the bug report .
i hope that the video provide the information you need to know

Comment 20

[Dave Miller [:justdave]]

The video shows Excel saying "This is dangerous, don't do it" and you overriding it to do it anyway.

"Microsoft Office has identified a potential security concern.

File Path: C:\Users\Mohammed\Downloads\bugs-2016-04-15.csv

Automatic update of links has been disabled. If you choose to enable automatic update of links, your computer may no longer be secure. Do not enable this content unless you trust the source of this file."

Comment 21

[Muhammed Gamal]

CSV Injection is one of Command Injection bugs and has a critical Impact 
as u can see at "https://bugzilla.mozilla.org/show_bug.cgi?id=1054702"
the Bugzilla platform was Vulnerable to this bug 2 years ago , and i am just bypassed the fix
and if you see that's not a bugzilla bug you can check 
"https://bugzilla.mozilla.org/show_bug.cgi?id=1054702#c7"

Comment 22

[Dave Miller [:justdave]]

What characters in that string are triggering it?  I see nothing in that string that I wouldn't want to allow in a bug summary, to be honest, so if we're going to do anything, we need to know what the trigger actually is.

However....   The fact that Excel triggers a warning when you open the file makes me care way less about this one.  The previous bug you're citing was a case where you could execute things from loading a csv file without any warning when you opened it.  This one, Excel actually attempts to prevent you from running the dangerous code.  It's not really our problem if you ignore the warnings.

Comment 23

[Daniel Veditz [:dveditz]]

This bug is not eligible for a bug bounty -- the dangerous functionality is clearly on the Excel side (and somewhat mitigated). This attack supposes that someone will download a CSV of a buglist, open it, and then think it is perfectly normal for a Bugzilla buglist to be launching external functionality.

Comment 24

[Muhammed Gamal]

In the other report The researcher tested it in LibreOffice , and if you tested it in it or any CSV reading file it will be run without warning .
and if you tried the old one with Excel it will be warned also !
I think command execution possibility should be get some more attention even with warning.
most of big companies know the impact of the Vulnerability even with Excel warnings 
there are some reports of Big Companies and has warning box also ! 
Hackerone : https://hackerone.com/reports/72785 , https://hackerone.com/reports/111192
Zendesk : https://hackerone.com/reports/90131
Uber : https://hackerone.com/reports/126109

and about the word you should triggering , CMD , HYPERLINK and all words can be executed in the CSV file

Comment 25

[Muhammed Gamal]

@Daniel 
please check what you said in the old report !
https://bugzilla.mozilla.org/show_bug.cgi?id=1054702#c7

Comment 26

[Daniel Veditz [:dveditz]]

I should note that we weren't happy paying a bounty for bug 1054702 either -- it's still an excel bug exposing such dangerous functionality, but in that case the unbelievable stupidity of excel launching external programs without prompt put users at enough risk that it was worth trying throw some blocks up.

Comment 27

[Muhammed Gamal]

it's not excel only which this works for , any csv files reader will execute the command , you have to know that there are alot of people use different software to read it .
and the big companies don't give a chance to harm their user through this vulnerability so it filtering their inputs from this commands .
i think this case should be considered as the other one !

Comment 28

[Mario Gomes]

If I may suggest a patch, you guys could add an escape("\x20") before the bugs summary when it starts with "-" and "+", as you already do when it begins with "=". Do it like this:
=========================
"Bug ID","Product","Component","Assignee","Status","Resolution","Summary","Changed"
1120074,"Bugzilla","User Accounts","[e-mail]","UNCONFIRMED","---"," -SOME-CMD-CODE","2015-08-20 07:12:39"

Putting the space will prevent it from being interpreter as a macro.

Comment 29

[Mario Gomes]

Ops, what I meant "space" not scape :P

Comment 30

[Daniel Veditz [:dveditz]]

Looks like bug 1054702 only patched '=' and we ought to have escaped -+@ as well. See "Update (April 2016)" at http://www.contextis.com/resources/blog/comma-separated-vulnerabilities/ (triggered by
https://twitter.com/albinowax/status/717642300961255425 )

Also an initial ' might have been a better escape than a space.

Comment 31

[Daniel Veditz [:dveditz]]

Note that blog and tweet came from the reporter of bug 1054702

Comment 32

[Muhammed Gamal]

i think this is strengthening opening the case again !

Comment 33

[Frédéric Buclin]

Blindly interpreting everything is an impressive stupidity from Microsoft & co. The CSV RFC (rfc 4180) doesn't mention this possibility, see section 3:

"Security considerations: CSV files contain passive text data that should not pose any risks."

It's totally Excel's fault to execute code coming from a CSV file. Note that Excel is aware of this as it throws 2 clear warnings. But it shouldn't throw these warnings. It should simply ignore "special" characters coming from CSV files. Period.


As we already treat "=" specifically, we can simply add +, - and @ to the regexp. But this is *not* a Bugzilla bug, to make things clear.

Comment 34

[Ibram Marzouk]

I totally agree with Muhammed Gamal! 
This bug is not an Excel bug, it's a Bugzilla bug because the bug will affect Bugzilla users and Bugzilla is the only application who can fix this and protect the users.
And as Muhammed mentioned, other bug companies agree that CSV injection is a web-application vulnerability and have nothing to do with Excel, And they also reward bounties for it and take it seriously.

Comment 35

[Paresh]

(In reply to Frédéric Buclin from comment #33)
> Blindly interpreting everything is an impressive stupidity from Microsoft &
> co.

Haha !
Please someone add Jonathan or jesse or @shurik41 from 'Microsoft Security Response Center'  here :D

Comment 36

[Mario Gomes]

Google has a very good statement about "csv injection" severity: https://sites.google.com/site/bughunteruniversity/nonvuln/csv-excel-formula-injection
"""
CSV files are just text files (the format is defined in RFC 4180) and evaluating formulas is a behavior of only a subset of the applications opening them - it's rather a side effect of the CSV format and not a vulnerability in our products which can export user-created CSVs. This issue should mitigated by the application which would be importing/interpreting data from an external source, as Microsoft Excel does (for example) by showing a warning. In other words, the proper fix should be applied when opening the CSV files, rather then when creating them.
"""

Comment 37

[Frédéric Buclin]

(In reply to Paresh from comment #35)
> Please someone add Jonathan or jesse or @shurik41 from 'Microsoft Security
> Response Center'  here :D

I don't care about their opinion, really. They interpret passive text as formula, fine. Their problem!

Comment 38

[Ibram Marzouk]

(In reply to Mario Gomes from comment #36)

I kinda agree with what you've said.. But then why did you guys fix this one #1054702?
As I can see, It's almost the same!

Comment 39

[Muhammed Gamal]

I think the important is the safty of the users not who is responsible for , and when you can avoid that and you did that before , why don't you do the same here ?!

Comment 40

[Frédéric Buclin]

(In reply to Muhammed Gamal from comment #39)
> I think the important is the safty of the users not who is responsible for ,
> and when you can avoid that and you did that before , why don't you do the
> same here ?!

I didn't say we won't do it (I marked the bug as NEW, didn't I?), though we can still decide to close the bug as WONTFIX later. What I'm saying is that due to Microsoft inability to follow RFCs, other applications are forced to take some actions to prevent some undesirable behaviors in Excel.

Comment 41

[Frédéric Buclin]

Attachment: patch, v1

Simon: you wrote the original code to catch "=". Want to test this one?

Comment 42

[Daniel Veditz [:dveditz]]

space-escaping makes me worry some helpful filter is going to trim and then we're back where we started. I suppose if Excel import doesn't trim then we've solved the immediate problem, but I'd rather have uglier summaries (starting with ') in this rare case than have to worry.

'rare' in relative terms, BMO has 1178 bugs matching short_desc=^[-+=@]&short_desc_type=regexp (about one in a thousand)

Comment 43

[Muhammed Gamal]

does that back the case to Flags: sec-bounty?

Comment 44

[Frédéric Buclin]

(In reply to Daniel Veditz [:dveditz] from comment #42)
> space-escaping makes me worry some helpful filter is going to trim and then
> we're back where we started.

I agree with that, but in bug 1054702 sgreen said the single quote (correctly) remained visible in LibreOffice, and glob also asked to use a whitespace instead. Honestly, there is no reason for an application importing a CSV file to remove leading quotes (I call this data corruption), and so Excel again behave incorrectly. The whitespace is a compromise between readability and preventing Excel from executing formula.


> I suppose if Excel import doesn't trim then
> we've solved the immediate problem, but I'd rather have uglier summaries
> (starting with ') in this rare case than have to worry.

I would rather think the other way. If this fixes the problem with the Excel importer, then we did our job, and I don't think we should make another application's life harder due to leading quotes which are not part of the original data.

Comment 45

[Muhammed Gamal]

Updates ?

Comment 46

[Andre Klapper]

@Tazuwk: See previous comments in this task. 
If you have a specific question to somebody, please ask it. Thanks.

Comment 47

[Muhammed Gamal]

Does all of that make the bug eligable for bounty ?!

Comment 48

[:glob ✱]

(In reply to Muhammed Gamal from comment #47)
> Does all of that make the bug eligable for bounty ?!

this issue has already been marked as not eligible, regardless of the approach taken to mitigate excel's issue.

Comment 49

[Muhammed Gamal]

I still think it should be eliagble for bounty , the issue is many attack ma use your website for attack mozilla users ! 
If it's not critical bug why did u pay bounty for it in the past report ! .
Many bug compaines pay good bounties for bug like this !
And of course mozilla is a big one !

Comment 50

[:glob ✱]

(In reply to Muhammed Gamal from comment #49)
> I still think it should be eliagble for bounty

please take any further discussion of the bug's bounty eligibility to [e-mail], and not on this bug.

thank you.

Comment 51

[Paresh]

(In reply to Muhammed Gamal from comment #49)
> I still think it should be eliagble for bounty , the issue is many attack ma
> use your website for attack mozilla users ! 
> If it's not critical bug why did u pay bounty for it in the past report ! .
> Many bug compaines pay good bounties for bug like this !
> And of course mozilla is a big one !

If you want to discuss about Bounty mail here: [e-mail]  .

Comment 52

[Paresh]

there's another way to execute commands in excel using CSV.
Problem described here: http://stackoverflow.com/questions/2668678/importing-csv-with-line-breaks-in-excel-2007

Comment 53

[Muhammed Gamal]

i sent an email to [e-mail] and still has no answer , i still don't know how it is now not a critical issue and it was that in the past report ! , i could use this vulnerability to harm alot of bugzilla members which is a the place where all security reporter comes , and that means i will get these reports all and the reporters machines too !

Comment 54

[Frédéric Buclin]

(In reply to Muhammed Gamal from comment #53)
> i sent an email to [e-mail] and still has no answer , i still
> don't know how it is now not a critical issue and it was that in the past
> report ! , i could use this vulnerability to harm alot of bugzilla members
> which is a the place where all security reporter comes , and that means i
> will get these reports all and the reporters machines too !

Please stop complaining about this bug not being eligible for a bounty. The reasons have already been written several times, especially in comment 20, comment 22 and comment 23: Excel throws warnings about potential security issues when enabling formula execution, and you decided to ignore these warnings! You have been warned! This is what makes this bug report different from bug 1054702, where code was automatically executed.

Also, as mentioned in comment 33 and comment 36, executing data coming from a CSV file is not specified in RFC 4180. This is an issue with the CSV file reader, not the CSV file generator.

Comment 55

[Frédéric Buclin]

To ssh://[e-mail]/bugzilla/bugzilla.git
   e2b1136..ff441bc  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   ea0c502..8b0d558  5.0 -> 5.0

To ssh://[e-mail]/bugzilla/bugzilla.git
   94d623c..36253a7  4.4 -> 4.4