1487167 - Rooting hazards, Aug 2018 edition

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Steve Fink [:sfink] [:s:]]

I fixed some more bugs in the hazard analysis, which exposed several hazards.

Probably the nastiest were the ones in Content{Process,Frame}MessageManager::GetOrCreateWrapper which are of the form:

  AutoJSAPI jsapi;
  RootedValue v(jsapi.cx());
  ...
  return &v.toObject();

The problem is that the analysis has figured out that ~AutoJSAPI can GC, and the return value at that time has been unwrapped from the Rooted and is sitting on the stack, waiting to be invalidated.

And there's no cx around to use for a more restricted scope for AutoJSAPI; that's where we're getting the cx from in the first place.

It looks like we're now using root lists that sit on the cx themselves. So either we re-add the ability to use Rooted with a runtime, or we keep that functionality restricted to PersistentRooted. I went with the latter change, but to make it less ugly I still needed to improve PersistentRooted's interface when using a runtime. (The alternative would be to root with Maybe<PersistentRooted<Value>>.

There might be better options, if I can get to a RootingCx or something here? It's still a bit weird that I'm getting the runtime from the doomed AutoJSAPI.

~AutoJSAPI GCs via

    void mozilla::dom::AutoJSAPI::~AutoJSAPI()
    void mozilla::dom::AutoJSAPI::~AutoJSAPI(int32)
    void mozilla::dom::AutoJSAPI::ReportException()
    void xpc::ErrorReport::LogToConsoleWithStack(JS::Handle<JSObject*>, JS::Handle<JSObject*>, uint64)
    FieldCall: nsIScriptError.SetErrorMessageName

That particular method is not a big deal, but you'll notice that LogToConsoleWithStack takes Handles -- it has multiple ways to GC.

The ServiceWorkerContainer.cpp hazard is especially facepalmy -- we have a rooted local, but mistakenly use the unrooted aGlobal instead. :( I'm not sure whether IsInPrivateBrowsing can really GC or not, but it doesn't matter.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Various DOM rooting hazards

At last, green on try!

Jon, this review request is mostly about the PersistentRooted API additions and whether a different approach for the related hazards would be better.

bz, I can find other reviewers for the other pieces of this if you're not the right person for all of them. Let me know.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9005074 [details] [diff] [review]
Various DOM rooting hazards

>+  // PersistentRooted, since it can root with a runtime.

Just use RootingCx() to root with, like so:

  JS::RootedValue val(RootingCx());
  {
    // Scope to make sure ~AutoJSAPI finishes before we're trying
    // to work with a raw JSObject*.
    etc
  }
  return &val.toObject();

This applies to both message managers.

r=me on the dom bits with that changed.  Up to you whether you still want the RootingAPI change.

Comment 3

[Jon Coppeard (:jonco)]

Comment on attachment 9005074 [details] [diff] [review]
Various DOM rooting hazards

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

The RootingAPI.h changes look fine but I'd prefer if you did what bz suggested and left this as it is.

Comment 4

[Steve Fink [:sfink] [:s:]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #2)
> Comment on attachment 9005074 [details] [diff] [review]
> Various DOM rooting hazards
> 
> >+  // PersistentRooted, since it can root with a runtime.
> 
> Just use RootingCx() to root with, like so:

Hah. I found RootingCx(), but I wasn't sure when it was valid to use or how to get to it. That's much better, thanks; I'll remove the PersistentRooted change.

Comment 5

[Boris Zbarsky [:bzbarsky]]

RootingCx() is valid to use any time you're on the main thread or a web worker thread and not too early in  startup or too late in shutdown.  Any time you might want to create an AutoJSAPI it's OK to use it, for  sure.

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Various DOM rooting issues,

Uploading the actual patch I will land, for sec-approval.

Comment 7

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9005501 [details] [diff] [review]
Various DOM rooting issues,

Inheriting r=bz

Comment 8

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9005501 [details] [diff] [review]
Various DOM rooting issues,

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

For people familiar with the GC setup, it points to a potential UAF possibility.  None of these sites strikes me as very easy to control, though. It looks fairly hard to me.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No. Though if someone were looking for rooting hazards specifically, they could spot added roots in the code pretty easily.

> Which older supported branches are affected by this flaw?

I'm guessing this goes back at least to esr52.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I do not. I wouldn't be surprised if some parts failed to apply, but they would be easy to update (or more likely, just delete; if the code is new, the old code probably wouldn't have a similar hazard.)

> How likely is this patch to cause regressions; how much testing does it need?

Unlikely.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

This isn't going to land for 62. I'm giving sec-approval+ for landing on September 19, two weeks into the next cycle. Please don't land it on trunk before then. At that point, we'll want Beta and ESR60 patches made and nominated as well.

Comment 10

[Pascal Chevrel:pascalc (PTO until April 26)]

(In reply to Al Billings [:abillings] from comment #9)
> This isn't going to land for 62. I'm giving sec-approval+ for landing on
> September 19, two weeks into the next cycle. Please don't land it on trunk
> before then. At that point, we'll want Beta and ESR60 patches made and
> nominated as well.

Steve, it probably needs landing and uplifts soon, thanks!

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05ca2a671aef4bca6ef7119a5b2611ec90ac1433

The attached patch applies cleanly to Beta, but ESR60 is going to need a rebased one.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/05ca2a671aef

Comment 13

[Steve Fink [:sfink] [:s:]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)
> The attached patch applies cleanly to Beta, but ESR60 is going to need a
> rebased one.

I rebased it, but there was nothing left -- none of the relevant code existed in esr60. I looked at related code, but saw no issues of this type.

Comment 14

[Pascal Chevrel:pascalc (PTO until April 26)]

Steve, I am not seeing a patch for uplift into beta, can you provide one? Thanks

Comment 15

[Ryan VanderMeulen [:RyanVM]]

To be clear, per comment 11, this just needs an approval request. It grafts cleanly to Beta as-is.

Comment 16

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9005501 [details] [diff] [review]
Various DOM rooting issues,

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: multiple, old

User impact if declined: UAF crashes, theoretically possibly exploitable.

Is this code covered by automated tests?: Yes

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): Just adding rooting, which we do all the time (these lingered because the analysis was missing them, not because they're unusually difficult or risky to address.)

String changes made/needed: none

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3e4a2d14ca35

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9005501 [details] [diff] [review]
Various DOM rooting issues,

Approved for 63.0b13, thanks.