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

RESOLVED FIXED

Status

Firefox OS
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: vicamo, Assigned: vicamo)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 795865 [details]
compiler-errors.txt
(Assignee)

Comment 1

4 years ago
Created attachment 795871 [details]
Sandbox.e
(Assignee)

Comment 2

4 years ago
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
(Assignee)

Updated

4 years ago
Assignee: nobody → vyang
(Assignee)

Comment 3

4 years ago
Created attachment 795886 [details] [diff] [review]
patch
Attachment #795886 - Flags: review?(gdestuynder)
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).
Attachment #795886 - Flags: review?(gdestuynder) → review-
(Assignee)

Comment 5

4 years ago
Created attachment 800663 [details] [diff] [review]
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?
Attachment #795886 - Attachment is obsolete: true
Attachment #800663 - Flags: feedback?(gdestuynder)
Comment on attachment 800663 [details] [diff] [review]
patch : v2

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

Yes - that's probably the best way
Attachment #800663 - Flags: feedback?(gdestuynder) → feedback+
(Assignee)

Updated

4 years ago
Attachment #800663 - Flags: review?(gdestuynder)
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
(Assignee)

Comment 8

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

Going to Oslo.  Will update the patch and re-request for your review.
Attachment #800663 - Flags: review?(gdestuynder)
(Assignee)

Comment 9

4 years ago
Created attachment 801573 [details] [diff] [review]
patch : v3

Address comment 7.
Attachment #800663 - Attachment is obsolete: true
Attachment #801573 - Flags: review?(gdestuynder)
(Assignee)

Comment 10

4 years ago
build on all platforms: https://tbpl.mozilla.org/?tree=Try&rev=c83a655a8d90
Attachment #801573 - Flags: review?(gdestuynder) → review+
(Assignee)

Comment 11

4 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/566aa5309ecc
https://hg.mozilla.org/mozilla-central/rev/566aa5309ecc
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.