1274873 - glib "gmain" thread may block the signal used for BroadcastSetThreadSandbox()

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Julian Hector [:tedd] [:jhector]]

To enable seccomp for all threads on systems that do not support TSYNC, we currently have a workaround, where we install a signal handler for an unused signal and then signal each thread of the process which in return triggers the signal handler which then enables seccomp.

Since glib 'gmain' thread blocks all signals (Bug 1176099), we can't install the signal handler which causes the process to crash [1], because the 'gmain' thread is non responsive.

Similar to Bug 1176099, we need to unmask the signal being used for the broadcast, but the problem here is that the signal number is not fixed and determined at run-time [2]

[1] https://dxr.mozilla.org/mozilla-central/rev/16663eb3dcfa759f25b5e27b101bc79270c156f2/security/sandbox/linux/Sandbox.cpp#429
[2] https://dxr.mozilla.org/mozilla-central/rev/16663eb3dcfa759f25b5e27b101bc79270c156f2/security/sandbox/linux/Sandbox.cpp#331

Comment 1

[Julian Hector [:tedd] [:jhector]]

I did some testing regarding this problem using the approach mentioned in Bug 1176099 Comment 1. In theory it should work to register the signal handler in SandboxEarlyInit(), but during my testing it seemed that the handler didn't run.

It turned out that the first signal to be found free was SIGRTMIN+2, after the signal handler was registered, it was overwritten later on [1]. The code in nsMemoryInfoDumper overwrites any previous signal handler without checking if the signal is used or not.

For test purposes, I modified the FindFreeSignalNumber() [2] to search from SIGRTMAX to SIGRTMIN which solved the problem.

[1] https://dxr.mozilla.org/mozilla-central/rev/16663eb3dcfa759f25b5e27b101bc79270c156f2/xpcom/base/nsMemoryInfoDumper.cpp#279
[2] https://dxr.mozilla.org/mozilla-central/rev/16663eb3dcfa759f25b5e27b101bc79270c156f2/security/sandbox/linux/Sandbox.cpp#254

Comment 2

[Julian Hector [:tedd] [:jhector]]

Attachment: WIP Register signal handler for enabling seccomp in SandboxEarlyInit

Comment 3

[Julian Hector [:tedd] [:jhector]]

Attachment: PoC change signal checking order (DO NOT COMMIT)

With this patch applied, the WIP patch works. I am not sure how we want to handle this problem, so it is just a PoC

Comment 4

[Julian Hector [:tedd] [:jhector]]

:jld, how do you think we should handle the nsMemoryInfoDumper problem? Maybe fix its code to properly check for free signals?

Comment 5

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

Some history: The original version (from bug 970676) used to just hardcode SIGRTMIN + 3, to get the next signal after the nsMemoryInfoDumper signals.  But then there was bug 1038900, when something on B2G devices was doing that kind of dynamic allocation and also got SIGRTMIN + 3, which is why it works the way it does now.

The thing about the nsMemoryInfoDumper signals is that they're used from outside the process — definitely on B2G, and I've also seen it suggested (using kill -NN with whatever the actual signal number is) as a way to get info out of Linux desktop Firefox — so they can't be dynamic.

All that said: I can't think of anything that would break if having the sandbox code searches from SIGRTMAX decreasing.

Comment 6

[Julian Hector [:tedd] [:jhector]]

Attachment: Part 1: Change search order for free signal r=jld

Comment 7

[Julian Hector [:tedd] [:jhector]]

Attachment: Part 2: Move signal handler set up to SandboxEarlyInit() r=jld

Comment 8

[Julian Hector [:tedd] [:jhector]]

Given Comment 5, I added a patch that changes the search order.

Try push for build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa622e0b7293

I tested it locally with MOZ_FAKE_NO_SECCOMP_TSYNC=1. My latest try push with tests, include these two patches as well (additionally a patch to enable seccomp with triggers this code):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=855e7084f80d

Test failures are not related to this patch, rather to missing entries in the seccomp whitelist which is another story.

Comment 9

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

Comment on attachment 8756862 [details] [diff] [review]
Part 2: Move signal handler set up to SandboxEarlyInit() r=jld

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

Looks good, but a few suggestions:

::: security/sandbox/linux/Sandbox.cpp
@@ +64,5 @@
>  __sanitizer_sandbox_on_notify(__sanitizer_sandbox_arguments *args);
>  } // extern "C"
>  #endif // MOZ_ASAN
>  
> +extern int gBroadcastSignum;

A more specific name would be nice, given that this has extern linkage and is in the root namespace.

Also, if you can move the definition here and the extern declaration into SandboxHooks, that will avoid breaking the B2G build.

@@ +349,5 @@
>          continue;
>        }
>        // Reset the futex cell and signal.
>        gSetSandboxDone = 0;
> +      if (syscall(__NR_tgkill, pid, tid, gBroadcastSignum) != 0) {

Is there any way this could be reached with gBroadcastSignum == 0?  That might be worth a MOZ_RELEASE_ASSERT.  (The result would be a crash anyway, since the signal handler will never run, but a specific assertion would be easier to debug.)

@@ +421,5 @@
>      rewinddir(taskdp);
>    } while (sandboxProgress);
> +
> +  void (*oldHandler)(int);
> +  oldHandler = signal(gBroadcastSignum, SIG_DFL);

Maybe gBroadcastSignum should be set back to 0 after this?

Comment 10

[Julian Hector [:tedd] [:jhector]]

Attachment: Part 2: Move signal handler set up to SandboxEarlyInit() r=jld

Carry over r+ from :jld.
Try push for build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8adbdc577057

Thanks jed, I incorporated your suggestions, and tested it locally with MOZ_FAKE_NO_SECCOMP_TSYNC=1.

Before I request checkin-needed, I renamed the signal number variable to:
gSeccompTsyncBroadcastSignum

Is that descriptive enough?

Comment 11

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

Looks good.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae5286493f15
Part 1: Change search order for free signal r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/62646bfa1f95
Part 2: Move signal handler set up to SandboxEarlyInit() r=jld

Comment 13

[Carsten Book [:Tomcat]]

sorry had to back this out, seems this caused https://treeherder.mozilla.org/logviewer.html#?job_id=29651943&repo=mozilla-inbound after landing

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b070f6f0ace2
Backed out changeset 62646bfa1f95 
https://hg.mozilla.org/integration/mozilla-inbound/rev/25cf270ae8c5
Backed out changeset ae5286493f15 for frequent timeouts in browser_ManifestObtainer_obtain.js

Comment 15

[Julian Hector [:tedd] [:jhector]]

Sorry to have the caused the trouble, I will dig into it. Thanks for backing it out again.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1526b47e25e4
Part 1: Change search order for free signal r=jld
https://hg.mozilla.org/integration/mozilla-inbound/rev/17c1f2315eb5
Part 2: Move signal handler set up to SandboxEarlyInit() r=jld

Comment 17

[Carsten Book [:Tomcat]]

(In reply to Julian Hector [:tedd] [:jhector] from comment #15)
> Sorry to have the caused the trouble, I will dig into it. Thanks for backing
> it out again.

Hi Julian,
very sorry turned out your patch is innocent and was something else that landed before. Relanded your patches!

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1526b47e25e4
https://hg.mozilla.org/mozilla-central/rev/17c1f2315eb5