Closed Bug 1294641 Opened 8 years ago Closed 7 years ago

Running Nightly from a repo in home directory results in crash when home directory read access is blocked

Categories

(Core :: Security: Process Sandboxing, defect)

Product:
Core
Component:
Security: Process Sandboxing
Version:
51 Branch
Platform:
Unspecified
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox51 --- affected
firefox55 --- fixed

People

(Reporter: haik, Assigned: Alex_Gaynor)

References

Details

(Whiteboard: sbmc2, sblc3)

Attachments

(2 files)

Hack patch that disables home dir read/write access in the content sandbox
887 bytes, patch
Details | Diff | Splinter Review
Bug 1294641 - whitelist reads from the .app directory in the macOS sandbox
59 bytes, text/x-review-board-request
haik
: review+
froydnj
: review+
Details 
This isn't a problem now, but it is something that we need to be resolved on OS X when we remove read access to the home directory from the content process. With the attached patch, running Nightly from a built repo in my home directory results in plugin-container crashes and causes several sandbox deny log entries such as 

  plugin-container(82675) deny file-read-data /Users/haftandilian/mozilla-central/toolkit/components/satchel/formSubmitListener.js

We want to block plugin-container from reading from the home directory, but we have to deal with the special cases 1) when the application bundle is installed in the home directory (this may work already) and 2) when the browser is being run by developers from a repo in the home directory. 2 is the problem I'm running into here.

Windows and Linux are likely to be affected by this too, once read access to the home directory is disabled.
Whiteboard: sbmc2
Whiteboard: sbmc2 → sb?
Whiteboard: sb? → sbmc2, sblc3
Blocks: 1308400
Blocks: 1332190
Started to look at this, I think (1) is still a problem, when I blacklist all of |~/| for content processes, all the initial sandbox violations are inside the .app; for example:


    SandboxViolation: plugin-container(12428) deny(1) file-read-metadata /Users/agaynor/projects/mozilla-central/obj-x86_64-apple-darwin16.5.0/dist/Nightly.app/Contents/Resources/components/TooltipTextProvider.js

Right now we whitelist access to |appdir-path|, which is |/Users/agaynor/projects/mozilla-central/obj-x86_64-apple-darwin16.5.0/dist/Nightly.app/Contents/Resources/browser|.

Does it make sense to simply go up 3 path segments, and allow file-read* on everything under |Nightly.app/|, I assume there's nothing dangerous to read there.
Flags: needinfo?(haftandilian)
(In reply to Alex Gaynor from comment #1)
> Started to look at this, I think (1) is still a problem, when I blacklist
> all of |~/| for content processes, all the initial sandbox violations are
> inside the .app; for example:
> 
>     SandboxViolation: plugin-container(12428) deny(1) file-read-metadata
> /Users/agaynor/projects/mozilla-central/obj-x86_64-apple-darwin16.5.0/dist/
> Nightly.app/Contents/Resources/components/TooltipTextProvider.js
> 
> Right now we whitelist access to |appdir-path|, which is
> |/Users/agaynor/projects/mozilla-central/obj-x86_64-apple-darwin16.5.0/dist/
> Nightly.app/Contents/Resources/browser|.
> 
> Does it make sense to simply go up 3 path segments, and allow file-read* on
> everything under |Nightly.app/|, I assume there's nothing dangerous to read
> there.

See some of the discussion on Bug 1308400 "Construct a file broker policy for default-deny read access on the Linux Desktop". In addition to the problem you mentioned, for the developer case where we run the browser out of the repo, we have symlinks in the .app that point out of the .app into the repo source files and object dir. For a local build, Contents/Resources/components/TooltipTextProvider.js is a symlink outside of the .app.

Unlike with Linux broker case, we can't make a runtime decision after content has started to follow symlinks or not. One idea I had for the developer case was at startup to have the parent process examine all the resources it needs in the .app and build a list of additional directories to whitelist.

There are some other options discussed on 1308400 such as having "mach run" use an environment variable pointing to the object/src dir which we pick up in the browser and whitelist. Or we could pass additional read access directories on the command line.

But any of these options we implement should only apply to the developer use case under the assumption that release/packaged builds never need to refer outside of the .app dir. 

And we should think about what (if any) parts of the implementation can be common with Linux and Windows.
Flags: needinfo?(haftandilian)
Attachment #8855906 - Flags: review?(haftandilian)
Assignee: nobody → agaynor
Comment on attachment 8855906 [details]
Bug 1294641 - whitelist reads from the .app directory in the macOS sandbox

https://reviewboard.mozilla.org/r/127788/#review130488

r- For now. We're not ready to land a change that removes read access to the home directory yet. At the moment, that would break some WebExtensions use cases. That is being addressed by 1334550 (in progress.)

Adding the $HOME read restrictions will be addressed in Bug 1332190 by putting the read restrictions in a new level (3). Since you're working on this, it would make sense to land a level 3 that isn't yet enabled. I'll get that posted for review in a new bug. Then 1332190 will end up being a fix that makes level 3 the default.

The environment variable approach would break for developers that run the binaries directly out of the .app dir (i.e., not with mach run). I've done this a few times, but I don't think it's common. We could ask on #developers or perhaps the build peers would have a good idea of how common that is. And I think we'd want to announce the change on dev-platform once it lands.

For this approach, we'd need to get a build peer to review the build changes and I can review from the sandbox perspective.

Thanks for working on this.
Attachment #8855906 - Flags: review?(haftandilian) → review-
This implements the changes described at https://bugzilla.mozilla.org/show_bug.cgi?id=1308400#c1 option (3). Would love feedback from build peers as to whether this is an acceptable approach.
Flags: needinfo?(ted)
Flags: needinfo?(nfroyd)
Flags: needinfo?(mh+mozilla)
I'm not very comfortable with --enable-release modifying the resulting build beyond options that alter how things are built (as opposed to what the code does).
Flags: needinfo?(mh+mozilla)
With the current version of the patch, the new environment variable isn't being set for DEBUG builds. Otherwise it works as expected for me.
Thanks for the feedback Mike, that makes sense to me. Is there an alternate flag or option that exists today that you'd recommend using for this?

This code is really only applicable to when you're using a development build (i.e. one where the .app is full of symlinks), so I'd like to avoid this code being in release builds.

Thanks!
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #6)
> This implements the changes described at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1308400#c1 option (3). Would
> love feedback from build peers as to whether this is an acceptable approach.

Does this extra environment variable only get added when you `mach run` or does it get added for things like `mach mochitest`, etc.?  It looks like the former, but I am willing to believe that the latter somehow plumbs into `mach run`.

Bug 1308400 comment 3 or bug 1308400 comment 4 look like reasonable ways forward here...or could we detect when we're not using a packaged omni.ja?  I know Linux builds function *very* differently when they're run from the filesystem vs. being run from the result of `mach package` or similar.  Maybe we could detect that and change behavior appropriately.
Flags: needinfo?(nfroyd)
I put that code directly in |mach run|; I don't know much about how |mach| is plumbed together, would |mochitest| need this env var? If it's running in a "with symlinks" build I think the correct behavior would be for it to get the env vars (meaning either going through |mach run| is correct, or the env vars needs to be in code shared by both |run| and |mochitest|).

The approach in comment 4 doesn't seem workable for macOS, since we don't get a chance to execute our own code on every |open|, we have to pre-declare all valid filesystem locations.

Do you have any hints as to how to go about detecting packaged vs. not?
Flags: needinfo?(nfroyd)
Confirmed that |mach mochitest| does not go through |mach run| (and in fact, they don't appear to share any |env| code), so it requires it's own modifications. Good call on looking at that.
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #12)
> Confirmed that |mach mochitest| does not go through |mach run| (and in fact,
> they don't appear to share any |env| code), so it requires it's own
> modifications. Good call on looking at that.

Yeah, there are probably a number of places that exec firefox from mach that would need their own modifications: reftest, xpcshell, etc. etc.

As for detecting packaged builds vs. not, the quick-and-dirty way that comes to mind is looking for omni.ja in appropriate places.  On Linux, it would live alongside the firefox executable; I'm not sure where it would appear on Mac.  I'm unsure if that's subject to weird edge cases, but it would at least be a start.
Flags: needinfo?(nfroyd)
Ok, so the idea is that |omni.ja| is a file that only exists in packaged builds, and we can use it to test for packaging. Makes sense! I'll see if we can detect its existence reasonably.
Attachment #8855906 - Flags: review?(nfroyd)
I think froydnj has you covered here, let me know if you need anything specific from me.
Flags: needinfo?(ted)
Comment on attachment 8855906 [details]
Bug 1294641 - whitelist reads from the .app directory in the macOS sandbox

https://reviewboard.mozilla.org/r/127788/#review132080

I'm not thrilled about having to modify N places (and probably miss some) to pass `MOZ_DEVELOPER_REPO_DIR`.  The only alternative to doing that that I can think of is to pass in the topsrcdir as a `#define`, and use that if we detect that we're a developer build.  (Maybe only passing that if `MOZ_UPDATE_CHANNEL` is `default`, like it is for normal developer builds?)  But thatmakes me vaguely uneasy...so I guess we have to do this.

Some comments below.

::: dom/ipc/ContentChild.cpp:1267
(Diff revision 3)
> +IsDevelopmentBuild(nsAutoCString appDir)
> +{
> +  nsresult rv;
> +  nsCOMPtr<nsIFile> path = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &rv);
> +  path->InitWithNativePath(appDir);
> +  path->AppendNative(NS_LITERAL_CSTRING("omni.ja"));

We should use `OMNIJAR_NAME` for this, which doesn't exist as a `#define` for C++ code.  So we'll have to add something like:

```
DEFINES['OMNIJAR_NAME'] = '"%s"' % CONFIG['OMNIJAR_NAME']
```

to the `moz.build` file for `dom/ipc/`.  The comment for this function would also need to be updated.

::: testing/mochitest/mach_commands.py:142
(Diff revision 3)
>                             if isinstance(l, logging.StreamHandler)]
>          for handler in remove_handlers:
>              logging.getLogger().removeHandler(handler)
>  
>          options = Namespace(**kwargs)
> +        options.topsrcdir = self.topsrcdir

We're going to have to figure out how to shoehorn this into at least:

http://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/mach_commands.py
http://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/mach_commands.py
http://dxr.mozilla.org/mozilla-central/source/testing/web-platform/mach_commands.py
http://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/mach_commands.py#1055
http://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/mach_commands.py#1764

as well.  (Not totally sure about the last two; gtests are compiled-code tests that really shouldn't need anything from the srcdir, but you never know...)
Attachment #8855906 - Flags: review?(nfroyd)
Comment on attachment 8855906 [details]
Bug 1294641 - whitelist reads from the .app directory in the macOS sandbox

https://reviewboard.mozilla.org/r/127788/#review132080

Having modified the N places (and absolutely missed some :-)), I'm now wondering if making `topsrcdir` into a `#define` isn't the most sensisible thing. Non-packaged builds are already non-portable since the symlinks are absolute paths, and it fixes the case of running the binary by hand (not through `./mach run`).

That said, I defer to your expertise in the build system.

Regardless of which direction, should I go ahead and add the `MOZ_UPDATE_CHANNEL` is `default` check to `IsDevelopmentBuild`?
Comment on attachment 8855906 [details]
Bug 1294641 - whitelist reads from the .app directory in the macOS sandbox

https://reviewboard.mozilla.org/r/127788/#review132080

We have topsrcdir embedded in the binary in various places, but none of them are used for behavior modification; they're all for data (e.g. `about:buildconfig`).  Similarly, we use `MOZ_UPDATE_CHANNEL` in various places, but none of them switch on the `default` value, and there have been breakages in various pieces of code that have tried to do things with `MOZ_UPDATE_CHANNEL` when `MOZ_UPDATE_CHANNEL` is `"default"` vs. `""`, though I don't recall if we use `""` anymore...

Both of these things combined make me uneasy; checking `MOZ_UPDATE_CHANNEL` and using topsrcdir in the appropriate places seems like an obvious thing, but I'm also aware that this is not my strongest area of expertise, so I'm inclined to be cautious.

Checking `MOZ_UPDATE_CHANNEL == default` also doesn't work great if you want to do a beta-like build (`--enable-update-channel=beta`) on your own computer.  I think we want to avoid checking `MOZ_UPDATE_CHANNEL` for the time being.
Oh, forgot to mention, too: I think the test scripts are theoretically meant to be usable outside of a build, or even a configured directory.  So bare references to topsrcdir or similar are a no-go. =/
(In reply to Nathan Froyd [:froydnj] from comment #20)
> Checking `MOZ_UPDATE_CHANNEL == default` also doesn't work great if you want
> to do a beta-like build (`--enable-update-channel=beta`) on your own
> computer.  I think we want to avoid checking `MOZ_UPDATE_CHANNEL` for the
> time being.

s/for the time being/ever/
MOZ_UPDATE_CHANNEL's only meaning is what the default channel is for update checks, and that's all. It has been abused for other things in the past, and they ended up being burned (leading to, for example, e10s never being enabled on Linux distro builds)

omni.ja is kinda sorta more reliable, but that's not entirely true either. One can still have a packaged build that doesn't have an omni.ja. But if you really want to use omni.ja, don't build the path yourself, just use mozilla::Omnijar::GetPath(mozilla::Omnijar::GRE)
Ok, so |MOZ_UPDATE_CHANNEL| is a definite no-no. Thanks for the pointer to |mozilla::Omnijar|.

When the tests are run outside of a build, are they running using a packaged copy of Firefox?
Yes, it's not really possible to run from the objdir because of the symlinks.
Comment on attachment 8855906 [details]
Bug 1294641 - whitelist reads from the .app directory in the macOS sandbox

https://reviewboard.mozilla.org/r/127788/#review132684

Still not sure about the goodness of embedding the source directory and using that for non-packaged builds vs. what we're doing here, but this patch has been through enough.

Please land this and if/when the sandbox is tightened down on OS X, post to dev-platform about the new constraints with information about what we did and how to fix things if problems are encountered.  If we run into a lot of problems with other test suites, etc., we may want to consider using the embedded `topsrcdir` approach.

::: dom/ipc/ContentChild.cpp:1271
(Diff revision 6)
> +IsDevelopmentBuild()
> +{
> +  nsCOMPtr<nsIFile> path = mozilla::Omnijar::GetPath(mozilla::Omnijar::GRE);
> +
> +  bool exists;
> +  // XXX: Is `path == nullptr` sufficient?

Yes, I believe it is.
Attachment #8855906 - Flags: review?(nfroyd) → review+
Comment on attachment 8855906 [details]
Bug 1294641 - whitelist reads from the .app directory in the macOS sandbox

https://reviewboard.mozilla.org/r/127788/#review132684

Thanks for taking the time to go through this!

(For the onlookers, I'm going to wait for an additional r+ from :haik for the sandboxing perspective)
Comment on attachment 8855906 [details]
Bug 1294641 - whitelist reads from the .app directory in the macOS sandbox

https://reviewboard.mozilla.org/r/127788/#review133286
Attachment #8855906 - Flags: review?(haftandilian) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/225683fed1d1
whitelist reads from the .app directory in the macOS sandbox r=froydnj,haik
Keywords: checkin-needed
Ok, fixed the bustage. Fixes were straight forward (they were all in the testrunner scripts, inline with the original ones I made, just in places where I hadn't triggered try, of course), and try is green, so I'm going to just re-checkin-needed, I don't think an additional review is necessary (though feedback is always welcome!)
Flags: needinfo?(agaynor)
Keywords: checkin-needed
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/271868b27893
whitelist reads from the .app directory in the macOS sandbox r=froydnj,haik
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/271868b27893
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: