907002 - seccomp sandbox needs to allow restart_syscall, to debug failures

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: seccomp-restart-syscall-hg0.diff

If I attach to a seccomp'ed b2g child process with gdb while it's in a syscall (e.g., if it was idle), then as soon as I `continue`, it receives SIGSYS for attempting system call 0, which is __NR_restart_syscall.  This makes it difficult (and confusing) to try to debug seccomp failures.

However, I don't know whether globally allowing syscall 0, as I've done in the attached patch, could be used to indirect to arbitrary syscalls and completely bypass seccomp.  That would be bad.

Comment 1

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

Comment on attachment 792573 [details] [diff] [review]
seccomp-restart-syscall-hg0.diff

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

Yes, it's ok to have restart_syscall. It's called internally (i.e. the kernel calls it).

::: security/sandbox/seccomp_filter.h
@@ +81,5 @@
>    ALLOW_SYSCALL(getpriority), \
>    ALLOW_SYSCALL(setpriority), \
>    ALLOW_SYSCALL(sigprocmask), \
> +  /* Seen when resuming from gdb; does this need more restriction? */ \
> +  ALLOW_SYSCALL(restart_syscall), \

I would put it here instead:

  ALLOW_SYSCALL(setpriority), \
  ALLOW_SYSCALL(sigprocmask), \
  /* Always last and always OK calls */ \
  SECCOMP_WHITELIST_ADD \
  /* restart_syscall is called internally, generally when debugging */ \
+  ALLOW_SYSCALL(restart_syscall), \
  ALLOW_SYSCALL(exit_group), \
  ALLOW_SYSCALL(exit)

This is because I believe this system call can stay - and because it shouln't be called often, it's at the bottom of the allowed list, but not in the "should remove" list.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: seccomp-restart-syscall-hg1.diff

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comment on attachment 793758 [details] [diff] [review]
seccomp-restart-syscall-hg1.diff

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

Apparently I'm doing bug flags wrong, or at least suboptimally.  Carrying over r+ from attachment 792573 [details] [diff] [review] (r=kang).  Also this is pretty much comment #1 reformatted as a patch.

Comment 4

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

sorry - I'm away this week hence the delay. i was going to do it regardless, but thinking about it, checkin-needed is probably better indeed :) if nothing happened when im back around or if this becomes a pain let me know i'll *try* to get it landed (ive intermittent internets)

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13ecac047855

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/13ecac047855