1155985 - EXC_BAD_ACCESS in js`JS_GetClass(JSObject*) [inlined] JSObject::getClass(this=0x0000000104685200) const at jsobj.h:128

Status: RESOLVED FIXED

(Core :: js-ctypes, defect)

Description

[Tooru Fujisawa [:arai]]

Derived from bug 891107

happened on 64bit cgc build, on OS X.

Configure parameter: (compacting)
  --enable-optimize --enable-debug --enable-stdcxx-compat --enable-ctypes --disable-shared-js --enable-nspr-build
Environment variable:
  JS_GC_ZEAL=14

Code:
for (let i = 0; i < 1000; i++) {
  let test_struct = ctypes.StructType("test_struct", [{ "x": ctypes.int32_t },
                                                      { "bar": ctypes.uint32_t }]);

  try {
    new test_struct("foo", "x");
  } catch (e) {
  }
}

Debug log:
(lldb) run  ~/Desktop/test.js
Process 91361 launched: '/Users/arai/projects/mozilla-central/obj-sm-cgc/dist/bin/js' (x86_64)
Process 91361 stopped
* thread #1: tid = 0x99497c, 0x0000000100712244 js`JS_GetClass(JSObject*) [inlined] JSObject::getClass(this=0x0000000104685200) const at jsobj.h:128, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=2, address=0x104685200)
    frame #0: 0x0000000100712244 js`JS_GetClass(JSObject*) [inlined] JSObject::getClass(this=0x0000000104685200) const at jsobj.h:128
   125 	    }
   126 	
   127 	    const js::Class* getClass() const {
-> 128 	        return group_->clasp();
   129 	    }
   130 	    const JSClass* getJSClass() const {
   131 	        return Jsvalify(getClass());
(lldb) bt
* thread #1: tid = 0x99497c, 0x0000000100712244 js`JS_GetClass(JSObject*) [inlined] JSObject::getClass(this=0x0000000104685200) const at jsobj.h:128, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=2, address=0x104685200)
  * frame #0: 0x0000000100712244 js`JS_GetClass(JSObject*) [inlined] JSObject::getClass(this=0x0000000104685200) const at jsobj.h:128
    frame #1: 0x0000000100712244 js`JS_GetClass(JSObject*) [inlined] JSObject::getJSClass(this=0x0000000104685200) const at jsobj.h:131
    frame #2: 0x0000000100712244 js`JS_GetClass(obj=0x0000000104685200) + 4 at jsapi.cpp:1769
    frame #3: 0x000000010002f16e js`js::ctypes::CType::IsSizeDefined(JSObject*) [inlined] js::ctypes::CType::IsCType(obj=0x0000000104685200) + 5 at CTypes.cpp:3480
    frame #4: 0x000000010002f169 js`js::ctypes::CType::IsSizeDefined(obj=0x0000000104685200) + 9 at CTypes.cpp:3609
    frame #5: 0x0000000100032ef7 js`js::ctypes::ImplicitConvert(cx=0x0000000103088180, val=<unavailable>, targetType_=<unavailable>, buffer=0x0000000103031198, isArgument=false, freePointer=0x0000000000000000) + 87 at CTypes.cpp:2293
    frame #6: 0x000000010003fe5a js`js::ctypes::CType::ConstructData(JSContext*, unsigned int, JS::Value*) + 260 at CTypes.cpp:5177
    frame #7: 0x000000010003fd56 js`js::ctypes::CType::ConstructData(cx=0x0000000103088180, argc=2, vp=0x00000001048090b8) + 758 at CTypes.cpp:3249
    frame #8: 0x00000001002531ba js`js::CallJSNativeConstructor(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [inlined] js::CallJSNative(native=0x000000010003fa60)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 160 at jscntxtinlines.h:235
    frame #9: 0x000000010025311a js`js::CallJSNativeConstructor(cx=0x0000000103088180, native=0x000000010003fa60, args=0x00007fff5fbfeb60)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 106 at jscntxtinlines.h:268
    frame #10: 0x000000010022c330 js`js::InvokeConstructor(cx=0x0000000103088180, args=CallArgs at 0x00007fff5fbfeb60) + 160 at Interpreter.cpp:821
    frame #11: 0x00000001002222e8 js`Interpret(cx=0x0000000103088180, state=0x00007fff5fbff278) + 45864 at Interpreter.cpp:2953
    frame #12: 0x0000000100216f8f js`js::RunScript(cx=0x0000000103088180, state=0x00007fff5fbff278) + 335 at Interpreter.cpp:677
    frame #13: 0x000000010022d017 js`js::ExecuteKernel(cx=0x0000000103088180, script=<unavailable>, scopeChainArg=0x00000001046742e0, thisv=0x00007fff5fbff388, type=EXECUTE_GLOBAL, evalInFrame=<unavailable>, result=<unavailable>) + 1383 at Interpreter.cpp:902
    frame #14: 0x000000010022d36f js`js::Execute(cx=0x0000000103088180, script=<unavailable>, scopeChainArg=<unavailable>, rval=0x0000000000000000) + 431 at Interpreter.cpp:941
    frame #15: 0x000000010071a549 js`ExecuteScript(cx=0x0000000103088180, obj=<unavailable>, scriptArg=<unavailable>, rval=0x0000000000000000) + 505 at jsapi.cpp:4133
    frame #16: 0x0000000100007f9a js`Process(JSContext*, char const*, bool) [inlined] RunFile(JSContext*, char const*, __sFILE*, bool) + 464 at js.cpp:467
    frame #17: 0x0000000100007dca js`Process(cx=0x0000000103088180, filename=<unavailable>, forceTTY=<unavailable>) + 1866 at js.cpp:597
    frame #18: 0x0000000100004f9b js`main + 64 at js.cpp:5789
    frame #19: 0x0000000100004f5b js`main [inlined] Shell(JSContext*, js::cli::OptionParser*, char**) + 348 at js.cpp:6055
    frame #20: 0x0000000100004dff js`main(argc=<unavailable>, argv=<unavailable>, envp=<unavailable>) + 15951 at js.cpp:6376
    frame #21: 0x0000000100000fa4 js`start + 52


I guess this is same bug as following, in bug 891107 comment #28:
https://treeherder.mozilla.org/logviewer.html#?job_id=9001639&repo=mozilla-inbound

Comment 1

[Tooru Fujisawa [:arai]]

more debug log.

(lldb) up
frame #2: 0x0000000100712244 js`JS_GetClass(obj=0x0000000104685200) + 4 at jsapi.cpp:1769
   1766	JS_PUBLIC_API(const JSClass*)
   1767	JS_GetClass(JSObject* obj)
   1768	{
-> 1769	    return obj->getJSClass();
   1770	}
   1771	
   1772	JS_PUBLIC_API(bool)
(lldb) up
frame #3: 0x000000010002f16e js`js::ctypes::CType::IsSizeDefined(JSObject*) [inlined] js::ctypes::CType::IsCType(obj=0x0000000104685200) + 5 at CTypes.cpp:3480
   3477	bool
   3478	CType::IsCType(JSObject* obj)
   3479	{
-> 3480	  return JS_GetClass(obj) == &sCTypeClass;
   3481	}
   3482	
   3483	bool
(lldb) up
frame #4: 0x000000010002f169 js`js::ctypes::CType::IsSizeDefined(obj=0x0000000104685200) + 9 at CTypes.cpp:3609
   3606	bool
   3607	CType::IsSizeDefined(JSObject* obj)
   3608	{
-> 3609	  MOZ_ASSERT(CType::IsCType(obj));
   3610	
   3611	  jsval size = JS_GetReservedSlot(obj, SLOT_SIZE);
   3612	
(lldb) up
frame #5: 0x0000000100032ef7 js`js::ctypes::ImplicitConvert(cx=0x0000000103088180, val=<unavailable>, targetType_=<unavailable>, buffer=0x0000000103031198, isArgument=false, freePointer=0x0000000000000000) + 87 at CTypes.cpp:2293
   2290	                bool* freePointer)
   2291	{
   2292	  RootedObject targetType(cx, targetType_);
-> 2293	  MOZ_ASSERT(CType::IsSizeDefined(targetType));
   2294	
   2295	  // First, check if val is either a CData object or a CDataFinalizer
   2296	  // of type targetType.
(lldb) up
frame #6: 0x000000010003fe5a js`js::ctypes::CType::ConstructData(JSContext*, unsigned int, JS::Value*) + 260 at CTypes.cpp:5177
   5174	    for (FieldInfoHash::Range r = fields->all(); !r.empty(); r.popFront()) {
   5175	      const FieldInfo& field = r.front().value();
   5176	      STATIC_ASSUME(field.mIndex < fields->count());  /* Quantified invariant */
-> 5177	      if (!ImplicitConvert(cx, args[field.mIndex], field.mType,
   5178	             buffer + field.mOffset,
   5179	             false, nullptr))
   5180	        return false;

Comment 2

[Tooru Fujisawa [:arai]]

The object is FieldInfo.mType. I guess it's not traced correctly in StructType::DefineInternal, and the pointer is not updated on move.

mType is assigned here:

https://dxr.mozilla.org/mozilla-central/source/js/src/ctypes/CTypes.cpp#4966
>      FieldInfo info;
>      info.mType = fieldType;
>      info.mIndex = i;
>      info.mOffset = fieldOffset;
>      ASSERT_OK(fields->add(entryPtr, name, info));

but not traced until `fields` gets stored to reserved slot, am I correct?

https://dxr.mozilla.org/mozilla-central/source/js/src/ctypes/CTypes.cpp#4999
>   JS_SetReservedSlot(typeObj, SLOT_FIELDINFO, PRIVATE_TO_JSVAL(fields.release()));

if so, mType should be assigned just before JS_SetReservedSlot, from fieldRoots array.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Set FieldInto::mType just before storing to reserved slot.

Changed to assign mType field just before JS_SetReservedSlot.

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 8594353 [details] [diff] [review]
Set FieldInto::mType just before storing to reserved slot.

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

Forwarding review.

Comment 5

[Jon Coppeard (:jonco)]

Comment on attachment 8594353 [details] [diff] [review]
Set FieldInto::mType just before storing to reserved slot.

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

Thanks for tracking this down!

This will fix the issue.  I think this code needs some refactoring but I'm not going to ask you to do that here.

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 8594353 [details] [diff] [review]
Set FieldInto::mType just before storing to reserved slot.

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

::: js/src/ctypes/CTypes.cpp
@@ +4998,5 @@
>  
> +  int i = 0;
> +  for (FieldInfoHash::Range r = fields->all(); !r.empty(); r.popFront()) {
> +    FieldInfo& field = r.front().value();
> +    MOZ_ASSERT(i == field.mIndex);

I get a warning here about comparison of integers of different signs.  Best change i to be a size_t like FieldInfo::mIndex.

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Set FieldInto::mType just before storing to reserved slot. r=jonco

Thank you for reviewing :)
Fixed `int i = 0;` to `size_t i = 0;`.


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

I have no idea how to exploit this.
This is basically use-after-free, but currently it occurs only in compacting JS shell build with JS_GC_ZEAL=14 env, this is very different situation about GC than normal build. Normal build has very reduced chance to cause this issue. Also, only chrome-priv code can access js-ctypes on browser. so it might be difficult to exploit it.

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

The code comment "Value of fields are not yet traceable here." may describe the problem, but doesn't provide enough information how to exploit it.
The test shows how to cause EXC_BAD_ACCESS, but this also doesn't provice enough information for exploit, I guess.

> Which older supported branches are affected by this flaw?

All supported branches has almost same code in StructType::DefineInternal.

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

Not yet prepared though, it will be easy and low risk.

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

Not likely, if it passes all automated tests (especially js-ctypes tests in jstests and mochitest, which are passed locally).

Comment 8

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

Comment on attachment 8594748 [details] [diff] [review]
Set FieldInto::mType just before storing to reserved slot. r=jonco

sec-approval+ though it is a sec-moderate now so technically it doesn't need it.

Comment 9

[Tooru Fujisawa [:arai]]

Thank you for approval :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/7eee0cdd0feb

Comment 10

[Tooru Fujisawa [:arai]]

backed out for xpcshell-test failure.
https://hg.mozilla.org/integration/mozilla-inbound/rev/680258d72d0c
might be related to the patches landed at sametime though.

Comment 11

[Tooru Fujisawa [:arai]]

that wasn't this patch's issue
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5888cd6bb3f1
landed again.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ba8d6cc53c0

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3ba8d6cc53c0

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Fx39. Not sure if we care about this for ESR/B2G.

Comment 14

[Tooru Fujisawa [:arai]]

Comment on attachment 8594748 [details] [diff] [review]
Set FieldInto::mType just before storing to reserved slot. r=jonco

Approval Request Comment
> [Feature/regressing bug #]
js-ctypes

> [User impact if declined]
Possible crash due to use-after-free (the possibility is very low tho).

> [Describe test coverage new/current, TreeHerder]
newly added js/src/jit-test/tests/ctypes/bug1155985.js covers the case.

> [Risks and why]
Low. Exactly same patch is landed to firefox 40, and no regression happens until now.

> [String/UUID change made/needed]
None

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8594748 [details] [diff] [review]
Set FieldInto::mType just before storing to reserved slot. r=jonco

Might as well ask and see what RelMan says about ESR/B2G.

Comment 16

[Josh Cheng [:josh]]

Comment on attachment 8594748 [details] [diff] [review]
Set FieldInto::mType just before storing to reserved slot. r=jonco

Approving and please NI for any side effect.
Thanks

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/ae1282b33319

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/11e5d073db75
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/11e5d073db75

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8594748 [details] [diff] [review]
Set FieldInto::mType just before storing to reserved slot. r=jonco

Approved for uplift to beta; adds test coverage and fixes a sec-moderate regression.

Comment 20

[Tooru Fujisawa [:arai]]

Thank you!

https://hg.mozilla.org/releases/mozilla-beta/rev/04e07d5a9b00

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8594748 [details] [diff] [review]
Set FieldInto::mType just before storing to reserved slot. r=jonco

sec-moderate (esr expects high or critical security fixes), "possibility [of having the crash] is very low", not taking it.