Closed Bug 1531475 Opened 4 years ago Closed 3 years ago

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

Categories

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

59 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla75
Tracking Status
firefox-esr68 - wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox73 --- wontfix
firefox74 --- wontfix
firefox75 + fixed

People

(Reporter: robert.strong.bugs, Assigned: mossop)

References

Details

(Keywords: sec-audit, Whiteboard: [post-critsmash-triage][adv-main75-])

Attachments

(1 file, 1 obsolete file)

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.

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

See Also: → CVE-2019-9794
Priority: -- → P3
See Also: → CVE-2020-6799

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.

Flags: needinfo?(mh+mozilla)
Flags: needinfo?(dtownsend)

(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.

Flags: needinfo?(dtownsend)
Attached patch patch (obsolete) — Splinter Review

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.

Assignee: nobody → dtownsend
Status: NEW → ASSIGNED
Flags: needinfo?(nfroyd)
Attachment #9118943 - Attachment is obsolete: true

(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)

Flags: needinfo?(nfroyd)

We just got bitten by them again (bug 1606596)

You are not authorized to access bug 1606596.

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

You are not authorized to access bug 1606596.

I've CC'd you and :froydnj

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.

Flags: needinfo?(mh+mozilla)

(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.

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.

(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.

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.

Flags: needinfo?(snorp)

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.

Flags: needinfo?(dtownsend)

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.
Flags: needinfo?(dtownsend)
Attachment #9118944 - Flags: sec-approval?

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

Attachment #9118944 - Flags: sec-approval? → sec-approval+
Group: firefox-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

Please request esr68 uplift when you get a chance.

Flags: needinfo?(dtownsend)

(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.

Flags: needinfo?(dtownsend)
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main75-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.