Closed Bug 1189343 Opened 9 years ago Closed 8 years ago

OOMs during JitRuntime initialization can cause UAF

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1221385

People

(Reporter: jandem, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-uaf, sec-high)

In JSRuntime::createJitRuntime, we do:

    JitRuntime::AutoMutateBackedges amb(jrt);
    jitRuntime_ = jrt;

    if (!jitRuntime_->initialize(cx)) {
        ReportOutOfMemory(cx);

        js_delete(jitRuntime_);
        jitRuntime_ = nullptr;

The js_delete(jitRuntime_) seems dangerous because:

(1) The AutoMutateBackedges destructor will write to jitRuntime_-> mutatingBackedgeList_.

(2) More dangerous: JitRuntime::initialize could have allocated JitCode instances for trampolines/wrappers. These reference ExecutablePools, stored as part of the ExecutableAllocator. The js_delete here will destroy the ExecutableAllocator as well, so if we GC later we'll access an invalid ExecutablePool.

Not sure what to do about this. We could:

(1) Move ExecutableAllocator from JitRuntime back to JSRuntime, to ensure it always outlives any JitCode objects.

(2) MOZ_CRASH if JitRuntime initialization fails.

I can't think of any other options that aren't scary..
I noticed this when I triggered random OOMs in AssemblerBuffer::ensureSpace, while investigating another issue.
(In reply to Jan de Mooij [:jandem] from comment #0)
> In JSRuntime::createJitRuntime, we do:
> 
>     JitRuntime::AutoMutateBackedges amb(jrt);
>     jitRuntime_ = jrt;
> 
>     if (!jitRuntime_->initialize(cx)) {
>         ReportOutOfMemory(cx);
> 
>         js_delete(jitRuntime_);
>         jitRuntime_ = nullptr;
> 
> The js_delete(jitRuntime_) seems dangerous because:
> 
> (1) The AutoMutateBackedges destructor will write to jitRuntime_->
> mutatingBackedgeList_.

We can solve this one by taking a reference to the JitRuntime pointer.  Thus it would get clear, and we can check for that in the destructor.

> (2) More dangerous: JitRuntime::initialize could have allocated JitCode
> instances for trampolines/wrappers. These reference ExecutablePools, stored
> as part of the ExecutableAllocator. The js_delete here will destroy the
> ExecutableAllocator as well, so if we GC later we'll access an invalid
> ExecutablePool.

I have really hard time understanding the ownership model of the ExecutableAllocator, as the ExecutableAllocator seems to be the only structure to increment ExecutablePool reference counter.

I would have expected us to at least increment this counter each time a JitCode reference an ExecutablePool, and to decrement it as well with a JitCode finalizer.

Could we add a finalizer to the JitCode, to decrement to counter of its ExecutablePool, and share the ownership of the ExecutablePool between the ExecutableAllocator, and the JitCode?

In which case, the destructor of the ExecutableAllocator, should only call purge(), and not force the release of all ExecutablePools.

Another option, would be to nuke all ExecutablePool pointer from the JitCode, when we free the JitRuntime, but this implies keeping weak references to the JitCode within the ExecutablePools, which we already do for Ion code.

> Not sure what to do about this. We could:
> 
> (1) Move ExecutableAllocator from JitRuntime back to JSRuntime, to ensure it
> always outlives any JitCode objects.

This sounds like a hack, as the ownership do belong to the JitRuntime / JitCode.

> (2) MOZ_CRASH if JitRuntime initialization fails.

I am not a big fan of this one, if this would be a good way to backport a fix.  The reason I do not like it, is that we have a limit of 20 workers per origin, and that we create a new JSRuntime & JitRuntime for each, and it sounds likelythat we can hit this OOM.

Also, I don't think we have any JS shell tests to emulate another JSRuntime / JitRuntime.
(In reply to Nicolas B. Pierron [:nbp] from comment #2)
> I would have expected us to at least increment this counter each time a
> JitCode reference an ExecutablePool, and to decrement it as well with a
> JitCode finalizer.

That's what we're doing, JitCode has a finalizer that decrements the pool reference.

> > (1) Move ExecutableAllocator from JitRuntime back to JSRuntime, to ensure it
> > always outlives any JitCode objects.
> 
> This sounds like a hack, as the ownership do belong to the JitRuntime /
> JitCode.

At least it's easy to understand... Having ExecutablePools without the ExecutableAllocator they belong too, or something, seems more scary.
(In reply to Jan de Mooij [:jandem] from comment #3)
> (In reply to Nicolas B. Pierron [:nbp] from comment #2)
> > I would have expected us to at least increment this counter each time a
> > JitCode reference an ExecutablePool, and to decrement it as well with a
> > JitCode finalizer.
> 
> That's what we're doing, JitCode has a finalizer that decrements the pool
> reference.
> 

Ok, I have some trouble finding it in dxr then.

Then we should just call "purge" in the destructor of the ExecutableAllocator [1], and remove the willDestroy flag on the remove function of the ExecutablePool.  The assertion no longer holds, as the JitCode will live longer than the JitRuntime in such case.

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/ExecutableAllocator.h?from=ExecutableAllocator#190-194
Group: core-security → javascript-core-security
Nicolas, has there been any add'l discussion about this?
Flags: needinfo?(nicolas.b.pierron)
(In reply to Matt Wobensmith from comment #5)
> Nicolas, has there been any add'l discussion about this?

No.
Flags: needinfo?(nicolas.b.pierron)
Fixed in bug 1221385.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Group: javascript-core-security
You need to log in before you can comment on or make changes to this bug.