Closed Bug 810169 Opened 7 years ago Closed 7 years ago

JSCompartment::global_ needs a read barrier

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
Tracking Status
firefox17 --- wontfix
firefox18 - wontfix
firefox19 - fixed
firefox20 - fixed
firefox-esr10 --- unaffected
firefox-esr17 - wontfix
b2g18 --- wontfix

People

(Reporter: billm, Assigned: billm)

Details

(Keywords: csectype-uaf, sec-critical, Whiteboard: [js:t][adv-main19+])

Attachments

(1 file)

Let us have pity on this field, for it is weak.
Whiteboard: [js:t]
Status update - pinged Bill, he says he will try to look into this today.
Attached patch patchSplinter Review
This converts the field to ReadBarriered. It also handles a potential problem where JSContext::global() might return an unrooted global.

Sorry about the code movement to inlines.h files. jscntxt.h is included before jscompartment.h, so it can't access the compartment's fields. And maybeGlobal() needs to be in an inlines file because it invokes a barrier now, and barrier code is in inline headers.
Attachment #688041 - Flags: review?(luke)
Comment on attachment 688041 [details] [diff] [review]
patch

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

jscompartmentinlines.h is missing from the patch.  Assuming it is just more moved definitions, r+.

::: js/src/jscompartment.h
@@ +560,5 @@
>  JSContext::global() const
>  {
> +    /*
> +     * It's safe to use |unsafeGet()| here because any compartment that is
> +     * on-stack will not be marked automatically, so there's no need for a read

s/not//
Attachment #688041 - Flags: review?(luke) → review+
Comment on attachment 688041 [details] [diff] [review]
patch

[Security approval request comment]
How easily can the security issue be deduced from the patch?
Not so easily. There's a single crucial line that changes a type to ReadBarriered. The rest of the patch is a fix for something that won't be a problem until we eliminate the conservative stack scanner.

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

Which older supported branches are affected by this flaw?
The field in question was introduced in bug 687724 (FF16).

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

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

How likely is this patch to cause regressions; how much testing does it need?
Not likely to cause regressions.
Attachment #688041 - Flags: sec-approval?
Attachment #688041 - Flags: sec-approval? → sec-approval+
Is this patch safe enough to land on 18? Or is this undiscoverable enough (seems obscure) that we can let it bake through a cycle?
https://hg.mozilla.org/mozilla-central/rev/d96db52bedc4
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment on attachment 688041 [details] [diff] [review]
patch

As far as I know, bug 743311 (from FF19) is the first time we started relying on their being a read barrier on this field. However, it's possible I'm forgetting something; the problem could go back as far as bug 687724 (FF18). Also, the place where we use the field in bug 743311 can only be called from chrome code, which makes it less worrisome for security.

Overall, I don't think we have much to worry from this. We could take it or not on 18.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 687724 or bug 743311?
User impact if declined: Possible crashes.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None
Attachment #688041 - Flags: approval-mozilla-beta?
Attachment #688041 - Flags: approval-mozilla-aurora?
Flags: needinfo?(wmccloskey)
Comment on attachment 688041 [details] [diff] [review]
patch

Given that, let's land in 19/ESR17:19+ for the first time and limit risk.
Attachment #688041 - Flags: approval-mozilla-beta?
Attachment #688041 - Flags: approval-mozilla-beta-
Attachment #688041 - Flags: approval-mozilla-aurora?
Attachment #688041 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/267d0c53fc6d

Alex, I didn't understand your last comment. What does ESR17:19+ mean? Do I need to land anywhere else?
My understanding is that it means this should land on esr17 once Mozilla19 hits beta so that the fix is released at the same time for all builds that it landed on (in other words, we don't want an esr17 release with this fix shipping with a Mozilla18 release that doesn't).
(In reply to Ryan VanderMeulen from comment #11)
> My understanding is that it means this should land on esr17 once Mozilla19
> hits beta so that the fix is released at the same time for all builds that
> it landed on (in other words, we don't want an esr17 release with this fix
> shipping with a Mozilla18 release that doesn't).

How will we remember to do that?
(Not speaking on behalf of the release drivers) Presumably that's what tracking-firefox-esr17:19+ is for.
Yes, I think at some later point they will ask you to land it. That's what has happened with some other sec bugs for which the landing was being held off until a later date.
(In reply to Bill McCloskey (:billm) from comment #8)
> Comment on attachment 688041 [details] [diff] [review]
> patch
> 
> As far as I know, bug 743311 (from FF19) is the first time we started
> relying on their being a read barrier on this field.

Upon further review, we'll actually wontfix for ESR17/B2G18. Sorry for the churn.
Whiteboard: [js:t] → [js:t][adv-main19+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.