Bug 1289718 restricted writes. We can restrict reads too, notably from messing around in user's homedirs, and potentially parts of the Firefox profile.

I started implementing this, but ran into an issue with how our builds work. In bug 1289718, we made the file broker resolve symlinks and verify the policy against the target of a symlink. However, our build (when not packaged), constructs a bunch of files in the output dir by symlinking back to the source dir. The source dir isn't part of the allowed policy, so Firefox fails to open its internal files and crashes. e.g. objdir-desktop/dist/bin/chrome/toolkit/res/ lrwxrwxrwx 1 morbo morbo 57 mrt 20 17:01 counterstyles.css -> /home/morbo/hg/firefox/layout/style/res/counterstyles.css I see some options: 1) Change the broker to also allow access to symlinks if the symlink itself sits in an allowed place. I think this is a security flaw, as you can legally create symlinks that point to files you aren't allowed to read, so this will bypass the broker restrictions. 2) Change our build process so we don't create symlinks back to the source directory inside the object directory, but copy (or even hardlink or whatever). This symlinking seems pretty nasty to me as it creates this backref into the source tree, but I don't know the underlying reasons for it. 3) Hack up something so we detect the ./mach run case and add the SRCDIR into the file system broker policy. This is dirty and hackish, but if (2) is not desirable, it's probably our only option? Needinfo-ing a bunch of build peers to get input on this.

> 2) Change our build process so we don't create symlinks back to the source directory inside the object directory, but copy (or even hardlink or whatever). This symlinking seems pretty nasty to me as it creates this backref into the source tree, but I don't know the underlying reasons for it. We do this because symlinking is way cheaper than copying. This is one of the (many) reasons Windows builds are slower. We could alternately have the linker generate binaries directly into dist/bin, but the build system currently wipes out dist/bin at the start of the build because we don't have good dependency tracking for files being removed, etc. Additionally, we're likely to continue to have symlinks for some things because frontend developers like being able to edit JS files in the srcdir without having to rebuild. > 3) Hack up something so we detect the ./mach run case and add the SRCDIR into the file system broker policy. This is dirty and hackish, but if (2) is not desirable, it's probably our only option? This seems fine, we could just have a special env var that passes the value of $topsrcdir down and allow access to that directory. We could even make this code only work when the build is not `--enable-release` (which is currently only used to tweak some build options that make the build faster for local developers: https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/build/autoconf/compiler-opts.m4#9 )

(In reply to Gian-Carlo Pascutto [:gcp] from comment #1) > 3) Hack up something so we detect the ./mach run case and add the SRCDIR > into the file system broker policy. This is dirty and hackish, but if (2) is > not desirable, it's probably our only option? Does this imply putting some sort of environment variable hack into the binaries that we would release? That seems really undesirable. We could make it a pref instead, similar to the magic-but-scary xpconnect one, and then have ./mach run and various and sundry things set it? I guess that breaks executing dist/bin/firefox directly...

There's another approach I can think of, which is loosely inspired by Multics (http://multicians.org/mgr.html#ringbrackets): higher-privilege software can execute a resource only if lower-privilege software can't write to it. In this case, we'd be “executing“ a symlink by letting it delegate broker permissions to its target path. So, if we had a flag for “trust symlinks here” — which would also forbid symlink/unlink/rename/...? if it's set, regardless of other flags — and applied it to the policy rule for NS_GRE_DIR (or whatever directory is the problem here), that ought to take care of this. But we'd need to replace realpath() with our own path canonicalizer to make this work. Alternately, the absence of broker write permission could act as the “trust symlinks here” indication; you wouldn't want to allow propagating write permission through trusted symlinks (because a symlink could seem non-writable but actually be writable through some other path), but it looks like we wouldn't need that anyway. And there are probably other variations. But it seems more least-privilege to have a specific opt-in for this. If nothing else, this might be worth keeping in mind in case we run into non-development situations that need this kind of symlink handling (maybe to deal with fonts or config files or libraries or something like that).

(In reply to Nathan Froyd [:froydnj] from comment #3) > (In reply to Gian-Carlo Pascutto [:gcp] from comment #1) > > 3) Hack up something so we detect the ./mach run case and add the SRCDIR > > into the file system broker policy. This is dirty and hackish, but if (2) is > > not desirable, it's probably our only option? > > Does this imply putting some sort of environment variable hack into the > binaries that we would release? That seems really undesirable. Adding a pref or environment var exception to sandboxing policy isn't a major concern right now. In fact the first version of sandboxing we're shipping on Linux has explicit perfs to work around the sandbox, for compatibility reasons. There is no real security concern, chrome runs with all privileges and if you control the environment and execution you can replace Firefox itself etc. > We could make it a pref instead, similar to the magic-but-scary xpconnect > one, and then have ./mach run and various and sundry things set it? I guess > that breaks executing dist/bin/firefox directly... I think breaking the direct execution is reasonable, at least if it's the output of ./mach build and not ./mach package etc. That said: >Alternately, the absence of broker write permission could act as the “trust symlinks here” indication; you wouldn't want >to allow propagating write permission through trusted symlinks (because a symlink could seem non-writable but actually >be writable through some other path), but it looks like we wouldn't need that anyway. And there are probably other >variations. But it seems more least-privilege to have a specific opt-in for this. For maximum compatibility (like cases pointed out in the next paragraph) I like this approach. I'm not sure how much of a real gain opting in paths to this really is: if the basic idea is sound (and it looks like it), opt-in doesn't matter, and if it isn't, it's not clear opt-in avoids whatever issue either.

The path canonicalization is fairly tricky. We currently rely on realpath() to find the real paths that we are passed in, but that does two things at once: a) resolve symlinks b) resolve relative paths. To get this to work, we need to split it up. e.g. with only realpath(): /tmp/password.db -> /home/gcp/.mozilla/firefox/profile/password.db This would fail the write permission check, because we aren't allowed to write that file, as realpath resolution will tell us. But we shouldn't be able to read it, either. For resolving the write permissions check, just doing a+b (aka using normal realpath) seems OK. But for resolving the read, I think we need to process the path part by part and check each component against the write whitelist, bailing when we get a hit?

MozReview-Commit-ID: 7xLVjTyy96U

Too late for firefox 52, mass-wontfix.

Comment on attachment 8855443 [details] Bug 1308400 - Don't allow content processes to access the weave director on macOS https://reviewboard.mozilla.org/r/127296/#review130028 Whoops, wrong bug #. Sorry about that!

I got a complete implementation, but Firefox still isn't working. Logging reveals the problem to be this: Sandbox: Client requesting path /etc/localtime Sandbox: Client requesting path /home/morbo/hg/firefox/objdir-desktop/dist/bin/components/TooltipTextProvider.js Sandbox: Client requesting path /home/morbo/hg/firefox/objdir-desktop/dist/bin/components/TooltipTextProvider.js Sandbox: Client requesting path /home/morbo/hg/firefox/objdir-desktop/dist/bin/modules/XPCOMUtils.jsm Sandbox: Client requesting path /home/morbo/hg/firefox/objdir-desktop/dist/bin/modules/XPCOMUtils.jsm Sandbox: Readlink on path /home/morbo/hg/firefox/objdir-desktop/dist/bin/modules/XPCOMUtils.jsm Sandbox: Client requesting path /home/morbo/hg/firefox/objdir-desktop/dist/bin/modules/XPCOMUtils.jsm Sandbox: Client requesting path /home/morbo/hg/firefox/js/xpconnect/loader/XPCOMUtils.jsm The first few requests will succeed because the bindir and subdirs have read access. The last one is the problem though. It's asking to open XPCOMUtils.jsm again, but not from a relative path, as I had suspected was the case, but from the resolved path, which was readlinked just earlier. For whatever reason, something in content is resolving the path. The resolving doesn't seem to be from nsIFile::Normalize, though. I think I'm going to need to find out who's doing the readlinking and why. Worst case, the broker can create a an inverse symlink mapping for files it has resolved and still work, but this complicates things. If the path resolution isn't really necessary and done in our code, that could be avoided.

Sandbox: crash reporter is disabled (or failed); trying stack trace: Sandbox: frame #01: __restore_rt (sigaction.c:?) Sandbox: frame #02: __GI___readlink (/build/glibc-9tT8Do/glibc-2.23/io/../sysdeps/unix/syscall-template.S:84) Sandbox: frame #03: nsLocalFile::GetNativeTarget(nsACString&) (/home/morbo/hg/firefox/xpcom/io/nsLocalFileUnix.cpp:1763) Sandbox: frame #04: nsFileChannel::nsFileChannel(nsIURI*) (/home/morbo/hg/firefox/netwerk/protocol/file/nsFileChannel.cpp:277) Sandbox: frame #05: nsFileProtocolHandler::NewChannel2(nsIURI*, nsILoadInfo*, nsIChannel**) (/home/morbo/hg/firefox/netwerk/protocol/file/nsFileProtocolHandler.cpp:192)

Presumably this is for: https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/file/nsFileChannel.cpp#255 Great. What now?

MozReview-Commit-ID: 7xLVjTyy96U

Comment on attachment 8861111 [details] [diff] [review] WIP Review of attachment 8861111 [details] [diff] [review]: ----------------------------------------------------------------- This uses the "trust some symlinks" approach that was originally proposed. FWIW, on macOS we ended up going forward with env var from mach to give read permissions to the src directory. It might make sense for Linux to consolidate on that approach -- not sure though, interested in other folk's thoughts.

This approach doesn't just cover the Firefox developer case, it supports symlinks in all places that we (still) need to read in content. This approach isn't possible on macOS because the sandbox there can't make file access decisions on runtime. I'm not sure "consolidating" is the right word here because there's a large difference in compatibility with the filesystem setups users might have, which are going to be *way* more varied on Linux compared to macOS.

One issue with the approach in the patches (remembering the symlinks we resolve so we can invert them) is that we have no way to track users of this mapping, which means we can't ever remove files from the HashMap. I'm not sure this is a real problem, if file browsing is going to be done in a separate process then presumably there is a limited and fixed set of configuration files and directories we end up reading into through a symlink. (And if we'd run out of memory, it would always be in a situation that wouldn't have worked at all without this patch).

Comment on attachment 8861547 [details] Bug 1308400 - Support file process, whitelist path prefs. https://reviewboard.mozilla.org/r/133516/#review136380 ::: xpcom/io/nsLocalFileUnix.cpp:550 (Diff revision 1) > nsLocalFile::Normalize() > { > char resolved_path[PATH_MAX] = ""; > char* resolved_path_ptr = nullptr; > > + fprintf(stderr, "nsIFile: Normalize (\"%s\")\n", mPath.get()); Leftover debugging, will remove.

GECKO(21069) | Sandbox: SandboxBroker: denied op=2 rflags=400000 perms=0 path=/home/morbo/hg/firefox/objdir-desktop/_tests/testing/mochitest/extensions/specialpowers/chrome/specialpowers/content/MozillaLogger.js MochitestServer : launching [u'/home/morbo/hg/firefox/objdir-desktop/dist/bin/xpcshell', '-g', u'/home/morbo/hg/firefox/objdir-desktop/dist/bin', '-v', '170', '-f', u'/home/morbo/hg/firefox/objdir-desktop/dist/bin/components/httpd.js', '-e', "const _PROFILE_PATH = '/tmp/tmprOsVgf.mozrunner'; const _SERVER_PORT = '8888'; const _SERVER_ADDR = '127.0.0.1'; const _TEST_PREFIX = undefined; const _DISPLAY_RESULTS = true;", '-f', '/home/morbo/hg/firefox/objdir-desktop/_tests/testing/mochitest/server.js'] objdir-desktop/_tests is not a whitelisted path, it's the dirs under objdir-desktop/dist/bin that are whitelisted. The question is how and where we end up reading a file outside those paths in the content process.

The paths are injected via the temporary profile that is put in /tmp: tests.manifest content mochitests /home/morbo/hg/firefox/objdir-desktop/_tests/testing/mochitest/ contentaccessible=yes extensions.json {"name":"Special Powers","description":"Special powers for use in testing.","creator":"Mozilla","homepageURL":null},"visible":true,"active":true,"userDisabled":false,"appDisabled":false,"descriptor":"/home/morbo/hg/firefox/objdir-desktop/_tests/testing/mochitest/extensions/specialpowers"

froydnj, I see you reviewed bug 1294641. Adding support from that for Linux seems trivial, and would fix the above test failure, at least locally. One thing I'm not sure about though: is that exception (i.e. the conditions that trigger bug 1294641 to activate) consistently true for builds that run on our test infra? My problem here is that mochitests run by loading in SpecialPowers JS, from disk, into the content process, and this JS sits in a "random" dir (from a security perspective). Whitelisting the entire build dir is an easy way out, but if the above isn't true, I need some other way to detect the case where we're running tests and really-this-time-it's-ok-to-read-random-files-from-disk.

(In reply to Gian-Carlo Pascutto [:gcp] from comment #26) > froydnj, I see you reviewed bug 1294641. Adding support from that for Linux > seems trivial, and would fix the above test failure, at least locally. One > thing I'm not sure about though: is that exception (i.e. the conditions that > trigger bug 1294641 to activate) consistently true for builds that run on > our test infra? This question confuses me. Which conditions, exactly, are you talking about?

(In reply to Nathan Froyd [:froydnj] from comment #28) > (In reply to Gian-Carlo Pascutto [:gcp] from comment #26) > > froydnj, I see you reviewed bug 1294641. Adding support from that for Linux > > seems trivial, and would fix the above test failure, at least locally. One > > thing I'm not sure about though: is that exception (i.e. the conditions that > > trigger bug 1294641 to activate) consistently true for builds that run on > > our test infra? > > This question confuses me. Which conditions, exactly, are you talking about? http://searchfox.org/mozilla-central/rev/abe68d5dad139e376d5521ca1d4b7892e1e7f1ba/dom/ipc/ContentChild.cpp#1275 Meanwhile, we found out empirically that running mochitests doesn't work against the bundled/package macOS binaries. mach has an option for it, but the sandbox renders it non-working.

(this is xpcshell running httpd.js) [task 2017-05-08T18:07:24.828164Z] 18:07:24 INFO - Sandbox: policy for /home/worker/workspace/build/application/firefox/: 1 -> 35 [task 2017-05-08T18:07:24.831859Z] 18:07:24 INFO - Sandbox: policy for /home/worker/workspace/build/tests/bin/: 1 -> 35 (this is the firefox under test) [task 2017-05-08T18:07:26.538083Z] 18:07:26 INFO - GECKO(1262) | Sandbox: policy for /home/worker/workspace/build/application/firefox/: 1 -> 35 [task 2017-05-08T18:07:26.543093Z] 18:07:26 INFO - GECKO(1262) | Sandbox: policy for /home/worker/workspace/build/application/firefox/browser/: 1 -> 35 [task 2017-05-08T18:07:46.387231Z] 18:07:46 INFO - GECKO(1262) | Sandbox: SandboxBroker: denied op=2 rflags=400000 perms=0 path=/home/worker/workspace/build/tests/mochitest/extensions/specialpowers/chrome/specialpowers/content/MozillaLogger.js for pid=1316 error="No such file or directory" [task 2017-05-08T18:07:46.393454Z] 18:07:46 INFO - GECKO(1262) | Sandbox: Rejected errno -13 op 2 flags 0400000 path /home/worker/workspace/build/tests/mochitest/extensions/specialpowers/chrome/specialpowers/content/MozillaLogger.js [task 2017-05-08T18:07:48.435477Z] 18:07:48 INFO - GECKO(1262) | JavaScript error: http://mochi.test:8888/tests/SimpleTest/setup.js, line 108: ReferenceError: SpecialPowers is not defined [task 2017-05-08T18:07:48.835173Z] 18:07:48 INFO - GECKO(1262) | TEST-UNEXPECTED-FAIL: setup.js | error parsing http://mochi.test:8888/tests.json (TypeError: RunSet is undefined) the build/tests/mochitest path is not under any of the paths added, suggesting MOZ_DEVELOPER_REPO_DIR didn't work. And indeed I see no sign of that in the log.

Oh boy! "test-linux32/opt-mochitest-e10s-1 tc-M-e10s(1)" launches /home/worker/workspace/build/tests/mochitest/runtests.py --total-chunks 10 --this-chunk 1 --appname=/home/worker/workspace/build/application/firefox/firefox --utility-path=tests/bin --extra-profile-file=tests/bin/plugins Using --appname. So the exception for the sourcedir won't work here. This must be a difference between how we run on macOS?

Comment on attachment 8861547 [details] Bug 1308400 - Support file process, whitelist path prefs. https://reviewboard.mozilla.org/r/133516/#review143308 Sorry for taking so long to get to this. And more so for being the bearer of bad news, but there seem to be some problems with symlinks to directories. ::: security/sandbox/linux/broker/SandboxBroker.cpp:593 (Diff revision 1) > + } > + > + // Resolve relative paths, but stop at any symlink (for the permission checking), > + // and fail if any part of the path is writable. > + pathLen = CheckSymlinkPath(mPolicy.get(), pathBuf, sizeof(pathBuf), pathLen); > + perms = mPolicy->Lookup(nsDependentCString(pathBuf, pathLen)); Currently (see comment in the modified `realpath`) a path of the form `/safe-dir/symlink/a/b/c` will be turned into `/safe-dir/symlink`, and here we're using this now just for the policy lookup but also for the actual syscall, so we'll end up accessing the wrong path. But we also need to be prepared for paths of the form `/safe-dir/symlink/../../../../../etc/passwd` — if we just use the rest of the received path as-is, then that allows read access to everything as long as there's a symlink from some readable location to any directory. ::: security/sandbox/linux/broker/SandboxBroker.cpp:761 (Diff revision 1) > + // Record the mapping so we can invert the file to the original > + // symlink > + nsDependentCString orig(pathBuf, pathLen); > + nsDependentCString xlat(respBuf, respSize); > + if (!orig.Equals(xlat)) { > + mSymlinkMap.Put(xlat, orig); I don't think this will do anything useful if the symlink is relative (if `xlat` doesn't start with `/`), or if it's a symlink to a directory and the client is going to try to use the `realpath` of a file inside it. ::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:194 (Diff revision 1) > + } > + } > + } > + } > + > + // ~/.local/share (for themes) There are a lot of things here (`~/.local/share/keyrings`?), so we'll probably want a followup bug to trim this down to what Gtk actually uses. ::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:221 (Diff revision 1) > + if (NS_SUCCEEDED(rv)) { > + policy->AddDir(rdonly, tmpPath.get()); > + } > + } > + > + rv = NS_GetSpecialDirectory(NS_XPCOM_CURRENT_PROCESS_DIR, getter_AddRefs(ffDir)); This seems dangerous if it's the actual working directory (I'm not sure I'm completely following everything that's going on in the directory service with `MOZILLA_FIVE_HOME`), because that could be anything. ::: security/sandbox/linux/broker/SandboxBrokerRealpath.cpp:218 (Diff revision 1) > + errno = EPERM; > + return (NULL); > + } else { > + // No write permission, if we can read or execute the > + // base path, we'll allow reading whatever is behind the link. > + break; It looks like this will delete any path components after the symlink. That won't make a difference for a symlink to a file, but a symlink to a directory will break in confusing ways.

Comment on attachment 8876236 [details] Bug 1308400 - Whitelist paths with test files during testing. https://reviewboard.mozilla.org/r/147690/#review151994 ::: testing/mochitest/runtests.py:1755 (Diff revision 1) > > options.extraPrefs.append( > "browser.tabs.remote.autostart=%s" % > ('true' if options.e10s else 'false')) > - if options.strictContentSandbox: > - options.extraPrefs.append("security.sandbox.content.level=1") > + # if options.strictContentSandbox: > + # options.extraPrefs.append("security.sandbox.content.level=1") can we just delete this instead of comment it out? If we are not using this, do we need the strictContentSandbox option? ::: testing/mozbase/mozprofile/mozprofile/profile.py:114 (Diff revision 1) > self.permissions = Permissions(self.profile, self._locations) > prefs_js, user_js = self.permissions.network_prefs(self._proxy) > + > + if self._whitelistpaths: > + prefs_js.append(("security.sandbox.content.read_path_whitelist", > + ",".join(self._whitelistpaths))) I am not so excited about adding whitelistpaths into mozprofile when we end up with a pref. Could we just append this to the prefs before creating the new profile() ? ::: testing/mozharness/configs/unittests/linux_unittest.py:120 (Diff revision 1) > "--log-errorsummary=%(error_summary_file)s", > "--use-test-media-devices", > "--screenshot-on-fail", > "--cleanup-crashes", > "--marionette-startup-timeout=180", > + "--work-path=" + ABS_WORK_DIR, I would rather see: "--work-path=%(abs_work_dir)s", similar to: https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/unittests/linux_unittest.py?q=path%3Alinux_unittest.py&redirect_type=single#294

a few issues here, overall this is good- lets address the issues raised and verify talos on try!

(In reply to Joel Maher ( :jmaher) from comment #51) > ::: testing/mozbase/mozprofile/mozprofile/profile.py:114 > (Diff revision 1) > > self.permissions = Permissions(self.profile, self._locations) > > prefs_js, user_js = self.permissions.network_prefs(self._proxy) > > + > > + if self._whitelistpaths: > > + prefs_js.append(("security.sandbox.content.read_path_whitelist", > > + ",".join(self._whitelistpaths))) > > I am not so excited about adding whitelistpaths into mozprofile when we end > up with a pref. Could we just append this to the prefs before creating the > new profile() ? I'm not sure what you mean here. Isn't that exactly what this code is doing? Note that this pref can't go into user_prefs.js or similar, as those are only read after the sandbox is already initialized.

I would prefer to modify the prefs going into the profile() constructor instead of adding a new parameter.

(In reply to Joel Maher ( :jmaher) from comment #54) > I would prefer to modify the prefs going into the profile() constructor > instead of adding a new parameter. The problem is that those end up in user_prefs.js, which as explained above won't work. See for example: https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprofile/mozprofile/profile.py#105 https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprofile/mozprofile/profile.py#183 It's possible to make the solution I used more generic, but that'd still use a new parameter for the non-user.js preferences.

I see the problem then. Given that, I would be ok with the bump on mozprofile.

Comment on attachment 8861548 [details] Bug 1308400 - Compilation fixes. https://reviewboard.mozilla.org/r/133518/#review155166 One small comment; the patch itself looks fine. However, I wonder if the API we expose should be `const char *mozilla::DevelopmentBuildRepoDir()` instead, to share the env reading between the paths? What do you think, I could go either way. ::: dom/ipc/ContentChild.h:751 (Diff revision 2) > > uint64_t > NextWindowID(); > > +bool > +IsDevelopmentBuild(); If this is a public API, should it really be in `mozilla::dom`?

Comment on attachment 8861548 [details] Bug 1308400 - Compilation fixes. https://reviewboard.mozilla.org/r/133518/#review156288 (Moving to r- so bugzilla stops complaing at me :-))

Comment on attachment 8861548 [details] Bug 1308400 - Compilation fixes. https://reviewboard.mozilla.org/r/133518/#review156408 ::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:160 (Diff revision 2) > + // If this is a developer build the resources are symlinks to outside the binary dir. > + // Therefore in non-release builds we allow reads from the whole repository. > + // MOZ_DEVELOPER_REPO_DIR is set by mach run. > + const char *developer_repo_dir = PR_GetEnv("MOZ_DEVELOPER_REPO_DIR"); > + if (developer_repo_dir) { > + policy->AddDir(rdonly, developer_repo_dir); Now that we have the path whitelist pref, can we use that and get rid of the `MOZ_DEVELOPER_REPO_DIR` env var?

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #59) > Comment on attachment 8861548 [details] > Bug 1308400 - Exclude the repo dir in development builds. > > https://reviewboard.mozilla.org/r/133518/#review156408 > > ::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:160 > (Diff revision 2) > > + // If this is a developer build the resources are symlinks to outside the binary dir. > > + // Therefore in non-release builds we allow reads from the whole repository. > > + // MOZ_DEVELOPER_REPO_DIR is set by mach run. > > + const char *developer_repo_dir = PR_GetEnv("MOZ_DEVELOPER_REPO_DIR"); > > + if (developer_repo_dir) { > > + policy->AddDir(rdonly, developer_repo_dir); > > Now that we have the path whitelist pref, can we use that and get rid of the > `MOZ_DEVELOPER_REPO_DIR` env var? Maybe for the Linux build, but we don't intend to carry the prefs outside Linux. (Because they're only there to deal with the increased system/config variance, which we don't expect on Win/Mac)

Comment on attachment 8876236 [details] Bug 1308400 - Whitelist paths with test files during testing. https://reviewboard.mozilla.org/r/147690/#review157238 ::: layout/tools/reftest/runreftest.py:302 (Diff revision 1) > if platform.system() in ("Windows", "Microsoft") and \ > '5.1' in platform.version() and options.e10s: > prefs['layers.acceleration.disabled'] = True > > + # reftest js is read in through content > + if platform.system() == 'Linux': Can you drop the `platform` check or include `'Darwin'` as well? We're going to need it there as well. ::: testing/mochitest/runtests.py:1783 (Diff revision 1) > if platform.system() in ("Windows", "Microsoft") and \ > '5.1' in platform.version() and options.e10s: > prefs['layers.acceleration.disabled'] = True > > + # SpecialPowers and logging are read in through content > + if platform.system() == 'Linux': Same.

Comment on attachment 8876235 [details] Bug 1308400 - Symlink handling for read brokering. https://reviewboard.mozilla.org/r/147688/#review156482 ::: security/sandbox/linux/broker/SandboxBroker.cpp:451 (Diff revision 1) > +SandboxBroker::CheckSymlinkPath(const Policy* policy, char* aPath, size_t aBufSize, size_t aPathLen) > +{ > + char* result = SandboxBroker::SymlinkPath(policy, aPath, NULL); > + if (result != NULL) { > + strncpy(aPath, result, aBufSize); > + aPath[aBufSize - 1] = '\0'; We could also assert that the last byte was already zeroed by strncpy (i.e., that the buffer is large enough and the path wasn't truncated). ::: security/sandbox/linux/broker/SandboxBroker.cpp:504 (Diff revision 1) > + > + nsCString symlinkDest; > + nsCString orig = ReverseSymlinks(nsDependentCString(pathBufSymlink, pathLenSymlink), > + symlinkDest); > + if (!orig.IsEmpty()) { > + SANDBOX_LOG_ERROR("Reversing %s -> %s", aPath, orig.get()); I assume this was meant to be verbose-only? ::: security/sandbox/linux/broker/SandboxBroker.cpp:505 (Diff revision 1) > + nsCString symlinkDest; > + nsCString orig = ReverseSymlinks(nsDependentCString(pathBufSymlink, pathLenSymlink), > + symlinkDest); > + if (!orig.IsEmpty()) { > + SANDBOX_LOG_ERROR("Reversing %s -> %s", aPath, orig.get()); > + strcpy(pathBufSymlink, orig.get()); This `strcpy` makes me uneasy. Maybe release-assert that the string isn't too long? (Or can `pathBufSymlink` be just an `nsCString` instead? The server size doesn't need to avoid heap allocations.) ::: security/sandbox/linux/broker/SandboxBroker.cpp:528 (Diff revision 1) > + // We need to realpath this as well. It's not because it's the destination of > + // a symlink that it doesn't have relative paths left in it. > + char *linkToPath = realpath(symlinkDest.get(), nullptr); > + if (realDestPath && linkToPath) { > + // OK if linkToPath is a prefix of realDestPath > + if (strncmp(realDestPath, linkToPath, symlinkDest.Length())) { `symlinkDest.Length()` is not the length of `linkToPath`. Also, this lets you `..` up to the target of the first symlink. If the user's home directory (or an ancestor) is a symlink, then `~/.local/share/../../.ssh/id_rsa` will be readable. I wonder if we can't just `realpath` everything, including the paths in the policy, and deal with the test file symlink farm either as a special case (like on Mac?) or by walking the entire tree ahead of time and allowing each symlinked file. ::: security/sandbox/linux/broker/SandboxBroker.cpp:838 (Diff revision 1) > + // Record the mapping so we can invert the file to the original > + // symlink. > + nsDependentCString orig(pathBuf, pathLen); > + nsDependentCString xlat(respBuf, respSize); > + if (!orig.Equals(xlat)) { > + SANDBOX_LOG_ERROR("Recording mapping %s -> %s", This also looks like it should be verbose-only. ::: security/sandbox/linux/broker/SandboxBroker.cpp:840 (Diff revision 1) > + nsDependentCString orig(pathBuf, pathLen); > + nsDependentCString xlat(respBuf, respSize); > + if (!orig.Equals(xlat)) { > + SANDBOX_LOG_ERROR("Recording mapping %s -> %s", > + xlat.get(), orig.get()); > + mSymlinkMap.Put(xlat, orig); This is unlikely to do anything useful if `xlat[0] != '/'`. ::: security/sandbox/linux/broker/SandboxBroker.cpp:845 (Diff revision 1) > + mSymlinkMap.Put(xlat, orig); > + } > + // Make sure we can invert a fully resolved mapping too. If our > + // caller is realpath, and there's a relative path involved, the > + // client side will try to open this one. > + char *resolvedBuf = realpath(pathBuf, nullptr); This is more complicated than I'd like, but if it works then maybe we should just land it and see if we can clean things up later. It occurs to me that we don't need to let the client normalize the path in the first place — we can return `EINVAL` (not a symlink) instead. The question then is whether there are any cases where we still need to do an actual `readlink`, given that we can `realpath` on the server side if we need to. ::: security/sandbox/linux/broker/SandboxBroker.cpp:849 (Diff revision 1) > + // client side will try to open this one. > + char *resolvedBuf = realpath(pathBuf, nullptr); > + if (resolvedBuf) { > + nsDependentCString resolvedXlat(resolvedBuf); > + if (!orig.Equals(resolvedXlat) && !xlat.Equals(resolvedXlat)) { > + SANDBOX_LOG_ERROR("Recording mapping %s -> %s", And another verbose. (Maybe we should have a `SANDBOX_LOG_VERBOSE` for this, but that doesn't need to happen in this bug.) ::: security/sandbox/linux/broker/SandboxBrokerRealpath.cpp:207 (Diff revision 1) > + free(resolved); > + errno = ELOOP; > + return (NULL); > + } > + // Our changes start here: > + // It's a symlink, check for write permissions, in which case Can we have a non-writable symlink inside a writeable directory? Because then you could rename the directory and affect the symlink that way.

Comment on attachment 8861547 [details] Bug 1308400 - Support file process, whitelist path prefs. https://reviewboard.mozilla.org/r/133516/#review156390 ::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:211 (Diff revision 2) > + } > + > + nsAdoptingCString extraWritePathString = > Preferences::GetCString("security.sandbox.content.write_path_whitelist"); > - if (extraPathString) { > - for (const nsCSubstring& path : extraPathString.Split(',')) { > + if (extraWritePathString) { > + for (const nsCSubstring& path : extraWritePathString.Split(',')) { This duplicated code could be factored out easily; the only local variable it depends on is `policy`. ::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:220 (Diff revision 2) > } > } > > + // file:// processes get global read permissions > + if (aFileProcess) { > + policy->AddDir(rdonly, "/"); We could skip reading the read_path_whitelist pref for file processes, I think, but I don't know that it makes much difference in practice. ::: security/sandbox/test/browser_content_sandbox_fs.js:263 (Diff revision 2) > // that will be read from either a web or file process. > let tests = []; > > + // The Linux test runners create the temporary profile in the same > + // system temp dir we give write access to, so this gives a false > + // positive. Seems like there should be a followup bug to undo this when we stop allowing write access to /tmp.

(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #63) > ::: security/sandbox/linux/broker/SandboxBroker.cpp:528 > (Diff revision 1) > > + // We need to realpath this as well. It's not because it's the destination of > > + // a symlink that it doesn't have relative paths left in it. > > + char *linkToPath = realpath(symlinkDest.get(), nullptr); > > + if (realDestPath && linkToPath) { > > + // OK if linkToPath is a prefix of realDestPath > > + if (strncmp(realDestPath, linkToPath, symlinkDest.Length())) { > > `symlinkDest.Length()` is not the length of `linkToPath`. > > Also, this lets you `..` up to the target of the first symlink. If the > user's home directory (or an ancestor) is a symlink, then > `~/.local/share/../../.ssh/id_rsa` will be readable. Thinking about it, I think this case simply won't work at all: the home directory doesn't have read permissions, so stopping the permission resolution at the first symlink will cause a reject. > I wonder if we can't just `realpath` everything, including the paths in the > policy, Wont work: r+ on ~/.local/foo (in policy) ~/.local/foo/bar -> ~/baz (will fail as it's not in an r+ path on lookup) > or by walking the entire tree ahead of time and allowing each > symlinked file. I'm concerned that some of the directories we allow on r+ on are potentially huge. > ::: security/sandbox/linux/broker/SandboxBroker.cpp:840 > (Diff revision 1) > > + nsDependentCString orig(pathBuf, pathLen); > > + nsDependentCString xlat(respBuf, respSize); > > + if (!orig.Equals(xlat)) { > > + SANDBOX_LOG_ERROR("Recording mapping %s -> %s", > > + xlat.get(), orig.get()); > > + mSymlinkMap.Put(xlat, orig); > > This is unlikely to do anything useful if `xlat[0] != '/'`. Why? Symlinks can be of the form foo->baz right? Don't need an absolute path. > It occurs to me that we don't need to let the client normalize the path in > the first place — we can return `EINVAL` (not a symlink) instead. The > question then is whether there are any cases where we still need to do an > actual `readlink`, given that we can `realpath` on the server side if we > need to. This might be a nice trick if it works. But we need to lie in stat() as well.

(In reply to Gian-Carlo Pascutto [:gcp] from comment #65) > (In reply to Jed Davis [:jld] (⏰UTC-6) from comment #63) > > > + mSymlinkMap.Put(xlat, orig); > > > > This is unlikely to do anything useful if `xlat[0] != '/'`. > > Why? > > Symlinks can be of the form foo->baz right? Don't need an absolute path. Right, but relative symlinks are resolved relative to the directory the symlink is in; storing that relative path as-is in mSymlinkMap won't work right, because we're keying into it with (prefixes of) the path the client gives us, which is either absolute or relative to its working directory. > > It occurs to me that we don't need to let the client normalize the path in > > the first place — we can return `EINVAL` (not a symlink) instead. The > > question then is whether there are any cases where we still need to do an > > actual `readlink`, given that we can `realpath` on the server side if we > > need to. > > This might be a nice trick if it works. But we need to lie in stat() as well. Good point; lstat would have to be mapped to regular stat.

Comment on attachment 8861548 [details] Bug 1308400 - Compilation fixes. https://reviewboard.mozilla.org/r/133518/#review158954 r+, one small nit. ::: security/sandbox/common/SandboxSettings.cpp:15 (Diff revision 3) > #include "mozilla/ModuleUtils.h" > #include "mozilla/Preferences.h" > > namespace mozilla { > > +bool IsDevelopmentBuild() Can you include the comment from the original function?

Comment on attachment 8876236 [details] Bug 1308400 - Whitelist paths with test files during testing. https://reviewboard.mozilla.org/r/147690/#review158956 Mostly looks fine, two small comments. There's going to be conflicts with bug 1374557 though. ::: layout/tools/reftest/runreftest.py:311 (Diff revision 2) > + try: > + if options.workPath: > + sandbox_whitelist_paths.append(options.workPath) > + except AttributeError: > + pass > + # Trailing slashes are needed to indicate directories For the macOS version of this patch, I ended up needing to wrap the "add a trailing slash" logic in `if is_linux`. The macOS sandbox insists on not having trailing slahes in paths for some reason. ::: testing/xpcshell/runxpcshelltests.py:954 (Diff revision 2) > # Don't override the user's choice here. See bug 1049688. > self.env.setdefault('MOZ_DISABLE_NONLOCAL_CONNECTIONS', '1') > if self.mozInfo.get("topsrcdir") is not None: > self.env["MOZ_DEVELOPER_REPO_DIR"] = self.mozInfo["topsrcdir"].encode() > + if sys.platform.startswith('linux'): > + self.env.setdefault('MOZ_DISABLE_CONTENT_SANDBOX', '1') This can be removed, if you look 3 lines down, this is already done cross platform.

Comment on attachment 8861548 [details] Bug 1308400 - Compilation fixes. https://reviewboard.mozilla.org/r/133518/#review159118 ::: security/sandbox/common/SandboxSettings.cpp:15 (Diff revision 3) > #include "mozilla/ModuleUtils.h" > #include "mozilla/Preferences.h" > > namespace mozilla { > > +bool IsDevelopmentBuild() Bleh, as pointed out, the comment is in the header.

Comment on attachment 8876236 [details] Bug 1308400 - Whitelist paths with test files during testing. https://reviewboard.mozilla.org/r/147690/#review160856 ::: testing/mozharness/scripts/desktop_unittest.py:244 (Diff revision 3) > return self.abs_dirs > abs_dirs = super(DesktopUnittest, self).query_abs_dirs() > > c = self.config > dirs = {} > + dirs['abs_work_dir'] = abs_dirs['abs_work_dir'] Is this necessary? I didn't dfind it necessary on macOS. ::: testing/mozharness/scripts/desktop_unittest.py:368 (Diff revision 3) > error_summary_file = os.path.join(dirs['abs_blob_upload_dir'], > '%s_errorsummary.log' % suite) > str_format_values = { > 'binary_path': self.binary_path, > 'symbols_path': self._query_symbols_url(), > + 'abs_work_dir' : dirs['abs_work_dir'], This duplicates a line a few below. ::: testing/xpcshell/runxpcshelltests.py:953 (Diff revision 3) > + if sys.platform.startswith('linux'): > + self.env.setdefault('MOZ_DISABLE_CONTENT_SANDBOX', '1') These can be removed, they duplicate the line below

Comment on attachment 8884933 [details] Bug 1308400 - Report failures in file processes too. https://reviewboard.mozilla.org/r/155764/#review163064

Comment on attachment 8876235 [details] Bug 1308400 - Symlink handling for read brokering. https://reviewboard.mozilla.org/r/147688/#review163066 Ship it!

https://hg.mozilla.org/integration/mozilla-inbound/rev/167f91f87172c3fd4ca7ac8f8e1f6bd6a2bf2dc1 Bug 1308400 - Support file process, whitelist path prefs. r=jld https://hg.mozilla.org/integration/mozilla-inbound/rev/84b1aafdc1c21d18e060a7059f0af373b683ed9e Bug 1308400 - Symlink handling for read brokering. r=jld https://hg.mozilla.org/integration/mozilla-inbound/rev/f180a4ed7aabe89f5c038f59a4f7a69d0200a6db Bug 1308400 - Whitelist paths with test files during testing. r=Alex_Gaynor https://hg.mozilla.org/integration/mozilla-inbound/rev/b5fa08551d6e74d8300fa94f3161afdffd867764 Bug 1308400 - Report failures in file processes too. r=jld

https://hg.mozilla.org/integration/mozilla-inbound/rev/1dea32a148a9bf79a83dae48a3ef46fb36ce1e99 Bug 1308400 - Disable logging again, fix removed object dir. r=bustage

https://hg.mozilla.org/mozilla-central/rev/167f91f87172 https://hg.mozilla.org/mozilla-central/rev/84b1aafdc1c2 https://hg.mozilla.org/mozilla-central/rev/f180a4ed7aab https://hg.mozilla.org/mozilla-central/rev/b5fa08551d6e https://hg.mozilla.org/mozilla-central/rev/1dea32a148a9

Minor commit-message-crafting gripe: it would have been nice if at least one of the commit messages here had included the word "sandbox", to give a better hint about the feature that's involved. (I was just sifting through a pushlog with hundreds of commits, trying to find the regression that caused bug 1386502 on Linux, and I had a suspicion that it was sandboxing-related but my find-in-page attempt for "sandbox" didn't turn up anything. And when I skimmed the hundreds of commit messages for anything that looked sandbox-realted, this bug's messages didn't jump out as relevant to me, because they were fairly generic (e.g. "Report failures in file processes too", "Support file process, whitelist path prefs"). I only isolated this bug after multiple rounds of hg bisect + rebuild + retest.) IMO one of the important things to convey in commit messages is "roughly what feature / part-of-the-code is changing" -- this is super handy for people skimming commit logs to find possible candidates that may have caused a regression.

Jed how did you review this? You have been working to support musl downstream in sandbox yet you permit an internal glibc header to be used which will break the compilation. If these type of regressions are gonna stay the norm for mozilla products, maybe it is time I am away from mozilla products in general.

I filed bug 1389078 for what I'm assuming to be the problem. We don't have any musl libc builds in automation, so yes, these kind of regressions are going to stay the norm, particularly for stuff where macOS clang and Android bionic are 'bug compatible' with libc but musl isn't. This might be the reason why even Jed got fooled by my shenanigans here: the lifted source comes out of bionic and is itself taken from FreeBSD, so I think he can be excused for missing the GNU libc specific cdefs.h usage here, being lulled into a false sense of security. It happens - evidently - to the best of us. The sandbox in particular sits closely to the syscall/libc interfaces and is reasonably prone to run into other libc related issues aside from this one.

Bug 1388046 effectively disabled this on 56.

Release Note Request (optional, but appreciated) [Why is this notable]: Major new security feature with potential compatibility impact [Affects Firefox for Android]: No [Suggested wording]: The content process now has a stricter security sandbox that blocks filesystem reading and writing (Linux). [Links (documentation, blog post, etc)]: Will write a blog post

Gian-Carlo, do you have a blog post to link to for this note? Checking in with you since we are looking at adding this release note for tomorrow.

http://www.morbo.org/2017/11/linux-sandboxing-improvements-in.html is his post. (Leaving the ni? in case there's extra context he wants to provide, but it's late where Gian-Carlo lives :-))

Alex already pointed it out, and it's in fact already linked in the release notes.

This was added to 57 release notes.