579388 - e10s: Remote nsOSHelperAppService

Status: RESOLVED FIXED

(Firefox :: File Handling, defect)

Description

[Michael Wu [:mwu]]

nsOSHelperAppService on Android requires access to the JRE, which is not available in the child process.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

The "unix" implementation (desktop Linux and similar) is a problem for sandboxing: it reads the mailcap file and runs shell commands specified there (the "test" field) in response to web content accessing properties of navigator.mimeTypes.

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

Android only

Comment 3

[Gian-Carlo Pascutto [:gcp]]

(In reply to Jed Davis [:jld] [⏰PDT; UTC-7] from comment #1)
> The "unix" implementation (desktop Linux and similar) is a problem for
> sandboxing: it reads the mailcap file and runs shell commands specified
> there (the "test" field) in response to web content accessing properties of
> navigator.mimeTypes.

The use of this code seems to be triggered from 
https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2716
https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2624

But that code seems to currently run in the parent:
[Parent 8919] WARNING: The dialog should nullify the dialog progress listener: file /home/morbo/git/gecko-dev/uriloader/exthandler/nsExternalHelperAppService.cpp, line 216

We couldn't produce a seccomp (fork/execve) violation by feeding Firefox unknown mimetypes, so I think this no longer blocks sandboxing on Linux.

Comment 4

[Julian Hector [:tedd] [:jhector]]

This bug seems to be filed a while back, :mwu or :jld does either of you know if this is still an issue regarding sandboxing or if this code has already been remoted in the meantime?

Comment 5

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

If I recall correctly, I ran into this just by starting Firefox with the content process under seccomp-bpf, not by doing anything special.  So if it's not obviously broken now, that could mean it changed at some point in the ~15 months since comment #1.  (In hindsight I wish I'd attached a stack trace so we'd have a better idea of what parts of the codebase were involved.)

Comment 6

[Jim Mathies [:jimm]]

AFAICT these services have been remoted through PExternalHelperApp and PHandlerService in bug 562444. I debugged this on Windows and found that calls to GetTypeFromExtension were occurring in the parent.

In comment 1 jld mentions navigator.mimeTypes. Need to look into the functionality of that.

Comment 7

[Jim Mathies [:jimm]]

Attachment: mimetypes.html

navigator.mimeTypes appears to be plugin related.

Comment 8

[Jim Mathies [:jimm]]

(In reply to Michael Wu [:mwu] from comment #0)
> nsOSHelperAppService on Android requires access to the JRE, which is not
> available in the child process.

Also the original bug was android related. I think we can untrack this from the linux seccompp nightly landing.

Comment 9

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Jim Mathies [:jimm] from comment #8)
> Also the original bug was android related. I think we can untrack this from
> the linux seccompp nightly landing.

Unraveling a few things here: I think I did not recall correctly in comment #5, and I was thinking of something else that reproduced just by starting the browser.  Rereading the comment about web content accessing navigator.mimeTypes… that was probably me going to some well-known webpage that tried to do feature detection, and extracting the navigator.mimeTypes part of it from the stack trace.

As for whether comment #1 and comment #0 are the same bug… I have no idea.  At the time, they seemed to be the same problem (nsOSHelperAppService accessing a system content-type registry directly from the content process), and since this bug was still open I assumed that the manifestation I was seeing was a duplicate.  But if bug 562444 fixed the Android version of this, then maybe not?  In any case, whether or not that comment should have been a separate bug is independent of whether it's still a seccomp-bpf blocker.

Comment 10

[Jim Mathies [:jimm]]

(In reply to Jed Davis [:jld] [away until 06-01] [⏰MDT; UTC-6] from comment #9)
> As for whether comment #1 and comment #0 are the same bug… I have no idea. 
> At the time, they seemed to be the same problem (nsOSHelperAppService
> accessing a system content-type registry directly from the content process),
> and since this bug was still open I assumed that the manifestation I was
> seeing was a duplicate.  But if bug 562444 fixed the Android version of
> this, then maybe not?  In any case, whether or not that comment should have
> been a separate bug is independent of whether it's still a seccomp-bpf
> blocker.

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/unix/nsOSHelperAppService.cpp
http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp

This code definitely accesses local files still. Open question is if nsExternalHelperAppService (which calls into nsOSHelperAppService) tries to do this in the content process.

Comment 11

[Julian Hector [:tedd] [:jhector]]

When debugging some of the test failures with seccomp-bpf enabled, I came across a failure which seems to trigger this bug.

When running: uriloader/exthandler/tests/mochitest/test_badMimeType.html, content will crash because of a seccomp violation trying to fork/exec using sys_clone.
I attached with gdb and tried to trace it back, it was executing 'test -x /usr/bin/vim'.

When running the test with NSPR_LOG_MODULES="HelperAppService:4", I get the following output:

> [Main Thread]: D/HelperAppService -- UnescapeCommand
> [Main Thread]: D/HelperAppService Command to escape: 'test -x /usr/bin/vim'
> [Main Thread]: D/HelperAppService UnescapeCommand really needs some work -- it should actually do some unescaping
> [Main Thread]: D/HelperAppService Escaped command: 'test -x /usr/bin/vim'
> [Main Thread]: D/HelperAppService Running Test: test -x /usr/bin/vim

Which is handled here: https://dxr.mozilla.org/mozilla-central/rev/ec20b463c04f57a4bfca1edb987fcb9e9707c364/uriloader/exthandler/unix/nsOSHelperAppService.cpp#1069

So the test case definitely shows that this is still executed in the content process, which blocks seccomp from being enabled on nightly.

:jimm, do you know anybody who could pick this up? And since it is an issue on desktop linux, should it be tracked for e10s?
I don't know the code at all and it would take me a while, but I think we want to get seccomp on nightly asap, what do you think?

Comment 12

[Jim Mathies [:jimm]]

I've done some work in here, it can be a bit confusing because the uriloader c++ code implements various xpcom interfaces, one of which is defined by an idl over in netwerk. Overall though it doesn't look like there's an issue here.

I did quick survey of the use of the routines in the unix implementation of nsOSHelperAppService and found that local file access was restricted to two public methods of nsIMIMEService -

Interfaces:
---------------------------
nsIExternalHelperAppService  (urihandler)
nsIExternalProtocolService (urihandler)
nsIMIMEService (netwerk)

Methods:
---------------------------

GetFromTypeAndExtension (nsIMIMEService)
  GetMIMEInfoFromOS
    GetFromType, GetFromExtension - file access

GetTypeFromExtension (nsIMIMEService)
  GetMIMEInfoFromOS
    GetFromType, GetFromExtension - file access

GetProtocolHandlerInfo (nsIExternalProtocolService)
  GetProtocolHandlerInfoFromOS (nsGNOMERegistry use)

ExternalProtocolHandlerExists (nsIExternalProtocolService)
  OSProtocolHandlerExists  (nsGNOMERegistry use)

GetApplicationDescription (nsIExternalProtocolService)
  GetApplicationDescription  (nsGNOMERegistry use)


local file read base helpers in nsOSHelperAppService
----------------------------------------------------

GetFromExtension
  GetFileTokenForPath - file access

GetFromExtension
  LookUpHandlerAndDescription
    DoLookUpHandlerAndDescription
      GetFileLocation - path env
      GetHandlerAndDescriptionFromMailcapFile
        GetHandlerAndDescriptionFromMailcapFile - file access

GetFromExtension
  LookUpTypeAndDescription
    GetFileLocation
    GetTypeAndDescriptionFromMimetypesFile - file access

GetFromType
  GetFileTokenForPath - file access

GetFromType
  DoLookUpHandlerAndDescription
    GetFileLocation
    GetHandlerAndDescriptionFromMailcapFile
      GetHandlerAndDescriptionFromMailcapFile - file access

GetFromType
  LookUpExtensionsAndDescription
    GetFileLocation
    GetTypeAndDescriptionFromMimetypesFile - file access

Poking around for uses if these two methods, I don't see cases where these two methods get called from content - 

http://mxr.mozilla.org/mozilla-central/search?string=GetFromTypeAndExtension
http://mxr.mozilla.org/mozilla-central/search?string=GetTypeFromExtension

(Lets try to confirm this through some debugging.)

The test you found that failed looks like a crash test that tries to load nsIMIMEService in content, which we will not allow content to do. We can disable this test to work around it.

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/tests/mochitest/test_badMimeType.html

Thoughts?

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Jim Mathies [:jimm] from comment #12)
> The test you found that failed looks like a crash test that tries to load
> nsIMIMEService in content, which we will not allow content to do.

Should that interface be marked main_process_scriptable_only? (See also bug 997325.)

Comment 14

[Julian Hector [:tedd] [:jhector]]

So basically, the test creates a scenario (using nsIMIMEService in content) that doesn't occur in release because we don't use it this way in content?

Comment 15

[Julian Hector [:tedd] [:jhector]]

This would also explain, why :gcp and I couldn't trigger a crash.

Comment 16

[Gian-Carlo Pascutto [:gcp]]

(In reply to Jim Mathies [:jimm] from comment #12)
> We can disable this test to work around it.
>

Slight preference for "just" fixing the test. Removing test coverage on features can end up backfiring.

Comment 17

[Gian-Carlo Pascutto [:gcp]]

As well as doing what jld says in comment 13, for that matter.

Comment 18

[Gian-Carlo Pascutto [:gcp]]

Attachment: Bug 579388 - nsIMIMEService should run in chrome.

Review commit: https://reviewboard.mozilla.org/r/59316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59316/

Comment 19

[Gian-Carlo Pascutto [:gcp]]

Attachment: Bug 579388 - Only allow nsIMIMEService scripting from chrome.

Review commit: https://reviewboard.mozilla.org/r/59318/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59318/

Comment 20

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8762858 [details]
Bug 579388 - nsIMIMEService should run in chrome.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59316/diff/1-2/

Comment 21

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8762859 [details]
Bug 579388 - Only allow nsIMIMEService scripting from chrome.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59318/diff/1-2/

Comment 22

[Josh Aas]

Comment on attachment 8762858 [details]
Bug 579388 - nsIMIMEService should run in chrome.

https://reviewboard.mozilla.org/r/59316/#review57792

Comment 23

[Josh Aas]

Comment on attachment 8762859 [details]
Bug 579388 - Only allow nsIMIMEService scripting from chrome.

https://reviewboard.mozilla.org/r/59318/#review57794

Comment 24

[Josh Aas]

This is the first time I've reviewed something with Review Board. I hope I did it correctly. I'm struggling to understand what the point of that additional UI disaster is over the old review system, but this isn't really the place for that discussion...

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f276538c9493
nsIMIMEService should run in chrome. r=jaas+18821
https://hg.mozilla.org/integration/autoland/rev/a1eab626db95
Only allow nsIMIMEService scripting from chrome. r=jaas+18821

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f276538c9493
https://hg.mozilla.org/mozilla-central/rev/a1eab626db95

Comment 27

[Julian Hector [:tedd] [:jhector]]

clearing needinfo