909658 - B2G Emulator-x86: build failure due to __NR_recv, __NR_msgget, __NR_semget undeclared when compiling seccomp-bfp sandboxing

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

Attachment: compiler-errors.txt

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

Attachment: Sandbox.e

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

From AOSP bionic documents[1]:

> Bionic intentionally does not provide support for System-V IPCs mechanisms,
> like the ones provided by semget(), shmget(), msgget(). The reason for this
> is to avoid denial-of-service.

However, it does include __NR_foo definitions on ARM and SH[2].

semget/msgget are only implemented internally with __NR_ipc in bug 859779.

__NR_recv was deprecated but still defined in both ARM and SH[2].  Never used in i386.

[1]: https://android.googlesource.com/platform/bionic/+/ics-mr1-release/libc/docs/OVERVIEW.TXT
[2]: https://android.googlesource.com/platform/bionic/+/ics-mr1-release/libc/kernel/arch-arm/asm/unistd.h , https://android.googlesource.com/platform/bionic/+/ics-mr1-release/libc/kernel/arch-sh/asm/unistd_64.h

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch

Comment 4

[Guillaume Destuynder [:kang] [this account is no longer active]]

Comment on attachment 795886 [details] [diff] [review]
patch

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

Since I believe this patch currently will build but not run properly with a seccomp enabled kernel on i386, I set review-. However, with the fix (or any similar fix that allows __NR_ipc) and perhaps a test run I believe this should be fine.

Thanks for catching this, btw (and sorry for the review delay, was in PTO)

::: security/sandbox/seccomp_filter.h
@@ +25,5 @@
> +/**
> + * Bug 909658: no __NR_recv, __NR_msgget, __NR_semget in emulator-x86.
> + *
> + * Bionic explicitly excludes some SYSV-ipc to avoid DoS.  These NRs
> + * are even not defined on i386 platform.

The systemcalls are defined (when missing, too) in gecko/security/sandbox/{arm,x86_32,x86_64}_linux_syscalls.h

x86_32 (i.e. i386) does not define those calls (as you pointed out), hence the issue.

I would rather just say - for clarity:
* Bug 909658: no __NR_recv, __NR_msgget, __NR_semget in emulator-x86.
*
* These system calls are not defined on i386.

@@ +47,5 @@
> +  ALLOW_SYSCALL(semget),
> +#else
> +#define SECCOMP_WHITELIST_ADD_semget
> +#endif
> +

I am wondering if it wouldn't be clearer to use a single define switch such as:
#if defined(__i386__)
#define SECCOMP_WHITELIST_ADD_i386 \
  ALLOW_SYSCALL(ipc),
#else
#define SECCOMP_WHITELIST_ADD_i386 \
  ALLOW_SYSCALL(recv),
  ALLOW_SYSCALL(msgget),
  ALLOW_SYSCALL(semget),
#endif

Also, note the ALLOW_SYSCALL(ipc) (this one is only defined for i386), which I believe is necessary for the content processes to work properly (after the build, try running apps otherwise, it probably gets killed by seccomp without it).

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch : v2

Don't have a seccomp enabled kernel on i386 either, request for feedback first.  Or, do you have a way to test it? Enable seccomp on B2G emulator-x86?

Comment 6

[Guillaume Destuynder [:kang] [this account is no longer active]]

Comment on attachment 800663 [details] [diff] [review]
patch : v2

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

Yes - that's probably the best way

Comment 7

[Guillaume Destuynder [:kang] [this account is no longer active]]

Comment on attachment 800663 [details] [diff] [review]
patch : v2

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

::: security/sandbox/seccomp_filter.h
@@ +36,5 @@
> +  ALLOW_SYSCALL(msgget), \
> +  ALLOW_SYSCALL(msgget),
> +#endif
> +
> +#define SECCOMP_WHITELIST_ADD \

while the overall logic is fine, the ipc calls have to be at the top of the list for performance (its the most used call, so when its at the top it shortcuts the checks more quickly, when a call is matched in the filter we exit the loop)
sorry, somehow didn't catch it this morning
it's fine to just have SECCOMP_WHITELIST_ADD_i386 at the very top of the whitelist

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 800663 [details] [diff] [review]
patch : v2

Going to Oslo.  Will update the patch and re-request for your review.

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch : v3

Address comment 7.

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

build on all platforms: https://tbpl.mozilla.org/?tree=Try&rev=c83a655a8d90

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/566aa5309ecc

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/566aa5309ecc