Closed Bug 1111248 Opened 5 years ago Closed 5 years ago

Crash in BooleanGetPrimitiveValueSlow


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 + fixed
firefox37 + fixed
firefox38 + fixed
firefox-esr31 36+ fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed


(Reporter: evilpie, Assigned: evilpie)


(Keywords: sec-critical, Whiteboard: [adv-main36+][adv-esr31.5+])


(4 files, 4 obsolete files)

Attached file crasher.js
We really need to double check our ObjectClassIs, or maybe just return false for ScriptedDirectProxies for the moment.
Attached patch bool-unbox (obsolete) — Splinter Review
Attachment #8536074 - Attachment is obsolete: true
Attached patch bool-unbox (obsolete) — Splinter Review
I am not really used to working on sec-bugs. So not sure what the right procedure is here.
Bug 1111243 should at least make this test-case impossible, but there might still be some other way to have chained proxies.
Attached patch Cleanup BooleanGetPrimitiveValue (obsolete) — Splinter Review
Assignee: nobody → evilpies
Attachment #8536751 - Attachment is obsolete: true
Attachment #8540229 - Flags: review?(jwalden+bmo)
Comment on attachment 8540229 [details] [diff] [review]
Cleanup BooleanGetPrimitiveValue

Review of attachment 8540229 [details] [diff] [review]:

::: js/src/jsbool.cpp
@@ +143,5 @@
>      if (!ctor)
>          return nullptr;
> +    if (!GlobalObject::initBuiltinConstructor(cx, global, JSProto_Boolean, ctor, booleanProto))
> +        return nullptr;

Don't move this from where it was.  This should be the last step of these init methods, because it's the thing that indicates completion.  With it here, if one of the two succeeding calls failed, code that happened to fail in just the wrong place could observe an incompletely-initialized Boolean constructor/prototype on a global object.

(Granted, we don't always adhere to this perfectly, but generally we have, and certainly we shouldn't backslide.)

::: js/src/json.cpp
@@ +892,5 @@
>  js_InitJSONClass(JSContext *cx, HandleObject obj)
>  {
>      Rooted<GlobalObject*> global(cx, &obj->as<GlobalObject>());
> +    RootedObject proto(cx, global->getOrCreateObjectPrototype(cx));

Needs a

    if (!proto)
        return nullptr;

@@ +897,1 @@
>      RootedObject JSON(cx, NewObjectWithClassProto(cx, &JSONClass, proto, global, SingletonObject));

...hmm.  Should this have been NewObjectWithGivenProto?  I have no idea what the "class" proto is that would be implied by this, or how it would differ from a "given" proto.
Attachment #8540229 - Flags: review?(jwalden+bmo) → review+
Thanks Jeff.
So, landing this probably depends on what we decide in Bug 1111243.
Jeff, is this a definite security issue or something we just might want?  What rating should it get?  Thanks.
Flags: needinfo?(jwalden+bmo)
Determining whether this is or isn't a security issue would require auditing all the ObjectClassIs uses, knowing the exact nature of every object that might pass that check wrongly, and tracking down a whole lot of details in a whole lot of time.  Oh, and it's a pretty large set of cases to track down, and proxy types to inspect, and so on.  The chances of incorrectly auditing are pretty decent.  Could be just a crash, could be exploitable, could just be leaking the contents of memory that shouldn't be directly accessed.

So I don't know, and frankly I don't think we should care to figure out.  We should just assume the worst (well, not quite worst -- not unless someone shows up with a proof of concept or 0day) and fix this everywhere without too much omphaloskepsis.  I'd treat this as sec-critical, with there being some unknown chance of it being lower in actuality.
Flags: needinfo?(jwalden+bmo)
Attached patch Updated (obsolete) — Splinter Review
Not sure about NewObjectWithClassProto, it definitely looks weird. JSON.__proto__ seems to be the plain object anyway.

So are we going to land this just on nightly or uplift?
Attachment #8540229 - Attachment is obsolete: true
Attachment #8544854 - Flags: review?(jwalden+bmo)
This sounds like something that has affected us forever.  Please update the flag or post here or something if it is a more recent regression.
Comment on attachment 8544854 [details] [diff] [review]

Review of attachment 8544854 [details] [diff] [review]:

I think we land everywhere.  This is small enough that I'd say try to get it in the release in a week, but I imagine there'll probably be pushback on that.  :-\  Given we're somewhat tilting at unknowns here, it's probably not the end of the world if we fix this now, everywhere except in the super-forthcoming release.

::: js/src/json.cpp
@@ +899,1 @@
>      RootedObject JSON(cx, NewObjectWithClassProto(cx, &JSONClass, proto, global, SingletonObject));

Given that js_InitMathClass successfully uses NewObjectWithGivenProto for a functionally identical case (not sure why I didn't think to compare with that before), let's switch to that.
Attachment #8544854 - Flags: review?(jwalden+bmo) → review+
Tom, please remember to get sec-approval before landing.
Attachment #8544854 - Attachment is obsolete: true
Comment on attachment 8546049 [details] [diff] [review]
final version for aurora/beta

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not straightforward. This requires a good understanding of how our proxies work.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I intent to use the commit message "Cleanup JSON and Boolean code."

Which older supported branches are affected by this flaw?
All of them as far as I can tell.

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?
We need js::Unbox. Which was introduced in Bug 1050340.

How likely is this patch to cause regressions; how much testing does it need?
Unlikely. Unboxing is relatively often used nowadays. This specific feature of JSON.stringify is rather uncommon.
Attachment #8546049 - Flags: sec-approval?
Comment on attachment 8546049 [details] [diff] [review]
final version for aurora/beta

sec-approval+ but for checkin on January 26 or later (two weeks into the next cycle).
Attachment #8546049 - Flags: sec-approval? → sec-approval+
Whiteboard: [checkin on 1/26]
Please also create Aurora, Beta, and ESR31 patches (and nominate them) when it is on trunk.
Only change is in code that is deleted. JSObject::defineProperty was renamed to DefineProperty.

Original patch applies to beta and aurora.
ESR36 doesn't have js::Unbox, so I am not sure how to fix this bug there.
Flags: needinfo?(jwalden+bmo)
ESR31 you meant? The "tracking-firefox-esr31: 36+" flag just means "Tracking the esr31 release shipping alongside Firefox 36"
You are right, I meant ESR31.
Something like UncheckedUnwrap(obj)->as<BooleanObject>().unbox(), maybe?  I'd run that past someone familiar with unwrapping, just to be sure -- and get a review of it.  I wouldn't bother with all the window-dressing fixing here, just get rid of the one place the method is used problematically.
Flags: needinfo?(jwalden+bmo)
Attachment #8546049 - Attachment is obsolete: true

Please post an esr31 patch at your earliest convenience. Also, please request Aurora/Beta approval on the existing patch as soon as possible.
Flags: needinfo?(evilpies)
Flags: in-testsuite?
Whiteboard: [checkin on 1/26]
Attachment #8546049 - Attachment description: final version → final version for aurora/beta
Attachment #8546049 - Attachment is obsolete: false
Comment on attachment 8546049 [details] [diff] [review]
final version for aurora/beta

Approval Request Comment
Please, see comment 15
Flags: needinfo?(evilpies)
Attachment #8546049 - Flags: approval-mozilla-beta?
Attachment #8546049 - Flags: approval-mozilla-aurora?
This uses CheckedUnwrap, like we used to do for Date objects:

This doesn't unwrap user-proxies (new Proxy(..))!
Attachment #8554211 - Flags: review?(jwalden+bmo)
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Attachment #8546049 - Flags: approval-mozilla-beta?
Attachment #8546049 - Flags: approval-mozilla-beta+
Attachment #8546049 - Flags: approval-mozilla-aurora?
Attachment #8546049 - Flags: approval-mozilla-aurora+
Resolution: FIXED → ---
Keywords: leave-open
Why are we reopening this? Bug resolution tracks landing on m-c, status flags track branch uplifts :-)
The ESR31 patch hasn't even been reviewed yet and closed bugs don't show up on my dashboard.

Re-closing per offline discussion w/ evilpie.
Closed: 5 years ago5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Comment on attachment 8554211 [details] [diff] [review]
v1 - Patch for ESR31

Review of attachment 8554211 [details] [diff] [review]:

::: js/src/jsbool.cpp
@@ +199,5 @@
>  js::BooleanGetPrimitiveValueSlow(HandleObject wrappedBool)
>  {
> +    JSObject *obj = CheckedUnwrap(wrappedBool);
> +    if (!obj || !obj->is<BooleanObject>())
> +        return false;

efaust and I both are fairly sure, given the precondition documented above, that this if-condition will never hold and could be eliminated.  But, paranoia.  So it's fine to have, and at worst it's a little extra dead code, at best it converts places that *would* be errors, into silent failures.  For an ESR that's plenty good enough.  So r+.
Attachment #8554211 - Flags: review?(jwalden+bmo) → review+
Attachment #8554211 - Flags: approval-mozilla-esr31?
Attachment #8554211 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Whiteboard: [adv-main36+][adv-esr31.5+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.