1111248 - Crash in BooleanGetPrimitiveValueSlow

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S. (please needinfo tschuster)]

Attachment: crasher.js

We really need to double check our ObjectClassIs, or maybe just return false for ScriptedDirectProxies for the moment.

Comment 1

[Tom S. (please needinfo tschuster)]

Attachment: bool-unbox

Comment 2

[Tom S. (please needinfo tschuster)]

Attachment: bool-unbox

I am not really used to working on sec-bugs. So not sure what the right procedure is here.

Comment 3

[Daniel Veditz [:dveditz]]

https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 4

[Tom S. (please needinfo tschuster)]

Bug 1111243 should at least make this test-case impossible, but there might still be some other way to have chained proxies.

Comment 5

[Tom S. (please needinfo tschuster)]

Attachment: Cleanup BooleanGetPrimitiveValue

Comment 6

[Jeff Walden [:Waldo]]

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.

Comment 7

[Tom S. (please needinfo tschuster)]

Thanks Jeff.
So, landing this probably depends on what we decide in Bug 1111243.

Comment 8

[Andrew McCreight [:mccr8]]

Jeff, is this a definite security issue or something we just might want?  What rating should it get?  Thanks.

Comment 9

[Jeff Walden [:Waldo]]

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.

Comment 10

[Tom S. (please needinfo tschuster)]

Attachment: Updated

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?

Comment 11

[Andrew McCreight [:mccr8]]

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 12

[Jeff Walden [:Waldo]]

Comment on attachment 8544854 [details] [diff] [review]
Updated

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.

Comment 13

[Andrew McCreight [:mccr8]]

Tom, please remember to get sec-approval before landing.

Comment 14

[Tom S. (please needinfo tschuster)]

Attachment: final version for aurora/beta

Comment 15

[Tom S. (please needinfo tschuster)]

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.

Comment 16

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

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).

Comment 17

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

Please also create Aurora, Beta, and ESR31 patches (and nominate them) when it is on trunk.

Comment 18

[Tom S. (please needinfo tschuster)]

Attachment: final version for nightly

Only change is in code that is deleted. JSObject::defineProperty was renamed to DefineProperty.

Original patch applies to beta and aurora.

Comment 19

[Tom S. (please needinfo tschuster)]

ESR36 doesn't have js::Unbox, so I am not sure how to fix this bug there.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

ESR31 you meant? The "tracking-firefox-esr31: 36+" flag just means "Tracking the esr31 release shipping alongside Firefox 36"

Comment 21

[Tom S. (please needinfo tschuster)]

You are right, I meant ESR31.

Comment 22

[Jeff Walden [:Waldo]]

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.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e1aaece372a5

Please post an esr31 patch at your earliest convenience. Also, please request Aurora/Beta approval on the existing patch as soon as possible.

Comment 24

[Tom S. (please needinfo tschuster)]

Comment on attachment 8546049 [details] [diff] [review]
final version for aurora/beta

Approval Request Comment
Please, see comment 15

Comment 25

[Tom S. (please needinfo tschuster)]

Attachment: v1 - Patch for ESR31

This uses CheckedUnwrap, like we used to do for Date objects: https://hg.mozilla.org/mozilla-central/rev/a4014b7b418a.

This doesn't unwrap user-proxies (new Proxy(..))!

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e1aaece372a5

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Why are we reopening this? Bug resolution tracks landing on m-c, status flags track branch uplifts :-)

Comment 28

[Tom S. (please needinfo tschuster)]

The ESR31 patch hasn't even been reviewed yet and closed bugs don't show up on my dashboard.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0ae484ba89d4
https://hg.mozilla.org/releases/mozilla-beta/rev/7f44816c0449

Re-closing per offline discussion w/ evilpie.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/0ae484ba89d4

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/bc93d4b84bb2

Comment 32

[Jeff Walden [:Waldo]]

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+.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/6150867fc6a8
https://hg.mozilla.org/releases/mozilla-esr31/rev/5891a302300a
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/af6e951d18ca

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/bc93d4b84bb2
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/6150867fc6a8