Closed Bug 1288726 Opened 3 years ago Closed 3 years ago

Seccomp sandbox doesn't play well with Valgrind

Categories

(Core :: Security: Process Sandboxing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: jseward, Unassigned)

References

Details

(Whiteboard: sblc1)

Attachments

(1 file, 2 obsolete files)

Running m-c with the seccomp sandbox enabled, on x86_64-linux, on
Valgrind, results in the process getting killed.  It may be that
Valgrind performs syscalls that are not allowed even though Firefox
itself doesn't perform any offending syscalls.  Setting the pref
security.sandbox.content.level = 0 avoids the problem.

Per https://groups.google.com/a/chromium.org/forum/#!topic/chromium-reviews/X8kXhdFrEbI valgrind and the seccomp sandbox don't get on.

This bug makes it impossible for me to run Mochitests on Valgrind
since the processes get nuked.

I propose to disable the sandbox when running on V.  I made a patch
analogous to the one shown in the URL just above, but adding a deliberate
syntax error to it doesn't break the build, so I guess I'm looking in
the wrong place.  Where's the right place?
What's the right way to do this?
We're not using Chromium's code for sandboxing feature checks; see http://searchfox.org/mozilla-central/rev/496904277ce0143bc1a952f2eb2c7e6a81aa3d4d/security/sandbox/linux/common/SandboxInfo.cpp#67

Another possibility would be to disable sandboxing completely at build time, like in bug 1182565 (and, soon, bug 1287971).
(In reply to Jed Davis [:jld] [⏰MDT; UTC-6] from comment #2)
> Another possibility would be to disable sandboxing completely at build time,
> like in bug 1182565 (and, soon, bug 1287971).

Ah, thanks for the pointer.  Given that that seems to be the preferred
approach for TSan and ASan/LSan, I'll do that too.
Attached patch bug1288393-2.cset (obsolete) — Splinter Review
Automatically disable the sandbox on Linux and Android if
--enable-valgrind is given at configure time.
Attachment #8773745 - Attachment is obsolete: true
Attachment #8774120 - Flags: review?(julian.r.hector)
Comment on attachment 8774120 [details] [diff] [review]
bug1288393-2.cset

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

lgtm
Attachment #8774120 - Flags: review?(julian.r.hector) → review+
On second thoughts, maybe it isn't so clever to do this at configure
time; maybe it would be better to do a runtime check instead.

The scenario I am concerned about is that if we ever do a production build
with --enable-valgrind then we will silently lose sandboxing, which will
be a disaster.  And --enable-valgrind is unlike building with ASan or MSan
in that it doesn't in itself cause a big and obvious performance hit.
So we might not notice.
Are we doing production builds with valgrind?
No, not as far as I know.  But the builds that are done for automation
Linux and Linux64 builds are --enable-valgrind, I believe, so we'd wind
up running tests with the sandbox disabled.
Oh ok, running tests without a sandbox would be pointless.

:jld, do you know a bug number where we patched this for b2g?
Flags: needinfo?(jld)
(In reply to Julian Seward [:jseward] from comment #8)
> No, not as far as I know.  But the builds that are done for automation
> Linux and Linux64 builds are --enable-valgrind, I believe, so we'd wind
> up running tests with the sandbox disabled.

Thanks; I hadn't been aware of that about the mozconfigs.  Ignore my previous comment, then.

(In reply to Julian Hector [:tedd] [:jhector] from comment #9)
> :jld, do you know a bug number where we patched this for b2g?

It looks like we didn't; according to https://developer.mozilla.org/en-US/docs/Archive/Firefox_OS/Debugging/Debugging_B2G_using_valgrind the workaround was just to add --disable-sandbox locally.
Flags: needinfo?(jld)
Revised patch per comments above.  Disables the sandbox only if
both configure-time and run-time checks indicate that should happen.
Attachment #8774120 - Attachment is obsolete: true
Attachment #8775532 - Flags: review?(julian.r.hector)
Comment on attachment 8775532 [details] [diff] [review]
bug1288726-3.cset

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

Just for clarification, RUNNING_ON_VALGRIND will be set by valgrind/valgrind.h and is set to 0 if running natively?

If that is the case, then everything looks good to me.
Attachment #8775532 - Flags: review?(julian.r.hector) → review+
(In reply to Julian Hector [:tedd] [:jhector] from comment #12)
> Comment on attachment 8775532 [details] [diff] [review]

Yes, RUNNING_ON_VALGRIND is a magic macro defined in valgrind/valgrind.h,
which produces zero when run natively and 1 or greater when run on Valgrind.
Whiteboard: sblc1
Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fcedf633473
Seccomp sandbox doesn't play well with Valgrind.  r=julian.r.hector.
https://hg.mozilla.org/mozilla-central/rev/3fcedf633473
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.