Closed Bug 1637745 (CVE-2020-15658) Opened 4 years ago Closed 4 years ago

An a.download with null characters results in unexpected filename on disk

Categories

(Toolkit :: Downloads API, defect, P2)

Product:
Toolkit
Component:
Downloads API
Version:
75 Branch
Type:
defect
Points:
3

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla80
Iteration:
79.2 - June 15 - June 28
Tracking Flags:
Tracking Status
firefox-esr68 --- wontfix
firefox-esr78 79+ verified
firefox78 --- wontfix
firefox79 + verified
firefox80 + verified

People

(Reporter: belden.lyman, Assigned: mak)

References

Details

(Keywords: csectype-spoof, sec-low, Whiteboard: [adv-main79+][adv-ESR78.1+])

Attachments

(4 files)

Screen Shot 2020-05-13 at 1.33.25 PM.png
1.36 MB, image/png
Details
Bug 1637745. r=gijs
47 bytes, text/x-phabricator-request
RyanVM
: approval-mozilla-beta+
RyanVM
: approval-mozilla-esr78+
Details | Review
Bug 1637745 - Test. r=gijs
47 bytes, text/x-phabricator-request
Details | Review
advisory.txt
295 bytes, text/plain
Details

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/81.0.4044.138 Safari/537.36

Steps to reproduce:

Triggering a client-side download of data can be done using this code:

// this is essentially how https://www.npmjs.com/package/downloadjs works.
const a = document.createElement('a');
a.href = URL.createObjectURL(new Blob(["<html><body><script>alert('hello')</script></body></html>"]));
a.download = 'my-filename.html\u0000.csv';
a.dispatchEvent(new MouseEvent('click'));

Actual results:

Running the code above results in Firefox opening the download manager and prompting the user to open or save the file. If the user chooses to save the file, the suggested filename appears to be "my-filename.html*.csv", where * is a unicode font graphic indicating U+0000.

In reality, the saved filename is "my-filename.html". Later, when the user navigates to their downloads and opens "my-filename.html", the javascript payload will be executed by their default browser.

The saved file becomes "HTML Text".

Expected results:

Chrome handles this by replacing the U+0000 in the filename with a U+005F (underscore).

Other characters above and beyond U+0000 may require similar treatment.

I assume someone smarter than me would be able to figure out how to use this as an attack vector, so I have ticked the security box for this bug.

I've updated to Firefox 76.0.1 and still see this bug.

Thanks for the report.

Marco, do you have cycles to investigate this?

Component: Untriaged → Downloads API
Flags: needinfo?(mak)
Product: Firefox → Toolkit

I'll have a look.

There are 2 mitigations to the problem I see here:

  1. The save file dialog says it's an html document
  2. the downloads lists (Library/Panel) also show it as my-filename.html

So, ideally the users know they're launching an html file. It's of course confusing that I see a name but we end up saving with another name

Assignee: nobody → mak
Severity: -- → S3
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(mak)
Priority: -- → P2

(In reply to Marco Bonardo [:mak] from comment #3)

I'll have a look.

There are 2 mitigations to the problem I see here:

  1. The save file dialog says it's an html document

I know there's a lot going on in the screenshot I provided - but the save dialog actually says it's a CSV file:

You have chosen to open:
my-filename.html.\u0000.csv
which is: CSV file (58 bytes)
from: blob:
  1. the downloads lists (Library/Panel) also show it as my-filename.html

So, ideally the users know they're launching an html file. It's of course confusing that I see a name but we end up saving with another name

So there's actually only 1 mitigation here, which is the user noticing the wrong extension.

Interesting, I tried both Firefox 76.01 and 78, and I see Firefox HTML Document there. I'm on Windows though.

Points: --- → 3
Iteration: --- → 78.2 - May 18 - May 31

(In reply to Marco Bonardo [:mak] from comment #5)

Interesting, I tried both Firefox 76.01 and 78, and I see Firefox HTML Document there. I'm on Windows though.

Got it - I'm on macOS Catalina - Version 10.15.3.

(I'm sorry for not including that earlier! I thought it was captured from my browser when I filed the bug report.)

Do we potentially launch the wrong kind of helper for the file, based on the wrong name? On Mac it looks like at that point we know the real extension and don't, but maybe on windows we're passing the name with the embedded null and coming up with a different answer. We should at least replace any embedded \x00 in any string we pass to the OS.

Keywords: csectype-spoof, sec-low

I must note Chrome replaces the null char with an underscore and respects the original extension.

Here is what I could check so far.
On Windows in the dialog I see the original filename, and Firefox HTML Document, the final name is my-filename.html.
Having csv in about:preferences application doesn't change things, I still see Firefox HTML Document.
I debugged into netwerk and nsExternalHelperAppService to check if by chance we were mishandling the filename string (for example when we serialize it through ipc), but we seem to handle is correctly, as a 21 chars string. The memory representation of the string is correct.
The contentType and mime in nsExternalHelperAppService is text/html.

Next steps: test on Mac (once I finish updating its OS), check at which point we replace the .html\u000.csv extension with the primary extension, check where we decide that contentType is text/html.

Iteration: 78.2 - May 18 - May 31 → 79.2 - June 15 - June 28

I still cannot reproduce getting a "CSV File" description here, either on Mac (Catalina) or Windows 10, it's always recognized as an HTML document. That description would come from here:
https://searchfox.org/mozilla-central/rev/eef39502e08bcd3c40573c65a6827828dce4a032/uriloader/exthandler/nsExternalHelperAppService.cpp#2625
Recently quite some of this code changed, so it's possible recent version have a different behavior, though it's strange in your case we'd not have either a recognized extension nor mime type. But it's clearly associated to Chrome and also the file icon points to that.
Could you please check if a a more recent version of Firefox still returns CSV File?
Could you please attach your handlers.json file from the profile folder?

Flags: needinfo?(belden.lyman)

We could reproduce getting csv by adding {type: "text/csv"} to the blob constructor. I must find where we decide the content type for the blob.

I think the problem happens here https://searchfox.org/mozilla-central/rev/5e6c7717255ca9638b2856c2b2058919aec1d21d/uriloader/exthandler/nsIHelperAppLauncherDialog.idl#82-83
nsExternalHelperAppService.cpp is calling into promptForSaveToFileAsync passing raw pointers (aDefaultFile.get(), aFileExtension.get()) to wstrings, that are likely to be confused by NULL. We should replace those chars a bit sooner, so we show a proper name also in the dialog itself and keep using sanitized strings without nulls that would confuse this call.

Attached file Bug 1637745. r=gijs —

Depends on D80345

Flags: in-testsuite?

My handlers.json

{"defaultHandlersVersion":{"en-US":4,"en-GB":4},"mimeTypes":{"application/pdf":{"action":3,"extensions":["pdf"]},"application/x-apple-diskimage":{"action":0,"ask":true,"extensions":["dmg"]},"application/binary":{"action":4,"ask":true,"extensions":["dmg"]},"text/plain":{"action":0,"ask":true,"extensions":["txt","asc","text","csv"]},"text/html":{"action":0,"ask":true,"handlers":[{"name":"Firefox","path":"/Applications/Firefox.app"}],"extensions":["csv","txt","asc","text"]}},"schemes":{"webcal":{"action":4,"ask":true},"ircs":{"action":2,"ask":true,"handlers":[null,{"name":"Mibbit","uriTemplate":"https://www.mibbit.com/?url=%s"}]},"mailto":{"action":4,"handlers":[null,{"name":"Yahoo! Mail","uriTemplate":"https://compose.mail.yahoo.com/?To=%s"},{"name":"Gmail","uriTemplate":"https://mail.google.com/mail/?extsrc=mailto&url=%s"}]},"irc":{"action":2,"ask":true,"handlers":[null,{"name":"Mibbit","uriTemplate":"https://www.mibbit.com/?url=%s"}]}}}

I just updated Firefox to 77.0.1 (64-bit) and am still able to reproduce this by pasting the code I provided in the initial description (i.e. I don't need to supply {type: "text/csv"}).

I'm on Catalina 10.15.3 (19D76). I can update to latest but don't think it'll affect this particular issue.

I've changed my default browser to Firefox which affects the Finder window's icon for the "csv" (actually .html) documents I'm downloading, but obviously the default browser doesn't change the filename which is written to disk.

Flags: needinfo?(belden.lyman)

In that list I see

text/plain":{"action":0,"ask":true,"extensions":["txt","asc","text","csv"]}
"text/html":{"action":0,"ask":true,"handlers":[{"name":"Firefox","path":"/Applications/Firefox.app"}],"extensions":["csv","txt","asc","text"]}}

I'm not sure why csv is marked as an extension for text/html, Gijs may have more insight here.
The patch should fix the bug at hand anyway, I was just curious about why we would use the fallback description here...

In the review Gijs asked me to check content-disposition too.
Apparently NS_GetContentDispositionFromHeader reads the content-disposition header through nsMIMEHeaderParamImpl::DoParameterInternal that pretty much stops at the first null, so we skip anything after a null: https://searchfox.org/mozilla-central/rev/82c04b9cad5b98bdf682bd477f2b1e3071b004ad/netwerk/mime/nsMIMEHeaderParamImpl.cpp#465

I was curious to check what Chrome does in this case, and to my surprise I got an ERR_INVALID_HTTP_REPONSE error page. Digging around I found https://bugs.chromium.org/p/chromium/issues/detail?id=927364 where developers say Chrome by choice returns an invalid http response error if the headers contain null characters. And they also say Firefox developers were contacted to ask whether interested into doing the same.
I tried to find a bug in Bugzilla about that, but I couldn't.

Basically if we'd error out on headers containing null characters, we 'd not require any handling here, otherwise I'm not sure what outcome to expect, should we change the parsing code to replace nulls with _ in certain cases? It seems a scarier path to take.

Flags: needinfo?(dd.mozilla)

Ok, I found the source of the discussion:
https://github.com/whatwg/xhr/issues/165
and our bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1453318

Since Chrome has this for a long time without major problems, we are safe to implement the same, i.e. return a protocol error if null is found.

Flags: needinfo?(dd.mozilla)

(In reply to Dragana Damjanovic [:dragana] from comment #17)

Ok, I found the source of the discussion:
https://github.com/whatwg/xhr/issues/165
and our bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1453318

Since Chrome has this for a long time without major problems, we are safe to implement the same, i.e. return a protocol error if null is found.

Thank you, that's much appreciated.
I think for the report here we should be fine with my patch, since we can't really receive a filename containing null through headers, so the "download" attribute seems the only other relevant source. I assume the network team can take care of bug 1453318 separately.

https://hg.mozilla.org/integration/autoland/rev/0cabef091ee349b80772c963fe333d5dae4947fd
https://hg.mozilla.org/mozilla-central/rev/0cabef091ee3

Group: firefox-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox80: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla80

Should we try to get this into 79 and/or 78esr?

Flags: needinfo?(mak)

I think the risk is very small, so why not.

Flags: needinfo?(mak)

Comment on attachment 9157885 [details]
Bug 1637745. r=gijs

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: this can lead the user to think they're downloading a certain kind of file, when in reality it's something different, that can bring to unexpected code execution
  • User impact if declined: Unexpected code execution
  • Fix Landed on Version: 80
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The changes are simple, mostly replacing null characters with underscores
    In the future this will be covered by an automated test
  • String or UUID changes made by this patch:

Beta/Release Uplift Approval Request

  • User impact if declined: Unexpected code execution
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: See comment 0 testcase
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The changes are simple, mostly replacing null characters with underscores
    In the future this will be covered by an automated test
  • String changes made/needed:
Attachment #9157885 - Flags: approval-mozilla-esr68?
Attachment #9157885 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Comment on attachment 9157885 [details]
Bug 1637745. r=gijs

ESR Uplift Approval Request

Oops, I mearked the wrong ESR, pease see previous request.

Attachment #9157885 - Flags: approval-mozilla-esr68? → approval-mozilla-esr78?

Comment on attachment 9157885 [details]
Bug 1637745. r=gijs

Approved for 79.0rc1 and 78.1esr.

Attachment #9157885 - Flags: approval-mozilla-esr78?
Attachment #9157885 - Flags: approval-mozilla-esr78+
Attachment #9157885 - Flags: approval-mozilla-beta?
Attachment #9157885 - Flags: approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

Reproduced the initial issue using old Nightly from 2020-05-13, verified that this is fixed using Latest Nightly 80.0a1, 78.1.0esr and 79.0 RC across platforms (macOS 10.15, Windows 10 64bit and Ubuntu 18.04 64bit).

Status: RESOLVED → VERIFIED
status-firefox79: fixed → verified
status-firefox80: fixed → verified
status-firefox-esr78: fixed → verified
Flags: qe-verify+
Whiteboard: [adv-main79+]
Whiteboard: [adv-main79+] → [adv-main79+][adv-ESR78.1+]
Attached file advisory.txt —
Alias: CVE-2020-15658
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: