1481978 - Unsafe heap allocation in SandboxFork::StartChrootServer

Status: RESOLVED FIXED

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

Description

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

Just like bug 1480401, but for this line of code in SandboxFork::StartChrootServer
instead:

> base::CloseSuperfluousFds([this](int fd) { return fd == mChrootServer; });

Comment 1

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

Attachment: Patch

Comment 2

[Mike Hommey [:glandium]]

It seems like CloseSuperfluousFds is a continuous footgun. Why not change it so that it doesn't take a possibly heap allocated value?

Comment 3

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

Attachment: Patch [v2; removes footgun]

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8998704 [details] [diff] [review]
Patch [v2; removes footgun]

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

::: ipc/chromium/src/base/process_util_posix.cc
@@ +120,5 @@
>  };
>  typedef mozilla::UniquePtr<DIR, ScopedDIRClose> ScopedDIR;
>  
>  
> +void CloseSuperfluousFds(void* aCtx, bool (*aShouldPreserve)(void*, int))

I was about to say "third party, yadayada", but we've crossed that bridge with the std::function version anyways.

::: security/sandbox/linux/launch/SandboxLaunch.cpp
@@ +589,5 @@
>      SANDBOX_LOG_ERROR("capset (chroot helper): %s", strerror(errno));
>      MOZ_DIAGNOSTIC_ASSERT(false);
>    }
>  
> +  base::CloseSuperfluousFds(this, [](void* aCtx, int aFd) {

You could pass down mChrootServer instead of this.

Comment 5

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

(In reply to Mike Hommey [:glandium] from comment #4)
> Comment on attachment 8998704 [details] [diff] [review]
…
> ::: ipc/chromium/src/base/process_util_posix.cc
…
> I was about to say "third party, yadayada", but we've crossed that bridge
> with the std::function version anyways.

ipc/chromium/src can be freely modified; we gave up on the idea of trying to re-merge with upstream years ago, and “upstream” no longer really exists after Chromium's switch to Mojo.

> ::: security/sandbox/linux/launch/SandboxLaunch.cpp
> > +  base::CloseSuperfluousFds(this, [](void* aCtx, int aFd) {
> 
> You could pass down mChrootServer instead of this.

I could, but the casts for that are more visual noise than this way, and `this` works.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/542197624ba2
Change IPC CloseSuperfluousFds to prevent accidentally heap-allocating closures. r=glandium

Comment 7

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/542197624ba2

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Wontfix for 62 from discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=1480401#c17.