1468217 - (CVE-2018-12368) .SettingContent-ms file extension bypasses 'dangerous file' prompt leading to WebExt RCE

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Abdulrahman Alqabandi]

Attachment: download.openRCE.zip

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

Steps to reproduce:

Download attached SettingContent-ms file, or install attached extension for RCE.


Actual results:

SettingContent-ms is a new windows 10 extension that can be used to execute arbitrary local files with parameters.


Expected results:

Treat it as executable.

I am CCing Matt Nelson who originally found this file, I simply connected the dots with the browser RCE potential. 

He wrote about this file in public https://posts.specterops.io/the-tale-of-settingcontent-ms-files-f1ea253e4d39

Web Extension RCE part is not mentioned.

Comment 1

[Abdulrahman Alqabandi]

Attachment: test.SettingContent-ms

Comment 2

[Andrew McCreight [:mccr8]]

Bob, do you have any idea what we should do with this? It looks like it could be kind of Windows platform integration-y.

Comment 3

[Matt Nelson]

FWIW, this was reported to MSRC and they responded that the severity of the issue is below the bar for servicing and that the case will be closed.

Cheers,
Matt N.

Comment 4

[Matt Nelson]

I should be more clear, the "this" in the above comment was the .SettingContent-ms file format. The MS disclosure timeline is at the bottom of this post: https://posts.specterops.io/the-tale-of-settingcontent-ms-files-f1ea253e4d39


Cheers,
Matt N.

Comment 5

[Abdulrahman Alqabandi]

Would also like to highlight an important part of the article Matt wrote (in case you didn't read it, worth a read tho!)  - The mark of web is created for the file once downloaded with healthy content, but for some reason windows does not show any warning before executing. So this is a clean RCE requiring a single user click. (after installing malicious extension ofc)

Comment 6

[Johann Hofmann [:johannh]]

The extension is calling browser.downloads.open, at which point the OS is up to protect you against arbitrary RCE. Microsoft is apparently considering this issue too low severity to fix. Now, we could either follow them and WONTFIX or we say that we consider this severe enough to add a blacklist of sorts to prevent opening this filetype via the downloads API.

In any case it seems to me like that decision should be made at the WebExtensions API level, so moving this to WebExtensions for now...

Comment 7

[:Gijs (he/him)]

(In reply to Johann Hofmann [:johannh] from comment #6)
> The extension is calling browser.downloads.open, at which point the OS is up
> to protect you against arbitrary RCE. Microsoft is apparently considering
> this issue too low severity to fix. Now, we could either follow them and
> WONTFIX or we say that we consider this severe enough to add a blacklist of
> sorts to prevent opening this filetype via the downloads API.

I thought we had a similar blacklist to what Windows does in terms of which files are executable, e.g. the stuff in $PATHEXT or whatever the env var is? I would expect us to warn for calling downloads.open() on this filetype in the same way as when a user clicks a download that would pop up the 'this file is executable, are you sure you want to run it' warning.

:paolo might know more about this too, but he's not accepting needinfo requests...

Comment 8

[:Gijs (he/him)]

+ni to keep this on Paolo's radar. :-)

Comment 9

[Bob Owen (:bobowen)]

I don't know too much about how we interact with this sort of thing I'm afraid.

Comment 10

[:Paolo Amadini]

Attachment: The patch

Adding "SettingContent-ms" to the list of executable file extensions solves the issue of the missing warning, both when opening from the Downloads Panel and from extension code.

In fact, we had other cases where Windows didn't warn before executing some file types even when they had the zone information, hence we now display our own warning dialog for every executable extension except "exe", even if it may be redundant.

The patch is straightforward and the general issue is public, so I've just stated what it does in the commit message, without any details on the steps to reproduce.

I already tested this with a local build that I ran on Windows 10. I believe it should be quite safe to land the patch in all branches, even at RC stage, so it may be already available to users in the release next week.

Comment 11

[:Paolo Amadini]

Comment on attachment 8985901 [details] [diff] [review]
The patch

Since this patch is quite straightforward, I'm already requesting the uplift flags so we can get this on the Release Management radar sooner. As mentioned in the previous comment, I think this is quite safe to land on all branches.

Let us know if you'd like us to land this on mozilla-central first, or just have the sheriffs land everywhere at the same time.

Approval Request Comment
[Feature/Bug causing the regression]: Recently announced security issue
[User impact if declined]: Code execution on the local machine without warning if an extension creates a file with a "SettingContent-ms" extension
[Is this code covered by automated tests?]: Not automatically testable, it effectively patches a Windows issue with a missing warning even if the file is correctly marked as having a remote source
[Has the fix been verified in Nightly?]: Yes, but only on one local build
[Needs manual test from QE? If yes, steps to reproduce]: Yes, using the proof of concept attached to this bug
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Very low risk
[Why is the change risky/not risky?]: This only affects the download and file opening code path, and we don't actually use this file extension for anything in the source tree anyways.
[String changes made/needed]: None
[If this is not a sec:{high,crit} bug, please state case for ESR consideration]: Improves security from code execution on the local machine with minimal risk.

Comment 12

[:Gijs (he/him)]

Comment on attachment 8985901 [details] [diff] [review]
The patch

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

rs=me.

For a separate follow-up: Is there no Windows API we can use here that would avoid forever playing catch-up with whatever Windows decides are executable files?

Comment 13

[Ryan VanderMeulen [:RyanVM]]

This needs to get sec-approval first (or at least be assessed a sec rating of moderate or lower) before it can land. Also, we're about to build the RC of Fx61, so I'm a bit iffy about taking this at this point in the cycle.

Comment 14

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to :Gijs (he/him) from comment #12)
> Comment on attachment 8985901 [details] [diff] [review]
> The patch
> 
> Review of attachment 8985901 [details] [diff] [review]:
> -----------------------------------------------------------------
> For a separate follow-up: Is there no Windows API we can use here that would
> avoid forever playing catch-up with whatever Windows decides are executable
> files?

Looks like AssocIsDangerous is what Explorer itself uses. Ironically it doesn't appear to pick up ".settingcontent-ms" when testing on my machine.
https://msdn.microsoft.com/en-us/library/windows/desktop/bb773465(v=vs.85).aspx

Comment 15

[:Gijs (he/him)]

Dan/Al, can you rate this?

As filed, the webextensions side of things is limited to sec-moderate anyway, so then it wouldn't need sec-approval.

I don't think the "manual" download + open side of things is severe enough to be sec-high/sec-crit, but even if it is, given the time in the cycle we're at and the safety of the patch (1 entry addition to a list of file extensions), I think we should just land this on all branches in time for shipping 61.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)
> This needs to get sec-approval first (or at least be assessed a sec rating
> of moderate or lower) before it can land. Also, we're about to build the RC
> of Fx61, so I'm a bit iffy about taking this at this point in the cycle.

So, to be really clear, I think this is about as safe a C++ patch as we're likely to see, and we should take it.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

This looks like a sec-moderate to me, for the reasons stated in comment 15.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8985901 [details] [diff] [review]
The patch

I'm building the Fx61 RC build today, so this needs to land on inbound ASAP to have any chance of being included.

Comment 18

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #17)
> Comment on attachment 8985901 [details] [diff] [review]
> The patch
> 
> I'm building the Fx61 RC build today, so this needs to land on inbound ASAP
> to have any chance of being included.

Your wish etc.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca7b77362ad3f1740eb866e3435cd3acddfe3ce9

Comment 19

[:Paolo Amadini]

Thanks Gijs for landing this sooner, I was traveling today.

(In reply to Aaron Klotz [:aklotz] from comment #14)
> Looks like AssocIsDangerous is what Explorer itself uses. Ironically it
> doesn't appear to pick up ".settingcontent-ms" when testing on my machine.

This may be the reason why Windows Explorer does not warn about the file, as the documentation mentions that "ShellExecuteEx uses AssocIsDangerous to trigger zone checking".

Ideally we would have a way to serve additional entries in this list remotely using one of the already available mechanisms, or at least have the list defined in JavaScript so we could use a hotfix if necessary. We can also supplement the list using AssocIsDangerous, but it wouldn't handle cases like this bug.

While we're thinking about possible improvements, one thing I noticed is that the warning can be disabled globally and permanently using the checkbox. I think this ability should be removed, because it applies to every file type, not just the individual file being opened, and also applies to actions triggered by browser extensions. This is unclear from the current user interface, and there is no way to undo this action save from editing "about:config".

Comment 20

[:Gijs (he/him)]

(In reply to :Paolo Amadini from comment #19)
> Thanks Gijs for landing this sooner, I was traveling today.
> 
> (In reply to Aaron Klotz [:aklotz] from comment #14)
> > Looks like AssocIsDangerous is what Explorer itself uses. Ironically it
> > doesn't appear to pick up ".settingcontent-ms" when testing on my machine.
> 
> This may be the reason why Windows Explorer does not warn about the file, as
> the documentation mentions that "ShellExecuteEx uses AssocIsDangerous to
> trigger zone checking".
> 
> Ideally we would have a way to serve additional entries in this list
> remotely using one of the already available mechanisms, or at least have the
> list defined in JavaScript so we could use a hotfix if necessary. We can
> also supplement the list using AssocIsDangerous, but it wouldn't handle
> cases like this bug.
> 
> While we're thinking about possible improvements, one thing I noticed is
> that the warning can be disabled globally and permanently using the
> checkbox. I think this ability should be removed, because it applies to
> every file type, not just the individual file being opened, and also applies
> to actions triggered by browser extensions. This is unclear from the current
> user interface, and there is no way to undo this action save from editing
> "about:config".

Can you file bugs for these follow-ups? I think we should at least split off the webextensions usage from the 'normal' usage of the pref (ie prompt for the webext case even if the user turns off the prompt generally).

Comment 21

[:Paolo Amadini]

Yes, I planned to do this next week after the release.

For the checkbox, I can consider replacing it with two separate "about:config" settings, but if there is no way to undo the choice from "about:preferences" then I don't think we should have the checkbox in the first place. The equivalent Windows warnings can also only be disabled globally from the registry. We can also ask the UX team for an opinion on this. We should also not reuse the existing "about:config" setting just in case users disabled the warning accidentally, possibly years in the past.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8985901 [details] [diff] [review]
The patch

Safe fix for a Windows sec issue. Approved for 61.0rc1, ESR 60.1, and ESR 52.9.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/6f3df7ea04aa
https://hg.mozilla.org/releases/mozilla-esr60/rev/ef41d7f4cc72
https://hg.mozilla.org/releases/mozilla-esr52/rev/909ac90494b4

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ca7b77362ad3

Comment 25

[marius.santa]

Attachment: screenshots.zip

I've tested the issue on:
Firefox 61.0 (20180618212916)
Firefox 60.0.3esr (20180618213404)
Firefox 52.8.2 (20180618215607) - Still reproducible

The extension scenarios works as shown in (open file notification.gif)the gif attached, and the warning is displayed but the second scenario with downloading the file does not trigger any warning as you can see in (download 1.gif).
Please provide alternative steps or a new file to test both scenarios and validate this issue.
Issue is still reproducible in both scenarios on Firefox 52.8.2 (20180618215607), no warning is displayed for the user when opening the file from the browser.

Comment 26

[Matt Nelson]

If you guys need a truly hosted PoC, I have one up here: https://nofile.io/f/ctDCYhkUnob/ControlPanel.settingcontent-ms

Just click "download". Not sure if the format is pulled differently linked vs attached here. 

Cheers,
Matt N.

Comment 27

[:Paolo Amadini]

Thanks Matt for the link!

(In reply to marius.santa from comment #25)
> The extension scenarios works as shown in (open file notification.gif)the
> gif attached, and the warning is displayed but the second scenario with
> downloading the file does not trigger any warning as you can see in
> (download 1.gif).

In your case, the file was downloaded with a ".xml" extension, which depends on the content type of the attachment, which is "application/xml". Whether the extension is added or not may depend on the local configuration, but when it's added, there is no security issue because the file will not be executed by Windows.

The link Matt provided may solve this issue. If this doesn't you may have to set up a local HTTP server, which is what I did for testing.

> Issue is still reproducible in both scenarios on Firefox 52.8.2
> (20180618215607), no warning is displayed for the user when opening the file
> from the browser.

This can be an issue with the build being tested not having the patch, the preference to disable the warning being accidentally enabled in the profile, or the WebExtensions API not going through the same code path as the newer versions.

If you can test the Downloads Panel flow there, this will tell us if we are in the latter scenario.

Comment 28

[marius.santa]

Attachment: screenshots 2.zip

Retested and verified on Windows 10 64Bit:
Firefox 60.1.0esr| (20180619173714)
Firefox 61.0 (20180618212916)
Screenshot for working scenarios is attached.

Issue is still reproducible on:
Firefox 52.9.0 (20180619102821).
Screenshot reproducing the bug is attached.
The same profile was used to test on 52est and the other builds, so this is not a profile related issue.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

We typically don't reopen bugs for branch-specific verification issues. Setting the status back to affected and NI the assignee is a better way :). Also, was this verified on Nightly?

Comment 30

[:Paolo Amadini]

(In reply to marius.santa from comment #25)
> Firefox 52.8.2 (20180618215607) - Still reproducible
> Firefox 52.9.0 (20180619102821).

I don't know how to go from the build date to the exact changeset from which it was built, so we can verify that the revision from comment 23 is actually included. Ryan, can you help here? This is the first thing that I would check, since the verification fails on both the Downloads Panel flow and the extension flow, which is strange.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

The 52.9.0 build would definitely have this patch. It landed before the version was bumped.

Comment 32

[:Paolo Amadini]

Ah, so what we miss in Firefox 52 is bug 1369771, which only landed in 55. I'm not sure this is upliftable at this point in the cycle though, especially given the rating of sec-moderate. Opinions?

Comment 33

[Daniel Veditz [:dveditz]]

(In reply to :Gijs (he/him) from comment #12)
> Is there no Windows API we can use here that would avoid forever playing
> catch-up with whatever Windows decides are executable files?

Apparently not since the blog post was about getting Microsoft's star application to open these files, because it's not on the list they use either.

(In reply to :Paolo Amadini from comment #32)
> Ah, so what we miss in Firefox 52 is bug 1369771, which only landed in 55.
> I'm not sure this is upliftable at this point in the cycle though,
> especially given the rating of sec-moderate. Opinions?

bug 1369771 looks like it would land pretty cleanly so I could support taking it. ESR-52 doesn't have WebExtensions, but without bug 1369771 the problem is arguably worse.

Comment 34

[Daniel Veditz [:dveditz]]

We may have ESR 52.9 candidate builds already though.

Comment 35

[Matt Nelson]

Regarding AssocIsDangerous, it’s a good API to use. I’m reaching out to MS to see if they will consider adding this file format, but if they do, it will probably only be in new Windows 10 builds.

Thanks for the quick response to this!!

Comment 36

[Julien Cristau [:jcristau]]

Bug 1369771 will be in 52.9.0-build2.

Comment 37

[marius.santa]

Retested and verified in Windows 10 64Bit on Firefox 62.0a1 (20180621100048).

Comment 38

[Julien Cristau [:jcristau]]

I'm assuming the change to 52 status in comment 37 was not intentional.

Comment 39

[marius.santa]

Retested and verified in Windows 10 64Bit on Firefox 52.9.0 (20180621064021).

Comment 40

[:Paolo Amadini]

This is now described in the release notes, so I'll go ahead and file the dependent bugs.