Closed Bug 1414768 Opened 2 years ago Closed 2 years ago

Crash [@ JSObject::getClass] or Self-hosted JavaScript assertion info: "builtin/TypedArray.js:6: non-typed array asked for its buffer"

Categories

(Core :: JavaScript Engine, defect, P1, major)

x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox-esr52 --- wontfix
firefox57 --- wontfix
firefox58 + wontfix
firefox59 + fixed
firefox60 --- verified

People

(Reporter: decoder, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:update][adv-main59+])

Crash Data

Attachments

(1 file, 2 obsolete files)

The following testcase crashes on mozilla-central revision 4e6df5159df3 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

function sharedConstructor(baseConstructor) {}
const typedArrayConstructors = Object.freeze([ Int8Array ]);
const sharedTypedArrayConstructors = Object.freeze(typedArrayConstructors.map(sharedConstructor));
const anyTypedArrayConstructors = Object.freeze([...typedArrayConstructors, ...sharedTypedArrayConstructors, ]);
for (var constructor of anyTypedArrayConstructors)
  for (let v of wrapWithProto(constructor.from({}), new constructor())) {}


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x00000000009e08de in JSObject::getClass (this=<optimized out>) at js/src/jsobj.h:103
#0  0x00000000009e08de in JSObject::getClass (this=<optimized out>) at js/src/jsobj.h:103
#1  JSObject::is<js::SharedArrayBufferObject> (this=<optimized out>) at js/src/jsobj.h:519
#2  intrinsic_IsInstanceOfBuiltin<js::SharedArrayBufferObject> (cx=0x7ffff694d000, argc=1, vp=0x7ffff4213300) at js/src/vm/SelfHosting.cpp:202
#3  0x0000000000523c14 in js::CallJSNative (args=..., native=0x9e08d0 <intrinsic_IsInstanceOfBuiltin<js::SharedArrayBufferObject>(JSContext*, unsigned int, JS::Value*)>, cx=0x7ffff694d000) at js/src/jscntxtinlines.h:291
#4  js::InternalCallOrConstruct (cx=0x7ffff694d000, args=..., construct=<optimized out>) at js/src/vm/Interpreter.cpp:472
#5  0x0000000000516515 in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:527
#6  Interpret (cx=0x7ffff694d000, state=...) at js/src/vm/Interpreter.cpp:3061
#7  0x000000000052388a in js::RunScript (cx=0x7ffff694d000, state=...) at js/src/vm/Interpreter.cpp:422
#8  0x0000000000523ddb in js::InternalCallOrConstruct (cx=0x7ffff694d000, args=..., construct=<optimized out>) at js/src/vm/Interpreter.cpp:494
#9  0x0000000000516515 in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:527
#10 Interpret (cx=0x7ffff694d000, state=...) at js/src/vm/Interpreter.cpp:3061
#11 0x000000000052388a in js::RunScript (cx=0x7ffff694d000, state=...) at js/src/vm/Interpreter.cpp:422
#12 0x0000000000523ddb in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff694d000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:494
#13 0x0000000000524035 in InternalCall (cx=cx@entry=0x7ffff694d000, args=...) at js/src/vm/Interpreter.cpp:521
#14 0x0000000000524098 in js::Call (cx=cx@entry=0x7ffff694d000, fval=..., fval@entry=..., thisv=..., args=..., rval=...) at js/src/vm/Interpreter.cpp:540
#15 0x0000000000a01bce in CallSelfHostedNonGenericMethod (cx=0x7ffff694d000, args=...) at js/src/vm/SelfHosting.cpp:1703
#16 0x00000000008ce528 in js::CallNativeImpl (args=..., impl=0xa01960 <CallSelfHostedNonGenericMethod(JSContext*, JS::CallArgs const&)>, cx=0x7ffff694d000) at js/src/jscntxtinlines.h:307
#17 js::ForwardingProxyHandler::nativeCall (this=<optimized out>, cx=0x7ffff694d000, test=<optimized out>, impl=0xa01960 <CallSelfHostedNonGenericMethod(JSContext*, JS::CallArgs const&)>, args=...) at js/src/proxy/Wrapper.cpp:239
#18 0x00000000008d429c in js::Proxy::nativeCall (cx=0x7ffff694d000, test=<optimized out>, impl=<optimized out>, args=...) at js/src/proxy/Proxy.cpp:543
#19 0x000000000091c1bb in JS::detail::CallMethodIfWrapped (cx=<optimized out>, test=test@entry=0xa03410 <Is<js::TypedArrayObject>(JS::Handle<JS::Value>)>, impl=impl@entry=0xa01960 <CallSelfHostedNonGenericMethod(JSContext*, JS::CallArgs const&)>, args=...) at js/src/vm/CallNonGenericMethod.cpp:28
#20 0x0000000000a0222e in JS::CallNonGenericMethod<&(bool Is<js::TypedArrayObject>(JS::Handle<JS::Value>)), &(CallSelfHostedNonGenericMethod(JSContext*, JS::CallArgs const&))> (args=..., cx=<optimized out>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/instrumentation/none/type/opt/dist/include/js/CallNonGenericMethod.h:102
#21 CallNonGenericSelfhostedMethod<Is<js::TypedArrayObject> > (cx=<optimized out>, argc=2, vp=0x7ffff42131e8) at js/src/vm/SelfHosting.cpp:1741
#22 0x0000000000523c14 in js::CallJSNative (args=..., native=0xa02180 <CallNonGenericSelfhostedMethod<Is<js::TypedArrayObject> >(JSContext*, unsigned int, JS::Value*)>, cx=0x7ffff694d000) at js/src/jscntxtinlines.h:291
[...]
#43 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:8962
rax	0x0	0
rbx	0x7ffff694d000	140737330335744
rcx	0x7ffff692b000	140737330196480
rdx	0x7ffff4213300	140737289204480
rsi	0x1	1
rdi	0x7ffff694d000	140737330335744
rbp	0x7fffffffb2d0	140737488335568
rsp	0x7fffffffb128	140737488335144
r8	0x7ffff4213318	140737289204504
r9	0x7ffff4213318	140737289204504
r10	0xfffdffffffffffff	-562949953421313
r11	0x7ffffffff000	140737488351232
r12	0x7fffffffb170	140737488335216
r13	0x9e08d0	10356944
r14	0x7fffffffb800	140737488336896
r15	0x7ffff694d000	140737330335744
rip	0x9e08de <intrinsic_IsInstanceOfBuiltin<js::SharedArrayBufferObject>(JSContext*, unsigned int, JS::Value*)+14>
=> 0x9e08de <intrinsic_IsInstanceOfBuiltin<js::SharedArrayBufferObject>(JSContext*, unsigned int, JS::Value*)+14>:	mov    (%rax),%rcx
   0x9e08e1 <intrinsic_IsInstanceOfBuiltin<js::SharedArrayBufferObject>(JSContext*, unsigned int, JS::Value*)+17>:	lea    0x11687b8(%rip),%rax        # 0x1b490a0 <_ZN2js23SharedArrayBufferObject6class_E>


Marking s-s because the assertion is about typed array vs. regular array confusion which sounds potentially dangerous.
Simpler:

wrapWithProto(Int8Array.from({}), Int8Array.prototype)[Symbol.iterator]() 

By simple examination of our selfhosted code, this obviously can't be happening, but there doesn't seem to be a RESOLVED IMPOSSIBLE setting on this web site...
TypedArrayValues
  -> IsTypedArrayEnsuringArrayBuffer
    -> callFunction(CallTypedArrayMethodIfWrapped, arg, arg, "GetAttachedArrayBuffer");
      -> GetAttachedArrayBuffer
        -> ViewedArrayBufferIfReified
          -> AssertionFailed

The problem is this code:

```
function IsTypedArrayEnsuringArrayBuffer(arg) {
    if (IsObject(arg) && IsTypedArray(arg)) {
        GetAttachedArrayBuffer(arg);
        return true;
    }

    // This is a bit hacky but gets the job done: the first `arg` is used to
    // test for a wrapped typed array, the second as an argument to
    // GetAttachedArrayBuffer.
    callFunction(CallTypedArrayMethodIfWrapped, arg, arg, "GetAttachedArrayBuffer");
    return false;
}
```

The comment is wrong. The hack works correctly if `arg` is a cross-compartment wrapper, but not if it is a same-compartment wrapper.

Same-compartment wrappers, what a drag...
OK, I have a patch, but just found another method that's affected by the same bug...
Assignee: nobody → jorendorff
Priority: -- → P1
Keywords: sec-high
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/32a6f67bd8e8
user:        Boris Zbarsky
date:        Tue Mar 22 13:49:36 2016 -0400
summary:     Bug 1256342.  Fix typed array iteration to work correctly over Xrays.  r=till

This iteration took 236.905 seconds to run.
Attachment #8928791 - Flags: review?(bzbarsky)
Attachment #8926136 - Attachment is obsolete: true
Boris, these changes make the TypedArray stuff cope with same-compartment wrappers.

If same-compartment wrappers can't happen in the browser, or at least can't happen for TypedArrays, then we can ban them from the shell instead of taking this patch (and unhide this bug).

We would need to replace the wrapWithProto() shell builtin with something that makes cross-compartment wrappers. Maybe a newGlobal() option, instead of a single-object sort of thing...
I actually don't know whether browser does same-compartment wrappers, and if so when.  :(  bholley might, I guess...  We should really have this stuff documented better.
Comment on attachment 8928791 [details] [diff] [review]
Handle same-compartment wrappers in TypedArray methods

I have to admit to not understanding how this patch fixes the problem...  It could use an explanation of that in the commit message.
CallTypedArrayMethodIfWrapped (and the CallNonGeneric machinery throughout
the engine) unwraps the `this` argument, but the other arguments are only
rewrapped for the target compartment.

The pattern being used before this patch to get the length of a TypedArray
or possible TypedArray wrapper is:

    callFunction(CallTypedArrayMethodIfWrapped, O, O, "TypedArrayLength")

The first O is the `this` value and the second is an argument.
If O is a cross-compartment wrapper, this works fine. The first O is unwrapped,
revealing the actual TypedArray object; the second O is rewrapped for that
TypedArray's compartment, producing the same TypedArray.

However, if O is a same-compartment wrapper, this doesn't work. The first O
is unwrapped, revealing the actual TypedArray object in the same compartment;
rewrapping the other O does nothing to it, since it is already an object in the
target compartment. Thus TypedArrayLength receives a `this` value that's an
unwrapped TypedArray, but an argument that is still a wrapper.

The fix is to have CallTypedArrayMethodIfWrapped targets only expect `this`
to be an unwrapped TypedArray.
Attachment #8928990 - Flags: review?(bzbarsky)
Attachment #8928791 - Attachment is obsolete: true
Attachment #8928791 - Flags: review?(bzbarsky)
> If same-compartment wrappers can't happen in the browser, or at least can't happen for TypedArrays

I think they can, though you have to jump through some hoops to do it.  In particular, it might be possible for a SandboxProxyHandler to work this way if someone creates a sandbox with a typed array as its proto...  I wonder whether we could create a browser testcase using that.

The only other same-compartment wrappers in browser seem to be SandboxCallableProxyHandler and nsOuterWindowProxy.  I expect neither one can wrap a typed array.
Comment on attachment 8928990 [details] [diff] [review]
Handle same-compartment wrappers in TypedArray methods

r=me.  Thank you for the much better commit message!
Attachment #8928990 - Flags: review?(bzbarsky) → review+
Jason, wanna land this?
Flags: needinfo?(jorendorff)
Naveed, can you get someone to put this up for sec-approval? Thanks.
Flags: needinfo?(nihsanullah)
Comment on attachment 8928990 [details] [diff] [review]
Handle same-compartment wrappers in TypedArray methods

Sorry, this fell off my radar, obviously. Given comment 11 I think it probably isn't exploitable, but we should fix it anyway.

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

    Probably not exploitable, it's sec-high out of abundance of caution.

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?

    Everything since April 2016.

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

    Bug 1256376.

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

    No. It would be an average effort to create them.

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

    Possible. Needs fuzz-testing. The included tests are a good starting point.
Flags: needinfo?(nihsanullah)
Flags: needinfo?(jorendorff)
Attachment #8928990 - Flags: sec-approval?
sec-approval+ for trunk. Please nominate patches for Beta and ESR52.

Christian, is this something that you could fuzz based on request in comment 15.
Attachment #8928990 - Flags: sec-approval? → sec-approval+
Two days of fuzzing haven't revealed anything interesting here, assuming we're good to land.
Flags: needinfo?(choller)
https://hg.mozilla.org/mozilla-central/rev/8bb6aeb63d5d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
This is tracking 58 - are we going to uplift?
Flags: needinfo?(jorendorff)
Flags: needinfo?(abillings)
(In reply to Randell Jesup [:jesup] from comment #20)
> This is tracking 58 - are we going to uplift?

I'd like to see a beta patch nominated for that purpose.
Flags: needinfo?(abillings)
I think comment 11 means this bug is not sec-anything, and we should not backport.
Flags: needinfo?(jorendorff)
Severity: critical → major
Keywords: sec-high
I'll open the bug Monday unless someone has more to add.
Group: javascript-core-security → core-security-release
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main59+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.