1390882 - (CVE-2018-5105) WebExtension can bypass warnings and run arbitrary programs with download.download & downloads.open

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[Rob Wu [:robwu]]

Attachment: downloads-download-open.zip

Tested on Windows 7, Firefox 55.0.1 (20170809080026) and Firefox 57.0a1 (20170815100349)

Extensions written using the WebExtensions API are usually shielded from the user's OS and filesystem.

An extension can save arbitrary data to the user's filesystem using downloads.download.
And with downloads.open, the extension can open the file. If the file is dangerous (e.g. .exe opposed to .txt), then normally there is a warning dialog to make users aware that a program is being downloaded and run.

However, this check is trivial to bypass.
1. Load the attached extension (e.g. download, unzip it and load it via about:debugging).
2. On Windows, the extension will open a cmd shell (and start mspaint) without warning.
   (When I tested on Linux, a text editor was opened without warning)


What happens is that:
1. The extension calls downloads.download with a file name that ends with a NULL byte (e.g. "foo.bat\x00").
2. downloads.open ultimately ends up calling DownloadIntegration.launchDownload, where the download prompt is only guarded behind whether a file is "executable" [1].
3. isExecutable is implemented in nsLocalFileWin.cpp, as nsLocalFile::IsExecutable. When faced with a trailing NUL byte, the blacklist-based check fails (because "cmd\x00" is not equal to "cmd").
4. Then mimeInfo.preferredAction is set to system, and mimeInfo.launchWithFile is called. [2]
5. The system will ignore the NUL byte and treat the file as "foo.bat" and spawn a shell.


In Firefox beta (56), a user gesture requirement was added to downloads.open (bug 1369782), which makes this bug only a tiny little bit less severe (extensions can open tabs at will and it is not unlikely for the user to click or type within that page).


What should have happened is that the file extension was recognized as dangerous, either at the WebExtension layer in ext-downloads.js, or in nsLocalFile::IsExecutable in nsLocalFileWin.cpp

[1] https://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#607-617
[2] https://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#648-657

Comment 1

[Andrew Swan [:aswan]]

I think the windows version of nsLocalFile::IsExecutable() is the right place to fix this.

Comment 2

[:Paolo Amadini]

(In reply to Andrew Swan [:aswan] from comment #1)
> I think the windows version of nsLocalFile::IsExecutable() is the right
> place to fix this.

No, we should validate the file name instead, to avoid a number of other security issues like writing to Alternate Data Streams.

We had a long discussion about file name validation starting with the first part of bug 1254327 comment 14, where I also asked to file a release blocker bug, but that was not filed and hence forgotten, and the conversation ended with bug 1254327 comment 21.

The validation functions are not unified as I mentioned in that last comment, and moving them to a central place might make it easier to solve bugs like this, but you can also make a local copy first if you think it can help to solve the immediate issues in WebExtensions.

Comment 3

[Stephanie Ouillon [:arroway] (needinfo me)]

This looks like an actionable bug, and a serious enough security issue. Who is working on this? Andrew?

Comment 4

[Andrew Swan [:aswan]]

I'm not working on this and I have my hands full with other things for 57.  In any case, I don't think I have the necessary background, I asked for more detail in the other bug Paolo mentioned but as he says, the discussion there petered out a long time ago.

Comment 5

[:Paolo Amadini]

This and a significant number of other potential issues with the file names should have now been handled by 1402279, though according to the guidelines we shouldn't link the bugs together as dependencies or comment in the other bug just yet. Panos still recommended commenting here to keep everyone in the loop.

I just verified that the attached proof of concept doesn't work in Nightly as expected. After considering this bug and weighing the risks, Sylvestre recommended not to uplift the other bug to Beta, to which I agreed, so this should be fixed in Firefox 58.

Comment 6

[Panos Astithas (he/him) [:past] (please ni?)]

It sounds like we should close this bug per comment 5, but I'll leave that up to the sec triage folks.