892426 - Crash [@ compartment] or Opt-Crash [@ getGeneric] involving array_toSource and invalid read

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 3fc610532baf (run with --fuzzing-safe --ion-eager):


function setelem(o, i, v) {
    o[i] = o;
}
var arr = new Array();
setelem(arr, "prop0", 2);
setelem(arr, 0, 2);
setelem(arr, 1, 1);
setelem(arr, 0, 0);
assertEq(arr.prop0, 2);

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

Opt-Crash trace:


Program received signal SIGSEGV, Segmentation fault.
getGeneric (vp=JSVAL_VOID, id=..., receiver=(JSObject * const) 0x7ffff6264730 Cannot access memory at address 0xfffbfffff6264730, obj=(JSObject * const) 0x7ffff6264730 Cannot access memory at address 0xfffbfffff6264730, cx=0x16321e0) at js/src/jsobj.h:865
865             js::GenericIdOp op = obj->getOps()->getGeneric;
#0  getGeneric (vp=JSVAL_VOID, id=..., receiver=(JSObject * const) 0x7ffff6264730 Cannot access memory at address 0xfffbfffff6264730, obj=(JSObject * const) 0x7ffff6264730 Cannot access memory at address 0xfffbfffff6264730, cx=0x16321e0) at js/src/jsobj.h:865
#1  getProperty (vp=JSVAL_VOID, name=<optimized out>, receiver=(JSObject * const) 0x7ffff6264730 Cannot access memory at address 0xfffbfffff6264730, obj=(JSObject * const) 0x7ffff6264730 Cannot access memory at address 0xfffbfffff6264730, cx=0x16321e0) at js/src/jsobj.h:889
#2  js::ValueToSource (cx=0x16321e0, v=...) at js/src/jsstr.cpp:3830
#3  0x000000000053cca0 in array_toSource_impl (args=..., cx=0x16321e0) at js/src/jsarray.cpp:834
#4  CallNonGenericMethod<IsArray, array_toSource_impl> (args=..., cx=0x16321e0) at js/src/jsapi.h:712
#5  array_toSource (cx=0x16321e0, argc=0, vp=0x7fffffffaa28) at js/src/jsarray.cpp:869
#6  0x000000000045a3a9 in CallJSNative (args=..., native=<optimized out>, cx=0x16321e0) at ../jscntxtinlines.h:224
#7  js::Invoke (cx=0x16321e0, args=..., construct=<optimized out>) at js/src/vm/Interpreter.cpp:479
rax     0xf6264700      -1125900072106240
rip     0x622d02 <js::ValueToSource(JSContext*, JS::Handle<JS::Value>)+194>
=> 0x622d02 <js::ValueToSource(JSContext*, JS::Handle<JS::Value>)+194>: mov    (%rax),%rax


It looks like obj being used here is corrupted somehow. I assume if you can control what obj points to, this becomes sec-critical.

Comment 3

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/3efe3f3d2c25
user:        Jan de Mooij
date:        Wed Jun 19 19:10:04 2013 +0200
summary:     Bug 882111 - Don't push an interpreter frame when calling into the JITs. r=djvj

This iteration took 328.403 seconds to run.

Comment 4

[Jan de Mooij [:jandem]]

autoBisect is wrong, it's a regression from bug 774006 I think. It looks like we have a SetElementIC and use the same register for the object and the value on x64 and end up tagging + storing the elements pointer instead of the value. Nicolas can you take a look?

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> autoBisect is wrong, it's a regression from bug 774006 I think. It looks
> like we have a SetElementIC and use the same register for the object and the
> value on x64 and end up tagging + storing the elements pointer instead of
> the value. Nicolas can you take a look?

Your analysis is right, the attachDenseElement is re-using the JSObject pointer as the element pointer, but as we reuse the same register, instead of packing a JSObject inside the value, we insert a an Element inside the Value, which cause a problem when we try to serialize the content of the Array in AssertEq.

Comment 6

[Nicolas B. Pierron [:nbp]]

Attachment: Add an additional temp-Register to SetElementIC.

This patch remove the logic for extracting a scratch register from the object, as the object can alias the value.  As we do not have any logic to mention that we want a tmpCopy of an input if it alias another input, I just added an extra temp register as input of the SetElementIC.

Comment 8

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 775398 [details] [diff] [review]
Add an additional temp-Register to SetElementIC.

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

Makes sense.

::: js/src/ion/IonCaches.h
@@ +693,5 @@
>  class SetElementIC : public RepatchIonCache
>  {
>    protected:
>      Register object_;
> +    Register element_;

Nit: use temp0 and temp1 (or something similar) - if we add more stubs (for instance for obj[string] = foo) these stubs will use the registers for something else.

::: js/src/ion/LIR-Common.h
@@ +4064,5 @@
>  
>      const LAllocation *object() {
>          return getOperand(0);
>      }
> +    const LDefinition *element() {

Same here.

Comment 10

[Nicolas B. Pierron [:nbp]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/334b24c05703

Comment 11

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

This should have gone through sec-approval before landing since it affects multiple versions and we need to take it on Aurora to fix it there. 

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

Before approving it for Aurora (which means before landing there), please answer the questions in the sec-approval? template and the aurora approval template.

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 775398 [details] [diff] [review]
Add an additional temp-Register to SetElementIC.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 774006
User impact if declined: SEGV on GC.
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): low, it removes a previous incorrect optimization.
String or IDL/UUID changes made by this patch: N/A

Comment 13

[Nicolas B. Pierron [:nbp]]

Comment on attachment 775398 [details] [diff] [review]
Add an additional temp-Register to SetElementIC.

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

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

Which older supported branches are affected by this flaw?
24

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

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

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

Comment 14

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

Comment on attachment 775398 [details] [diff] [review]
Add an additional temp-Register to SetElementIC.

Ok. This looks good. Let's get this on Aurora.

Comment 15

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b5e891883c50

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/334b24c05703

Comment 17

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.