Closed Bug 1409900 Opened 6 years ago Closed 6 years ago
Stop allowing statfs and quotactl in content processes
59 bytes, text/x-review-board-request
59 bytes, text/x-review-board-request
We're allowing statfs() and quotactl() in our sandbox policy; they both take filesystem paths, so they'll break when chrooted. nsLocalFile::GetDiskSpaceAvailable calls both of them, but it's not clear why we'd be calling that in a content process in normal usage (as opposed to unit tests that were moved into content without considering whether that made sense). There may also be other callers; I haven't investigated yet. statfs could be polyfilled on top of open and fstatfs (which we seem to need for fontconfig; bug 1285293), but there doesn't seem to be an equivalent for quotactl. We could fail quotactl with ENOSYS (not supported by the kernel) or ESRCH (not supported by the filesystem or not enabled for this user), but that risks someone's quota getting filled because something thinks it can use the entire filesystem size. So we need to understand what's going on here.
Once again, PulseAudio: Sandbox: frame #01: __statfs[/lib/x86_64-linux-gnu/libc.so.6 +0xf6db7] Sandbox: frame #05: shm_open[/lib/x86_64-linux-gnu/librt.so.1 +0x4483] Sandbox: frame #08: pa_shm_create_rw[/usr/lib/x86_64-linux-gnu/pulseaudio/libpulsecommon-8.0.so +0x42263] Sandbox: frame #12: webrtc::AudioDeviceLinuxPulse::Init() (/home/jld/src/gecko-dev/media/webrtc/trunk/webrtc/modules/audio_device/linux/audio_device_pulse_linux.cc:172) Note that ipc::Shmem uses /dev/shm directly but doesn't call shm_open from libc. (glibc, for reference, uses /dev/shm if it's a tmpfs; otherwise, it scans /proc/mounts for the first tmpfs it finds (!) and uses that.) But if other system libraries use shm_open then we might still need to do the open/fstatfs workaround.
…and also, once again, GIO via the helper app service. It wants to know if a file is on NFS to decide how to monitor it, which we won't allow anyway (bug 1408497), but it doesn't know that. It considers an error to mean "not remote", so that's one option — and it would prevent it from trying to use an NFS monitor on actual NFS, which might be good given that the only implementation for NFS monitoring seems to involve famd(8) and thus networking. But that's a pretty indirect side-effect to be relying on, and it's also exactly the wrong thing to do in the shm_open case. But see also bug 1408497 comment #5; we may need a more general answer to this file monitor problem anyway.
Assignee: nobody → jld
Priority: P3 → P2
Comment on attachment 8922167 [details] Bug 1409900 - Disallow quotactl in sandboxed content processes. https://reviewboard.mozilla.org/r/193190/#review198692
Attachment #8922167 - Flags: review?(gpascutto) → review+
Comment on attachment 8922168 [details] Bug 1409900 - Handle sandboxed statfs() by replacing it with open+fstatfs. https://reviewboard.mozilla.org/r/193192/#review198694
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/072007f83431 Disallow quotactl in sandboxed content processes. r=gcp https://hg.mozilla.org/integration/autoland/rev/83296a355dd4 Handle sandboxed statfs() by replacing it with open+fstatfs. r=gcp
Backed out 2 changesets (bug 1409900) for failing browser chrome on Linux opt at browser/base/content/test/general/browser_bug590206.js r=backout a=backout. Failure: https://treeherder.mozilla.org/logviewer.html#?job_id=140178158&repo=autoland&lineNumber=3030 https://hg.mozilla.org/mozilla-central/rev/ae49d4a5762264ded3aae4006baddc2203b79b94
This may be a use-after-free bug in glib, but also I got the statfs64 case wrong. The actual system calls also take the size of the struct, approximately like this: long statfs64(const char *pathname, size_t sz, struct statfs64 *buf); long fstatfs64(unsigned fd, size_t sz, struct statfs64 *buf);
(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #10) > This may be a use-after-free bug in glib On second thought, probably not. The missing argument (i.e., the one whose value is whatever happens to be in the stack cell where the argument should have been) is a pointer that's written to, so this could easily cause arbitrary heap corruption. That might also explain why it didn't show up when I ran Try.
Comment on attachment 8922168 [details] Bug 1409900 - Handle sandboxed statfs() by replacing it with open+fstatfs. Testing done: downloaded the 32-bit build from Try, ran it locally, and used gdb to inject a statfs64() call into a content process. On several different automation builds with the bad patch, including ones that didn't fail normal testing, this reliably left garbage in the statfs buffer and crashed the process immediately after continuing; with this patch, it worked.
Attachment #8922168 - Flags: review+ → review?(gpascutto)
Comment on attachment 8922168 [details] Bug 1409900 - Handle sandboxed statfs() by replacing it with open+fstatfs. https://reviewboard.mozilla.org/r/193192/#review199634
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/9f9c7289c55b Disallow quotactl in sandboxed content processes. r=gcp https://hg.mozilla.org/integration/autoland/rev/862de4b75640 Handle sandboxed statfs() by replacing it with open+fstatfs. r=gcp
You need to log in before you can comment on or make changes to this bug.