919090 - seccomp-bpf vs. profiling

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

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

I've found two more un-whitelisted syscalls while trying to use profiling features on my keon:

1. Starting the profiler requires sigaction, to establish the signal handler.  This isn't a problem for starting the profiler with MOZ_PROFILER_STARTUP, because that runs before the seccomp filter is established, but blocking it prevents starting the profiler asynchronously later on.

2. tgkill is used to send signals to a specific thread when the "threads" feature is enabled.

It'd be a trivial change to add those syscalls, but we could do better:

1. We can't inspect the struct sigaction passed by pointer, but we can restrict sigaction() to specific signals — i.e., we can allow changing the handling of SIGPROF only (but not, e.g., SIGSEGV or SIGSYS).

2. tgkill is always called with tgid equal to the current process ID (which is constant over the lifetime of the filter program), and used only to send SIGPROF.  Other uses can be disallowed.

We may want to get the simple fix in now, but this is also an opportunity to start being more nuanced.

Comment 1

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

Attachment: bug919090-profiler-seccomp-quick-fix.diff

This is the quick fix.

Comment 2

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

We should get the simple fix in now, I think.  I broke off bug 920372 to tighten the filter.

However… do we want to make this change for all builds (as in attachment 808083 [details] [diff] [review]), or just MOZ_PROFILING builds?  The answer might sound obvious, but users will try to profile on non-profiling builds.

Comment 3

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

Comment on attachment 808083 [details] [diff] [review]
bug919090-profiler-seccomp-quick-fix.diff

Proposal: allow profiling always, even on non-profiling builds.

Comment 4

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

Attachment: bug919090-profiler-seccomp-conditional.diff

Alternate proposal: allow threaded profiling and async profiler start only on MOZ_PROFILING builds.  How do we feel about this limitation?

Comment 5

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

Comment on attachment 810740 [details] [diff] [review]
bug919090-profiler-seccomp-conditional.diff

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

On the basis that tgkill can be used like kill(), we shouldn't allow this by default. In particular, there are plans to use the sandbox as a strong separation between processes instead of the uid sandbox (which prevents from using kill on another process right now), then unify the code so that it can be used on desktop or b2g alike (ie, same whitelist for both desktop and b2g, and possibly no setuid for child processes so that no root parent is required).

Thus, I would be more favorable to have tgkill only enabled when specifically requested for profiling.

Comment 6

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

Comment on attachment 808083 [details] [diff] [review]
bug919090-profiler-seccomp-quick-fix.diff

Clearing review request for this version - the patch is correct but as per previous comment, if it is really necessary to have the profiling syscalls always enabled I would want to discuss it more first.

Comment 7

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

Comment on attachment 808083 [details] [diff] [review]
bug919090-profiler-seccomp-quick-fix.diff

I think this approach is effectively r- at this point.

Comment 8

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

Comment on attachment 810740 [details] [diff] [review]
bug919090-profiler-seccomp-conditional.diff

I asked :BenWa about this on IRC yesterday.  Summarizing: given that profiling does work (possibly without native stack traces) on non-`--enable-profiling` builds, and given that profiling part of the devtools, there is resistance to making profiling require MOZ_PROFILING to work at all.

Breaking profiling across the board has been described as a "regression", although my understanding is that the state of profiler testing is such that this wouldn't have broken tbpl if tbpl were using seccomp.

So there's that.  I'm wondering if making all "eng" builds --enable-profiling is a thing that would help.  Input should probably be solicited from dev-b2g in any case.

Comment 9

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

I think it's reasonable to check in attachment 810740 [details] [diff] [review], since it's at least an improvement over the current situation in terms of functionality, and it has r+ from the security side.

Comment 10

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

I'm also wondering if its possible to change profiling to not do those calls - or else to have a runtime flag such as --sandbox-debug which uses a relaxed whitelist or even no whitelist at all (chrome does the later IIRC)

Comment 11

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

sigaction would be pretty easy — always install the signal handler on startup, even if profiling will never be used, and do the rest of profiler initialization (or not) normally.

tgkill is harder, because we need to periodically stop a specific thread and read its state, which includes registers and the stack and the pseudostack, while it's stopped.  Signals are pretty much the only way to do that as far as I know (short of ptrace, but I'm pretty sure we don't want to allow that in any form).

I guess one possibility would be to have the main process send the signals, but that does "interesting" things as far as the current mechanism for not sending a signal until the previous one is done.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/59fe91efb226

Had to be rebased on top of bug 914716. Please make sure it turned out OK :)

Comment 13

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #12)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/59fe91efb226
> 
> Had to be rebased on top of bug 914716. Please make sure it turned out OK :)

looks good!

(In reply to Jed Davis [:jld] from comment #11)
> sigaction would be pretty easy — always install the signal handler on
> startup, even if profiling will never be used, and do the rest of profiler
> initialization (or not) normally.
yeah that's be fine

> 
> tgkill is harder, because we need to periodically stop a specific thread and
> read its state, which includes registers and the stack and the pseudostack,
> while it's stopped.  Signals are pretty much the only way to do that as far
> as I know (short of ptrace, but I'm pretty sure we don't want to allow that
> in any form).
Correct ;-)

> 
> I guess one possibility would be to have the main process send the signals,
> but that does "interesting" things as far as the current mechanism for not
> sending a signal until the previous one is done.

Chromium also seems to use the flag way as proposed by comment 10:
http://www.chromium.org/developers/deep-memory-profiler (--no-sandbox)

would that be too inconvenient in our use cases?

Comment 14

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

(In reply to Guillaume Destuynder [:kang] from comment #13)
> Chromium also seems to use the flag way as proposed by comment 10:
> http://www.chromium.org/developers/deep-memory-profiler (--no-sandbox)
> 
> would that be too inconvenient in our use cases?

I found this in tools/profiler/platform.cpp:

  // The only way to profile secondary threads on b2g
  // is to build with profiling OR have the profiler
  // running on startup.

If I understand correctly: in the case that MOZ_PROFILING isn't defined at compile time and MOZ_PROFILER_STARTUP isn't set in the environment, thread profiling won't work anyway, so blocking tgkill won't affect functionality.

Comment 15

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

Comment on attachment 810740 [details] [diff] [review]
bug919090-profiler-seccomp-conditional.diff

seccomp isn't a Feature for B2G 1.2, but it's enabled on the geeksphone devices (at least for developer builds), which resulted in a regression in profiler functionality there.  So we should fix that.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 790923
User impact if declined: Profiler fails to work in some cases on some devices.
Testing completed (on m-c, etc.): Used asynchronously started non-main-thread profiling (uses both affected syscalls) on m-c and m-a.
Risk to taking this patch (and alternatives if risky): No impact if seccomp isn't enabled on the device in question, and as far as I know it won't be on non-Geeksphone devices.  No impact if seccomp isn't enabled in the Gecko build, as is the case on non-B2G platforms.  No impact if profiling isn't enabled, as on production builds.
String or IDL/UUID changes made by this patch: None.

The patch as posted should be apply cleanly to m-a; the rebased version committed to inbound would need to be re-rebased.

Comment 16

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/59fe91efb226

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1767b159729a

Comment 18

[Benoit Girard (:BenWa)]

(In reply to Jed Davis [:jld] from comment #14)
> If I understand correctly: in the case that MOZ_PROFILING isn't defined at
> compile time and MOZ_PROFILER_STARTUP isn't set in the environment, thread
> profiling won't work anyway, so blocking tgkill won't affect functionality.

I've been removing this locally and hardcoding it.

Can you document the implications this has to profiling here:
https://developer.mozilla.org/en-US/docs/Performance/Profiling_with_the_Built-in_Profiler