e10s: Remote nsOSHelperAppService

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
9 years ago
2 years ago

People

(Reporter: mwu, Unassigned)

Tracking

(Blocks 1 bug)

unspecified
Firefox 50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s-, firefox50 fixed)

Details

(Whiteboard: sblc1)

Attachments

(3 attachments)

(Reporter)

Description

9 years ago
nsOSHelperAppService on Android requires access to the JRE, which is not available in the child process.
(Reporter)

Updated

9 years ago
OS: Linux → All
Hardware: x86_64 → All
Blocks: 1040920
No longer blocks: old-e10s-m2
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.
(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.
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?
Flags: needinfo?(mwu.code)
Flags: needinfo?(jld)
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.)
Flags: needinfo?(jld)
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.
Posted file mimetypes.html
navigator.mimeTypes appears to be plugin related.
(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.
(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.
(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.
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?
Flags: needinfo?(jmathies)
Whiteboard: sblc1
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?
Flags: needinfo?(jmathies)
(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.)
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?
This would also explain, why :gcp and I couldn't trigger a crash.
(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.
As well as doing what jld says in comment 13, for that matter.
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/
Attachment #8762858 - Flags: review?(jocheng)
Attachment #8762859 - Flags: review?(jocheng)
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/
Attachment #8762858 - Flags: review?(jocheng) → review?(jaas)
Attachment #8762859 - Flags: review?(jocheng) → review?(jaas)

Updated

3 years ago
Component: File Handling → File Handling
Product: Core → Firefox
Version: Trunk → unspecified

Updated

3 years ago
Attachment #8762858 - Flags: review?(jaas) → review+

Comment 23

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

https://reviewboard.mozilla.org/r/59318/#review57794
Attachment #8762859 - Flags: review?(jaas) → review+

Comment 24

3 years ago
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

3 years ago
Pushed by gpascutto@mozilla.com:
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f276538c9493
https://hg.mozilla.org/mozilla-central/rev/a1eab626db95
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
clearing needinfo
Flags: needinfo?(mwu.code)
You need to log in before you can comment on or make changes to this bug.