Closed Bug 1481978 Opened Last year Closed Last year

Unsafe heap allocation in SandboxFork::StartChrootServer

Categories

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

Unspecified
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: jld, Assigned: jld)

References

Details

Attachments

(1 file, 1 obsolete file)

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

> base::CloseSuperfluousFds([this](int fd) { return fd == mChrootServer; });
Attached patch Patch (obsolete) — Splinter Review
Attachment #8998689 - Flags: review?(gpascutto)
It seems like CloseSuperfluousFds is a continuous footgun. Why not change it so that it doesn't take a possibly heap allocated value?
Attachment #8998689 - Attachment is obsolete: true
Attachment #8998689 - Flags: review?(gpascutto)
Attachment #8998704 - Flags: review?(gpascutto)
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.
Attachment #8998704 - Flags: review?(gpascutto) → review+
(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.
Pushed by jedavis@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/542197624ba2
Change IPC CloseSuperfluousFds to prevent accidentally heap-allocating closures. r=glandium
https://hg.mozilla.org/mozilla-central/rev/542197624ba2
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.