We can probably get away with blocking this, and it solves some issues when passing files to the parent via the temporary dir.

Can we also disallow symlink?

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #1) > Can we also disallow symlink? It looks like we can. And that almost removes the need for the check for “writeable” symlinks in bug 1308400, but not quite, because “write permission” would allow rename().

Apparently we also don't need rename() anymore? In which case I think we can also GC the stuff for requests with two paths.

This is an improvement, not a regression.

It looks like I won't have to make this into a security bug like I thought I would. If a client could create a relative symlink and then rename it to another location, it could gain access through the symlink (either with the new symlink inheritance semantics or through just accessing the symlink directly by relative path) to things that it should not be able to access. However, we won't create relative symlinks. What we might do is normalize a relative path (relative to the process's cwd, which is wrong) and create an absolute symlink, so things might break mysteriously if code were actually doing that (and the thesis of this bug is that it's actually not), but that won't be usable for escapes — the (oddly normalized) target has to be writable according to the policy, and the referent won't change if the symlink is moved. The one oddity here is that if something were write-only then you could also get read permission, but we only ever did that on B2G with the log devices (and B2G's cancellation was the day before bug 1289718 landed, if I'm reading correctly).

Comment on attachment 8896044 [details] Bug 1380701 - Remove brokering for link, unlink, and rename. https://reviewboard.mozilla.org/r/160510/#review172634

Comment on attachment 8896045 [details] Bug 1380701 - Remove the file broker protocol support for two-path operations. https://reviewboard.mozilla.org/r/160512/#review172638 ::: security/sandbox/linux/broker/SandboxBroker.cpp:567 (Diff revision 1) > - } > > - // First path should fit in maximum path length buffer. > - size_t first_len = strlen(recvBuf); > - if (first_len <= kMaxPathLen) { > - strcpy(pathBuf, recvBuf); > + // Look up the pathname but first translate relative paths. > + // (Make a copy so we can get back the original path if needed.) > + char pathBuf[kMaxPathLen + 1]; > + memset(pathBuf, 0, sizeof(pathBuf)); Given that recvBuf is zero terminated, I'm not sure why we're doing memset/memcpy here instead of strcpy. I generally prefer string instructions if we know the data is in fact zero terminated strings. Sizes don't seem to be an issue here, and if they were, I think strlcpy is available here.

Comment on attachment 8896044 [details] Bug 1380701 - Remove brokering for link, unlink, and rename. https://reviewboard.mozilla.org/r/160510/#review172730 There's a test for symlinking in the JS filesystem tests that's currently behind an `isMac()` that you should be able to run on Linux as well now.

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #10) > There's a test for symlinking in the JS filesystem tests that's currently > behind an `isMac()` that you should be able to run on Linux as well now. symlink() will crash on Nightly after this patch, not just fail. We do have support for not crashing instead, but it's not exposed usefully yet; there's also bug 1338741's proposal of letting the process crash and checking that it crashes in the expected way, but that's not implemented yet either.

So I found another bug: readlink() doesn't null-terminate, so if the symlink target is exactly 4097 bytes, we'll fill the entire buffer and feed a nonterminated string to nsDependentCString. On a debug build, that will assert, but on a release build it will proceed and… probably be harmless. Copying the nsDependentCString to make the hashtable keys/values will terminate it, if I'm reading the code correctly, and the only thing that expects null-termination is a verbose log message that would need to be enabled with an environment variable (and which will, worst case, crash; the log message buffer is much smaller than PATH_MAX, so it won't even leak data to stderr). Also, Linux won't create symlinks with targets longer than 4095 bytes, even over NFS. This is why there are weasel words in the last paragraph: I can't easily test this case. I don't know if there are cases where it would copy out 4097 bytes from readlink() if the long symlink already existed somehow (e.g., NFS), but that's a *very* unlikely threat model even if this were dangerous — which, as described above, I don't think it is.

Comment on attachment 8896045 [details] Bug 1380701 - Remove the file broker protocol support for two-path operations. I got rid of the buffer zeroing and replaced it with explicit zero-termination (thus the previous comment) and strlcpy; this could use another review.

Comment on attachment 8896045 [details] Bug 1380701 - Remove the file broker protocol support for two-path operations. https://reviewboard.mozilla.org/r/160512/#review174468

Pushed by jedavis@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4456ebfe5657 Remove brokering for link, unlink, and rename. r=gcp https://hg.mozilla.org/integration/autoland/rev/6cef83dd4d11 Remove the file broker protocol support for two-path operations. r=gcp

Backed out for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=123610626&repo=autoland https://hg.mozilla.org/integration/autoland/rev/bb2e59766bada3c9528e48c70b60ee0681c3db1e

Pushed by jedavis@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0d56979a6efa Remove brokering for link, unlink, and rename. r=gcp https://hg.mozilla.org/integration/autoland/rev/9fb892c41a9e Remove the file broker protocol support for two-path operations. r=gcp

https://hg.mozilla.org/mozilla-central/rev/0d56979a6efa https://hg.mozilla.org/mozilla-central/rev/9fb892c41a9e

Backed out for several reasons, as seen on crash-stats: 1. fontconfig is calling link(), as part of a lock file when updating (creating?) a cache. It shouldn't be doing that, but we need to convince it not to. 2. Mesa drivers are using rename(), probably for shader caches. 3. PulseAudio is using symlink() for some reason; I don't yet understand why, or why it didn't do this in testing, but PulseAudio is going away soon anyway.

Backout by jedavis@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4ffacd080dc6 Backed out 3 changesets (bug 1380701, bug 1384804)

Merge of backout: https://hg.mozilla.org/mozilla-central/rev/4ffacd080dc6