Closed Bug 1035395 Opened 5 years ago Closed 5 years ago

Fix current crop of b2g rooting hazards

Categories

(Core :: JavaScript: GC, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox30 --- unaffected
firefox31 --- unaffected
firefox32 + fixed
firefox33 + fixed
firefox-esr24 --- unaffected
firefox-esr31 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-uaf, regression, sec-critical)

Attachments

(2 files, 2 obsolete files)

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.
Perhaps there's an easy way to get a JSContext* in here?
Attachment #8451857 - Flags: review?(bobbyholley)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
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).
Attachment #8451857 - Flags: review?(bobbyholley) → review-
Sadly, AutoJSContext::AutoJSContext can GC.

See https://ftp-ssl.mozilla.org/pub/mozilla.org/b2g/try-builds/sfink@mozilla.com-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.)
Attachment #8451900 - Flags: review?(bugs)
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+.
Attachment #8451900 - Flags: review?(bugs) → review+
Group: javascript-core-security
(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.
Attachment #8457640 - Flags: review?(bobbyholley)
Attachment #8451857 - Attachment is obsolete: true
(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.
Attachment #8457640 - Flags: review?(bobbyholley) → review+
Attached patch ,Splinter Review
Combined patch for approval and backporting.
Attachment #8457640 - Attachment is obsolete: true
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
Attachment #8457670 - Flags: sec-approval?
Attachment #8457670 - Flags: review+
Attachment #8457670 - Flags: approval-mozilla-aurora?
Comment on attachment 8457670 [details] [diff] [review]
,

Let's get this into Aurora.
Attachment #8457670 - Flags: sec-approval?
Attachment #8457670 - Flags: sec-approval+
Attachment #8457670 - Flags: approval-mozilla-aurora?
Attachment #8457670 - Flags: approval-mozilla-aurora+
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Group: javascript-core-security
Group: core-security
Keywords: regression
You need to log in before you can comment on or make changes to this bug.