The goal is to provide a broker policy for filesystem access that allows most modern Linux distributions to work. Seccomp-bpf currently intercepts open, openat, access, faccessat, stat, lstat, fstatat. Note that for this to be effective, some syscalls like symlink also need to be restricted.

We discussed on IRC that the seccomp design requires the handlers to be async-signal safe, and while there's conflicting information about realpath, both the API design and the libc implementation use malloc, which isn't. So this patch will have to be revised to translate in the chrome process.

Comment on attachment 8784455 [details] Bug 1289718 - Extend sandbox file broker to handle paths, support more syscalls. https://reviewboard.mozilla.org/r/73902/#review72960 ::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:39 (Diff revision 1) > // When broker is running in permissive mode, we enable it > // automatically regardless of the device. > if (SandboxInfo::Get().Test(SandboxInfo::kPermissive)) { > return true; > } > +#elif defined(XP_LINUX) `XP_LINUX` is always defined if `security/sandbox/linux` is being built. ::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:119 (Diff revision 1) > > policy->AddPath(rdonly, "/data/local/tmp/profiler.options", > SandboxBroker::Policy::AddAlways); // bug 1029337 > > mCommonContentPolicy.reset(policy); > +#elif defined(MOZ_CONTENT_SANDBOX) && defined(XP_LINUX) As above with the `XP_LINUX`. ::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:157 (Diff revision 1) > // Bug 1198552: memory reporting. > policy->AddPath(rdonly, nsPrintfCString("/proc/%d/statm", aPid).get()); > policy->AddPath(rdonly, nsPrintfCString("/proc/%d/smaps", aPid).get()); > > return policy; > -#else // MOZ_WIDGET_GONK > +#elif defined(XP_LINUX) …and here. A bunch of common code can be factored out of the ifdef. ::: security/sandbox/linux/SandboxBrokerClient.h:37 (Diff revision 4) > int Open(const char* aPath, int aFlags); > int Access(const char* aPath, int aMode); > int Stat(const char* aPath, struct stat* aStat); > int LStat(const char* aPath, struct stat* aStat); > + int Chmod(const char* aPath, int aMode); > + int Link(const char* aPath, const char* aPath2); It's not clear that we actually need `link`… but if we know we need `symlink` then it's not much more code to support both, and gets this landed sooner, so that's fine. ::: security/sandbox/linux/SandboxBrokerClient.h:51 (Diff revision 4) > int mFileDesc; > > - int DoCall(const Request* aReq, const char* aPath, struct stat* aStat, > + int DoCall(const Request* aReq, > + const char* aPath, > + const char* aPath2, > + void *aBuff, Nit: the name could indicate it's a buffer for the response. ::: security/sandbox/linux/SandboxBrokerClient.cpp:68 (Diff revision 4) > SANDBOX_LOG_ERROR("not rewriting unexpectedly long path %s", aPath); > } > } > > - struct iovec ios[2]; > - int respFds[2]; > + struct iovec ios[3]; > + int respFds[3]; Is the third fd used for anything? ::: security/sandbox/linux/SandboxBrokerClient.cpp:131 (Diff revision 4) > if (recvd == 0) { > SANDBOX_LOG_ERROR("Unexpected EOF, op %d flags 0%o path %s", > aReq->mOp, aReq->mFlags, path); > return -EIO; > } > - if (resp.mError != 0) { > + // mError is positive for failure here, we invert the logic Does this comment still apply now that you've flipped the sign on `mError`? ::: security/sandbox/linux/SandboxBrokerClient.cpp:135 (Diff revision 4) > } > - if (resp.mError != 0) { > + // mError is positive for failure here, we invert the logic > + if (resp.mError < 0) { > // If the operation fails, the return payload will be empty; > // adjust the iov_len for the following assertion. > ios[1].iov_len = 0; Do we even need this now that the assertion a few lines down is changed to `<=`? ::: security/sandbox/linux/SandboxBrokerClient.cpp:138 (Diff revision 4) > // If the operation fails, the return payload will be empty; > // adjust the iov_len for the following assertion. > ios[1].iov_len = 0; > } > - MOZ_ASSERT(static_cast<size_t>(recvd) == ios[0].iov_len + ios[1].iov_len); > - if (resp.mError == 0) { > + MOZ_ASSERT(static_cast<size_t>(recvd) <= ios[0].iov_len + ios[1].iov_len); > + if (resp.mError >= 0) { Why would `resp.mError` be greater than 0? ::: security/sandbox/linux/SandboxFilter.cpp:564 (Diff revision 4) > return Trap(StatTrap, mBroker); > CASES_FOR_lstat: > return Trap(LStatTrap, mBroker); > CASES_FOR_fstatat: > return Trap(StatAtTrap, mBroker); > + case __NR_chmod: We might run into problems at some point with the `at` versions of these syscalls (except `rmdir`, which apparently doesn't have one?), but given that we're already blocking them and haven't gotten any bug reports, apparently we're not at that point yet. ::: security/sandbox/linux/SandboxFilter.cpp:582 (Diff revision 4) > + return Trap(UnlinkTrap, mBroker); > + case __NR_readlink: > + return Trap(ReadlinkTrap, mBroker); > } > } else { > // No broker; allow the syscalls directly. )-: This case should also be adjusted. If we intend to always have a broker for desktop content, that adjustment would be a `MOZ_CRASH` (ifdef'ed) rather than allowing the added syscalls. ::: security/sandbox/linux/broker/SandboxBroker.cpp:373 (Diff revision 4) > + } > + return stat(aPath, (struct stat*)aBuff); > +} > + > +static int > +DoChmod(const char* aPath, int aMode) Do we need these `Do___` wrappers when they're just tail-calling the underlying libc function? ::: security/sandbox/linux/broker/SandboxBroker.cpp:423 (Diff revision 4) > +} > + > +size_t > +SandboxBroker::ConvertToRealPath(char* aPath, size_t aBufSize, size_t aPathLen) > +{ > + if (strstr(aPath, "..") != NULL) { This is a little weird – realpath will expand all symlinks, but the way we're using it here means that that depends on the presence of a `..` in a possibly unrelated part of the path. (And not even `..` as a path component, just `..` anywhere.) Also we're doing a bunch of stuff involving the filesystem and based on untrusted data from the client. It's better than letting the process do filesystem access directly, to be sure, but it would be good to try to find and (if possible) deal with the code that needs this as a followup. ::: security/sandbox/linux/broker/SandboxBroker.cpp:465 (Diff revision 4) > } > #endif > > while (true) { > struct iovec ios[2]; > + // We will receive the path strings in 1 buffer an split them back up. Typo nit: “and”. ::: security/sandbox/linux/broker/SandboxBroker.cpp:531 (Diff revision 4) > - pathLen = recvd - sizeof(req); > - // It shouldn't be possible for recvmsg to violate this assertion, > - // but one more predictable branch shouldn't have much perf impact: > - MOZ_RELEASE_ASSERT(pathLen <= kMaxPathLen); > + int perms; > + > + // Find end of first string > + char* split = static_cast<char*>(memchr(recvBuf, 0, sizeof(recvBuf))); > + size_t splitidx = split - recvBuf; > + if (splitidx <= kMaxPathLen + 1) { This comparison seems to be off by one — the strcpy will copy `splitidx + 1` bytes, so the `splitidx == kMaxPathLen + 1` case would be bad. ::: security/sandbox/linux/broker/SandboxBroker.cpp:537 (Diff revision 4) > + strcpy(pathBuf, recvBuf); > + strncpy(pathBuf2, split + 1, kMaxPathLen + 1); > + > + // Look up the first pathname. > + // Force 0 termination. > + pathLen = strnlen(pathBuf, kMaxPathLen + 1); Why would pathLen not be equal to splitidx? ::: security/sandbox/linux/broker/SandboxBrokerCommon.h:48 (Diff revision 4) > Operation mOp; > // For open, flags; for access, "mode"; for stat, O_NOFOLLOW for lstat. > int mFlags; > + // Size of return value buffer, if any > + size_t mBufSize; > // The rest of the packet is the pathname. It might be simpler if the request header had a length field for the two-pathname case, instead of using internal null termination. (Or two length fields. The reason it doesn't have one now is because the message length also carries that information and it seemed better to give the untrusted client as few ways as possible to communicate the same meaning, but if it makes the broker simpler to have in-band lengths for everything then that seems better.) ::: security/sandbox/linux/broker/SandboxBrokerCommon.h:53 (Diff revision 4) > // The rest of the packet is the pathname. > // SCM_RIGHTS for response socket attached. > }; > > struct Response { > int mError; // errno, or 0 for no error Comment should reflect that the error is negative now (like the Linux syscall ABI and unlike the C `errno` API). ::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:39 (Diff revision 4) > // When broker is running in permissive mode, we enable it > // automatically regardless of the device. > if (SandboxInfo::Get().Test(SandboxInfo::kPermissive)) { > return true; > } > +#elif defined(XP_LINUX) `defined(XP_LINUX)` is always true in this file. ::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:40 (Diff revision 4) > // automatically regardless of the device. > if (SandboxInfo::Get().Test(SandboxInfo::kPermissive)) { > return true; > } > +#elif defined(XP_LINUX) > + // XXX: is this check needed? Running with `MOZ_FAKE_NO_SANDBOX` might help answer this comment's question. ::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:119 (Diff revision 4) > > policy->AddPath(rdonly, "/data/local/tmp/profiler.options", > SandboxBroker::Policy::AddAlways); // bug 1029337 > > mCommonContentPolicy.reset(policy); > +#elif defined(MOZ_CONTENT_SANDBOX) && defined(XP_LINUX) `defined(XP_LINUX)` is always true in this file. ::: security/sandbox/linux/broker/SandboxBrokerPolicyFactory.cpp:161 (Diff revision 4) > // Bug 1198552: memory reporting. > policy->AddPath(rdonly, nsPrintfCString("/proc/%d/statm", aPid).get()); > policy->AddPath(rdonly, nsPrintfCString("/proc/%d/smaps", aPid).get()); > > return policy; > -#else // MOZ_WIDGET_GONK > +#elif defined(XP_LINUX) `defined(XP_LINUX)` is always true in this file.

(In reply to Jed Davis [:jld] {⏰UTC-6} from comment #38) > It's not clear that we actually need `link`… but if we know we need > `symlink` then it's not much more code to support both, and gets this landed > sooner, so that's fine. According to our tracking sheet it's used by some Japanese IME software. > Why would `resp.mError` be greater than 0? Added a comment to clarify. (There's a single syscall that has a meaningful return value) > This is a little weird – realpath will expand all symlinks, but the way > we're using it here means that that depends on the presence of a `..` in a > possibly unrelated part of the path. (And not even `..` as a path > component, just `..` anywhere.) > > Also we're doing a bunch of stuff involving the filesystem and based on > untrusted data from the client. Right. This would be nicer in the client, but alas, async-signal safety. I doubt we want to create a third process just to deal with the filesystem translation so I'm not sure what other options we have. Some notes: 1) The .. check is just a speedup check to skip the memory allocation overhead for 99% of the paths going through here. 2) There's no libc function that's realpath-without-symlinks, and I'm not confident writing our own would be better from a security perspective. Maybe we can get our own realpath-without-symlinks-but-async-safe, though, in which case it automatically would be better for security. For a first version I think we can live with realpath. 3) The things that require this is stuff in the content process that's trying to access various browser resources and components via relative paths. I think they're actually fine to do this, which means we can't avoid the translation? > This comparison seems to be off by one — the strcpy will copy `splitidx + 1` > bytes, so the `splitidx == kMaxPathLen + 1` case would be bad. Fixed and cleaned this up a bit, including an implicit reliance on unsigned wraparound. Also fixed the same error for the second path, oops. This is easily one of the most dangerous parts of this patch. > It might be simpler if the request header had a length field for the > two-pathname case, instead of using internal null termination. > > (Or two length fields. The reason it doesn't have one now is because the > message length also carries that information and it seemed better to give > the untrusted client as few ways as possible to communicate the same > meaning, but if it makes the broker simpler to have in-band lengths for > everything then that seems better.) I don't think it makes anything simpler for similar reasons: we can't trust the client to send the correct information, so we have to scan the buffer anyway to find where and how many 0's there are, and whether the strings fit etc. > Running with `MOZ_FAKE_NO_SANDBOX` might help answer this comment's question. Works fine, so removed that code. I've cleaned up some other parts that should make it more landable, such as splitting the logging between permissive mode, and denials. The latter are now logged in DEBUG mode.

Comment on attachment 8784455 [details] Bug 1289718 - Extend sandbox file broker to handle paths, support more syscalls. https://reviewboard.mozilla.org/r/73902/#review79048 See comment, otherwise looks good to me. I first suspected that one may be able to trick the logic with symlinks, but my idea didn't work and everything seems to work as it is supposed to. Regarding the realpath() in the parent, well we can't do it in the child because of async-signal safety. I mean ideally we don't want to have any path to contain ../, so getting rid of that code is the ideal case. But the code may reside in a library which we can't affect directly. So I think this is fine for now, but in the future we want to get rid of it again (like with other system calls). Your last push to try seems promising, although I suspect we will encounter some problems on nightly. Once you fixed try issues I think we can land this, I would suggest to write to dev-platform and let them know that this change will land on nightly and that problems may arise do to that. ::: security/sandbox/linux/SandboxFilter.cpp:586 (Diff revision 6) > + return Trap(ReadlinkTrap, mBroker); > } > } else { > +#ifdef DESKTOP > + MOZ_CRASH("No broker for filesystem calls set up."); > +#else Since we currently control broker/no broker through a pref, changing that pref with this ifdef in place won't change anything because the system calls are not whitelisted.

(In reply to Gian-Carlo Pascutto [:gcp] from comment #39) > (In reply to Jed Davis [:jld] {⏰UTC-6} from comment #38) > > It's not clear that we actually need `link`… but if we know we need > > `symlink` then it's not much more code to support both, and gets this landed > > sooner, so that's fine. > > According to our tracking sheet it's used by some Japanese IME software. Yes, that reflects bug 1285827. But then the patch for bug 1289500 disabled IME module loading in the content process, which might have taken care of link() as well as vfork(). > Right. This would be nicer in the client, but alas, async-signal safety. I > doubt we want to create a third process just to deal with the filesystem > translation so I'm not sure what other options we have. Well... if we know we're going to get a path with a specific configuration of ../../.. we could allow that path. But if it's something that Gecko developers assume they can do generally then that doesn't help, because we'll just get more. Anyway, not a blocker for this patch. > 2) There's no libc function that's realpath-without-symlinks, and I'm not > confident writing our own would be better from a security perspective. Maybe > we can get our own realpath-without-symlinks-but-async-safe, though, in > which case it automatically would be better for security. For a first > version I think we can live with realpath. Agreed. > 3) The things that require this is stuff in the content process that's > trying to access various browser resources and components via relative > paths. I think they're actually fine to do this, which means we can't avoid > the translation? If it's our code that's using .. because there's some information that's not in the XPCOM directory service and could be (for example) that might be fixable.

Comment on attachment 8784455 [details] Bug 1289718 - Extend sandbox file broker to handle paths, support more syscalls. https://reviewboard.mozilla.org/r/73902/#review79550 (Sorry for the delay in reviewing; I've been busy with NSS work.) For the most part I like the changes in this version of the patch, but there's one thing in the request parsing: ::: security/sandbox/linux/broker/SandboxBroker.cpp:494 (Diff revision 6) > - // Look up the pathname. > - pathLen = recvd - sizeof(req); > - // It shouldn't be possible for recvmsg to violate this assertion, > - // but one more predictable branch shouldn't have much perf impact: > - MOZ_RELEASE_ASSERT(pathLen <= kMaxPathLen); > - pathBuf[pathLen] = '\0'; > + // Clear permissions > + int perms; > + > + // Find end of first string > + char* split = static_cast<char*>(memchr(recvBuf, 0, sizeof(recvBuf))); > + size_t first_len = split - recvBuf; // excludes terminating 0 This is undefined behavior if recvBuf doesn't contain a 0 byte. But with the protocol change in this patch, if the received message doesn't end with a zero byte (`recvBufLen > 0 && recvBuf[recvBufLen - 1] == 0`, where `recvBufLen = static_cast<size_t>(recvd) - sizeof(req)`) then the message is invalid, and that could be checked before this point. And then this code could just use strlen.

(In reply to Julian Hector [:tedd] [:jhector] on leave til Oct. 17 from comment #44) > > } > > } else { > > +#ifdef DESKTOP > > + MOZ_CRASH("No broker for filesystem calls set up."); > > +#else > > Since we currently control broker/no broker through a pref, changing that > pref with this ifdef in place won't change anything because the system calls > are not whitelisted. Right. I added that after a review comment from jld. I guess the answer to "If we intend to always have a broker for desktop content..." is that currently we allows this to be controlled by a pref. (And I suspect that will be the case for quite a while?)

Comment on attachment 8784455 [details] Bug 1289718 - Extend sandbox file broker to handle paths, support more syscalls. https://reviewboard.mozilla.org/r/73902/#review80332 Ship it! ::: security/sandbox/linux/broker/SandboxBroker.cpp:369 (Diff revisions 6 - 7) > static int > DoStat(const char* aPath, void* aBuff, int aFlags) > { > +#if defined(__NR_stat64) > + if (aFlags & O_NOFOLLOW) { > + return lstat64(aPath, (struct stat64*)aBuff); Suggested followup (doesn't need to block landing this; can happen later): have some header file do a typedef for the correct `struct stat` variant and use that instead of `ifdef`ing everywhere the struct is used. ::: security/sandbox/linux/SandboxFilter.cpp:381 (Diff revision 7) > } > > static intptr_t StatTrap(ArgsRef aArgs, void* aux) { > auto broker = static_cast<SandboxBrokerClient*>(aux); > auto path = reinterpret_cast<const char*>(aArgs.args[0]); > auto buf = reinterpret_cast<struct stat*>(aArgs.args[1]); Not your change, but there should be a followup to fix these casts and all the other existing uses of `struct stat`. This was fine on Android, where `struct stat` was always really `stat64`, but (as you found out the hard way) not glibc. (Also I still don't quite understand that, because I would've thought we'd be building with 64-bit `off_t`, given that it's 2016 and not 1996, but maybe not?)

Comment on attachment 8796262 [details] Bug 1289718 - Clean up stat/stat64 wrapper. Deal with non-default TMPDIR. https://reviewboard.mozilla.org/r/82166/#review81514 ::: security/sandbox/linux/broker/SandboxBrokerCommon.h:8 (Diff revision 1) > /* This Source Code Form is subject to the terms of the Mozilla Public > * License, v. 2.0. If a copy of the MPL was not distributed with this file, > * You can obtain one at http://mozilla.org/MPL/2.0/. */ > > -#ifndef mozilla_SandboxBrokerTypes_h > -#define mozilla_SandboxBrokerTypes_h > +#ifndef mozilla_SandboxBrokerCommon_h > +#define mozilla_SandboxBrokerCommon_h Oops. Thanks for fixing this. ::: toolkit/crashreporter/nsExceptionHandler.cpp:1187 (Diff revision 1) > -#if defined(XP_MACOSX) || defined(__ANDROID__) > +#if defined(XP_MACOSX) || defined(__ANDROID__) || defined(XP_LINUX) > static size_t > EnsureTrailingSlash(XP_CHAR* aBuf, size_t aBufLen) > { > size_t len = XP_STRLEN(aBuf); > if ((len + 2) < aBufLen && aBuf[len - 1] != XP_PATH_SEPARATOR_CHAR) { Not your change, but this looks broken if `len == 0`. (Also I think the first comparison is one element stricter than it needs to be.) ::: toolkit/crashreporter/nsExceptionHandler.cpp:1263 (Diff revision 1) > BuildTempPath(char* aBuf, size_t aBufLen) > { > - // we assume it's always /tmp on unix systems > + const char *tempenv = PR_GetEnv("TMPDIR"); > NS_NAMED_LITERAL_CSTRING(tmpPath, "/tmp/"); > + if (!tempenv) { > + tempenv = tmpPath.get(); Suggestion: `tmpPath` could be replaced by a string literal, now that the `tmpPath.Length()` call is gone.

Pushed by gpascutto@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a79ec9afac7b Extend sandbox file broker to handle paths, support more syscalls. r=jld,tedd https://hg.mozilla.org/integration/autoland/rev/c838d2546cad Clean up stat/stat64 wrapper. Deal with non-default TMPDIR. r=jld

https://hg.mozilla.org/mozilla-central/rev/a79ec9afac7b https://hg.mozilla.org/mozilla-central/rev/c838d2546cad

With this patch, WebGL1 and WebGL2 no longer work for me. Found with mozregression, see https://bugzilla.mozilla.org/show_bug.cgi?id=1296345#c2

Filed as bug 1308851. I have a system with an NVIDIA card so can investigate.

this broke webgl when using the radeonsi driver (r600 was not tested), please see bug 1312678