Closed Bug 1286324 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Security: Process Sandboxing, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: tedd, Assigned: jld)

References

Details

(Whiteboard: sblc1)

Crash Data

Attachments

(1 file)

According to the man page [1], sys_sysfs is obsolete and systems with /proc should use /proc/filesystems instead.

[1] http://man7.org/linux/man-pages/man2/sysfs.2.html
0x78 is 120 which is (32-bit) clone; I'm not seeing anything for 0x87 which would be sysfs.
%ebx is the first argument, which is the clone flags, which in all three is 0x01200011 == SIGCHLD | CLONE_CHILD_CLEARTID | CLONE_CHILD_SETTID.  So it's a fork().
There are a bunch of 64-bit clone crashes too: https://crash-stats.mozilla.com/search/?product=Firefox&reason=~SIGSYS&address=%3D0x38&_sort=-build_id&_sort=-date&_facets=cpu_arch&_facets=address&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#crash-reports

Looks like there isn't already a bug for these.  Also, this is now the same summary as bug 1259508 but not the same underlying cause.

If we can't find the underying cause for this one reasonably quickly, we could try making fork() fail with EACCES or EPERM or something.
Summary: Seccomp sandbox violation: sys_sysfs called in content process of Firefox desktop → Seccomp sandbox violation: sys_clone called in content process of Firefox desktop
Thanks :jld, confused something somewhere.

Yes, I haven't filed bugs for the sys_clone violations yet, there are quite a lot of reports. I requested access to the minidumps so that I can start investigating.
I'm looking at https://crash-stats.mozilla.com/report/index/993f6b19-4ae9-49ca-b4b7-5a3572160712 (which is from Ubuntu Trusty, so I can just `apt-get download` packages on the VM I already have).  I found debug symbols for glib, and the frames breakpad reports at the top look plausible:

0x000000000008c54b
fork_exec_with_pipes
/build/buildd/glib2.0-2.40.2/./glib/gspawn.c:1326
0x000000000008ce17
g_spawn_sync
/build/buildd/glib2.0-2.40.2/./glib/gspawn.c:277 (discriminator 3)
0x000000000006361a
g_shell_parse_argv
/build/buildd/glib2.0-2.40.2/./glib/gshell.c:681
0x000000000008d516
g_spawn_command_line_sync
/build/buildd/glib2.0-2.40.2/./glib/gspawn.c:721

And then it goes into liboxygen-gtk.so.  I don't have symbols there, but I do have a disassembly; here are the first few reported addresses that are actually in .text (note that they're reported as one byte before the end of the call instruction, which is the convention for querying debug info for the state during a call):

000000000009a730 <_ZNK6Oxygen10QtSettings10runCommandERKSsRPc>:
...
   9a744:       e8 27 6f fb ff          callq  51670 <g_spawn_command_line_sync@plt>

000000000009cff0 <_ZNK6Oxygen10QtSettings17kdeConfigPathListEv>:
...
   9d04b:       e8 e0 d6 ff ff          callq  9a730 <_ZNK6Oxygen10QtSettings10runCommandERKSsRPc>

00000000000a9ff0 <_ZN6Oxygen10QtSettings10initializeEj>:
...
   aa0f2:       e8 f9 2e ff ff          callq  9cff0 <_ZNK6Oxygen10QtSettings17kdeConfigPathListEv>

Also looks plausible.  A little poking around in Oxygen::QtSettings::kdeConfigPathList() shows it passing a std::string it constructed from a string literal stored at 0x10908a (which shows up in the crash report, offset by -1, because breakpad saw it on the stack but didn't know that it's .rodata and not .text).  That string is:

00109080                                 6b 64 65 34 2d 63  |          kde4-c|
00109090  6f 6e 66 69 67 20 2d 2d  70 61 74 68 20 63 6f 6e  |onfig --path con|
001090a0  66 69 67 00                                       |fig             |
Thanks :jld for looking into this.

Looking at the oxygen-gtk source [1], it shows that the failure of runCommand() isn't fatal and it has fallback code. So returning an error for fork() instead of crash would let the content process keep running.

[1] https://lxr.kde.org/source/playground/artwork/oxygen-gtk/src/oxygenqtsettings.cpp#0358
Whiteboard: sblc1
Assignee: nobody → jld
Status: NEW → ASSIGNED
Duplicate of this bug: 1285637
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=399d1d678c2e

Tested with GTK3 Oxygen theme and SCIM (two fork() users I saw in crash reports), which both seem to work.  Also verified with gdb that GMP child processes still crash on fork() as expected.

A related thing from the sandboxing meeting this morning: while we initialize the widget toolkit in the child process, native widget rendering (e.g., form fields) happens in the parent process.  Hopefully this means there's more room for widget-related things not working quite right in the content process due to sandboxing, but I won't claim to know the graphics stack well enough to say anything definitive.

A possible followup: try to gather telemetry on how much we reject clone() calls like this.
Attachment #8771141 - Flags: review?(julian.r.hector)
Crash Signature: [@ libc-2.19.so@0xb6e74 ] [@ libc-2.20.so@0xbd306 ] [@ libc-2.19.so@0xba014 ] [@ libc-2.19.so@0xc0ee4 ] [@ libc-2.22.so@0xb8a96 ]
Crash Signature: [@ libc-2.19.so@0xb6e74 ] [@ libc-2.20.so@0xbd306 ] [@ libc-2.19.so@0xba014 ] [@ libc-2.19.so@0xc0ee4 ] [@ libc-2.22.so@0xb8a96 ] → [@ libc-2.19.so@0xb6e74 ] [@ libc-2.20.so@0xbd306 ] [@ libc-2.19.so@0xba014 ] [@ libc-2.19.so@0xc0ee4 ] [@ libc-2.22.so@0xb8a96 ] [@ libpthread-2.19.so@0x10e93 ] [@ libpthread-2.19.so@0x10ed3 ]
Crash Signature: [@ libc-2.19.so@0xb6e74 ] [@ libc-2.20.so@0xbd306 ] [@ libc-2.19.so@0xba014 ] [@ libc-2.19.so@0xc0ee4 ] [@ libc-2.22.so@0xb8a96 ] [@ libpthread-2.19.so@0x10e93 ] [@ libpthread-2.19.so@0x10ed3 ] → [@ libc-2.19.so@0xb6e74 ] [@ libc-2.20.so@0xbd306 ] [@ libc-2.19.so@0xba014 ] [@ libc-2.19.so@0xc0ee4 ] [@ libc-2.22.so@0xb8a96 ] [@ libpthread-2.19.so@0x10e93 ] [@ libpthread-2.19.so@0x10ed3 ] [@ libc-2.23.so@0xb6f9a ]
Comment on attachment 8771141 [details] [diff] [review]
Patch: change forbidden clones to fail with EPERM

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

LGTM. From what I saw, some of the code has fallback code in case fork() fails with an error. But getting telemetry would be nice.
Attachment #8771141 - Flags: review?(julian.r.hector) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b3f1cc706bd
Make fork() non-fatal in Linux content sandbox. r=jhector
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7b3f1cc706bd
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1287437
You need to log in before you can comment on or make changes to this bug.