Closed Bug 1487167 Opened 2 years ago Closed 2 years ago

Rooting hazards, Aug 2018 edition

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 - unaffected
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 + fixed
firefox64 + fixed

People

(Reporter: sfink, Assigned: sfink)

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [post-critsmash-triage][adv-main63+])

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Various DOM rooting hazards (obsolete) — Splinter Review
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.
Attachment #9005074 - Flags: review?(jcoppeard)
Attachment #9005074 - Flags: review?(bzbarsky)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
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.
Attachment #9005074 - Flags: review?(bzbarsky) → review+
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.
Attachment #9005074 - Flags: review?(jcoppeard)
Group: core-security → dom-core-security
(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.
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.
Priority: -- → P1
Uploading the actual patch I will land, for sec-approval.
Attachment #9005074 - Attachment is obsolete: true
Comment on attachment 9005501 [details] [diff] [review]
Various DOM rooting issues,

Inheriting r=bz
Attachment #9005501 - Flags: review+
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.
Attachment #9005501 - Flags: sec-approval?
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.
Whiteboard: [checkin on 9/19]
Attachment #9005501 - Flags: sec-approval? → sec-approval+
(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!
Flags: needinfo?(sphink)
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.
Whiteboard: [checkin on 9/19]
https://hg.mozilla.org/mozilla-central/rev/05ca2a671aef
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
(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.
Flags: needinfo?(sphink)
Steve, I am not seeing a patch for uplift into beta, can you provide one? Thanks
Flags: needinfo?(sphink)
QA Contact: overholt
To be clear, per comment 11, this just needs an approval request. It grafts cleanly to Beta as-is.
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
Flags: needinfo?(sphink)
Attachment #9005501 - Flags: approval-mozilla-beta?
Comment on attachment 9005501 [details] [diff] [review]
Various DOM rooting issues,

Approved for 63.0b13, thanks.
Attachment #9005501 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main63+]
Component: DOM → DOM: Core & HTML
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.