Closed Bug 1285768 Opened 4 years ago Closed 3 years ago

Seccomp sandbox violation: sys_getppid called in content process of Firefox desktop

Categories

(Core :: Security: Process Sandboxing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: tedd, Assigned: tedd)

References

Details

(Whiteboard: sblc1)

Crash Data

Attachments

(1 file, 1 obsolete file)

Try push for build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d6dfb303806d
Assignee: nobody → julian.r.hector
Attachment #8769502 - Flags: review?(gpascutto)
Do we know what the underlying requester is? The stacks look a bit broken, it might be gfx (GL) but not sure.

getppid looks like the kind of function that we'd rather block.
Comment on attachment 8769502 [details] [diff] [review]
Add sys_getppid to seccomp whitelist. r=gcp

Review of attachment 8769502 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to hold off and discuss this one.
Attachment #8769502 - Flags: review?(gpascutto)
Attachment #8769502 - Flags: feedback?(jld)
Comment on attachment 8769502 [details] [diff] [review]
Add sys_getppid to seccomp whitelist. r=gcp

getppid seems pretty harmless.  There's probably a copy of the pid already in the address space for one reason or another (if not our IPC layer, then maybe in glibc's structures somewhere), and the implementation is relatively simple, so it wouldn't be a concern from an attack surface point of view.

Once we're using pid namespaces, getppid() will return 0 and that might cause some problems, but I don't think that needs to block this patch.
Attachment #8769502 - Flags: feedback?(jld) → feedback+
Comment on attachment 8769502 [details] [diff] [review]
Add sys_getppid to seccomp whitelist. r=gcp

I agree with :jld, I don't think this is harmful. So far we only received 3 crash reports where this was an issue. I would be fine with landing this, but also don't mind waiting and investigating.
Attachment #8769502 - Flags: review?(gpascutto)
Comment on attachment 8769502 [details] [diff] [review]
Add sys_getppid to seccomp whitelist. r=gcp

Review of attachment 8769502 [details] [diff] [review]:
-----------------------------------------------------------------

Alright then. My main concern is that it's trying to get the parent pid for "something", and that "something" is probably "something" we don't want to allow either.
Attachment #8769502 - Flags: review?(gpascutto) → review+
I took a look at fglrx: it seems to be using getppid() so it can scan the parent process's command line for the names of profiling tools.

So here's another idea: we could make getppid() return 0 instead of the actual ppid, to pre-break anything that will break in a pid namespace leader (bug 1151624), where getppid() really does return 0.
Whiteboard: sblc1
Crash Signature: [@ libc-2.19.so@0xbade7 ] [@ libc-2.15.so@0xbff77 ] [@ libc-2.15.so@0xbff37 ] [@ libc-2.19.so@0xc1d07 ]
Implements the behavior mentioned in Comment 7.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3196cc327244
Attachment #8769502 - Attachment is obsolete: true
Attachment #8772897 - Flags: review?(gpascutto)
Attachment #8772897 - Flags: review?(gpascutto) → review+
Try in Comment 8.

Please check in after Bug 1286185 (to avoid merge conflict)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/414ef1361cd2
Let getppid() return 0 to simulate pid namespaces. r=gcp
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/414ef1361cd2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.