Closed Bug 1285960 Opened 8 years ago Closed 7 years ago

Assertion failure: byteOffset + byteLength <= arrayBuffer->byteLength(), at js/src/vm/TypedArrayObject.cpp:1418

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox-esr45 51+ verified
firefox50 --- wontfix
firefox51 + verified
firefox52 + verified
firefox53 + verified

People

(Reporter: anba, Assigned: Waldo)

Details

(Keywords: csectype-bounds, regression, sec-critical, Whiteboard: [post-critsmash-triage][adv-main51+][adv-esr45.7+])

Attachments

(8 files, 2 obsolete files)

m-c 679118259e91

Marked as s-s because it involves detached array buffers.


Assertion failure:
---
Assertion failure: byteOffset + byteLength <= arrayBuffer->byteLength(), at /home/andre/git/mozilla-central/js/src/vm/TypedArrayObject.cpp:1418
---


Test case:
---
var buffer = new ArrayBuffer(10);
var newTarget = Object.defineProperty(function(){}.bind(), "prototype", {
    get() {
        detachArrayBuffer(buffer, "same-data");
        return DataView.prototype;
    }
})
Reflect.construct(DataView, [buffer, 0, 1], newTarget);
---


Back trace:
---
0x0000000000e29749 in js::DataViewObject::create (cx=0x7ffff6960000, byteOffset=0, byteLength=1, arrayBuffer=..., protoArg=0x7ffff7e80080)
    at /home/andre/git/mozilla-central/js/src/vm/TypedArrayObject.cpp:1418
1418	    MOZ_ASSERT(byteOffset + byteLength <= arrayBuffer->byteLength());
(gdb) bt
#0  0x0000000000e29749 in js::DataViewObject::create (cx=0x7ffff6960000, byteOffset=0, byteLength=1, arrayBuffer=..., protoArg=0x7ffff7e80080)
    at /home/andre/git/mozilla-central/js/src/vm/TypedArrayObject.cpp:1418
#1  0x0000000000e29fb5 in js::DataViewObject::constructSameCompartment (cx=0x7ffff6960000, bufobj=..., args=...) at /home/andre/git/mozilla-central/js/src/vm/TypedArrayObject.cpp:1522
#2  0x0000000000e2a68a in js::DataViewObject::class_constructor (cx=0x7ffff6960000, argc=3, vp=0x7fffffffc8c8) at /home/andre/git/mozilla-central/js/src/vm/TypedArrayObject.cpp:1604
#3  0x0000000000d0fdc8 in js::CallJSNative (cx=0x7ffff6960000, native=0xe2a558 <js::DataViewObject::class_constructor(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/andre/git/mozilla-central/js/src/jscntxtinlines.h:232
#4  0x0000000000d0ff25 in js::CallJSNativeConstructor (cx=0x7ffff6960000, native=0xe2a558 <js::DataViewObject::class_constructor(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/andre/git/mozilla-central/js/src/jscntxtinlines.h:265
#5  0x0000000000ce8132 in InternalConstruct (cx=0x7ffff6960000, args=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:544
#6  0x0000000000ce84ab in js::Construct (cx=0x7ffff6960000, fval=..., args=..., newTarget=..., objp=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:595
#7  0x0000000000ed95ac in Reflect_construct (cx=0x7ffff6960000, argc=3, vp=0x7ffff3e3f090) at /home/andre/git/mozilla-central/js/src/builtin/Reflect.cpp:113
#8  0x0000000000d0fdc8 in js::CallJSNative (cx=0x7ffff6960000, native=0xed932a <Reflect_construct(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/andre/git/mozilla-central/js/src/jscntxtinlines.h:232
#9  0x0000000000ce7a1a in js::InternalCallOrConstruct (cx=0x7ffff6960000, args=..., construct=js::NO_CONSTRUCT) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:453
#10 0x0000000000ce7d5b in InternalCall (cx=0x7ffff6960000, args=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:498
#11 0x0000000000ce7d99 in js::CallFromStack (cx=0x7ffff6960000, args=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:504
#12 0x0000000000cf5309 in Interpret (cx=0x7ffff6960000, state=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:2873
#13 0x0000000000ce7673 in js::RunScript (cx=0x7ffff6960000, state=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:399
#14 0x0000000000ce8c00 in js::ExecuteKernel (cx=0x7ffff6960000, script=..., scopeChainArg=..., newTargetValue=..., evalInFrame=..., result=0x0)
    at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:679
#15 0x0000000000ce8ee9 in js::Execute (cx=0x7ffff6960000, script=..., scopeChainArg=..., rval=0x0) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:712
#16 0x0000000000a42a8c in ExecuteScript (cx=0x7ffff6960000, scope=..., script=..., rval=0x0) at /home/andre/git/mozilla-central/js/src/jsapi.cpp:4313
#17 0x0000000000a42e96 in JS_ExecuteScript (cx=0x7ffff6960000, scriptArg=...) at /home/andre/git/mozilla-central/js/src/jsapi.cpp:4346
#18 0x0000000000436910 in RunFile (cx=0x7ffff6960000, filename=0x7fffffffdfd1 "/tmp/g.js", file=0x7ffff3e3b000, compileOnly=false) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:539
#19 0x0000000000437cd6 in Process (cx=0x7ffff6960000, filename=0x7fffffffdfd1 "/tmp/g.js", forceTTY=false, kind=FileScript) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:860
#20 0x000000000044e941 in ProcessArgs (cx=0x7ffff6960000, op=0x7fffffffd9a0) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:6788
#21 0x000000000044ff27 in Shell (cx=0x7ffff6960000, op=0x7fffffffd9a0, envp=0x7fffffffdbb0) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:7137
#22 0x00000000004514ae in main (argc=2, argv=0x7fffffffdb98, envp=0x7fffffffdbb0) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:7518
---
Sounds like some kind of buffer overflow, so I'll mark it sec-high.
Group: core-security → javascript-core-security
This is critical for the same reason bug 1285833, bug 991981, and bug 982974 were critical.

I'd love to churn out a patch quickly now, but the mess of DataView construction handling, combined with a chokepoint function that's *supposed* to centralize this bounds-checking but does so too early to be usable, means it's non-trivial to fix in the ten minutes I have now.  Tonight or tomorrow.
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Keywords: sec-highsec-critical
This helps with understanding this code, and I think it may help with the next step of actually fixing this bug.  (But I'm not 100% certain of that yet, as I haven't written a fix yet, and I think there's a chance this needs a spec change -- although we may need a stopgap in advance of the spec being updated.)
Attachment #8818160 - Flags: review?(sphink)
Comment on attachment 8818160 [details] [diff] [review]
Just enter the wrapped ArrayBuffer's compartment when doing |new DataView(alienBuf)|, rather than calling an intrinsic

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

I agree, this is far easier to follow.

::: js/src/vm/TypedArrayObject.h
@@ +447,5 @@
>      defineGetter(JSContext* cx, PropertyName* name, HandleNativeObject proto);
>  
> +    static bool
> +    getAndCheckConstructorArgs(JSContext* cx, HandleObject bufobj, const CallArgs& args,
> +                               uint32_t *byteOffset, uint32_t* byteLength);

As long as you're here... uint32_t* byteOffset
Attachment #8818160 - Flags: review?(sphink) → review+
I think there's a good chance there should be one final is-detached check after the GetPrototypeFromConstructor in the spec, and this is a spec bug.  I have a patch implementing that, but it's super-finicky to set up a DataView backed by a detached ArrayBuffer, that faithfully implements the exact state that would happen if a DataView's underlying buffer were detached *after* DataView creation.  So maybe let's do a spot-fix.  (Also we can't really get the spec changed publicly without potentially tipping our hand as to exploitability.)  If the spec *doesn't* end up changing, my more-involved fix can be taken, at leisure.

This also has the small benefit of obscuring the patch slightly, but it doesn't take much to see DVO::create calls are immediately preceded by potentially-reentrant code, if you know the trick to make GetPrototypeFromConstructor invoke user-provided code.  So it's better than a well-commented fix, but still unideal.

Assuming this lands, the prior patch -- still a good idea -- will move to a separate bug.
Attachment #8818424 - Flags: review?(sphink)
Attachment #8818425 - Flags: review?(sphink)
For the record, here's a patch to make the test check for closer to (buggy?-)spec-compliant behavior, if needed.
Emulating the exact state of a DV backed by a detached ArrayBuffer, as this patch does, is pretty scary.  Hopefully we won't need this.
Attached patch Spot fix for beta (obsolete) — Splinter Review
Attached patch Spot fix for ESR45 (obsolete) — Splinter Review
The patch against trunk works on Aurora as well.  Beta/ESR need slightly tweaked patches, just uploaded.  Will wait for r+ and sec-approval on the trunk patch before requesting branch approvals, then once we have that we can figure out how/when to land this stuff, in coordinated fashion, so as to minimize exposure time.
Comment on attachment 8818424 [details] [diff] [review]
Non-spec-compliant spot-fix

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

At least this is a very straightforward fix. Sorry for the delay; this slipped through the cracks.
Attachment #8818424 - Flags: review?(sphink) → review+
Comment on attachment 8818425 [details] [diff] [review]
Test for non-compliant behavior

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

::: js/src/tests/ecma_6/extensions/dataview-detach-after-construction.js
@@ +1,1 @@
> +// |reftest| skip-if(!xulRuntime.shell) -- needs detachArrayBuffer

If it's a backport issue, detachArrayBuffer can be done with serialize(buffer, [buffer])
Attachment #8818425 - Flags: review?(sphink) → review+
Comment on attachment 8818424 [details] [diff] [review]
Non-spec-compliant spot-fix

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

Depends how easily people track DVO::create calls backward to recognize the problematic instance.  There are only a few of them, so it's really pretty easy.

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

The test would, but that's not landing for awhile, of course.  The patch itself adds an is-detached check that clearly guards a bunch of array accesses, so it's pretty close to a bulls-eye (if not exactly one).  Worth landing as late as possible in a cycle, IMO.

> Which older supported branches are affected by this flaw?
> 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?

Backports all but painless, attached already.

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

Very unlikely.  DVO::create can already throw, this just introduces another way, and the check guarding the throw is dead-simple.  The automated test in attachment 8818425 [details] [diff] [review] should be adequate testing.
Attachment #8818424 - Flags: sec-approval?
Comment on attachment 8818424 [details] [diff] [review]
Non-spec-compliant spot-fix

sec-approval+ for trunk. Please nominate the backport patches when you have a chance.
Attachment #8818424 - Flags: sec-approval? → sec-approval+
Comment on attachment 8818424 [details] [diff] [review]
Non-spec-compliant spot-fix

Approval Request Comment
[Feature/Bug causing the regression]: bug 1055472, introducing DataView subclassing
[User impact if declined]: sec-critical
[Is this code covered by automated tests?]: We have one here, but we can't well land it in advance of this.
[Has the fix been verified in Nightly?]: Not landed yet, but it fixed the test attached here.
[Needs manual test from QE? If yes, steps to reproduce]: Automated test should cover it.
[List of other uplifts needed for the feature/fix]: No.
[Is the change risky?]: Not really, per comment 14.
[Why is the change risky/not risky?]: See comment 14.
[String changes made/needed]: N/A
Attachment #8818424 - Flags: approval-mozilla-aurora?
Attachment #8819444 - Flags: approval-mozilla-beta?
Comment on attachment 8819445 [details] [diff] [review]
Spot fix for ESR45

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-critical
User impact if declined: Do you want a known pwn2own-level vulnerability in the product?  :-)
Fix Landed on Version: hasn't yet, will be 51
Risk to taking this patch (and alternatives if risky): Not really, see comment 14, comment 16.
String or UUID changes made by this patch: N/A
Attachment #8819445 - Flags: approval-mozilla-esr45?
Attached patch Aurora backportSplinter Review
Attachment #8818424 - Attachment is obsolete: true
Attachment #8818424 - Flags: approval-mozilla-aurora?
Attached patch Beta backportSplinter Review
Attachment #8819444 - Attachment is obsolete: true
Attachment #8819444 - Flags: approval-mozilla-beta?
Attached patch ESR45 backportSplinter Review
Attachment #8819445 - Attachment is obsolete: true
Attachment #8819445 - Flags: approval-mozilla-esr45?
Comment on attachment 8818424 [details] [diff] [review]
Non-spec-compliant spot-fix

Oops, I just blew away a bunch of stuff that had approvals pending because I thought I hadn't uploaded them.  Reverting the obsoletion here.  Because the new patches are direct from my tree-clones (so no changes would be needed to land them), I'll set approval flags on them rather than on the originals.
Attachment #8818424 - Attachment is obsolete: false
Comment on attachment 8823480 [details] [diff] [review]
Aurora backport

See comment 16.
Attachment #8823480 - Flags: approval-mozilla-aurora?
Comment on attachment 8823481 [details] [diff] [review]
Beta backport

See comment 16.
Attachment #8823481 - Flags: approval-mozilla-beta?
Comment on attachment 8823482 [details] [diff] [review]
ESR45 backport

See comment 17.
Attachment #8823482 - Flags: approval-mozilla-esr45?
Landed on inbound.

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

My general preference is to do a coordinated landing across all branches at once to minimize exposure time, wherever possible for 1) sufficiently-simple patches where baking on trunk is unlikely to give any new information 2) that have serious security implications best left unpublicized by public fixing, for as long as possible.  (And this bug/fix certainly qualify as such.)  However, rumor has it that patches that haven't landed don't always get quite as prompt branch-approval, and we're running close on deadlines for the various branches, so I jumped the gun against my usual preference.  :-\  (That said, the holiday/weekend timeframe probably played a part in any "delay" in this particular case.)
Comment on attachment 8823480 [details] [diff] [review]
Aurora backport

sec-critical fix for aurora52
Attachment #8823480 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8823482 [details] [diff] [review]
ESR45 backport

sec-critical fix for esr45
Attachment #8823482 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Comment on attachment 8823481 [details] [diff] [review]
Beta backport

sec-critical fix for 51beta, should probably go in today's b12 build
Attachment #8823481 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/bd0516d2e04a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
I reproduced the original issue using the poc from comment#0 using revision 679118259e91:

* Assertion failure: byteOffset + byteLength <= arrayBuffer->byteLength(), at /home/kjozwiak/mozcode/mozilla-central/js/src/vm/TypedArrayObject.cpp:1418

Once I managed to reproduced the original issue, I went through verification using the test case from comment#0 with the following builds:

* fx53, rev: 20094311ab5e
* fx52, rev: d173a8f450cf
* fx51, rev: 95d20a617288
* fx45esr, rev: 0521b0e4707c

All four of the above builds generated the following error:

/home/kjozwiak/Desktop/fixedpoc.js:8:1 TypeError: attempting to access detached ArrayBuffer
Stack:
  @/home/kjozwiak/Desktop/fixedpoc.js:8:1

As per bug#1285833comment#19, I removed the "same-data" argument from |detachArrayBuffer| when running through the test case using fx53, fx52 and fx51. For fx45esr, I renamed the |detachArrayBuffer| function to |neuter|.
evilpie: attachment 8818430 [details] [diff] [review] fixes bug 1317382, but the breadth of that fix -- and the possibility that DataView should check its provided ArrayBuffer for detaching a second time -- meant that wasn't a good patch to land for a fast/safe/backportable fix.  Right now, I'm inclined to raise a spec issue once we've fixed this everywhere and given it time for people to update -- a month or so, maybe more -- and then that patch (suitably modified in response to any spec discussion) could land.

Which is *maybe* a way of saying you shouldn't take that bug, I guess?  But it's moderately unsatisfying to just sit on this work, and delay fixing that bug, just because of security stuff and spec chicaneries.  :-\
I filed bug 1329046 for cleaning up the DataView creation-with-compartments code, without its being entangled it in a security bug.  That bug's attachment 8824264 [details] [diff] [review] is identical to this bug's attachment 8818160 [details] [diff] [review], except to the extent of being generated more recently and possibly having been rebased some.  (Haven't written a similar linking comment in that bug, so as not to draw attention to a security issue in the code.)
Group: javascript-core-security → core-security-release
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main51+][adv-esr45.7+]
:sfink suggests this should be taken into consideration for a bounty, can you please elaborate in more detail? :)
Flags: sec-bounty?
Flags: needinfo?(sphink)
(In reply to Frederik Braun [:freddyb] from comment #34)
> :sfink suggests this should be taken into consideration for a bounty, can
> you please elaborate in more detail? :)

This is similar to a pretty major bug that was exploited in pwn2own, it wouldn't be horribly hard to find if you were looking in nearby places, yet we hadn't noticed the similarity in the code before anba uncovered it by going through the spec. Oh, and once you spot it, it's easy to exploit. So it seems like it was pretty ripe for causing us some major trouble. And I wasn't sure if anba normally requests bounties on his own.
Flags: needinfo?(sphink)
(In reply to Steve Fink [:sfink] [:s:] from comment #35)
> And I wasn't sure if anba normally requests bounties on his own.

No, I don't. (I didn't even know that it's possible to self-request bounties!)
Details on the client bounty program are at https://www.mozilla.org/en-US/security/client-bug-bounty/ (linked to from www.mozilla.org/security/ )

From the page:

Claiming a Bug Bounty

To claim a bounty:

    Make sure you have a Bugzilla account
    File a bug at bugzilla.mozilla.org describing the security issue.
    When creating the bug, be sure to check the box near the bottom of the entry form that marks this bug report as confidential.
    Attach a "proof of concept" testcase, or point to explicit code that identifies the vulnerability or exploit. While not required, such a testcase will help us judge submissions more quickly and accurately.
    If you have debug output or output from a tool demonstrating the issue, please include it in the bug. (If it is very long, please attach it to the bug as an attached file.)
    Notify the Mozilla Security Group by email to security@mozilla.org and include the number of the bug you filed and a brief summary of the issue.

Per the last line, people can (and normally should) self-request bounties by emailing security@mozilla.org once they file a security bug if they want to be considered for it.
Flags: sec-bounty? → sec-bounty+
Group: core-security-release
I filed https://github.com/tc39/ecma262/pull/1025 as followup on changing the spec algorithm somehow.  Don't know yet the precise approach that the spec might take to address this, but I *think* if they do what the PR currently proposes, it would be identical to the change made here.  (But someone would have to double-check that.)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: