1409900 - Stop allowing statfs and quotactl in content processes

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect, P2)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

…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.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Bug 1409900 - Disallow quotactl in sandboxed content processes.

Review commit: https://reviewboard.mozilla.org/r/193190/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/193190/

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: Bug 1409900 - Handle sandboxed statfs() by replacing it with open+fstatfs.

Review commit: https://reviewboard.mozilla.org/r/193192/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/193192/

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8922167 [details]
Bug 1409900 - Disallow quotactl in sandboxed content processes.

https://reviewboard.mozilla.org/r/193190/#review198692

Comment 6

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8922168 [details]
Bug 1409900 - Handle sandboxed statfs() by replacing it with open+fstatfs.

https://reviewboard.mozilla.org/r/193192/#review198694

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/072007f83431
https://hg.mozilla.org/mozilla-central/rev/83296a355dd4

Comment 9

[Attila Craciun [:arny]]

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

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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);

Comment 11

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(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 12

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8922167 [details]
Bug 1409900 - Disallow quotactl in sandboxed content processes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193190/diff/1-2/

Comment 13

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 8922168 [details]
Bug 1409900 - Handle sandboxed statfs() by replacing it with open+fstatfs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/193192/diff/1-2/

Comment 14

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

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.

Comment 15

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 8922168 [details]
Bug 1409900 - Handle sandboxed statfs() by replacing it with open+fstatfs.

https://reviewboard.mozilla.org/r/193192/#review199634

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9f9c7289c55b
https://hg.mozilla.org/mozilla-central/rev/862de4b75640