Open Bug 1380701 Opened 7 years ago Updated 2 years ago

Disallow link() and symlink() and rename() inside the content process.

Categories

(Core :: Security: Process Sandboxing, enhancement, P3)

Product:
Core
Component:
Security: Process Sandboxing
Platform:
All
Linux
Type:
enhancement

Tracking

()

Status:
REOPENED

People

(Reporter: gcp, Assigned: jld)

References

Details

(Whiteboard: sb+)

Attachments

(2 files)

Bug 1380701 - Remove brokering for link, unlink, and rename.
59 bytes, text/x-review-board-request
gcp
: review+
Details
Bug 1380701 - Remove the file broker protocol support for two-path operations.
59 bytes, text/x-review-board-request
gcp
: review+
Details 
We can probably get away with blocking this, and it solves some issues when passing files to the parent via the temporary dir.
Flags: needinfo?(jld)
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().
Assignee: nobody → jld
Flags: needinfo?(jld)
Summary: Disallow link() inside the content process. → Disallow link() and symlink() inside the content process.
Apparently we also don't need rename() anymore?  In which case I think we can also GC the stuff for requests with two paths.
Priority: -- → P1
Summary: Disallow link() and symlink() inside the content process. → Disallow link() and symlink() and rename() inside the content process.
Blocks: 1308400
Whiteboard: sb+
This is an improvement, not a regression.
No longer blocks: 1308400
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
Attachment #8896044 - Flags: review?(gpascutto) → review+
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.
Attachment #8896045 - Flags: review?(gpascutto) → review+
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.
Attachment #8896045 - Flags: review+ → review?(gpascutto)
Comment on attachment 8896045 [details]
Bug 1380701 - Remove the file broker protocol support for two-path operations.

https://reviewboard.mozilla.org/r/160512/#review174468
Attachment #8896045 - Flags: review?(gpascutto) → review+
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
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
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Flags: needinfo?(jld)
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
status-firefox57: fixed → ---
Target Milestone: mozilla57 → ---
Keywords: stale-bug
Priority: P1 → P3
See Also: → 1511099
See Also: → 1638466
See Also: → 1738146
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: