1573578 - Allow brk() in the common sandbox policy

Status: RESOLVED FIXED

(Core :: Security: Process Sandboxing, defect, P1)

Description

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

glibc's malloc will call sbrk, which uses the system call brk. We're allowing this for content and GMP processes, but not RDD. Because it's (potentially) used by malloc, it should be in the common policy.

It would also be nice if we could detect at build time whether we're using our own malloc, so we can make those rules conditional.

Comment 1

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

Here's an example, reported in IRC #developers. It's actually free trying to shrink the sbrk heap, but the principle is the same.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Attachment: Bug 1573578 - Whitelist brk syscall if jemalloc is disabled. r?jld

Comment 4

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=632813feb83c808288e3c8e839046eaa8e9079c2

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2679dd0ac879
Whitelist brk syscall if jemalloc is disabled. r=jld

Comment 6

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/2679dd0ac879

Comment 7

[Ryan VanderMeulen [:RyanVM]]

I assume this is something we're going to want to uplift for the sake of downstream distros?

Comment 8

[Gian-Carlo Pascutto [:gcp]]

This is riskfree so we can uplift, but I sure as hell hope downstream distros don't disable jemalloc because it would disable a lot of our memory hardening work.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Per IRC discussion, it doesn't sound like there's any compelling argument for backport here even though it's effectively NPOTB for builds we ship.

Comment 10

[Gian-Carlo Pascutto [:gcp]]

Comment on attachment 9085516 [details]
Bug 1573578 - Whitelist brk syscall if jemalloc is disabled. r?jld

Beta/Release Uplift Approval Request

• User impact if declined: Custom builds with jemalloc disabled and the sandbox enabled (this excludes ASAN) can crash.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The code change is not active for our own builds.
• String changes made/needed: