Closed Bug 1273852 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Security: Process Sandboxing, defect)

Product:
Core
Component:
Security: Process Sandboxing
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: gcp, Unassigned)

References

Details

(Whiteboard: sblc1)

Attachments

(3 files, 2 obsolete files)

Bug 1273852 - Always add seccomp-bpf socketcall dispatcher.
58 bytes, text/x-review-board-request
jld
: review+
Details
Bug 1273852 - Allow getsockopt in EvaluateSocketCall.
58 bytes, text/x-review-board-request
jld
: review+
Details
Bug 1273852 - Update chromium's list of linux-x86-32 syscalls.
58 bytes, text/x-review-board-request
jld
: review+
Details 
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.
Whiteboard: sblc1
Blocks: desktop-seccomp
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.
(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)
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)
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)
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/
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/
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/
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+
(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.
(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.
/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.
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/
Attachment #8761335 - Attachment is obsolete: true
Attachment #8761336 - Attachment is obsolete: true
Comment on attachment 8761661 [details]
Bug 1273852 - Allow getsockopt in EvaluateSocketCall.

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

Carry over
Attachment #8761661 - Flags: review+
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+
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+
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/
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/
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.

Attachment

General

Creator:
Created:
Updated:
Size: