1035395 - Fix current crop of b2g rooting hazards

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

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

The b2g build currently has 4 rooting hazards that have leaked into Aurora, which has exact rooting. The b2g hazard jobs currently expect 4, but they're all true hazards.

Comment 1

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

Attachment: Runtime-root to handle AutoJSContext constructor GC

Perhaps there's an easy way to get a JSContext* in here?

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 8451857 [details] [diff] [review]
Runtime-root to handle AutoJSContext constructor GC

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

::: caps/src/nsScriptSecurityManager.cpp
@@ +863,2 @@
>      AutoJSContext cx;
> +    global = js::UncheckedUnwrap(global, /* stopAtOuter = */ false);

We actually don't need the JSContext or the UncheckedUnwrap at all (we're not even using them!). This whole function can just be the two asserts plus the last line (with aGlobal as the arg).

Comment 3

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

Attachment: Fix AutoJSContext constructor GC rooting hazards et al

Sadly, AutoJSContext::AutoJSContext can GC.

See https://ftp-ssl.mozilla.org/pub/mozilla.org/b2g/try-builds/[e-mail]-a13983ab4dea/try-linux64-b2g-haz/hazards.txt.gz for the full hazards being fixed here (this patch fixes 3 of the 4. Or tries to, anyway.)

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8451900 [details] [diff] [review]
Fix AutoJSContext constructor GC rooting hazards et al

>-Navigator::HasNFCSupport(JSContext* /* unused */, JSObject* aGlobal)
>+Navigator::HasNFCSupport(JSContext* aCx, JSObject* aGlobal)
> {
>+  JS::Rooted<JSObject*> global(aCx, aGlobal);
>+
>   // Do not support NFC if NFC content helper does not exist.
>   nsCOMPtr<nsISupports> contentHelper = do_GetService("@mozilla.org/nfc/content-helper;1");
>   if (!contentHelper) {
>     return false;
>   }
> 
>-  nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(aGlobal);
>+  nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(global);


Couldn't you just move 
nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(aGlobal);
to be before do_GetService.
With that, r+.

Comment 5

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

Attachment: Runtime-root to handle AutoJSContext constructor GC

(In reply to Bobby Holley (:bholley) from comment #2)
> Comment on attachment 8451857 [details] [diff] [review]
> Runtime-root to handle AutoJSContext constructor GC
> 
> Review of attachment 8451857 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: caps/src/nsScriptSecurityManager.cpp
> @@ +863,2 @@
> >      AutoJSContext cx;
> > +    global = js::UncheckedUnwrap(global, /* stopAtOuter = */ false);
> 
> We actually don't need the JSContext or the UncheckedUnwrap at all (we're
> not even using them!). This whole function can just be the two asserts plus
> the last line (with aGlobal as the arg).

Oh. Well dang, that's a lot simpler then! I can't believe I didn't notice that in the first place. Dead code is dead.

Re-requesting review because you marked it r-, but it's just deleting the 2 lines of dead code.

Comment 6

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

(In reply to Olli Pettay [:smaug] from comment #4)
> Comment on attachment 8451900 [details] [diff] [review]
> Fix AutoJSContext constructor GC rooting hazards et al
> 
> >-Navigator::HasNFCSupport(JSContext* /* unused */, JSObject* aGlobal)
> >+Navigator::HasNFCSupport(JSContext* aCx, JSObject* aGlobal)
> > {
> >+  JS::Rooted<JSObject*> global(aCx, aGlobal);
> >+
> >   // Do not support NFC if NFC content helper does not exist.
> >   nsCOMPtr<nsISupports> contentHelper = do_GetService("@mozilla.org/nfc/content-helper;1");
> >   if (!contentHelper) {
> >     return false;
> >   }
> > 
> >-  nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(aGlobal);
> >+  nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(global);
> 
> 
> Couldn't you just move 
> nsCOMPtr<nsPIDOMWindow> win = GetWindowFromGlobal(aGlobal);
> to be before do_GetService.
> With that, r+.

Ok, thanks. XPCOM code still scares me, and in my skimming I assumed it might not be valid to call GetWindowFromGlobal if the do_GetService failed. Which is silly, given what service we're talking about.

Comment 7

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

Attachment: ,

Combined patch for approval and backporting.

Comment 8

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

Comment on attachment 8457670 [details] [diff] [review]
,

Carrying over review from bholley and smaug.


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

Pretty hard, but this does provide indirect access to somebody else's memory.

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

To one familiar with rooting, yes. That's hard to avoid.

Which older supported branches are affected by this flaw?

Aurora only. Before that, we did not use exact rooting.

If not all supported branches, which bug introduced the flaw?

Unknown, multiple bugs. Doesn't matter that much.

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

Yes. Cosmetic differences, automatically fixed.

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

Unlikely. If it passes the regular tests once, it should be fine.

Approval Request Comment
[Feature/regressing bug #]: Various. Doesn't matter much at this point.

[User impact if declined]: Nasty crashes, potentially exploitable if you're really really clever.

[Describe test coverage new/current, TBPL]: It makes the static analysis happy, and try pushes with it are green.

[Risks and why]: If I mess this sort of thing up, it tends to break everything immediately. So the risk is quite low.

[String/UUID change made/needed]: none

Comment 9

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

Comment on attachment 8457670 [details] [diff] [review]
,

Let's get this into Aurora.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

This was landed on inbound and Aurora by Steve.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e9114741c6e
https://hg.mozilla.org/releases/mozilla-aurora/rev/943e0e47d815