1531475 - Evaluate whether the greomni and appomni command line args can be removed or have restrictions for their paths

Status: RESOLVED FIXED

(Toolkit :: Startup and Profile System, enhancement, P3)

Description

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

If these command line arguments can be removed then they should be. If they can't be removed they probably shouldn't allow UNC paths and possibly have other restrictions.

Comment 1

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

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

Comment 2

[:Gijs (he/him)]

Mike/Dave, do you know of any reason we need to keep supporting these arguments? We just got bitten by them again (bug 1606596), so if we can rm+rf this or make it child-process-specific or... well, anything to harden this avenue for abuse (signed omni.ja would also work...), that'd be great.

Comment 3

[Dave Townsend [:mossop]]

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

Mike/Dave, do you know of any reason we need to keep supporting these arguments? We just got bitten by them again (bug 1606596), so if we can rm+rf this or make it child-process-specific or... well, anything to harden this avenue for abuse (signed omni.ja would also work...), that'd be great.

I don't know of any reason to keep supporting these in the main process. Not sure about the child process, I assume we pass it for a reason but I don't know what that is off-hand. I can try experimenting with that.

Comment 4

[Dave Townsend [:mossop]]

Looks like geckoview is using this: https://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoThread.java#298

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch

As far as I can tell this doesn't break anything, it leaves the handling in place for android where it seems to be required and I figured that that is reasonably safe given that I don't know of any way to call apps with command line arguments there.

Comment 6

[Dave Townsend [:mossop]]

Attachment: Bug 1531475: Drop the greomni and appomni command line arguments. r=froydnj

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Dave Townsend [:mossop] (he/him) from comment #5)

As far as I can tell this doesn't break anything, it leaves the handling in place for android where it seems to be required and I figured that that is reasonably safe given that I don't know of any way to call apps with command line arguments there.

So reading through and trying to poke around a little bit, the argument is something like "Omnijar::Init should be able to figure out where these are anyway, and if it couldn't, then we have bigger problems?" (Also that AFAICT, those pieces of code in GeckoChildProcessHost.cpp are just performing the same path computation that we'd do in the child process anyway?)

Do we know why Omnijar::Init on Android can't figure out where those files are? (i.e. we should be able to remove the code in java.org.mozilla.gecko.GeckoThread)

Comment 8

[Mike Hommey [:glandium]]

We just got bitten by them again (bug 1606596)

You are not authorized to access bug 1606596.

Comment 9

[:Gijs (he/him)]

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

You are not authorized to access bug 1606596.

I've CC'd you and :froydnj

Comment 10

[Mike Hommey [:glandium]]

So... -greomni and -appomni are more or less a red herring. The exploit could just as well use any other option... like -xpcshell...

With that being said, the only reason these specific two options exist, as far as I remember, is that I've split, in bug 620931, a preexisting option that was added in bug 556644. And the reason that option had been added, is, presumably, https://bugzilla.mozilla.org/show_bug.cgi?id=556644#c29.

10 years later, if we can reliably get the right omni jars without the command line argument in child processes, we should just do that.

Comment 11

[:Gijs (he/him)]

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

So... -greomni and -appomni are more or less a red herring. The exploit could just as well use any other option... like -xpcshell...

Yeah, this is fair - though that doesn't mean we should leave this lying around as an obvious target.

I don't think xpcshell would work given the check at https://searchfox.org/mozilla-central/rev/4537228c0a18bc0ebba2eb7f5cbebb6ea9ab211c/browser/app/nsBrowserApp.cpp#170 requires that it is the first argument.

It wouldn't be a bad idea to audit all the other commandline switches we use for things that could take UNC paths but shouldn't. If we're invoking xpcshell, for instance, I'm pretty comfortable saying we shouldn't be loading scripts from anywhere but the local machine.

That said, all of these are mitigations - the -osint checks are supposed to prevent all of this. They are now very strict, and the only reason it's not working is that the external invocation that Excel uses here does not use that flag, whereas we do use it for all the default-associated files.

Comment 12

[Joshua Graham]

Also note that black listing UNC paths for the -greomni/appomni parameters won't work because bug 1606596 takes advantage of the fact that the current working directory of Firefox is set to the UNC path where the file was launched from. i.e. -greomni currentDir.ja grabs the file the current directory which is a UNC path even though it doesn't start "//"

The behaviour of setting the current working directory to the value of the file that was launched might cause other issues too which i'm looking into. if it does i'll open a new bug.

Comment 13

[Dave Townsend [:mossop]]

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

(In reply to Dave Townsend [:mossop] (he/him) from comment #5)

As far as I can tell this doesn't break anything, it leaves the handling in place for android where it seems to be required and I figured that that is reasonably safe given that I don't know of any way to call apps with command line arguments there.

So reading through and trying to poke around a little bit, the argument is something like "Omnijar::Init should be able to figure out where these are anyway, and if it couldn't, then we have bigger problems?" (Also that AFAICT, those pieces of code in GeckoChildProcessHost.cpp are just performing the same path computation that we'd do in the child process anyway?)

Do we know why Omnijar::Init on Android can't figure out where those files are? (i.e. we should be able to remove the code in java.org.mozilla.gecko.GeckoThread)

Yeah I'm not sure of this and I'd love to hear back from snorp. I did try removing the code on android too but it definitely caused failures so there is something not working there.

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

On Android, the omnijar is inside the APK, so the path looks like /data/app/org.mozilla.fenix.apk!/assets/omni.ja. Gecko doesn't know where the APK is unless we tell it, so that's why things blow up without this argument. We can change to something else, but an env var is probably the only reasonably easy thing for us.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:mossop, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 16

[Dave Townsend [:mossop]]

Comment on attachment 9118944 [details]
Bug 1531475: Drop the greomni and appomni command line arguments. r=froydnj

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It requires exploiting something else on the system first in order to call Firefox with the command line rguments.
• 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 of them
• 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?: I haven't tested but likely the patch will just apply to other branches, if not it should be trivial to fix it.
• How likely is this patch to cause regressions; how much testing does it need?: I would expect any regressions to show up in automated tests.

Comment 17

[Dave Townsend [:mossop]]

Ok well I think leaving the usage for android isn't really an issue so I think we should just land this.

Comment 18

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

https://hg.mozilla.org/integration/autoland/rev/3da091470b26ddc6cb8bd03c4b6a672e13fbb9af
https://hg.mozilla.org/mozilla-central/rev/3da091470b26

Comment 19

[Julien Cristau [:jcristau]]

Please request esr68 uplift when you get a chance.

Comment 20

[Dave Townsend [:mossop]]

(In reply to Julien Cristau [:jcristau] from comment #19)

Please request esr68 uplift when you get a chance.

I'm not sure that we should uplift this. It's a breaking change to something that I've heard that some folks use.

Comment 21

[Julien Cristau [:jcristau]]

Thanks, updating esr68 status per comment 20.