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

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: gcp, Unassigned)

Tracking

Trunk
mozilla50
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox49 affected, firefox50 fixed)

Details

(Whiteboard: sblc1)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
When playing YouTube video, related to PulseAudio:

Sandbox: seccomp sandbox violation: pid 22699, syscall 55, args 28 1 4 139736612919728 139736612919732 1.  Killing process.
Sandbox: crash reporter is disabled (or failed); trying stack trace:
Sandbox: frame #01: getsockopt[/lib/x86_64-linux-gnu/libc.so.6 +0xe965a]
Sandbox: frame #02: ???[/usr/lib/x86_64-linux-gnu/pulseaudio/libpulsecommon-7.1.so +0x474a5]
Sandbox: frame #03: pa_mainloop_dispatch[/usr/lib/x86_64-linux-gnu/libpulse.so.0 +0x2524a]
Sandbox: frame #04: pa_mainloop_iterate[/usr/lib/x86_64-linux-gnu/libpulse.so.0 +0x2545c]
Sandbox: frame #05: pa_mainloop_run[/usr/lib/x86_64-linux-gnu/libpulse.so.0 +0x25500]
Sandbox: frame #06: ???[/usr/lib/x86_64-linux-gnu/libpulse.so.0 +0x338e6]
Sandbox: frame #07: ???[/usr/lib/x86_64-linux-gnu/pulseaudio/libpulsecommon-7.1.so +0x542f8]
Sandbox: frame #08: ???[/lib/x86_64-linux-gnu/libpthread.so.0 +0x80a4]
Sandbox: frame #09: clone[/lib/x86_64-linux-gnu/libc.so.6 +0xe887d]
Sandbox: frame #10: ??? (???:???)
Sandbox: end of stack.

Trunk PulseAudio seems to have refactored that call out of the pa_mainloop_dispatch function, I'd have to look what the PA in Debian stable is doing exactly.

But we can just whitelist this one for now.
(Reporter)

Updated

3 years ago
Whiteboard: sblc1
(Reporter)

Updated

3 years ago
Comment on attachment 8753810 [details]
Bug 1273852 - Always add seccomp-bpf socketcall dispatcher.

https://reviewboard.mozilla.org/r/53496/#review51012

::: security/sandbox/linux/SandboxFilter.cpp:623
(Diff revision 1)
>      case __NR_inotify_add_watch:
>      case __NR_inotify_rm_watch:
>        return Allow();
> +
> +    case __NR_getsockopt:
> +      return Allow();

Does this still work if it's limited to `getsockopt(_, SOL_SOCKET, SO_ERROR, ...)`?
Attachment #8753810 - Flags: review?(jld)
I did some testing on try and it turned out that '__NR_getsockopt' isn't defined on the 32-bit Linux build. So you may need to put an ifdef around it.
(In reply to Julian Hector [:tedd] [:jhector] from comment #3)
> I did some testing on try and it turned out that '__NR_getsockopt' isn't
> defined on the 32-bit Linux build. So you may need to put an ifdef around it.

Oh, right: it's a socketcall(2).  So this would need to be in EvaluateSocketCall, not EvaluateSyscall; see also the way socketpair() is handled.
Well it is only a socketcall(2) on systems that use it. Afaik, x86-64 doesn't use socketcall(2).

For systems that don't have socketcall(2), we have an ifdef in place that handles those cases [1], so technically it is not a socketcall on x86-64 but the dispatcher reroutes it into EvaluateSocketCall.

So adding a SYS_GETSOCKOPT to this [2] ifdef should handle both systems with and without socketcall.

[1] https://dxr.mozilla.org/mozilla-central/rev/16663eb3dcfa759f25b5e27b101bc79270c156f2/security/sandbox/linux/SandboxFilterUtil.cpp#85
[2] https://dxr.mozilla.org/mozilla-central/rev/16663eb3dcfa759f25b5e27b101bc79270c156f2/security/sandbox/linux/SandboxFilter.cpp#435
Just read the man pages, it states: "However, starting in Linux 4.3, direct system calls are provided on x86-32 for the sockets API"

I think this could cause some problems in the long run, because on 32-bit, __NR_socketcall will be defined, but with kernels 4.3 and above, __NR_getsockopt will be defined as well. But with our current code the dispatcher will only be in place when __NR_socketcall is not defined. Which means that on 32-bit there are 2 entry points for sys_getsockopt, but only one of them will be whitelisted.
While running some tests on try, I encountered some more seccomp violations regarding the socket API.

Could we maybe rename this bug and include SYS_BIND and SYS_LISTEN into the patch? Or should I rather file a separate bug for it. Unfortunately I don't have backtraces. But if absolutely necessary I could check the logs on the server again.
(Reporter)

Comment 8

3 years ago
(In reply to Julian Hector [:tedd] [:jhector] from comment #7)
> Could we maybe rename this bug and include SYS_BIND and SYS_LISTEN into the
> patch? Or should I rather file a separate bug for it. Unfortunately I don't
> have backtraces. But if absolutely necessary I could check the logs on the
> server again.

I'm think making separate bugs is handy because they help trace the cause of needing the exception, and the STR to test if its fixed. (There's also the spreadsheet, but I like Bugzilla for traceability)
(Reporter)

Comment 9

3 years ago
Comment on attachment 8753810 [details]
Bug 1273852 - Always add seccomp-bpf socketcall dispatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53496/diff/1-2/
Attachment #8753810 - Flags: review?(jld)
Comment on attachment 8753810 [details]
Bug 1273852 - Always add seccomp-bpf socketcall dispatcher.

https://reviewboard.mozilla.org/r/53496/#review54522

::: security/sandbox/linux/SandboxFilter.cpp:622
(Diff revision 2)
>      case __NR_inotify_init1:
>      case __NR_inotify_add_watch:
>      case __NR_inotify_rm_watch:
>        return Allow();
> +
> +    case __NR_getsockopt:

This still needs to move to EvaluateSocketCall.  Also, can you make this a separate patch/commit from the socketcall change?

::: security/sandbox/linux/SandboxFilterUtil.cpp:69
(Diff revision 2)
>  #endif // ANDROID
> -#else // __NR_socketcall
> -#define DISPATCH_SOCKETCALL(sysnum, socketnum)         \
> -    case sysnum:                                       \
> +#endif // __NR_socketcall
> +#define DISPATCH_SOCKETCALL(sysnum, socketnum)                       \
> +    case sysnum:                                                     \
>        return EvaluateSocketCall(socketnum).valueOr(InvalidSyscall())
> +#ifdef __NR_socket

This won't help without updating `security/sandbox/chromium/sandbox/linux/services/x86_32_linux_syscalls.h`, and upstream Chromium doesn't have the individual socket calls yet, so we'd need a local patch.  (Upstream Chromium also changes that file's path, so that won't be an entirely trivial update in any case.)

Dealing with that could be a followup.

::: security/sandbox/linux/SandboxFilterUtil.cpp:83
(Diff revision 2)
> +#endif // __NR_socket
>  #ifdef __NR_send
>        DISPATCH_SOCKETCALL(__NR_send,        SYS_SEND);
>        DISPATCH_SOCKETCALL(__NR_recv,        SYS_RECV);
>  #endif // __NR_send
> +#ifdef __NR_sendto

The send/recv block should just be nested inside the new `__NR_socket` ifdef.  It's there because new architectures omit send/recv (and only those) in favor of sendto/recvfrom.

::: security/sandbox/linux/SandboxFilterUtil.cpp:100
(Diff revision 2)
>  #undef DISPATCH_SOCKETCALL
>  #ifndef ANDROID
>  #define DISPATCH_SYSVCALL(sysnum, ipcnum)         \
>      case sysnum:                                  \
>        return EvaluateIpcCall(ipcnum).valueOr(InvalidSyscall())
>        DISPATCH_SYSVCALL(__NR_semop,       SEMOP);

These will still need to be ifdef'ed — the SysV IPC calls don't have demultiplexed versions on 32-bit x86 even in Linux `master`.
Attachment #8753810 - Flags: review?(jld)
(Reporter)

Comment 11

3 years ago
Review commit: https://reviewboard.mozilla.org/r/58572/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58572/
Attachment #8753810 - Attachment description: MozReview Request: Bug 1273852 - Add sys_getsockopt to whitelist for content process on Firefox desktop. r?jld → Bug 1273852 - Always add seccomp-bpf socketcall dispatcher.
Attachment #8761335 - Flags: review?(jld)
Attachment #8761336 - Flags: review?(jld)
Attachment #8753810 - Flags: review?(jld)
(Reporter)

Comment 13

3 years ago
Comment on attachment 8753810 [details]
Bug 1273852 - Always add seccomp-bpf socketcall dispatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53496/diff/2-3/
(Reporter)

Comment 14

3 years ago
Comment on attachment 8753810 [details]
Bug 1273852 - Always add seccomp-bpf socketcall dispatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53496/diff/3-4/
(Reporter)

Comment 15

3 years ago
Comment on attachment 8761335 [details]
Bug 1273852 - Allow getsockopt in EvaluateSocketCall.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58572/diff/1-2/
(Reporter)

Comment 16

3 years ago
Comment on attachment 8761336 [details]
Bug 1273852 - Update chromium's list of linux-x86-32 syscalls.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58574/diff/1-2/
Attachment #8761336 - Flags: review?(jld) → review+
Comment on attachment 8753810 [details]
Bug 1273852 - Always add seccomp-bpf socketcall dispatcher.

https://reviewboard.mozilla.org/r/53496/#review55572

Assuming that ReviewBoard understands the old "r+ with comment" convention:

::: security/sandbox/linux/SandboxFilterUtil.cpp:83
(Diff revision 4)
>  #ifdef __NR_send
>        DISPATCH_SOCKETCALL(__NR_send,        SYS_SEND);
>        DISPATCH_SOCKETCALL(__NR_recv,        SYS_RECV);
> -#endif // __NR_send
> +#endif
> +#endif // __NR_socket
> +#ifdef __NR_sendto

Does there need to be a separate `#ifdef __NR_sendto` here?  What I was thinking, last review, was that an `#ifdef __NR_socket` around the entire list and a nested `#ifdef __NR_send` around `send` and `recv` would be the simplest set of ifdefs that works.

…and suddenly it occurs to me: with the patch to the syscall list, isn't `__NR_socket` always defined now?
Attachment #8753810 - Flags: review?(jld) → review+
(Reporter)

Comment 20

3 years ago
(In reply to Jed Davis [:jld] [⏰PDT; UTC-7] from comment #19)
> Does there need to be a separate `#ifdef __NR_sendto` here?
...
> …and suddenly it occurs to me: with the patch to the syscall list, isn't
> `__NR_socket` always defined now?

It is. So is sendto, but not send or socketcall. I think the proposed patch works correctly even if some of those #ifdefs are no longer strictly necessary.
(Reporter)

Comment 21

3 years ago
(In reply to Jed Davis [:jld] [⏰PDT; UTC-7] from comment #2)
> Does this still work if it's limited to `getsockopt(_, SOL_SOCKET, SO_ERROR,
> ...)`?

I lost track of this. I'll add a follow-up patch for it that we can test.
(Reporter)

Comment 22

3 years ago
/builds/slave/try-lx-d-000000000000000000000/build/src/security/sandbox/linux/SandboxFilterUtil.cpp:74:12: error: '__NR_accept' was not declared in this scope

Ok, so 32-bit Linux has __NR_socket but not __NR_accept.
(Reporter)

Comment 24

3 years ago
Comment on attachment 8753810 [details]
Bug 1273852 - Always add seccomp-bpf socketcall dispatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53496/diff/4-5/
(Reporter)

Updated

3 years ago
Attachment #8761335 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Attachment #8761336 - Attachment is obsolete: true
(Reporter)

Comment 27

3 years ago
Comment on attachment 8761661 [details]
Bug 1273852 - Allow getsockopt in EvaluateSocketCall.

https://reviewboard.mozilla.org/r/58764/#review55690

Carry over
Attachment #8761661 - Flags: review+
(Reporter)

Comment 28

3 years ago
Comment on attachment 8761662 [details]
Bug 1273852 - Update chromium's list of linux-x86-32 syscalls.

https://reviewboard.mozilla.org/r/58766/#review55692

Carry over
Attachment #8761662 - Flags: review+
For reference, here's a table of architecture vs. syscalls:

      | socketcall | send/recv | accept | open
------+------------+-----------+--------+-----
x86_32|      X     |           |        |  X  
x86_64|            |           |    X   |  X  
 arm32|            |     X     |    X   |  X  
 arm64|            |           |    X   |
Comment on attachment 8753810 [details]
Bug 1273852 - Always add seccomp-bpf socketcall dispatcher.

ReviewBoard is confused, but it'd be good to fix the unnecessary #ifdef __NR_sendto (vs. continuing the #ifdef __NR_socket) before landing.
Attachment #8761661 - Flags: review?(jld) → review+
Attachment #8761662 - Flags: review?(jld) → review+
(Reporter)

Comment 31

3 years ago
Comment on attachment 8753810 [details]
Bug 1273852 - Always add seccomp-bpf socketcall dispatcher.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53496/diff/5-6/
Attachment #8761661 - Flags: review?(jld)
Attachment #8761661 - Flags: review+
Attachment #8761662 - Flags: review?(jld)
Attachment #8761662 - Flags: review+
(Reporter)

Comment 32

3 years ago
Comment on attachment 8761661 [details]
Bug 1273852 - Allow getsockopt in EvaluateSocketCall.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58764/diff/1-2/
(Reporter)

Comment 33

3 years ago
Comment on attachment 8761662 [details]
Bug 1273852 - Update chromium's list of linux-x86-32 syscalls.

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

Comment 35

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/289f630f3b0b
https://hg.mozilla.org/mozilla-central/rev/35575b3633f7
https://hg.mozilla.org/mozilla-central/rev/adb1d2a92e0d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Attachment #8761661 - Flags: review?(jld) → review+
Attachment #8761662 - Flags: review?(jld) → review+
You need to log in before you can comment on or make changes to this bug.